[v23,2/6] dmadev: add control plane function support

  This patch add control plane functions for dmadev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>
Reviewed-by: Conor Walsh <conor.walsh@intel.com>
---
 doc/guides/prog_guide/dmadev.rst       |  41 +++
 doc/guides/rel_notes/release_21_11.rst |   1 +
 lib/dmadev/rte_dmadev.c                | 359 ++++++++++++++++++
 lib/dmadev/rte_dmadev.h                | 480 +++++++++++++++++++++++++
 lib/dmadev/rte_dmadev_core.h           |  62 +++-
 lib/dmadev/version.map                 |   9 +
 6 files changed, 951 insertions(+), 1 deletion(-)
  

Hi Chengwen

API looks good to me, thanks!

I have some questions below.

> This patch add control plane functions for dmadev.
> 
<snip>
> +/**
> + * DMA transfer direction defines.
> + *
> + * @see struct rte_dma_vchan_conf::direction  */ enum rte_dma_direction
> +{
> +       /** DMA transfer direction - from memory to memory.
> +        *
> +        * @see struct rte_dma_vchan_conf::direction
> +        */
> +       RTE_DMA_DIR_MEM_TO_MEM,
> +       /** DMA transfer direction - from memory to device.
> +        * In a typical scenario, the SoCs are installed on host servers as
> +        * iNICs through the PCIe interface. In this case, the SoCs works in
> +        * EP(endpoint) mode, it could initiate a DMA move request from
> memory
> +        * (which is SoCs memory) to device (which is host memory).
> +        *
> +        * @see struct rte_dma_vchan_conf::direction
> +        */
> +       RTE_DMA_DIR_MEM_TO_DEV,


I don't understand precisely the meaning of mem and dev.

What does it mean SoCs memory?

What does it mean host memory?

What is the memory HW in these two types?

How does the user get the addresses of SoCs memory?

How does the user get the addresses of host memory?


Can dpdk app here access physical memory not mapped\allocated to the app?

Matan



> +       /** DMA transfer direction - from device to memory.
> +        * In a typical scenario, the SoCs are installed on host servers as
> +        * iNICs through the PCIe interface. In this case, the SoCs works in
> +        * EP(endpoint) mode, it could initiate a DMA move request from device
> +        * (which is host memory) to memory (which is SoCs memory).
> +        *
> +        * @see struct rte_dma_vchan_conf::direction
> +        */
> +       RTE_DMA_DIR_DEV_TO_MEM,
> +       /** DMA transfer direction - from device to device.
> +        * In a typical scenario, the SoCs are installed on host servers as
> +        * iNICs through the PCIe interface. In this case, the SoCs works in
> +        * EP(endpoint) mode, it could initiate a DMA move request from device
> +        * (which is host memory) to the device (which is another host
> memory).
> +        *
> +        * @see struct rte_dma_vchan_conf::direction
> +        */
> +       RTE_DMA_DIR_DEV_TO_DEV,
> +};
> +
> +/**
> + * DMA access port type defines.
> + *
> + * @see struct rte_dma_port_param::port_type  */ enum
> rte_dma_port_type
> +{
> +       RTE_DMA_PORT_NONE,
> +       RTE_DMA_PORT_PCIE, /**< The DMA access port is PCIe. */ };
> +
> +/**
> + * A structure used to descript DMA access port parameters.
> + *
> + * @see struct rte_dma_vchan_conf::src_port
> + * @see struct rte_dma_vchan_conf::dst_port  */ struct
> +rte_dma_port_param {
> +       /** The device access port type.
> +        *
> +        * @see enum rte_dma_port_type
> +        */
> +       enum rte_dma_port_type port_type;
> +       union {
> +               /** PCIe access port parameters.
> +                *
> +                * The following model shows SoC's PCIe module connects to
> +                * multiple PCIe hosts and multiple endpoints. The PCIe module
> +                * has an integrated DMA controller.
> +                *
> +                * If the DMA wants to access the memory of host A, it can be
> +                * initiated by PF1 in core0, or by VF0 of PF0 in core0.
> +                *
> +                * \code{.unparsed}
> +                * System Bus
> +                *    |     ----------PCIe module----------
> +                *    |     Bus
> +                *    |     Interface
> +                *    |     -----        ------------------
> +                *    |     |   |        | PCIe Core0     |
> +                *    |     |   |        |                |        -----------
> +                *    |     |   |        |   PF-0 -- VF-0 |        | Host A  |
> +                *    |     |   |--------|        |- VF-1 |--------| Root    |
> +                *    |     |   |        |   PF-1         |        | Complex |
> +                *    |     |   |        |   PF-2         |        -----------
> +                *    |     |   |        ------------------
> +                *    |     |   |
> +                *    |     |   |        ------------------
> +                *    |     |   |        | PCIe Core1     |
> +                *    |     |   |        |                |        -----------
> +                *    |     |   |        |   PF-0 -- VF-0 |        | Host B  |
> +                *    |-----|   |--------|   PF-1 -- VF-0 |--------| Root    |
> +                *    |     |   |        |        |- VF-1 |        | Complex |
> +                *    |     |   |        |   PF-2         |        -----------
> +                *    |     |   |        ------------------
> +                *    |     |   |
> +                *    |     |   |        ------------------
> +                *    |     |DMA|        |                |        ------
> +                *    |     |   |        |                |--------| EP |
> +                *    |     |   |--------| PCIe Core2     |        ------
> +                *    |     |   |        |                |        ------
> +                *    |     |   |        |                |--------| EP |
> +                *    |     |   |        |                |        ------
> +                *    |     -----        ------------------
> +                *
> +                * \endcode
> +                *
> +                * @note If some fields can not be supported by the
> +                * hardware/driver, then the driver ignores those fields.
> +                * Please check driver-specific documentation for limitations
> +                * and capablites.
> +                */
> +               struct {
> +                       uint64_t coreid : 4; /**< PCIe core id used. */
> +                       uint64_t pfid : 8; /**< PF id used. */
> +                       uint64_t vfen : 1; /**< VF enable bit. */
> +                       uint64_t vfid : 16; /**< VF id used. */
> +                       /** The pasid filed in TLP packet. */
> +                       uint64_t pasid : 20;
> +                       /** The attributes filed in TLP packet. */
> +                       uint64_t attr : 3;
> +                       /** The processing hint filed in TLP packet. */
> +                       uint64_t ph : 2;
> +                       /** The steering tag filed in TLP packet. */
> +                       uint64_t st : 16;
> +               } pcie;
> +       };
> +       uint64_t reserved[2]; /**< Reserved for future fields. */ };
> +
> +/**
> + * A structure used to configure a virtual DMA channel.
> + *
> + * @see rte_dma_vchan_setup
> + */
> +struct rte_dma_vchan_conf {
> +       /** Transfer direction
> +        *
> +        * @see enum rte_dma_direction
> +        */
> +       enum rte_dma_direction direction;
> +       /** Number of descriptor for the virtual DMA channel */
> +       uint16_t nb_desc;
> +       /** 1) Used to describes the device access port parameter in the
> +        * device-to-memory transfer scenario.
> +        * 2) Used to describes the source device access port parameter in the
> +        * device-to-device transfer scenario.
> +        *
> +        * @see struct rte_dma_port_param
> +        */
> +       struct rte_dma_port_param src_port;
> +       /** 1) Used to describes the device access port parameter in the
> +        * memory-to-device transfer scenario.
> +        * 2) Used to describes the destination device access port parameter in
> +        * the device-to-device transfer scenario.
> +        *
> +        * @see struct rte_dma_port_param
> +        */
> +       struct rte_dma_port_param dst_port; };
> +
<snip>
  

