Re: [dpdk-dev] dmadev discussion summary

[fengchengwen <fengchengwen@huawei.com>]

On 2021/6/28 18:00, Bruce Richardson wrote:
>>   4) The driver's ops design (here we only list key points):
>>      [dev_info_get]: mainly return the number of HW-queues
>>      [dev_configure]: nothing important
>>      [queue_setup]: create one virt-queue, has following main parameters:
>>          HW-queue-index: the HW-queue index used
>>          nb_desc: the number of HW descriptors
>>          opaque: driver's specific info
>>          Note1: this API return virt-queue index which will used in later API.
>>                 If user want create multiple virt-queue one the same HW-queue,
>>                 they could achieved by call queue_setup with the same
>>                 HW-queue-index.
>>          Note2: I think it's hard to define queue_setup config paramter, and
>>                 also this is control API, so I think it's OK to use opaque
>>                 pointer to implement it.
> I'm not sure opaque pointer will work in practice, so I think we should try
> and standardize the parameters as much as possible. Since it's a control
> plane API, using a struct with a superset of parameters may be workable.
> Let's start with a minimum set and build up from there.

I tried to standardize a few parameters, which you can see on the new patch

>>      uint16_t rte_dmadev_completed(dev, vq_id, dma_cookie_t *cookie,
>>                                    uint16_t nb_cpls, bool *has_error)
>>        -- nb_cpls: indicate max process operations number
>>        -- has_error: indicate if there is an error
>>        -- return value: the number of successful completed operations.
>>        -- example:
>>           1) If there are already 32 completed ops, and 4th is error, and
>>              nb_cpls is 32, then the ret will be 3(because 1/2/3th is OK), and
>>              has_error will be true.
>>           2) If there are already 32 completed ops, and all successful
>>              completed, then the ret will be min(32, nb_cpls), and has_error
>>              will be false.
>>           3) If there are already 32 completed ops, and all failed completed,
>>              then the ret will be 0, and has_error will be true.
> +1 for this
> 
>>      uint16_t rte_dmadev_completed_status(dev_id, vq_id, dma_cookie_t *cookie,
>>                                           uint16_t nb_status, uint32_t *status)
>>        -- return value: the number of failed completed operations.
>>      And here I agree with Morten: we should design API which adapts to DPDK
>>      service scenarios. So we don't support some sound-cards DMA, and 2D memory
>>      copy which mainly used in video scenarios.
> 
> Can I suggest a few adjustments here to the semantics of this API. In
> future we may have operations which return a status value, e.g. our
> hardware can support ops like compare equal/not-equal, which means that
> this API would be meaningful even in case of success. Therefore, I suggest
> that the return value be changed to allow success also to be returned in
> the array, and the return value is not the number of failed ops, but the
> number of ops for which status is being returned.
> 
> Also for consideration: when trying to implement this in a prototype in our
> driver, it would be easier if we relax the restriction on the "completed"
> API so that we can flag has_error when an error is detected rather than
> guaranteeing to return all elements right up to the error. For example, if
> we have a burst of packets and one is problematic, it may be easier to flag
> the error at the start of the burst and then have a few successful entries
> at the start of the completed_status array. [Separate from this] We should
> also have a "has_error" or "more_errors" flag on this API too, to indicate
> when the user can switch back to using the regular "completed" API. This
> means that apps switch from one API to the other when "has_error" is true,
> and only switch back when it becomes false again.
> 

We've discussed this before, and I prefer a relatively straightforward API,
so in the new version I'll explicitly name it as rte_dmadev_completed_fails.

We can continue this on the new patch, and I think that's probably the biggest
difference.