Re: [dpdk-dev] [PATCH 4/8] raw/dpaa2_qdma: introduce the DPAA2 QDMA driver

[Shreyansh Jain <shreyansh.jain@nxp.com>]

On Saturday 07 April 2018 08:47 PM, Nipun Gupta wrote:
> DPAA2 QDMA driver uses MC DPDMAI object. This driver enables
> the user (app) to perform data DMA without involving CPU in
> the DMA process
> 
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---
>   MAINTAINERS                                        |   5 +
>   config/common_base                                 |   1 +
>   config/common_linuxapp                             |   1 +
>   drivers/raw/Makefile                               |   1 +
>   drivers/raw/dpaa2_qdma/Makefile                    |  34 +++
>   drivers/raw/dpaa2_qdma/dpaa2_qdma.c                | 290 +++++++++++++++++++++
>   drivers/raw/dpaa2_qdma/dpaa2_qdma.h                |  66 +++++
>   drivers/raw/dpaa2_qdma/dpaa2_qdma_logs.h           |  33 +++
>   .../raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map  |   4 +
>   mk/rte.app.mk                                      |   1 +
>   10 files changed, 436 insertions(+)
>   create mode 100644 drivers/raw/dpaa2_qdma/Makefile
>   create mode 100644 drivers/raw/dpaa2_qdma/dpaa2_qdma.c
>   create mode 100644 drivers/raw/dpaa2_qdma/dpaa2_qdma.h
>   create mode 100644 drivers/raw/dpaa2_qdma/dpaa2_qdma_logs.h
>   create mode 100644 drivers/raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map
> 

[...]