24/09/2021 12:53, Chengwen Feng:
> --- a/doc/guides/prog_guide/dmadev.rst
> +++ b/doc/guides/prog_guide/dmadev.rst
> @@ -62,3 +62,44 @@ identifiers:
>  
>  - A device name used to designate the DMA device in console messages, for
>    administration or debugging purposes.
> +
> +
> +Device Configuration
> +~~~~~~~~~~~~~~~~~~~~
> +
> +The rte_dma_configure API is used to configure a DMA device.
> +
> +.. code-block:: c
> +
> +   int rte_dma_configure(int16_t dev_id,
> +                         const struct rte_dma_conf *dev_conf);
> +
> +The ``rte_dma_conf`` structure is used to pass the configuration parameters
> +for the DMA device for example the number of virtual DMA channels to set up,
> +indication of whether to enable silent mode.
> +
> +
> +Configuration of Virtual DMA Channels
> +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> +
> +The rte_dma_vchan_setup API is used to configure a virtual DMA channel.
> +
> +.. code-block:: c
> +
> +   int rte_dma_vchan_setup(int16_t dev_id, uint16_t vchan,
> +                           const struct rte_dma_vchan_conf *conf);
> +
> +The ``rte_dma_vchan_conf`` structure is used to pass the configuration
> +parameters for the virtual DMA channel for example transfer direction, number of
> +descriptor for the virtual DMA channel, source device access port parameter,
> +destination device access port parameter.

