Thanks, comment inline
On 2021/7/29 18:44, Jerin Jacob wrote:
> On Thu, Jul 29, 2021 at 6:56 AM fengchengwen <fengcheng...@huawei.com> 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.
>
> +1.
>
>>
>>> 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.
>
> Yes. Could you add the following to enable scatter-gather support in
> rte_dmadev_info::sge_max
> /**<Maximum allowed number of scatter-gather entries in a single sg call. */
> uint16_t sge_max;
>
already add max_sges, the naming method (start with max_) complies with the
existing fields.
>
>>
>> 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
>>
>>> .
>>>
> .
>
