From: Bruce Richardson [mailto:bruce.richardson@intel.com]
Sent: Thursday, 1 July 2021 18.34

On Thu, Jul 01, 2021 at 08:31:00PM +0530, Jerin Jacob wrote:

quoted

On Sat, Jun 26, 2021 at 9:29 AM fengchengwen

[off-list ref] wrote:

quoted

quoted

Hi, all
  I analyzed the current DPAM DMA driver and drew this summary in

conjunction

quoted

quoted

with the previous discussion, and this will as a basis for the V2

implementation.

quoted

quoted

  Feedback is welcome, thanks

Thanks for the write-up.

quoted

<snip>

quoted

quoted

     [queue_setup]: create one virt-queue, has following main

parameters:

quoted

quoted

         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.

quoted

quoted

                If user want create multiple virt-queue one the

same HW-queue,

quoted

quoted

                they could achieved by call queue_setup with the

same

quoted

quoted

                HW-queue-index.
         Note2: I think it's hard to define queue_setup config

paramter, and

quoted

quoted

                also this is control API, so I think it's OK to use

opaque

quoted

quoted

                pointer to implement it.
      [dma_copy/memset/sg]: all has vq_id input parameter.
         Note: I notice dpaa can't support single and sg in one

virt-queue, and

quoted

quoted

               I think it's maybe software implement policy other

than HW

quoted

quoted

               restriction because virt-queue could share the same

HW-queue.

quoted

quoted

      Here we use vq_id to tackle different scenario, like local-

to-local/

quoted

quoted

      local-to-host and etc.

IMO, The index representation has an additional overhead as one needs
to translate it
to memory pointer. I prefer to avoid by having object handle and use
_lookup() API get to make it work
in multi-process cases to avoid the additional indirection. Like

mempool object.

While it doesn't matter to me that much which is chosen, an index seems
cleaner to me and more consistent with other device types in DPDK. The
objects pointed to by the memory pointers you refer too can't just be
stored in an internal array in your driver and accessed directly by
index,
saving that layer of redirection?

The rte_eth_rx/tx_burst() functions use the parameter "uint16_t port_id" to identify the device.

rte_eth_rx/tx_burst() needs to look up the rte_eth_dev pointer in the rte_eth_devices array, which could be avoided by using "struct rte_eth_dev *dev" instead of "uint16_t port_id". It would be faster.

However, in the case of rte_eth_rx/tx_burst(), the port_id is rapidly changing at runtime, and often comes from the mbuf or similar, where it is preferable storing a 16 bit value rather than a 64 bit pointer.

I think that the choice of DMA device (and virt-queue) for DMA fast path operations will be much more constant than the choice of port_id (and queue_id) in Ethdev fast path operations. If you agree with this, we should use "struct rte_dma_dev *dev" rather than "uint16_t dev_id" as parameter to the DMA fast path functions.

Going even further, why do we need to pass both dev_id and virt_queue_id to the DMA fast path functions, instead of just passing a pointer to the virt-queue? That pointer could be returned by the rte_dma_queue_setup() function. And it could be opaque or a well defined structure. Or even more exotic: It could be a structure where the first part is common and well defined, and the rest of the structure is driver specific.

quoted

quoted

  5) And the dmadev public data-plane API (just prototype):
     dma_cookie_t rte_dmadev_memset(dev, vq_id, pattern, dst, len,

flags)

quoted

quoted

       -- flags: used as an extended parameter, it could be

uint32_t

quoted

quoted

     dma_cookie_t rte_dmadev_memcpy(dev, vq_id, src, dst, len,

flags)

quoted

quoted

     dma_cookie_t rte_dmadev_memcpy_sg(dev, vq_id, sg, sg_len,

flags)

quoted

quoted

       -- sg: struct dma_scatterlist array
     uint16_t rte_dmadev_completed(dev, vq_id, dma_cookie_t

*cookie,

quoted

quoted

                                   uint16_t nb_cpls, bool

*has_error)

quoted

quoted

       -- nb_cpls: indicate max process operations number
       -- has_error: indicate if there is an error
       -- return value: the number of successful completed

operations.

quoted

quoted

       -- example:
          1) If there are already 32 completed ops, and 4th is

error, and

quoted

quoted

             nb_cpls is 32, then the ret will be 3(because 1/2/3th

is OK), and

quoted