You should avoid being redundant with the Doxygen documentation.
In the guide, it should be only explaining the concepts, not the details.
For the details of each function, we refer to Doxygen.


> --- a/lib/dmadev/rte_dmadev.c
> +++ b/lib/dmadev/rte_dmadev.c
> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name)
>  	if (dev == NULL)
>  		return -EINVAL;
>  
> +	if (dev->state == RTE_DMA_DEV_READY)
> +		return rte_dma_close(dev->dev_id);

What is the logic here?
The only exposed function should be rte_dma_close()
and it should call the freeing function.
The API should use the dev_id. As you said somewhere else,
the name is only for debugging.
Please remove the function rte_dma_pmd_release(const char *name).

[...]
> --- a/lib/dmadev/rte_dmadev.h
> +++ b/lib/dmadev/rte_dmadev.h
> + * The functions exported by the dmadev API to setup a device designated by its
> + * device identifier must be invoked in the following order:
> + *     - rte_dma_configure()
> + *     - rte_dma_vchan_setup()
> + *     - rte_dma_start()
> + *
> + * If the application wants to change the configuration (i.e. invoke
> + * rte_dma_configure() or rte_dma_vchan_setup()), it must invoke
> + * rte_dma_stop() first to stop the device and then do the reconfiguration
> + * before invoking rte_dma_start() again. The dataplane functions should not
> + * be invoked when the device is stopped.
> + *
> + * Finally, an application can close a dmadev by invoking the rte_dma_close()
> + * function.

Yes rte_dma_close, not rte_dma_pmd_release.

> + *
> + * About MT-safe, all the functions of the dmadev API exported by a PMD are

API is not exported by a PMD, but implemented.

> + * lock-free functions which assume to not be invoked in parallel on different
> + * logical cores to work on the same target dmadev object.
> + * @note Different virtual DMA channels on the same dmadev *DO NOT* support
> + * parallel invocation because these virtual DMA channels share the same
> + * HW-DMA-channel.
> + *
>   */

No need of final blank line in a comment.

> +/** DMA device support memory-to-memory transfer.
> + *
> + * @see struct rte_dma_info::dev_capa
> + */
> +#define RTE_DMA_CAPA_MEM_TO_MEM		RTE_BIT64(0)
> +/** DMA device support memory-to-device transfer.
> + *
> + * @see struct rte_dma_info::dev_capa
> + */
> +#define RTE_DMA_CAPA_MEM_TO_DEV		RTE_BIT64(1)

Same comment as in earlier version: please group the flags
in a doxygen group. Example of doxygen group:
https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/

[...]
You are using uint64_t bitfields and anonymous union in below struct,
it may not compile if not using __extension__ from RTE_STD_C11.

> +struct rte_dma_port_param {
> +	/** The device access port type.
> +	 *
> +	 * @see enum rte_dma_port_type
> +	 */
> +	enum rte_dma_port_type port_type;
> +	union {
[...]
> +		struct {
> +			uint64_t coreid : 4; /**< PCIe core id used. */
> +			uint64_t pfid : 8; /**< PF id used. */
> +			uint64_t vfen : 1; /**< VF enable bit. */
> +			uint64_t vfid : 16; /**< VF id used. */
> +			/** The pasid filed in TLP packet. */
> +			uint64_t pasid : 20;
> +			/** The attributes filed in TLP packet. */
> +			uint64_t attr : 3;
> +			/** The processing hint filed in TLP packet. */
> +			uint64_t ph : 2;
> +			/** The steering tag filed in TLP packet. */
> +			uint64_t st : 16;
> +		} pcie;
> +	};
> +	uint64_t reserved[2]; /**< Reserved for future fields. */
> +};

