Re: [dpdk-dev] [PATCH v11 1/2] dmadev: introduce DMA device library

[fengchengwen <fengchengwen@huawei.com>]

Thanks, inline comment

On 2021/7/28 19:13, Bruce Richardson wrote:
> On Tue, Jul 27, 2021 at 11:39:59AM +0800, Chengwen Feng wrote:
>> This patch introduce 'dmadevice' which is a generic type of DMA
>> device.
>>
>> The APIs of dmadev library exposes some generic operations which can
>> enable configuration and I/O with the DMA devices.
>>
>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>> Acked-by: Morten Brørup <mb@smartsharesystems.com>
>> ---
> 
> Thanks for this. Before it gets merged, I believe it needs to be split
> further into multiple patches (say 4 or so) rather than adding the whole
> lib in one go.
> 
> Normally, I believe the split would be something like:
> * basic device structures and infrastructure e.g. alloc and release
>   functions
> * device config functions (and structures to go along with them)
>   such as configure and queue_setup
> * data plane functions
> 

I will try for it
Maybe one patch for public file, one for pmd header file, one for
implementation, and last for doc.

> Documentation would be included in each of the patches, rather than done as
> a block at the end.
> 
> Besides that, I have one small additional requests for the API. Based off
> feedback for ioat driver, we added in the following function to that API,
> and we probably need something similar in dmadev:
> 
> rte_ioat_burst_capacity()
> 
> For our implementation this returns the number of elements that can be
> enqueued to the ring, at least for the current burst/batch of packets. We
> did the API this way because there can be additional limits beyond ring
> size on each individual burst beyond just the raw ring capacity, e.g. even
> if there are 4k ring elements free, there may be limits on the max burst
> size the hardware can do, or limits on the number of outstanding
> batches etc.
> 
> Therefore can I request the addition of rte_dmadev_burst_capacity() [or
> something similarly named] to the basic dmadev API set. For most hardware,
> I think this will likely be the remaining free ring size, but I don't
> believe the API should commit to that. The use case it was added for was to
> enable an application which needs to do a multi-copy operation to check
> that all copies can fit or not before enqueuing the first one. This is
> important for hardware that doesn't have scatter-gather list support.

Remaining capacity can be inferred by ring_idx which return from enqueue and
dequeue APIs.
So I don't think this API needs to be added.

For scatter-gather list, there maybe a hardware limit for max src or dst entry
size, I prefer add 'max_sges' filed in struct rte_dmadev_info to indicate it.

> 
> /Bruce
> 
> PS: One typo in code flagged below too.
> 
> <snip>
>> + */
>> +enum rte_dma_status_code {
>> +	RTE_DMA_STATUS_SUCCESSFUL,
>> +	/**< The operation completed successfully. */
>> +	RTE_DMA_STATUS_USRER_ABORT,
> 
>               Typo here ^^^
> 

OK, USRER->USER will fix later

> .
>