On 2021/6/28 18:00, Bruce Richardson wrote:

quoted

  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

quoted

     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

quoted

     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.