> --- a/lib/dmadev/rte_dmadev_core.h
> +++ b/lib/dmadev/rte_dmadev_core.h
> +/** @internal Used to get device information of a device. */
> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev,
> +				  struct rte_dma_info *dev_info,
> +				  uint32_t info_sz);

Please move all driver interfaces in a file dedicated to drivers.

[...]
> @@ -40,9 +96,13 @@ struct rte_dma_dev {
>  	int16_t dev_id; /**< Device [external] identifier. */
>  	int16_t numa_node; /**< Local NUMA memory ID. -1 if unknown. */
>  	void *dev_private; /**< PMD-specific private data. */
> +	/** Functions exported by PMD. */

s/exported/implemented/

> +	const struct rte_dma_dev_ops *dev_ops;
> +	struct rte_dma_conf dev_conf; /**< DMA device configuration. */
>  	/** Device info which supplied during device initialization. */
>  	struct rte_device *device;
>  	enum rte_dma_dev_state state; /**< Flag indicating the device state. */
> +	uint8_t dev_started : 1; /**< Device state: STARTED(1)/STOPPED(0). */
>  	uint64_t reserved[2]; /**< Reserved for future fields. */
>  } __rte_cache_aligned;
  

On 2021/10/5 18:16, Matan Azrad wrote:
> Hi Chengwen
> 
> API looks good to me, thanks!
> 
> I have some questions below.
> 
>> This patch add control plane functions for dmadev.
>>
> <snip>
>> +/**
>> + * DMA transfer direction defines.
>> + *
>> + * @see struct rte_dma_vchan_conf::direction  */ enum rte_dma_direction
>> +{
>> +       /** DMA transfer direction - from memory to memory.
>> +        *
>> +        * @see struct rte_dma_vchan_conf::direction
>> +        */
>> +       RTE_DMA_DIR_MEM_TO_MEM,
>> +       /** DMA transfer direction - from memory to device.
>> +        * In a typical scenario, the SoCs are installed on host servers as
>> +        * iNICs through the PCIe interface. In this case, the SoCs works in
>> +        * EP(endpoint) mode, it could initiate a DMA move request from
>> memory
>> +        * (which is SoCs memory) to device (which is host memory).
>> +        *
>> +        * @see struct rte_dma_vchan_conf::direction
>> +        */
>> +       RTE_DMA_DIR_MEM_TO_DEV,
> 
> 
> I don't understand precisely the meaning of mem and dev.
> 
> What does it mean SoCs memory?
> 
> What does it mean host memory?
> 
> What is the memory HW in these two types?
> 
> How does the user get the addresses of SoCs memory?
> 
> How does the user get the addresses of host memory?
> 

Hi Matan,

	System Bus
	    |     ----------PCIe module----------
	    |     Bus
	    |     Interface
	    |     -----        ------------------
	    |     |   |        | PCIe Core0     |
 DDR3 ------|     |   |        |                |        -----------
	    |     |   |        |   PF-0 -- VF-0 |        | Host A  |
	    |     |   |--------|        |- VF-1 |--------| Root    | ---DDR1
	    |     |   |        |   PF-1         |        | Complex |
	    |     |   |        |   PF-2         |        -----------
	    |     |   |        ------------------
	    |     |DMA|
	    |     |   |        ------------------
	    |     |   |        | PCIe Core1     |
	    |     |   |        |                |        -----------
	    |     |   |        |   PF-0 -- VF-0 |        | Host B  |
	    |-----|   |--------|   PF-1 -- VF-0 |--------| Root    | ---DDR2
	    |     |   |        |        |- VF-1 |        | Complex |
	    |     |   |        |   PF-2         |        -----------
	    |     |   |        ------------------
	    |     -----

---------------------SOC-------------------------        -------HOST--------

As shown in the above figure, the SOC is connected to two hosts.
The DDR3 is the SOC's memory, the DDR1 is HOST-A's memory, and DDR2 is HOST-B's memory.

To access DDR3 memory, APP could mmap and pass its IOVA address to DMA.
To access DDR1/2 memory, some devices use parameterized descriptor which contain the
function from which the request is sent and the destination address. In this case, it is
up to the user to confirm the function and destination address which could passed in as
parameters.

Thanks.

> 
> Can dpdk app here access physical memory not mapped\allocated to the app?
> 
> Matan
> 
> 
> 
>> +       /** DMA transfer direction - from device to memory.
>> +        * In a typical scenario, the SoCs are installed on host servers as
>> +        * iNICs through the PCIe interface. In this case, the SoCs works in
>> +        * EP(endpoint) mode, it could initiate a DMA move request from device
>> +        * (which is host memory) to memory (which is SoCs memory).
>> +        *
>> +        * @see struct rte_dma_vchan_conf::direction
>> +        */
>> +       RTE_DMA_DIR_DEV_TO_MEM,
>> +       /** DMA transfer direction - from device to device.
>> +        * In a typical scenario, the SoCs are installed on host servers as
>> +        * iNICs through the PCIe interface. In this case, the SoCs works in
>> +        * EP(endpoint) mode, it could initiate a DMA move request from device
>> +        * (which is host memory) to the device (which is another host
>> memory).
>> +        *
>> +        * @see struct rte_dma_vchan_conf::direction
>> +        */
>> +       RTE_DMA_DIR_DEV_TO_DEV,
>> +};
>> +
>> +/**
>> + * DMA access port type defines.
>> + *
>> + * @see struct rte_dma_port_param::port_type  */ enum
>> rte_dma_port_type
>> +{
>> +       RTE_DMA_PORT_NONE,
>> +       RTE_DMA_PORT_PCIE, /**< The DMA access port is PCIe. */ };
>> +
>> +/**
>> + * A structure used to descript DMA access port parameters.
>> + *
>> + * @see struct rte_dma_vchan_conf::src_port
>> + * @see struct rte_dma_vchan_conf::dst_port  */ struct
>> +rte_dma_port_param {
>> +       /** The device access port type.
>> +        *
>> +        * @see enum rte_dma_port_type
>> +        */
>> +       enum rte_dma_port_type port_type;
>> +       union {
>> +               /** PCIe access port parameters.
>> +                *
>> +                * The following model shows SoC's PCIe module connects to
>> +                * multiple PCIe hosts and multiple endpoints. The PCIe module
>> +                * has an integrated DMA controller.
>> +                *
>> +                * If the DMA wants to access the memory of host A, it can be
>> +                * initiated by PF1 in core0, or by VF0 of PF0 in core0.
>> +                *
>> +                * \code{.unparsed}
>> +                * System Bus
>> +                *    |     ----------PCIe module----------
>> +                *    |     Bus
>> +                *    |     Interface
>> +                *    |     -----        ------------------
>> +                *    |     |   |        | PCIe Core0     |
>> +                *    |     |   |        |                |        -----------
>> +                *    |     |   |        |   PF-0 -- VF-0 |        | Host A  |
>> +                *    |     |   |--------|        |- VF-1 |--------| Root    |
>> +                *    |     |   |        |   PF-1         |        | Complex |
>> +                *    |     |   |        |   PF-2         |        -----------
>> +                *    |     |   |        ------------------
>> +                *    |     |   |
>> +                *    |     |   |        ------------------
>> +                *    |     |   |        | PCIe Core1     |
>> +                *    |     |   |        |                |        -----------
>> +                *    |     |   |        |   PF-0 -- VF-0 |        | Host B  |
>> +                *    |-----|   |--------|   PF-1 -- VF-0 |--------| Root    |
>> +                *    |     |   |        |        |- VF-1 |        | Complex |
>> +                *    |     |   |        |   PF-2         |        -----------
>> +                *    |     |   |        ------------------
>> +                *    |     |   |
>> +                *    |     |   |        ------------------
>> +                *    |     |DMA|        |                |        ------
>> +                *    |     |   |        |                |--------| EP |
>> +                *    |     |   |--------| PCIe Core2     |        ------
>> +                *    |     |   |        |                |        ------
>> +                *    |     |   |        |                |--------| EP |
>> +                *    |     |   |        |                |        ------
>> +                *    |     -----        ------------------
>> +                *
>> +                * \endcode
>> +                *
>> +                * @note If some fields can not be supported by the
>> +                * hardware/driver, then the driver ignores those fields.
>> +                * Please check driver-specific documentation for limitations
>> +                * and capablites.
>> +                */
>> +               struct {
>> +                       uint64_t coreid : 4; /**< PCIe core id used. */
>> +                       uint64_t pfid : 8; /**< PF id used. */
>> +                       uint64_t vfen : 1; /**< VF enable bit. */
>> +                       uint64_t vfid : 16; /**< VF id used. */
>> +                       /** The pasid filed in TLP packet. */
>> +                       uint64_t pasid : 20;
>> +                       /** The attributes filed in TLP packet. */
>> +                       uint64_t attr : 3;
>> +                       /** The processing hint filed in TLP packet. */
>> +                       uint64_t ph : 2;
>> +                       /** The steering tag filed in TLP packet. */
>> +                       uint64_t st : 16;
>> +               } pcie;
>> +       };
>> +       uint64_t reserved[2]; /**< Reserved for future fields. */ };
>> +
>> +/**
>> + * A structure used to configure a virtual DMA channel.
>> + *
>> + * @see rte_dma_vchan_setup
>> + */
>> +struct rte_dma_vchan_conf {
>> +       /** Transfer direction
>> +        *
>> +        * @see enum rte_dma_direction
>> +        */
>> +       enum rte_dma_direction direction;
>> +       /** Number of descriptor for the virtual DMA channel */
>> +       uint16_t nb_desc;
>> +       /** 1) Used to describes the device access port parameter in the
>> +        * device-to-memory transfer scenario.
>> +        * 2) Used to describes the source device access port parameter in the
>> +        * device-to-device transfer scenario.
>> +        *
>> +        * @see struct rte_dma_port_param
>> +        */
>> +       struct rte_dma_port_param src_port;
>> +       /** 1) Used to describes the device access port parameter in the
>> +        * memory-to-device transfer scenario.
>> +        * 2) Used to describes the destination device access port parameter in
>> +        * the device-to-device transfer scenario.
>> +        *
>> +        * @see struct rte_dma_port_param
>> +        */
>> +       struct rte_dma_port_param dst_port; };
>> +
> <snip>
>
  

