On 2021/10/6 18:46, Thomas Monjalon wrote:

24/09/2021 12:53, Chengwen Feng:

quoted

--- 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]

quoted

--- 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.

[...]

quoted

--- 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.

quoted

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

API is not exported by a PMD, but implemented.

quoted

+ * 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.

quoted

+/** 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.

quoted

+struct rte_dma_port_param {
+	/** The device access port type.
+	 *
+	 * @see enum rte_dma_port_type
+	 */
+	enum rte_dma_port_type port_type;
+	union {

[...]

quoted

+		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. */
+};

quoted

--- 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.

[...]

quoted

@@ -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/

quoted

+	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