[PATCH] lib/dmadev: get DMA device using device ID

[PATCH] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

DMA library has a function to get DMA device based on device name but
there is no function to get DMA device using device id.

Added a function that lookup for the dma device using device id and
returns the pointer to the same.

Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
---
 lib/dmadev/rte_dmadev.c     |  9 +++++++++
 lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
 lib/dmadev/version.map      |  1 +
 3 files changed, 24 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4e5e420c82..83f49e77f2 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -397,6 +397,15 @@ rte_dma_is_valid(int16_t dev_id)
 		rte_dma_devices[dev_id].state != RTE_DMA_DEV_UNUSED;
 }
 
+struct rte_dma_dev *
+rte_dma_pmd_get_dev_by_id(const int dev_id)
+{
+	if (!rte_dma_is_valid(dev_id))
+		return NULL;
+
+	return &rte_dma_devices[dev_id];
+}
+
 uint16_t
 rte_dma_count_avail(void)
 {
diff --git a/lib/dmadev/rte_dmadev_pmd.h b/lib/dmadev/rte_dmadev_pmd.h
index c61cedfb23..f68c3ac6aa 100644
--- a/lib/dmadev/rte_dmadev_pmd.h
+++ b/lib/dmadev/rte_dmadev_pmd.h
@@ -167,6 +167,20 @@ struct rte_dma_dev *rte_dma_pmd_allocate(const char *name, int numa_node,
 __rte_internal
 int rte_dma_pmd_release(const char *name);
 
+/**
+ * @internal
+ * Get the rte_dma_dev structure device pointer for the device id.
+ *
+ * @param dev_id
+ *   Device ID value to select the device structure.
+ *
+ * @return
+ *   - rte_dma_dev structure pointer for the given device ID on success, NULL
+ *   otherwise.
+ */
+__rte_internal
+struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
index 2a3736514c..2cd57c43d3 100644
--- a/lib/dmadev/version.map
+++ b/lib/dmadev/version.map
@@ -26,6 +26,7 @@ INTERNAL {
 	rte_dma_fp_objs;
 	rte_dma_pmd_allocate;
 	rte_dma_pmd_release;
+	rte_dma_pmd_get_dev_by_id;
 
 	local: *;
 };
-- 
2.25.1

Re: [PATCH] lib/dmadev: get DMA device using device ID

[fengchengwen]

Hi Amit,

On 2023/12/8 15:55, Amit Prakash Shukla wrote:
> DMA library has a function to get DMA device based on device name but
> there is no function to get DMA device using device id.
> 
> Added a function that lookup for the dma device using device id and
> returns the pointer to the same.
> 
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> ---
>  lib/dmadev/rte_dmadev.c     |  9 +++++++++
>  lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
>  lib/dmadev/version.map      |  1 +
>  3 files changed, 24 insertions(+)
> 
> diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
> index 4e5e420c82..83f49e77f2 100644
> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c
> @@ -397,6 +397,15 @@ rte_dma_is_valid(int16_t dev_id)
>  		rte_dma_devices[dev_id].state != RTE_DMA_DEV_UNUSED;
>  }
>  
> +struct rte_dma_dev *
> +rte_dma_pmd_get_dev_by_id(const int dev_id)
> +{
> +	if (!rte_dma_is_valid(dev_id))
> +		return NULL;
> +
> +	return &rte_dma_devices[dev_id];
> +}
> +
>  uint16_t
>  rte_dma_count_avail(void)
>  {
> diff --git a/lib/dmadev/rte_dmadev_pmd.h b/lib/dmadev/rte_dmadev_pmd.h
> index c61cedfb23..f68c3ac6aa 100644
> --- a/lib/dmadev/rte_dmadev_pmd.h
> +++ b/lib/dmadev/rte_dmadev_pmd.h
> @@ -167,6 +167,20 @@ struct rte_dma_dev *rte_dma_pmd_allocate(const char *name, int numa_node,
>  __rte_internal
>  int rte_dma_pmd_release(const char *name);
>  
> +/**
> + * @internal
> + * Get the rte_dma_dev structure device pointer for the device id.
> + *
> + * @param dev_id
> + *   Device ID value to select the device structure.
> + *
> + * @return
> + *   - rte_dma_dev structure pointer for the given device ID on success, NULL
> + *   otherwise.
> + */
> +__rte_internal
> +struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
> +
>  #ifdef __cplusplus
>  }
>  #endif
> diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
> index 2a3736514c..2cd57c43d3 100644
> --- a/lib/dmadev/version.map
> +++ b/lib/dmadev/version.map
> @@ -26,6 +26,7 @@ INTERNAL {
>  	rte_dma_fp_objs;
>  	rte_dma_pmd_allocate;
>  	rte_dma_pmd_release;
> +	rte_dma_pmd_get_dev_by_id;

Should arranged alphabetically in version.map

With above fixed
Acked-by: Chengwen Feng <fengchengwen@huawei.com>

Thanks

>  
>  	local: *;
>  };
> 

Re: [PATCH] lib/dmadev: get DMA device using device ID

[Bruce Richardson]

On Fri, Dec 08, 2023 at 01:25:25PM +0530, Amit Prakash Shukla wrote:
> DMA library has a function to get DMA device based on device name but
> there is no function to get DMA device using device id.
> 
> Added a function that lookup for the dma device using device id and
> returns the pointer to the same.
> 
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> ---
>  lib/dmadev/rte_dmadev.c     |  9 +++++++++
>  lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
>  lib/dmadev/version.map      |  1 +
>  3 files changed, 24 insertions(+)
> 
What is the use-case for these functions? With the dmadev library
abstraction, other libs and apps should never need a pointer to an dmadev
struct.

RE: [EXT] Re: [PATCH] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

Hi Chengwen,

Thanks for the review and feedback. I will send the next version with suggested changes.

Thanks,
Amit Shukla

> -----Original Message-----
> From: fengchengwen <fengchengwen@huawei.com>
> Sent: Saturday, December 9, 2023 12:42 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Vamsi
> Krishna Attunuru <vattunuru@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; Anoob Joseph <anoobj@marvell.com>;
> mb@smartsharesystems.com
> Subject: [EXT] Re: [PATCH] lib/dmadev: get DMA device using device ID
> 
> External Email
> 
> ----------------------------------------------------------------------
> Hi Amit,
> 
> On 2023/12/8 15:55, Amit Prakash Shukla wrote:
> > DMA library has a function to get DMA device based on device name but
> > there is no function to get DMA device using device id.
> >
> > Added a function that lookup for the dma device using device id and
> > returns the pointer to the same.
> >
> > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> > ---
> >  lib/dmadev/rte_dmadev.c     |  9 +++++++++
> >  lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
> >  lib/dmadev/version.map      |  1 +
> >  3 files changed, 24 insertions(+)
> >
> > diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c index
> > 4e5e420c82..83f49e77f2 100644
> > --- a/lib/dmadev/rte_dmadev.c
> > +++ b/lib/dmadev/rte_dmadev.c
> > @@ -397,6 +397,15 @@ rte_dma_is_valid(int16_t dev_id)
> >  		rte_dma_devices[dev_id].state != RTE_DMA_DEV_UNUSED;  }
> >
> > +struct rte_dma_dev *
> > +rte_dma_pmd_get_dev_by_id(const int dev_id) {
> > +	if (!rte_dma_is_valid(dev_id))
> > +		return NULL;
> > +
> > +	return &rte_dma_devices[dev_id];
> > +}
> > +
> >  uint16_t
> >  rte_dma_count_avail(void)
> >  {
> > diff --git a/lib/dmadev/rte_dmadev_pmd.h
> b/lib/dmadev/rte_dmadev_pmd.h
> > index c61cedfb23..f68c3ac6aa 100644
> > --- a/lib/dmadev/rte_dmadev_pmd.h
> > +++ b/lib/dmadev/rte_dmadev_pmd.h
> > @@ -167,6 +167,20 @@ struct rte_dma_dev
> *rte_dma_pmd_allocate(const
> > char *name, int numa_node,  __rte_internal  int
> > rte_dma_pmd_release(const char *name);
> >
> > +/**
> > + * @internal
> > + * Get the rte_dma_dev structure device pointer for the device id.
> > + *
> > + * @param dev_id
> > + *   Device ID value to select the device structure.
> > + *
> > + * @return
> > + *   - rte_dma_dev structure pointer for the given device ID on success,
> NULL
> > + *   otherwise.
> > + */
> > +__rte_internal
> > +struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
> > +
> >  #ifdef __cplusplus
> >  }
> >  #endif
> > diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map index
> > 2a3736514c..2cd57c43d3 100644
> > --- a/lib/dmadev/version.map
> > +++ b/lib/dmadev/version.map
> > @@ -26,6 +26,7 @@ INTERNAL {
> >  	rte_dma_fp_objs;
> >  	rte_dma_pmd_allocate;
> >  	rte_dma_pmd_release;
> > +	rte_dma_pmd_get_dev_by_id;
> 
> Should arranged alphabetically in version.map
> 
> With above fixed
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> 
> Thanks
> 
> >
> >  	local: *;
> >  };
> >

RE: [EXT] Re: [PATCH] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

Hi Bruce,

Thanks for the review and feedback. Please find my reply in-line.

Thanks,
Amit Shukla

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday, December 11, 2023 3:54 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>
> Cc: Chengwen Feng <fengchengwen@huawei.com>; Kevin Laatz
> <kevin.laatz@intel.com>; dev@dpdk.org; Jerin Jacob Kollanukkaran
> <jerinj@marvell.com>; Vamsi Krishna Attunuru <vattunuru@marvell.com>;
> Nithin Kumar Dabilpuram <ndabilpuram@marvell.com>; Anoob Joseph
> <anoobj@marvell.com>; mb@smartsharesystems.com
> Subject: [EXT] Re: [PATCH] lib/dmadev: get DMA device using device ID
> 
> External Email
> 
> ----------------------------------------------------------------------
> On Fri, Dec 08, 2023 at 01:25:25PM +0530, Amit Prakash Shukla wrote:
> > DMA library has a function to get DMA device based on device name but
> > there is no function to get DMA device using device id.
> >
> > Added a function that lookup for the dma device using device id and
> > returns the pointer to the same.
> >
> > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> > ---
> >  lib/dmadev/rte_dmadev.c     |  9 +++++++++
> >  lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
> >  lib/dmadev/version.map      |  1 +
> >  3 files changed, 24 insertions(+)
> >
> What is the use-case for these functions? With the dmadev library
> abstraction, other libs and apps should never need a pointer to an dmadev
> struct.
[Amit]: The dmadev struct pointer is needed at driver layer to verify certain underlying device parameters which cannot be done as part of dma lib. This api is been used in the patch: https://patches.dpdk.org/project/dpdk/patch/20231208082835.2817601-3-amitprakashs@marvell.com/

[PATCH v2] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

DMA library has a function to get DMA device based on device name but
there is no function to get DMA device using device id.

Added a function that lookup for the dma device using device id and
returns the pointer to the same.

Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
Acked-by: Chengwen Feng <fengchengwen@huawei.com>
---
v2:
  - Arranged api in alphabetical order in version.map

 lib/dmadev/rte_dmadev.c     |  9 +++++++++
 lib/dmadev/rte_dmadev_pmd.h | 14 ++++++++++++++
 lib/dmadev/version.map      |  1 +
 3 files changed, 24 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 4e5e420c82..83f49e77f2 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -397,6 +397,15 @@ rte_dma_is_valid(int16_t dev_id)
 		rte_dma_devices[dev_id].state != RTE_DMA_DEV_UNUSED;
 }
 
+struct rte_dma_dev *
+rte_dma_pmd_get_dev_by_id(const int dev_id)
+{
+	if (!rte_dma_is_valid(dev_id))
+		return NULL;
+
+	return &rte_dma_devices[dev_id];
+}
+
 uint16_t
 rte_dma_count_avail(void)
 {
diff --git a/lib/dmadev/rte_dmadev_pmd.h b/lib/dmadev/rte_dmadev_pmd.h
index c61cedfb23..f68c3ac6aa 100644
--- a/lib/dmadev/rte_dmadev_pmd.h
+++ b/lib/dmadev/rte_dmadev_pmd.h
@@ -167,6 +167,20 @@ struct rte_dma_dev *rte_dma_pmd_allocate(const char *name, int numa_node,
 __rte_internal
 int rte_dma_pmd_release(const char *name);
 
+/**
+ * @internal
+ * Get the rte_dma_dev structure device pointer for the device id.
+ *
+ * @param dev_id
+ *   Device ID value to select the device structure.
+ *
+ * @return
+ *   - rte_dma_dev structure pointer for the given device ID on success, NULL
+ *   otherwise.
+ */
+__rte_internal
+struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/dmadev/version.map b/lib/dmadev/version.map
index 2a3736514c..046dbfa988 100644
--- a/lib/dmadev/version.map
+++ b/lib/dmadev/version.map
@@ -25,6 +25,7 @@ INTERNAL {
 
 	rte_dma_fp_objs;
 	rte_dma_pmd_allocate;
+	rte_dma_pmd_get_dev_by_id;
 	rte_dma_pmd_release;
 
 	local: *;
-- 
2.17.1

RE: [PATCH v2] lib/dmadev: get DMA device using device ID

[Anoob Joseph]

> 
> DMA library has a function to get DMA device based on device name but
> there is no function to get DMA device using device id.
> 
> Added a function that lookup for the dma device using device id and returns
> the pointer to the same.
> 
> Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> Acked-by: Chengwen Feng <fengchengwen@huawei.com>

Acked-by: Anoob Joseph <anoobj@marvell.com>

RE: [PATCH v2] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

Hi Thomas,

Gentle ping.

Could you please consider merging this patch. The driver series https://patches.dpdk.org/project/dpdk/patch/20231208082835.2817601-3-amitprakashs@marvell.com/ is dependent on this patch.

Thanks,
Amit Shukla

> -----Original Message-----
> From: Anoob Joseph <anoobj@marvell.com>
> Sent: Monday, January 8, 2024 8:17 PM
> To: Amit Prakash Shukla <amitprakashs@marvell.com>; Chengwen Feng
> <fengchengwen@huawei.com>; Kevin Laatz <kevin.laatz@intel.com>; Bruce
> Richardson <bruce.richardson@intel.com>
> Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>; Vamsi
> Krishna Attunuru <vattunuru@marvell.com>; Nithin Kumar Dabilpuram
> <ndabilpuram@marvell.com>; mb@smartsharesystems.com; Amit Prakash
> Shukla <amitprakashs@marvell.com>
> Subject: RE: [PATCH v2] lib/dmadev: get DMA device using device ID
> 
> >
> > DMA library has a function to get DMA device based on device name but
> > there is no function to get DMA device using device id.
> >
> > Added a function that lookup for the dma device using device id and
> > returns the pointer to the same.
> >
> > Signed-off-by: Amit Prakash Shukla <amitprakashs@marvell.com>
> > Acked-by: Chengwen Feng <fengchengwen@huawei.com>
> 
> Acked-by: Anoob Joseph <anoobj@marvell.com>
> 

Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[Thomas Monjalon]

19/12/2023 12:00, Amit Prakash Shukla:
> +struct rte_dma_dev *
> +rte_dma_pmd_get_dev_by_id(const int dev_id)

const does not make sense here for an int parameter.

> +{
> +	if (!rte_dma_is_valid(dev_id))
> +		return NULL;
> +
> +	return &rte_dma_devices[dev_id];
> +}
[...]
> +/**
> + * @internal
> + * Get the rte_dma_dev structure device pointer for the device id.
> + *
> + * @param dev_id
> + *   Device ID value to select the device structure.

This comment is not explanatory.
What is an ID? Where does it come from?
Where can we see such ID for DMA device?

> + *
> + * @return
> + *   - rte_dma_dev structure pointer for the given device ID on success, NULL
> + *   otherwise.
> + */
> +__rte_internal
> +struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);

Again, const does not make sense here.

Chengwen, please can you comment this patch as you maintain dmadev?

Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[fengchengwen]

Hi Thomas,


On 2024/2/9 0:25, Thomas Monjalon wrote:
> 19/12/2023 12:00, Amit Prakash Shukla:
>> +struct rte_dma_dev *
>> +rte_dma_pmd_get_dev_by_id(const int dev_id)
> const does not make sense here for an int parameter.


I think it could be OK with const even if the parameter is not pointer.

However, most DPDK APIs do not have const for simple types (e.g. 
int/uin16_t).

In this aspect, I think it's also OK to remove const of here for 
consistency.


>
>> +{
>> +	if (!rte_dma_is_valid(dev_id))
>> +		return NULL;
>> +
>> +	return &rte_dma_devices[dev_id];
>> +}
> [...]
>> +/**
>> + * @internal
>> + * Get the rte_dma_dev structure device pointer for the device id.
>> + *
>> + * @param dev_id
>> + *   Device ID value to select the device structure.
> This comment is not explanatory.
> What is an ID? Where does it come from?
> Where can we see such ID for DMA device?

This new API is used in the event-dma driver of cnxk [1]:

The rte_event_dma_adapter_vchan_add has parameter of dma_dev_id, and it then

invoke (*dev->dev_ops->dma_adapter_vchan_add)(dev, dma_dev_id, vchan, 
event),

at cnxk driver, this ops will check whether the DMA is 
cnxk_dmadev_pci_driver.

I think this is because the cnxk's event-and-dma implement has deep coupling

(because the cnxk's event device could interact with another vendor's 
dma device).


Maybe we should think of a better way to solve this kind of coupling 
problem.


Thanks


[1] 
https://patches.dpdk.org/project/dpdk/patch/20231208082835.2817601-3-amitprakashs@marvell.com/

>
>> + *
>> + * @return
>> + *   - rte_dma_dev structure pointer for the given device ID on success, NULL
>> + *   otherwise.
>> + */
>> +__rte_internal
>> +struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
> Again, const does not make sense here.
>
> Chengwen, please can you comment this patch as you maintain dmadev?
>
>

RE: [EXT] Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

Hi Thomas and Chengwen,

Thank you for the review and feedback. Please find my comment in-line.

Thanks,
Amit Shukla

> ----------------------------------------------------------------------
> Hi Thomas,
> 
> 
> On 2024/2/9 0:25, Thomas Monjalon wrote:
> > 19/12/2023 12:00, Amit Prakash Shukla:
> >> +struct rte_dma_dev *
> >> +rte_dma_pmd_get_dev_by_id(const int dev_id)
> > const does not make sense here for an int parameter.
> 
> 
> I think it could be OK with const even if the parameter is not pointer.
> 
> However, most DPDK APIs do not have const for simple types (e.g.
> int/uin16_t).
> 
> In this aspect, I think it's also OK to remove const of here for consistency.
> 
> 
> >
> >> +{
> >> +	if (!rte_dma_is_valid(dev_id))
> >> +		return NULL;
> >> +
> >> +	return &rte_dma_devices[dev_id];
> >> +}
> > [...]
> >> +/**
> >> + * @internal
> >> + * Get the rte_dma_dev structure device pointer for the device id.
> >> + *
> >> + * @param dev_id
> >> + *   Device ID value to select the device structure.
> > This comment is not explanatory.
> > What is an ID? Where does it come from?
> > Where can we see such ID for DMA device?
> 
> This new API is used in the event-dma driver of cnxk [1]:
> 
> The rte_event_dma_adapter_vchan_add has parameter of dma_dev_id, and it
> then
> 
> invoke (*dev->dev_ops->dma_adapter_vchan_add)(dev, dma_dev_id, vchan,
> event),
> 
> at cnxk driver, this ops will check whether the DMA is
> cnxk_dmadev_pci_driver.
> 
> I think this is because the cnxk's event-and-dma implement has deep coupling
> 
> (because the cnxk's event device could interact with another vendor's
> dma device).
> 
> 
> Maybe we should think of a better way to solve this kind of coupling
> problem.

Id, is the DMA dev id which is used in looking up DMA dev. This API is in-line with the other libraries.
Crypto library has an api rte_cryptodev_pmd_get_dev to get crypto device based on device id.

> 
> 
> Thanks
> 
> 
> [1]
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__patches.dpdk.org_project_dpdk_patch_20231208082835.2817601-
> 2D3-2Damitprakashs-
> 40marvell.com_&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=ALGdXl3fZgF
> GR69VnJLdSnADun7zLaXG1p5Rs7pXihE&m=b0t1Tduh89vXiJ7GQG0eb_yIMN
> k2aEkmL2losNL5qiqNycAqWUi7kLJRmmOunpvy&s=oy9mqDXsDKhXuVG9iy7
> SLmU_shlYhfjrglvjEoEQ4AM&e=
> 
> >
> >> + *
> >> + * @return
> >> + *   - rte_dma_dev structure pointer for the given device ID on success,
> NULL
> >> + *   otherwise.
> >> + */
> >> +__rte_internal
> >> +struct rte_dma_dev *rte_dma_pmd_get_dev_by_id(const int dev_id);
> > Again, const does not make sense here.
> >
> > Chengwen, please can you comment this patch as you maintain dmadev?
> >
> >

Re: [EXT] Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[Thomas Monjalon]

09/02/2024 12:05, Amit Prakash Shukla:
> > On 2024/2/9 0:25, Thomas Monjalon wrote:
> > > 19/12/2023 12:00, Amit Prakash Shukla:
> > >> +struct rte_dma_dev *
> > >> +rte_dma_pmd_get_dev_by_id(const int dev_id)
> > > const does not make sense here for an int parameter.
> > 
> > 
> > I think it could be OK with const even if the parameter is not pointer.
> > 
> > However, most DPDK APIs do not have const for simple types (e.g.
> > int/uin16_t).
> > 
> > In this aspect, I think it's also OK to remove const of here for consistency.
> > 
> > 
> > >
> > >> +{
> > >> +	if (!rte_dma_is_valid(dev_id))
> > >> +		return NULL;
> > >> +
> > >> +	return &rte_dma_devices[dev_id];
> > >> +}
> > > [...]
> > >> +/**
> > >> + * @internal
> > >> + * Get the rte_dma_dev structure device pointer for the device id.
> > >> + *
> > >> + * @param dev_id
> > >> + *   Device ID value to select the device structure.
> > > This comment is not explanatory.
> > > What is an ID? Where does it come from?
> > > Where can we see such ID for DMA device?
> > 
> > This new API is used in the event-dma driver of cnxk [1]:
> > 
> > The rte_event_dma_adapter_vchan_add has parameter of dma_dev_id, and it
> > then
> > 
> > invoke (*dev->dev_ops->dma_adapter_vchan_add)(dev, dma_dev_id, vchan,
> > event),
> > 
> > at cnxk driver, this ops will check whether the DMA is
> > cnxk_dmadev_pci_driver.
> > 
> > I think this is because the cnxk's event-and-dma implement has deep coupling
> > 
> > (because the cnxk's event device could interact with another vendor's
> > dma device).
> > 
> > 
> > Maybe we should think of a better way to solve this kind of coupling
> > problem.
> 
> Id, is the DMA dev id which is used in looking up DMA dev. This API is in-line with the other libraries.
> Crypto library has an api rte_cryptodev_pmd_get_dev to get crypto device based on device id.

OK I think I understand.
It is the library ID, the same as returned by
int rte_dma_get_dev_id_by_name(const char *name);

I can remove the const and apply if you are OK.
I would just change this comment:

+ * @param dev_id
+ *   Device ID value to select the device structure.

into

+ *   DMA device index in dmadev library.

RE: [EXT] Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[Amit Prakash Shukla]

<snip>
> > >
> > > invoke (*dev->dev_ops->dma_adapter_vchan_add)(dev, dma_dev_id,
> > > vchan, event),
> > >
> > > at cnxk driver, this ops will check whether the DMA is
> > > cnxk_dmadev_pci_driver.
> > >
> > > I think this is because the cnxk's event-and-dma implement has deep
> > > coupling
> > >
> > > (because the cnxk's event device could interact with another
> > > vendor's dma device).
> > >
> > >
> > > Maybe we should think of a better way to solve this kind of coupling
> > > problem.
> >
> > Id, is the DMA dev id which is used in looking up DMA dev. This API is in-line
> with the other libraries.
> > Crypto library has an api rte_cryptodev_pmd_get_dev to get crypto device
> based on device id.
> 
> OK I think I understand.
> It is the library ID, the same as returned by int
> rte_dma_get_dev_id_by_name(const char *name);
> 
> I can remove the const and apply if you are OK.

Sure, I am okay.

> I would just change this comment:
> 
> + * @param dev_id
> + *   Device ID value to select the device structure.
> 
> into
> 
> + *   DMA device index in dmadev library.

Sure.
Thanks.

Re: [EXT] Re: [PATCH v2] lib/dmadev: get DMA device using device ID

[Thomas Monjalon]

09/02/2024 12:30, Amit Prakash Shukla:
> 
> <snip>
> > > >
> > > > invoke (*dev->dev_ops->dma_adapter_vchan_add)(dev, dma_dev_id,
> > > > vchan, event),
> > > >
> > > > at cnxk driver, this ops will check whether the DMA is
> > > > cnxk_dmadev_pci_driver.
> > > >
> > > > I think this is because the cnxk's event-and-dma implement has deep
> > > > coupling
> > > >
> > > > (because the cnxk's event device could interact with another
> > > > vendor's dma device).
> > > >
> > > >
> > > > Maybe we should think of a better way to solve this kind of coupling
> > > > problem.
> > >
> > > Id, is the DMA dev id which is used in looking up DMA dev. This API is in-line
> > with the other libraries.
> > > Crypto library has an api rte_cryptodev_pmd_get_dev to get crypto device
> > based on device id.
> > 
> > OK I think I understand.
> > It is the library ID, the same as returned by int
> > rte_dma_get_dev_id_by_name(const char *name);
> > 
> > I can remove the const and apply if you are OK.
> 
> Sure, I am okay.
> 
> > I would just change this comment:
> > 
> > + * @param dev_id
> > + *   Device ID value to select the device structure.
> > 
> > into
> > 
> > + *   DMA device index in dmadev library.
> 
> Sure.
> Thanks.

I've also fixed the ID type to be int16_t.

Applied