On 2021/10/6 18:46, Thomas Monjalon wrote:
> 24/09/2021 12:53, Chengwen Feng:
>> --- a/doc/guides/prog_guide/dmadev.rst
>> +++ b/doc/guides/prog_guide/dmadev.rst
>> @@ -62,3 +62,44 @@ identifiers:
>>  
>>  - A device name used to designate the DMA device in console messages, for
>>    administration or debugging purposes.

[snip]

> 
>> --- a/lib/dmadev/rte_dmadev.c
>> +++ b/lib/dmadev/rte_dmadev.c
>> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name)
>>  	if (dev == NULL)
>>  		return -EINVAL;
>>  
>> +	if (dev->state == RTE_DMA_DEV_READY)
>> +		return rte_dma_close(dev->dev_id);
> 
> What is the logic here?
> The only exposed function should be rte_dma_close()
> and it should call the freeing function.
> The API should use the dev_id. As you said somewhere else,
> the name is only for debugging.
> Please remove the function rte_dma_pmd_release(const char *name).

The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter.

The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable
is used here. If the device is not ready, only resources need to be released. Otherwise, the close
interface of the driver is invoked.

For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to
implement two callbacks.

If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error
handling when PMD init, this can lead to conceptual inconsistencies because the initialization has
not been successful.