> diff --git a/drivers/raw/dpaa2_qdma/dpaa2_qdma.c b/drivers/raw/dpaa2_qdma/dpaa2_qdma.c
> new file mode 100644
> index 0000000..4b7fa13
> --- /dev/null
> +++ b/drivers/raw/dpaa2_qdma/dpaa2_qdma.c
> @@ -0,0 +1,290 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2018 NXP
> + */
> +
> +#include <string.h>
> +
> +#include <rte_eal.h>
> +#include <rte_fslmc.h>
> +#include <rte_atomic.h>
> +#include <rte_lcore.h>
> +#include <rte_rawdev.h>
> +#include <rte_rawdev_pmd.h>
> +#include <rte_malloc.h>
> +#include <rte_ring.h>
> +#include <rte_mempool.h>
> +
> +#include <mc/fsl_dpdmai.h>
> +#include <portal/dpaa2_hw_pvt.h>
> +#include <portal/dpaa2_hw_dpio.h>
> +
> +#include "dpaa2_qdma.h"
> +#include "dpaa2_qdma_logs.h"
> +
> +/* Dynamic log type identifier */
> +int dpaa2_qdma_logtype;
> +
> +/* QDMA device */
> +static struct qdma_device qdma_dev;
> +
> +/* QDMA H/W queues list */
> +TAILQ_HEAD(qdma_hw_queue_list, qdma_hw_queue);
> +static struct qdma_hw_queue_list qdma_queue_list
> +	= TAILQ_HEAD_INITIALIZER(qdma_queue_list);
> +
> +static const struct rte_rawdev_ops dpaa2_qdma_ops = {
> +};
> +
> +static int
> +add_hw_queues_to_list(struct dpaa2_dpdmai_dev *dpdmai_dev)
> +{
> +	struct qdma_hw_queue *queue;
> +	int i;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	for (i = 0; i < dpdmai_dev->num_queues; i++) {
> +		queue = rte_zmalloc(NULL, sizeof(struct qdma_hw_queue), 0);
> +		if (!queue) {
> +			DPAA2_QDMA_ERR(
> +				"Memory allocation failed for QDMA queue\n");

I think the definition of DPAA2_QDMA_ERR also contains an inbuilt '\n' - 
so, it would be double new lines. And this is valid at various other 
places in this patch.

> +			return -ENOMEM;
> +		}
> +
> +		queue->dpdmai_dev = dpdmai_dev;
> +		queue->queue_id = i;
> +
> +		TAILQ_INSERT_TAIL(&qdma_queue_list, queue, next);
> +		qdma_dev.num_hw_queues++;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +remove_hw_queues_from_list(struct dpaa2_dpdmai_dev *dpdmai_dev)
> +{
> +	struct qdma_hw_queue *queue;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	while ((queue = TAILQ_FIRST(&qdma_queue_list))) {

I think you might want to revisit this. TAILQ_FIRST will keep returning 
you the head of the list. TAILQ_FOREACH_SAFE is what you should be 
looking at.

> +		if (queue->dpdmai_dev == dpdmai_dev) {
> +			TAILQ_REMOVE(&qdma_queue_list, queue, next);
> +			rte_free(queue);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +dpaa2_dpdmai_dev_init(struct rte_rawdev *rawdev, int dpdmai_id)
> +{
> +	struct dpaa2_dpdmai_dev *dpdmai_dev = rawdev->dev_private;
> +	struct dpdmai_rx_queue_cfg rx_queue_cfg;
> +	struct dpdmai_attr attr;
> +	struct dpdmai_rx_queue_attr rx_attr;
> +	struct dpdmai_tx_queue_attr tx_attr;
> +	int ret, i;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	/* Open DPDMAI device */
> +	dpdmai_dev->dpdmai_id = dpdmai_id;
> +	dpdmai_dev->dpdmai.regs = rte_mcp_ptr_list[MC_PORTAL_INDEX];
> +	ret = dpdmai_open(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +			  dpdmai_dev->dpdmai_id, &dpdmai_dev->token);
> +	if (ret) {
> +		DPAA2_QDMA_ERR("dpdmai_open() failed with err: %d\n", ret);
> +		return ret;
> +	}
> +	DPAA2_QDMA_DEBUG("Opened dpdmai object successfully\n");
> +
> +	/* Get DPDMAI attributes */
> +	ret = dpdmai_get_attributes(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +				    dpdmai_dev->token, &attr);
> +	if (ret) {
> +		DPAA2_QDMA_ERR("dpdmai get attributes failed with err: %d\n",
> +			       ret);

Should dpdmai_close be called here? In case there is some state managed 
in the device which might impact its opening next time.
This would be valid for various errors generated in this function.

> +		return ret;
> +	}
> +	dpdmai_dev->num_queues = attr.num_of_priorities;
> +
> +	/* Set up Rx Queues */
> +	for (i = 0; i < attr.num_of_priorities; i++) {
> +		struct dpaa2_queue *rxq;
> +
> +		memset(&rx_queue_cfg, 0, sizeof(struct dpdmai_rx_queue_cfg));
> +		ret = dpdmai_set_rx_queue(&dpdmai_dev->dpdmai,
> +					  CMD_PRI_LOW,
> +					  dpdmai_dev->token,
> +					  i, &rx_queue_cfg);
> +		if (ret) {
> +			DPAA2_QDMA_ERR("Setting Rx queue failed with err: %d",
> +				       ret);
> +			return ret;
> +		}
> +
> +		/* Allocate DQ storage for the DPDMAI Rx queues */
> +		rxq = &(dpdmai_dev->rx_queue[i]);
> +		rxq->q_storage = rte_malloc("dq_storage",
> +					    sizeof(struct queue_storage_info_t),
> +					    RTE_CACHE_LINE_SIZE);
> +		if (!rxq->q_storage) {
> +			DPAA2_QDMA_ERR("q_storage allocation failed\n");
> +			return -ENOMEM;
> +		}
> +
> +		memset(rxq->q_storage, 0, sizeof(struct queue_storage_info_t));
> +		ret = dpaa2_alloc_dq_storage(rxq->q_storage);
> +		if (ret) {
> +			DPAA2_QDMA_ERR("dpaa2_alloc_dq_storage failed\n");
> +			return -ENOMEM;
> +		}
> +	}
> +
> +	/* Get Rx and Tx queues FQID's */
> +	for (i = 0; i < DPDMAI_PRIO_NUM; i++) {
> +		ret = dpdmai_get_rx_queue(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +					  dpdmai_dev->token, i, &rx_attr);
> +		if (ret) {
> +			DPAA2_QDMA_ERR("Reading device failed with err: %d",
> +				       ret);
> +			return ret;
> +		}
> +		dpdmai_dev->rx_queue[i].fqid = rx_attr.fqid;
> +
> +		ret = dpdmai_get_tx_queue(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +					  dpdmai_dev->token, i, &tx_attr);
> +		if (ret) {
> +			DPAA2_QDMA_ERR("Reading device failed with err: %d",
> +				       ret);
> +			return ret;
> +		}
> +		dpdmai_dev->tx_queue[i].fqid = tx_attr.fqid;
> +	}
> +
> +	/* Enable the device */
> +	ret = dpdmai_enable(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +			    dpdmai_dev->token);
> +	if (ret) {
> +		DPAA2_QDMA_ERR("Enabling device failed with err: %d", ret);
> +		return ret;
> +	}
> +
> +	/* Add the HW queue to the global list */
> +	ret = add_hw_queues_to_list(dpdmai_dev);
> +	if (ret) {
> +		DPAA2_QDMA_ERR("Adding H/W queue to list failed\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +dpaa2_dpdmai_dev_uninit(struct rte_rawdev *rawdev)
> +{
> +	struct dpaa2_dpdmai_dev *dpdmai_dev = rawdev->dev_private;
> +	int ret, i;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	/* Remove HW queues from global list */
> +	remove_hw_queues_from_list(dpdmai_dev);
> +
> +	ret = dpdmai_disable(&dpdmai_dev->dpdmai, CMD_PRI_LOW,
> +			     dpdmai_dev->token);
> +	if (ret)
> +		DPAA2_QDMA_ERR("dmdmai disable failed");
> +
> +	/* Set up the DQRR storage for Rx */
> +	for (i = 0; i < DPDMAI_PRIO_NUM; i++) {
> +		struct dpaa2_queue *rxq = &(dpdmai_dev->rx_queue[i]);
> +
> +		dpaa2_free_dq_storage(rxq->q_storage);
> +		rte_free(rxq->q_storage);
> +	}
> +
> +	/* Close the device at underlying layer*/
> +	ret = dpdmai_close(&dpdmai_dev->dpdmai, CMD_PRI_LOW, dpdmai_dev->token);
> +	if (ret)
> +		DPAA2_QDMA_ERR("Failure closing dpdmai device");
> +
> +	return 0;
> +}
> +
> +static int
> +rte_dpaa2_qdma_probe(struct rte_dpaa2_driver *dpaa2_drv,
> +		     struct rte_dpaa2_device *dpaa2_dev)
> +{
> +	struct rte_rawdev *rawdev;
> +	int ret;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	rawdev = rte_rawdev_pmd_allocate(dpaa2_dev->device.name,
> +			sizeof(struct dpaa2_dpdmai_dev),
> +			rte_socket_id());
> +	if (!rawdev) {
> +		DPAA2_QDMA_ERR("Unable to allocate rawdevice\n");
> +		return -EINVAL;
> +	}
> +
> +	dpaa2_dev->rawdev = rawdev;
> +	rawdev->dev_ops = &dpaa2_qdma_ops;
> +	rawdev->device = &dpaa2_dev->device;
> +	rawdev->driver_name = dpaa2_drv->driver.name;
> +
> +	/* For secondary processes, the primary has done all the work */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	/* Invoke PMD device initialization function */
> +	ret = dpaa2_dpdmai_dev_init(rawdev, dpaa2_dev->object_id);
> +	if (ret) {
> +		rte_rawdev_pmd_release(rawdev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +rte_dpaa2_qdma_remove(struct rte_dpaa2_device *dpaa2_dev)
> +{
> +	struct rte_rawdev *rawdev = dpaa2_dev->rawdev;
> +	int ret;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	dpaa2_dpdmai_dev_uninit(rawdev);
> +
> +	ret = rte_rawdev_pmd_release(rawdev);
> +	if (ret)
> +		DPAA2_QDMA_DEBUG("Device cleanup failed");
> +
> +	return 0;
> +}
> +
> +static struct rte_dpaa2_driver rte_dpaa2_qdma_pmd = {
> +	.drv_type = DPAA2_QDMA,
> +	.probe = rte_dpaa2_qdma_probe,
> +	.remove = rte_dpaa2_qdma_remove,
> +};
> +
> +RTE_PMD_REGISTER_DPAA2(dpaa2_qdma, rte_dpaa2_qdma_pmd);
> +
> +RTE_INIT(dpaa2_qdma_init_log);
> +

It is better to not add a new line here - that we we can relate the 
function below with the INIT macro above.

> +static void
> +dpaa2_qdma_init_log(void)
> +{
> +	dpaa2_qdma_logtype = rte_log_register("dpaa2.qdma");

The string for registeration should be of format 
'pmd.raw.<driver>.<feature>'.
See SHA: 7db274b9ada2221acb7110204a3b2c6a37d2614a for documentation.

> +	if (dpaa2_qdma_logtype >= 0)
> +		rte_log_set_level(dpaa2_qdma_logtype, RTE_LOG_INFO);
> +}

[...]

> +#endif /* __DPAA2_QDMA_H__ */
> diff --git a/drivers/raw/dpaa2_qdma/dpaa2_qdma_logs.h b/drivers/raw/dpaa2_qdma/dpaa2_qdma_logs.h
> new file mode 100644
> index 0000000..20902a8
> --- /dev/null
> +++ b/drivers/raw/dpaa2_qdma/dpaa2_qdma_logs.h
> @@ -0,0 +1,33 @@
> +/* SPDX-License-Identifier: BSD-3-Clause
> + * Copyright 2018 NXP
> + */
> +
> +#ifndef __DPAA2_QDMA_LOGS_H__
> +#define __DPAA2_QDMA_LOGS_H__
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +extern int dpaa2_qdma_logtype;
> +
> +#define DPAA2_QDMA_LOG(level, fmt, args...) \
> +	rte_log(RTE_LOG_ ## level, dpaa2_qdma_logtype, "%s(): " fmt "\n", \
> +		__func__, ##args)

Some logs don't need to have a function name associated with them in 
output - especially the error messages. All the macros here have 
'__func__'  as part of the output. Can you rethink if this is OK with you?

> +
> +#define DPAA2_QDMA_FUNC_TRACE() DPAA2_QDMA_LOG(DEBUG, ">>")
> +
> +#define DPAA2_QDMA_DEBUG(fmt, args...) \
> +	DPAA2_QDMA_LOG(DEBUG, fmt, ## args)
> +#define DPAA2_QDMA_INFO(fmt, args...) \
> +	DPAA2_QDMA_LOG(INFO, fmt, ## args)
> +#define DPAA2_QDMA_ERR(fmt, args...) \
> +	DPAA2_QDMA_LOG(ERR, fmt, ## args)
> +#define DPAA2_QDMA_WARN(fmt, args...) \
> +	DPAA2_QDMA_LOG(WARNING, fmt, ## args)
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __DPAA2_QDMA_LOGS_H__ */
> diff --git a/drivers/raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map b/drivers/raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map
> new file mode 100644
> index 0000000..9b9ab1a
> --- /dev/null
> +++ b/drivers/raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map
> @@ -0,0 +1,4 @@
> +DPDK_18.05 {
> +
> +	local: *;
> +};
> diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> index a6828c8..f2c778f 100644
> --- a/mk/rte.app.mk
> +++ b/mk/rte.app.mk
> @@ -118,6 +118,7 @@ endif
>   _LDLIBS-$(CONFIG_RTE_LIBRTE_PCI_BUS)        += -lrte_bus_pci
>   _LDLIBS-$(CONFIG_RTE_LIBRTE_VDEV_BUS)       += -lrte_bus_vdev
>   _LDLIBS-$(CONFIG_RTE_LIBRTE_DPAA_BUS)       += -lrte_bus_dpaa
> +_LDLIBS-$(CONFIG_RTE_LIBRTE_PMD_DPAA2_QDMA) += -lrte_pmd_dpaa2_qdma

This shouldn't be placed between the bus linking instructions. Maybe 
somewhere near the other rawdev PMD would be better.

>   ifeq ($(CONFIG_RTE_EAL_VFIO),y)
>   _LDLIBS-$(CONFIG_RTE_LIBRTE_FSLMC_BUS)      += -lrte_bus_fslmc
>   endif
>