Re: [dpdk-dev] [PATCH v21 1/7] dmadev: introduce DMA device library public APIs

[Thomas Monjalon <thomas@monjalon.net>]

09/09/2021 15:33, fengchengwen:
> On 2021/9/9 18:33, Thomas Monjalon wrote:
> > 07/09/2021 14:56, Chengwen Feng:
> >> + * 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/

OK thanks for the explanation.

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

[...]
> >> + * @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" ?

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

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