So I think it's better keep rte_dma_pmd_release.

> 
> [...]
>> --- a/lib/dmadev/rte_dmadev.h
>> +++ b/lib/dmadev/rte_dmadev.h
>> + * The functions exported by the dmadev API to setup a device designated by its
>> + * device identifier must be invoked in the following order:
>> + *     - rte_dma_configure()
>> + *     - rte_dma_vchan_setup()
>> + *     - rte_dma_start()
>> + *
>> + * If the application wants to change the configuration (i.e. invoke
>> + * rte_dma_configure() or rte_dma_vchan_setup()), it must invoke
>> + * rte_dma_stop() first to stop the device and then do the reconfiguration
>> + * before invoking rte_dma_start() again. The dataplane functions should not
>> + * be invoked when the device is stopped.
>> + *
>> + * Finally, an application can close a dmadev by invoking the rte_dma_close()
>> + * function.
> 
> Yes rte_dma_close, not rte_dma_pmd_release.
> 
>> + *
>> + * About MT-safe, all the functions of the dmadev API exported by a PMD are
> 
> API is not exported by a PMD, but implemented.
> 
>> + * lock-free functions which assume to not be invoked in parallel on different
>> + * logical cores to work on the same target dmadev object.
>> + * @note Different virtual DMA channels on the same dmadev *DO NOT* support
>> + * parallel invocation because these virtual DMA channels share the same
>> + * HW-DMA-channel.
>> + *
>>   */
> 
> No need of final blank line in a comment.
> 
>> +/** DMA device support memory-to-memory transfer.
>> + *
>> + * @see struct rte_dma_info::dev_capa
>> + */
>> +#define RTE_DMA_CAPA_MEM_TO_MEM		RTE_BIT64(0)
>> +/** DMA device support memory-to-device transfer.
>> + *
>> + * @see struct rte_dma_info::dev_capa
>> + */
>> +#define RTE_DMA_CAPA_MEM_TO_DEV		RTE_BIT64(1)
> 
> Same comment as in earlier version: please group the flags
> in a doxygen group. Example of doxygen group:
> https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/

