Re: [dpdk-dev] [RFC PATCH] dmadev: introduce DMA device library

[fengchengwen <fengchengwen@huawei.com>]

On 2021/6/16 0:38, Bruce Richardson wrote:
> On Tue, Jun 15, 2021 at 09:22:07PM +0800, Chengwen Feng wrote:
>> This patch introduces '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 <fengchengwen@huawei.com>
>> ---
> Thanks for sending this.
> 
> Of most interest to me right now are the key data-plane APIs. While we are
> still in the prototyping phase, below is a draft of what we are thinking
> for the key enqueue/perform_ops/completed_ops APIs.
> 
> Some key differences I note in below vs your original RFC:
> * Use of void pointers rather than iova addresses. While using iova's makes
>   sense in the general case when using hardware, in that it can work with
>   both physical addresses and virtual addresses, if we change the APIs to use
>   void pointers instead it will still work for DPDK in VA mode, while at the
>   same time allow use of software fallbacks in error cases, and also a stub
>   driver than uses memcpy in the background. Finally, using iova's makes the
>   APIs a lot more awkward to use with anything but mbufs or similar buffers
>   where we already have a pre-computed physical address.

The iova is an hint to application, and widely used in DPDK.
If switch to void, how to pass the address (iova or just va ?)
this may introduce implementation dependencies here.

Or always pass the va, and the driver performs address translation, and this
translation may cost too much cpu I think.

> * Use of id values rather than user-provided handles. Allowing the user/app
>   to manage the amount of data stored per operation is a better solution, I
>   feel than proscribing a certain about of in-driver tracking. Some apps may
>   not care about anything other than a job being completed, while other apps
>   may have significant metadata to be tracked. Taking the user-context
>   handles out of the API also makes the driver code simpler.

The user-provided handle was mainly used to simply application implementation,
It provides the ability to quickly locate contexts.

The "use of id values" seem like the dma_cookie of Linux DMA engine framework,
user will get a unique dma_cookie after calling dmaengine_submit(), and then
could use it to call dma_async_is_tx_complete() to get completion status.

How about define the copy prototype as following:
  dma_cookie_t rte_dmadev_copy(uint16_t dev_id, xxx)
while the dma_cookie_t is int32 and is monotonically increasing, when >=0 mean
enqueue successful else fail.
when complete the dmadev will return latest completed dma_cookie, and the
application could use the dma_cookie to quick locate contexts.

> * I've kept a single combined API for completions, which differs from the
>   separate error handling completion API you propose. I need to give the
>   two function approach a bit of thought, but likely both could work. If we
>   (likely) never expect failed ops, then the specifics of error handling
>   should not matter that much.

The rte_ioat_completed_ops API is too complex, and consider some applications
may never copy fail, so split them as two API.
It's indeed not friendly to other scenarios that always require error handling.

I prefer use completed operations number as return value other than the ID so
that application could simple judge whether have new completed operations, and
the new prototype:
 uint16_t rte_dmadev_completed(uint16_t dev_id, dma_cookie_t *cookie, uint32_t *status, uint16_t max_status, uint16_t *num_fails);

1) for normal case which never expect failed ops:
   just call: ret = rte_dmadev_completed(dev_id, &cookie, NULL, 0, NULL);
2) for other case:
   ret = rte_dmadev_completed(dev_id, &cookie, &status, max_status, &fails);
   at this point the fails <= ret <= max_status

> 
> For the rest, the control / setup APIs are likely to be rather
> uncontroversial, I suspect. However, I think that rather than xstats APIs,
> the library should first provide a set of standardized stats like ethdev
> does. If driver-specific stats are needed, we can add xstats later to the
> API.

Agree, will fix in v2

> 
> Appreciate your further thoughts on this, thanks.
> 
> Regards,
> /Bruce
> 
> /**
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice.
>  *
>  * Enqueue a copy operation onto the DMA device
>  *
>  * This queues up a copy operation to be performed by hardware, but does not
>  * trigger hardware to begin that operation.
>  *
>  * @param dev_id
>  *   The dmadev device id of the DMA instance
>  * @param src
>  *   The source buffer
>  * @param dst
>  *   The destination buffer
>  * @param length
>  *   The length of the data to be copied
>  * @return
>  *   - On success, id (uint16_t) of job enqueued
>  *   - On failure, negative error code
>  */
> static inline int
> __rte_experimental
> rte_dmadev_enqueue_copy(uint16_t dev_id, void * src, void * dst, unsigned int length);
> 
> /**
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice.
>  *
>  * Trigger hardware to begin performing enqueued operations
>  *
>  * This API is used to write the "doorbell" to the hardware to trigger it
>  * to begin the operations previously enqueued by e.g. rte_dmadev_enqueue_copy()
>  *
>  * @param dev_id
>  *   The dmadev device id of the DMA instance
>  * @return
>  *   0 on success, negative errno on error
>  */
> static inline int
> __rte_experimental
> rte_dmadev_perform_ops(uint16_t dev_id);
> 
> /**
>  * @warning
>  * @b EXPERIMENTAL: this API may change without prior notice.
>  *
>  * Returns details of operations that have been completed
>  *
>  * In the normal case of no failures in hardware performing the requested jobs,
>  * the return value is the ID of the last completed operation, and
>  * "num_reported_status" is 0.
>  *
>  * If errors have occured the status of "num_reported_status" (<= "max_status")
>  * operations are reported in the "status" array, with the return value being
>  * the ID of the last operation reported in that array.
>  *
>  * @param dev_id
>  *   The dmadev device id of the DMA instance
>  * @param max_status
>  *   The number of entries which can fit in the status arrays, i.e. max number
>  *   of completed operations to report.
>  * @param[out] status
>  *   Array to hold the status of each completed operation.
>  *   A value of RTE_DMA_OP_SKIPPED implies an operation was not attempted,
>  *   and any other non-zero value indicates operation failure.
>  * @param[out] num_reported_status
>  *   Returns the number of elements in status. If this value is returned as
>  *   zero (the expected case), the status array will not have been modified
>  *   by the function and need not be checked by software
>  * @return
>  *   On success, ID of the last completed/reported operation.
>  *   Negative errno on error, with all parameters unmodified.
>  */
> static inline int
> __rte_experimental
> rte_dmadev_completed_ops(uint16_t dev_id, uint8_t max_status,
>                 uint32_t *status, uint8_t *num_reported_status);
> 
> 
> .
>