> -----Original Message-----
> From: Gupta, Nipun
> Sent: Thursday, June 1, 2023 11:35 PM
> To: David Marchand <david.march...@redhat.com>
> Cc: dev@dpdk.org; tho...@monjalon.net; hka...@marvell.com;
> anatoly.bura...@intel.com; step...@networkplumber.org; Yigit, Ferruh
> <ferruh.yi...@amd.com>; Anand, Harpreet <harpreet.an...@amd.com>;
> Agarwal, Nikhil <nikhil.agar...@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.gu...@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.gu...@amd.com>
> >> Acked-by: Ferruh Yigit <ferruh.yi...@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
