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 <fengcheng...@huawei.com>
>> Acked-by: Bruce Richardson <bruce.richard...@intel.com>
>> Acked-by: Morten Brørup <m...@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
> .
>