Tried, but found it didn't coexist well with multi-line comments.

> 
> [...]
> You are using uint64_t bitfields and anonymous union in below struct,
> it may not compile if not using __extension__ from RTE_STD_C11.
> 
>> +struct rte_dma_port_param {
>> +	/** The device access port type.
>> +	 *
>> +	 * @see enum rte_dma_port_type
>> +	 */
>> +	enum rte_dma_port_type port_type;
>> +	union {
> [...]
>> +		struct {
>> +			uint64_t coreid : 4; /**< PCIe core id used. */
>> +			uint64_t pfid : 8; /**< PF id used. */
>> +			uint64_t vfen : 1; /**< VF enable bit. */
>> +			uint64_t vfid : 16; /**< VF id used. */
>> +			/** The pasid filed in TLP packet. */
>> +			uint64_t pasid : 20;
>> +			/** The attributes filed in TLP packet. */
>> +			uint64_t attr : 3;
>> +			/** The processing hint filed in TLP packet. */
>> +			uint64_t ph : 2;
>> +			/** The steering tag filed in TLP packet. */
>> +			uint64_t st : 16;
>> +		} pcie;
>> +	};
>> +	uint64_t reserved[2]; /**< Reserved for future fields. */
>> +};
> 
>> --- a/lib/dmadev/rte_dmadev_core.h
>> +++ b/lib/dmadev/rte_dmadev_core.h
>> +/** @internal Used to get device information of a device. */
>> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev,
>> +				  struct rte_dma_info *dev_info,
>> +				  uint32_t info_sz);
> 
> Please move all driver interfaces in a file dedicated to drivers.

There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h
And we build the following dependency:

                rte_dmadev.h   ---> rte_dmadev_core.h          // mainly because dataplane inline API.
                    ^
                    |
           ---------------------
           |                   |
       Application       rte_dmadev_pmd.h
                               ^
                               |
                             DMA PMD


If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional
dependency may exist, e.g.

                rte_dmadev.h   ---> rte_dmadev_core.h  ---> rte_dmadev_pmd.h
                    ^
                    |
           ---------------------
           |                   |
       Application       rte_dmadev_pmd.h
                               ^
                               |
                             DMA PMD

So I think it's better keep it that way.

> 
> [...]
>> @@ -40,9 +96,13 @@ struct rte_dma_dev {
>>  	int16_t dev_id; /**< Device [external] identifier. */
>>  	int16_t numa_node; /**< Local NUMA memory ID. -1 if unknown. */
>>  	void *dev_private; /**< PMD-specific private data. */
>> +	/** Functions exported by PMD. */
> 
> s/exported/implemented/
> 
>> +	const struct rte_dma_dev_ops *dev_ops;
>> +	struct rte_dma_conf dev_conf; /**< DMA device configuration. */
>>  	/** Device info which supplied during device initialization. */
>>  	struct rte_device *device;
>>  	enum rte_dma_dev_state state; /**< Flag indicating the device state. */
>> +	uint8_t dev_started : 1; /**< Device state: STARTED(1)/STOPPED(0). */
>>  	uint64_t reserved[2]; /**< Reserved for future fields. */
>>  } __rte_cache_aligned;
> 
> 
> 
> 
> .
> 

