[PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt handle

[Gupta, Nipun]

> -----Original Message-----
> From: Gupta, Nipun
> Sent: Thursday, June 1, 2023 11:35 PM
> To: David Marchand <david.marchand at redhat.com>
> Cc: dev at dpdk.org; thomas at monjalon.net; hkalra at marvell.com;
> anatoly.burakov at intel.com; stephen at networkplumber.org; Yigit, Ferruh
> <Ferruh.Yigit at amd.com>; Anand, Harpreet <harpreet.anand at amd.com>;
> Agarwal, Nikhil <nikhil.agarwal at amd.com>
> Subject: Re: [PATCH v5 3/5] eal/interrupts: add IRQ count in interrupt handle
> 
> Hi David,
> 
> On 6/1/2023 8:55 PM, David Marchand wrote:
> >
> > On Thu, May 25, 2023 at 12:08 PM Nipun Gupta <nipun.gupta at amd.com>
> wrote:
> >>
> >> Have total number of IRQ count support in interrupt handle.
> >> In case of VFIO this IRQ count is returned when
> >> VFIO_DEVICE_GET_IRQ_INFO ioctl is invoked. This IRQ_count can
> >> used by the devices to store/provide total number of interrupts
> >> available and to enable or disable these interrupts.
> >>
> >> Signed-off-by: Nipun Gupta <nipun.gupta at amd.com>
> >> Acked-by: Ferruh Yigit <ferruh.yigit at amd.com>
> >> ---
> >>   lib/eal/common/eal_common_interrupts.c | 21 +++++++++++++++++
> >>   lib/eal/common/eal_interrupts.h        |  1 +
> >>   lib/eal/include/rte_interrupts.h       | 32 ++++++++++++++++++++++++++
> >>   lib/eal/version.map                    |  2 ++
> >>   4 files changed, 56 insertions(+)
> >>
> >> diff --git a/lib/eal/common/eal_common_interrupts.c
> b/lib/eal/common/eal_common_interrupts.c
> >> index 97b64fed58..a0167d9ad4 100644
> >> --- a/lib/eal/common/eal_common_interrupts.c
> >> +++ b/lib/eal/common/eal_common_interrupts.c
> >> @@ -398,6 +398,27 @@ int rte_intr_elist_index_set(struct rte_intr_handle
> *intr_handle,
> >>          return -rte_errno;
> >>   }
> >>
> >> +int rte_intr_irq_count_set(struct rte_intr_handle *intr_handle,
> >> +       int irq_count)
> >> +{
> >> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> >> +
> >> +       intr_handle->irq_count = irq_count;
> >> +
> >> +       return 0;
> >> +fail:
> >> +       return -rte_errno;
> >> +}
> >> +
> >> +int rte_intr_irq_count_get(const struct rte_intr_handle *intr_handle)
> >> +{
> >> +       CHECK_VALID_INTR_HANDLE(intr_handle);
> >> +
> >> +       return intr_handle->irq_count;
> >> +fail:
> >> +       return -rte_errno;
> >> +}
> >> +
> >>   int rte_intr_vec_list_alloc(struct rte_intr_handle *intr_handle,
> >>          const char *name, int size)
> >>   {
> >> diff --git a/lib/eal/common/eal_interrupts.h
> b/lib/eal/common/eal_interrupts.h
> >> index 482781b862..eaf8e20187 100644
> >> --- a/lib/eal/common/eal_interrupts.h
> >> +++ b/lib/eal/common/eal_interrupts.h
> >> @@ -16,6 +16,7 @@ struct rte_intr_handle {
> >>          };
> >>          uint32_t alloc_flags;   /**< flags passed at allocation */
> >>          enum rte_intr_handle_type type;  /**< handle type */
> >> +       uint32_t irq_count;             /**< Total IRQ count */
> >>          uint32_t max_intr;             /**< max interrupt requested */
> >>          uint32_t nb_efd;               /**< number of available efd(event fd) */
> >>          uint8_t efd_counter_size;      /**< size of efd counter, used for vdev */
> >> diff --git a/lib/eal/include/rte_interrupts.h b/lib/eal/include/rte_interrupts.h
> >> index 487e3c8875..415d1fcac0 100644
> >> --- a/lib/eal/include/rte_interrupts.h
> >> +++ b/lib/eal/include/rte_interrupts.h
> >> @@ -506,6 +506,38 @@ __rte_internal
> >>   int
> >>   rte_intr_max_intr_get(const struct rte_intr_handle *intr_handle);
> >>
> >> +/**
> >> + * @internal
> >> + * Set the IRQ count field of interrupt handle with user
> >> + * provided IRQ count value.
> >
> > I am intrigued by this new notion.
> > We already have different sizes in the intr_handle, why do we need a new one?
> >
> > Plus, in the cdx patch using this new API, I see that an fd array is
> > filled based on nb_efd.
> > So it seems to me that this new irq_count is just a duplicate of nb_efd.
> 
> The API rte_intr_efd_enable() sets the nb_efd and max_intr to values
> provided by the caller (+ NB_OTHER_INTR for max_intr). These values are
> dependent on the number of interrupts which are enabled by any DPDK
> driver rather than the actual number of interrupts supported or provided
> by the Linux driver.
> 
> With CDX bus the devices can have interrupts which are not really bound
> by the number of queues, and as these devices are programmable (FPGA
> based), users are free to implement the interrupts as per the need. I
> need to provide the total number of interrupts supported by the device
> to the drivers.
> 
> nb_intr is also there, which is default initialized by
> RTE_MAX_RXTX_INTR_VEC_ID, due to which this does not seem to be best
> fit. Though as per the meaning it shall represent the total number of
> interrupts supported by the device. Maybe we can remove the default
> value and use this as a total interrupt count supported for the device?

On more look nb_intr seems to be more aligned towards the number of efd's
allocated, rather than the total number of interrupts supported on the device.
So, it should have a default value, but yes then I am not sure it this can be used
to represent total number of interrupt count. Please let me know if your views
on this.

Regards,
Nipun