Re: [dpdk-dev] [PATCH v19 1/7] dmadev: introduce DMA device library public APIs

[Kevin Laatz <kevin.laatz@intel.com>]

On 02/09/2021 14:13, Chengwen Feng wrote:
> The 'dmadevice' is a generic type of DMA device.
>
> This patch introduce the 'dmadevice' public APIs which expose generic
> operations that can enable configuration and I/O with the DMA devices.
>
> Maintainers update is also included in this patch.
>
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Jerin Jacob <jerinjacobk@gmail.com>
> ---
>   MAINTAINERS                            |   4 +
>   doc/api/doxy-api-index.md              |   1 +
>   doc/api/doxy-api.conf.in               |   1 +
>   doc/guides/rel_notes/release_21_11.rst |   5 +
>   lib/dmadev/meson.build                 |   4 +
>   lib/dmadev/rte_dmadev.h                | 949 +++++++++++++++++++++++++++++++++
>   lib/dmadev/version.map                 |  24 +
>   lib/meson.build                        |   1 +
>   8 files changed, 989 insertions(+)
>   create mode 100644 lib/dmadev/meson.build
>   create mode 100644 lib/dmadev/rte_dmadev.h
>   create mode 100644 lib/dmadev/version.map
>
<snip>
> +
> +/**
> + * rte_dma_direction - DMA transfer direction defines.
> + */
No need to have the struct name in the comment.
> +enum rte_dma_direction {
> +	RTE_DMA_DIR_MEM_TO_MEM,
> +	/**< DMA transfer direction - from memory to memory.
> +	 *
> +	 * @see struct rte_dmadev_vchan_conf::direction
> +	 */
> +	RTE_DMA_DIR_MEM_TO_DEV,
> +	/**< DMA transfer direction - from memory to device.
> +	 * In a typical scenario, the SoCs are installed on host servers as
> +	 * iNICs through the PCIe interface. In this case, the SoCs works in
> +	 * EP(endpoint) mode, it could initiate a DMA move request from memory
> +	 * (which is SoCs memory) to device (which is host memory).
> +	 *
> +	 * @see struct rte_dmadev_vchan_conf::direction
> +	 */
> +	RTE_DMA_DIR_DEV_TO_MEM,
> +	/**< DMA transfer direction - from device to memory.
> +	 * In a typical scenario, the SoCs are installed on host servers as
> +	 * iNICs through the PCIe interface. In this case, the SoCs works in
> +	 * EP(endpoint) mode, it could initiate a DMA move request from device
> +	 * (which is host memory) to memory (which is SoCs memory).
> +	 *
> +	 * @see struct rte_dmadev_vchan_conf::direction
> +	 */
> +	RTE_DMA_DIR_DEV_TO_DEV,
> +	/**< DMA transfer direction - from device to device.
> +	 * In a typical scenario, the SoCs are installed on host servers as
> +	 * iNICs through the PCIe interface. In this case, the SoCs works in
> +	 * EP(endpoint) mode, it could initiate a DMA move request from device
> +	 * (which is host memory) to the device (which is another host memory).
> +	 *
> +	 * @see struct rte_dmadev_vchan_conf::direction
> +	 */
> +};
> +
> +/**
> + * enum rte_dmadev_port_type - DMA access port type defines.
> + *
> + * @see struct rte_dmadev_port_param::port_type
> + */
> +enum rte_dmadev_port_type {
> +	RTE_DMADEV_PORT_NONE,
> +	RTE_DMADEV_PORT_PCIE, /**< The DMA access port is PCIe. */
> +};
> +
<snip>
> +
> +/**
> + * rte_dmadev_stats - running statistics.
> + */

No need to have the struct name in the comment. Maybe "Operation 
statistic counters"?


> +struct rte_dmadev_stats {
> +	uint64_t submitted;
> +	/**< Count of operations which were submitted to hardware. */
> +	uint64_t completed;
> +	/**< Count of operations which were completed. */
> +	uint64_t errors;
> +	/**< Count of operations which failed to complete. */
> +};

The comments here are a little ambiguous, it would be better to 
explicitly mention that "errors" is a subset of "completed" and not an 
independent statistic.


<snip>
> +
> +/**
> + * rte_dmadev_sge - can hold scatter-gather DMA operation request entry.
> + */
No need to have the struct name in the comment.
> +struct rte_dmadev_sge {
> +	rte_iova_t addr; /**< The DMA operation address. */
> +	uint32_t length; /**< The DMA operation length. */
> +};
> +

Apart from the minor comments, LGTM.

Reviewed-by: Kevin Laatz <kevin.laatz@intel.com>