09/09/2021 15:33, fengchengwen:

On 2021/9/9 18:33, Thomas Monjalon wrote:

quoted

07/09/2021 14:56, Chengwen Feng:

quoted

+ * The DMA controller could have multiple HW-DMA-channels (aka. HW-DMA-queues),
+ * each HW-DMA-channel should be represented by a dmadev.
+ *
+ * The dmadev could create multiple virtual DMA channels, each virtual DMA
+ * channel represents a different transfer context. The DMA operation request
+ * must be submitted to the virtual DMA channel. e.g. Application could create
+ * virtual DMA channel 0 for memory-to-memory transfer scenario, and create
+ * virtual DMA channel 1 for memory-to-device transfer scenario.

What is the benefit of virtual channels compared to have separate dmadevs
for the same HW channel?

This design is from the previous discussion [1]. If a dmadev is created for each
virtual channel, there are many associations between the dmadevs. For example,
channel operations of some devices need to interact with the kernel, the corresponding
kernel operation handles need to be shared among multiple dmadevs. It's going to get
more complicated.

[1] https://lore.kernel.org/dpdk-dev/c4a0ee30-f7b8-f8a1-463c-8eedaec82aea@huawei.com/ (local)

OK thanks for the explanation.

[...]

quoted

quoted

+ * be released by rte_dmadev_pmd_release() during the PCI/SoC device removing
+ * phase.

Again what is this phase?
I think freeing should be done only via the "close" function.

OK
The allocate/release will be reconstructed with reference to rte_eth_dev_pci_generic_probe.

You shouldn't always mimic ethdev, there can be some misconceptions :)
I think you don't need PCI specific helper.

[...]

quoted

quoted

+ * @note If the dmadev works in silent mode (@see RTE_DMADEV_CAPA_SILENT),
+ * application does not invoke the above two completed APIs.
+ *
+ * About the ring_idx which enqueue APIs (e.g. rte_dmadev_copy()
+ * rte_dmadev_fill()) returned, the rules are as follows:

I feel a word is missing above.

Can you point it out specifically ?
PS: I specifically examined by access https://www.nounplus.net/grammarcheck/ and found
it prompts the 'enqueue' to 'enqueues or enqueued'.

After second read, I think it is a tense problem.
What about "returned" -> "return" ?

[...]

quoted

quoted

+/**< DMA device support memory-to-memory transfer.
+ *
+ * @see struct rte_dmadev_info::dev_capa
+ */

It is preferred to have documentation before the item.

Is this particularly strong?
I use postfix comment style for whole doxygen comments. I think it's better to maintain a unified
style.

In general prefix comment is preferred.
Postfix comments are OK for short comments on the same line.

[...]

quoted

This series add one file per patch.
Instead it would be better to have groups of features per patch,
meaning the implementation and the driver interface should be
in the same patch.
You can split like this:
	1/ device allocation
	2/ configuration and start/stop
	3/ dataplane functions

I would suggest 2 more patches:
	4/ event notification
see https://patches.dpdk.org/project/dpdk/patch/20210730135533.417611-3-thomas@monjalon.net/
	5/ multi-process
see https://patches.dpdk.org/project/dpdk/patch/20210730135533.417611-5-thomas@monjalon.net/

The split mode you recommend is better.
But is this particularly strong ?

Yes, that's really better.

Because many acked-by and reviewed-by base on stand-alone file.
Does this division mean that a new acked/reviewed ?

You can keep the acks which are commong to the first 4 patches I guess
and ask for re-ack to others.