Re: [dpdk-dev] [PATCH v23 2/6] dmadev: add control plane function support

[Thomas Monjalon <thomas@monjalon.net>]

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;