Thanks
  

08/10/2021 09:55, fengchengwen:
> On 2021/10/6 18:46, Thomas Monjalon wrote:
> > 24/09/2021 12:53, Chengwen Feng:
> >> --- a/lib/dmadev/rte_dmadev.c
> >> +++ b/lib/dmadev/rte_dmadev.c
> >> @@ -218,6 +218,9 @@ rte_dma_pmd_release(const char *name)
> >>  	if (dev == NULL)
> >>  		return -EINVAL;
> >>  
> >> +	if (dev->state == RTE_DMA_DEV_READY)
> >> +		return rte_dma_close(dev->dev_id);
> > 
> > What is the logic here?
> > The only exposed function should be rte_dma_close()
> > and it should call the freeing function.
> > The API should use the dev_id. As you said somewhere else,
> > the name is only for debugging.
> > Please remove the function rte_dma_pmd_release(const char *name).
> 
> The rte_dma_pmd_release corresponding to pmd_allocate, so both use the 'const char *name' parameter.
> 
> The rte_dma_pmd_release is also used for error handling when PMD init. Therefore, a status variable
> is used here. If the device is not ready, only resources need to be released. Otherwise, the close
> interface of the driver is invoked.
> 
> For PMD, the rte_dma_pmd_release is only wrap for dev_close when remove device, it does not need to
> implement two callbacks.
> 
> If we replace rte_dma_pmd_release with rte_dma_close, then we should invoke rte_dma_close in error
> handling when PMD init, this can lead to conceptual inconsistencies because the initialization has
> not been successful.
> 
> So I think it's better keep rte_dma_pmd_release.

I will review again this logic in the next version.

> >> +/** DMA device support memory-to-memory transfer.
> >> + *
> >> + * @see struct rte_dma_info::dev_capa
> >> + */
> >> +#define RTE_DMA_CAPA_MEM_TO_MEM		RTE_BIT64(0)
> >> +/** DMA device support memory-to-device transfer.
> >> + *
> >> + * @see struct rte_dma_info::dev_capa
> >> + */
> >> +#define RTE_DMA_CAPA_MEM_TO_DEV		RTE_BIT64(1)
> > 
> > Same comment as in earlier version: please group the flags
> > in a doxygen group. Example of doxygen group:
> > https://patches.dpdk.org/project/dpdk/patch/20210830104232.598703-1-thomas@monjalon.net/
> 
> Tried, but found it didn't coexist well with multi-line comments.

What is not working? Example?
I think you didn't get what to do.
You must add a comment to give a title and group all these flags.

> >> --- a/lib/dmadev/rte_dmadev_core.h
> >> +++ b/lib/dmadev/rte_dmadev_core.h
> >> +/** @internal Used to get device information of a device. */
> >> +typedef int (*rte_dma_info_get_t)(const struct rte_dma_dev *dev,
> >> +				  struct rte_dma_info *dev_info,
> >> +				  uint32_t info_sz);
> > 
> > Please move all driver interfaces in a file dedicated to drivers.
> 
> There are three head file: rte_dmadev.h, rte_dmadev_core.h, rte_dmadev_pmd.h
> And we build the following dependency:
> 
>                 rte_dmadev.h   ---> rte_dmadev_core.h          // mainly because dataplane inline API.
>                     ^
>                     |
>            ---------------------
>            |                   |
>        Application       rte_dmadev_pmd.h
>                                ^
>                                |
>                              DMA PMD
> 
> 
> If move all driver interfaces to rte_dmadev_pmd.h from rte_dmadev_core.h, bidirectional
> dependency may exist, e.g.
> 
>                 rte_dmadev.h   ---> rte_dmadev_core.h  ---> rte_dmadev_pmd.h
>                     ^
>                     |
>            ---------------------
>            |                   |
>        Application       rte_dmadev_pmd.h
>                                ^
>                                |
>                              DMA PMD
> 
> So I think it's better keep it that way.

Please make sure only what is needed for inline is kept in the "core.h"
You should look at the current effort done by Konstanti in ethdev to hide everything.