Thanks, comment inline

On 2021/7/29 17:15, Bruce Richardson wrote:
> On Thu, Jul 29, 2021 at 09:26:31AM +0800, fengchengwen wrote:
>> 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.
>>
> 
> Yes, the app can always track it itself, but I still see value in adding
> this API. However, so long as you are open to having it added later, it
> doesn't matter if it's not present in the first versions of this API merged
> in.

Got it, I think we could discuss later when we adapt more drivers.

> 
> /Bruce
> .
> 

Reply via email to