quoted

             has_error will be true.
          2) If there are already 32 completed ops, and all

successful

quoted

quoted

             completed, then the ret will be min(32, nb_cpls), and

has_error

quoted

quoted

             will be false.
          3) If there are already 32 completed ops, and all failed

completed,

quoted

quoted

             then the ret will be 0, and has_error will be true.

+1. IMO, it is better to call ring_idx instead of a cookie. To

enforce

quoted

that it the ring index.

+1, I like that name better too.

If it is "ring index" then it probably should be an uintXX_t type, and thus the functions cannot return <0 to indicate error.

By "ring index", do you actually mean "descriptor index" (for the DMA engine's descriptors)? Does the application need to process this value as anything but an opaque handle? Wouldn't an opaque pointer type provide performance? It would also allow using NULL to indicate error.

Do you expect applications to store the cookie in structures that are instantiated many times (e.g. like the port_id is stored in the mbuf structure), so there is an advantage to using an uint16_t instead of a pointer?

quoted

quoted

     uint16_t rte_dmadev_completed_status(dev_id, vq_id,

dma_cookie_t *cookie,

quoted

quoted

                                          uint16_t nb_status,

uint32_t *status)

quoted

quoted

       -- return value: the number of failed completed operations.

See above. Here we are assuming it is an index otherwise we need to
pass an array
cookies.

quoted

     And here I agree with Morten: we should design API which

adapts to DPDK

quoted

quoted

     service scenarios. So we don't support some sound-cards DMA,

and 2D memory

quoted

quoted

     copy which mainly used in video scenarios.
  6) The dma_cookie_t is signed int type, when <0 it mean error,

it's

quoted

quoted

     monotonically increasing base on HW-queue (other than virt-

queue). The

quoted

quoted

     driver needs to make sure this because the damdev framework

don't manage

quoted

quoted

     the dma_cookie's creation.

+1 and see above.

quoted

  7) Because data-plane APIs are not thread-safe, and user could

determine

quoted

quoted

     virt-queue to HW-queue's map (at the queue-setup stage), so it

is user's

quoted

quoted

     duty to ensure thread-safe.

+1. But I am not sure how easy for the fast-path application to have

this logic,

quoted

Instead, I think, it is better to tell the capa for queue by driver
and in channel configuration,
the application can request for requirement (Is multiple producers

enq

quoted

to the same HW queue or not).
Based on the request, the implementation can pick the correct

function

quoted

pointer for enq.(lock vs lockless version if HW does not support
lockless)

Non-thread safety is the norm in DPDK for all other queue resources,
however, haivng multi-thread safety as a capability sounds reasonable.

quoted

quoted

  8) One example: vq_id = rte_dmadev_queue_setup(dev,
  config.{HW-queue-index=x, opaque}); if (vq_id < 0) { // create
  virt-queue failed return; } // submit memcpy task cookit =
  rte_dmadev_memcpy(dev, vq_id, src, dst, len, flags); if (cookie <

0)

quoted

quoted

  { // submit failed return; } // get complete task ret =
  rte_dmadev_completed(dev, vq_id, &cookie, 1, has_error); if
  (!has_error && ret == 1) { // the memcpy successful complete }

+1

quoted

  9) As octeontx2_dma support sg-list which has many valid buffers

in

quoted

quoted

  dpi_dma_buf_ptr_s, it could call the rte_dmadev_memcpy_sg API.

+1

quoted

  10) As ioat, it could delcare support one HW-queue at

dev_configure

quoted

quoted

  stage, and only support create one virt-queue.  11) As

dpaa2_qdma, I

quoted

quoted

  think it could migrate to new framework, but still wait for
  dpaa2_qdma guys feedback.  12) About the prototype src/dst

parameters

quoted

quoted

  of rte_dmadev_memcpy API, we have two candidates which are iova

and

quoted

quoted

  void *, how about introduce dma_addr_t type which could be va or

iova

quoted

quoted

  ?

I think, conversion looks ugly, better to have void * and share the
constraints of void * as limitation/capability using flag. So that

driver

quoted

can update it.

I'm ok with either rte_iova_t or void * as parameter type. Let's not
define a new type though,

+1

and +1 to just using capabilities to define what kinds
of addressing are supported by the device instances.

+1

/Bruce

And regarding naming: Consider rte_dma_xx() instead of rte_dmadev_xx().

-Morten