Re: [dpdk-dev] [PATCH 6/8] raw/dpaa2_qdma: support enq and deq operations

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

On Saturday 07 April 2018 08:47 PM, Nipun Gupta wrote:
> Signed-off-by: Nipun Gupta <nipun.gupta@nxp.com>
> ---
>   drivers/raw/dpaa2_qdma/dpaa2_qdma.c                | 332 +++++++++++++++++++++
>   drivers/raw/dpaa2_qdma/dpaa2_qdma.h                |  21 ++
>   drivers/raw/dpaa2_qdma/rte_pmd_dpaa2_qdma.h        |  70 +++++
>   .../raw/dpaa2_qdma/rte_pmd_dpaa2_qdma_version.map  |   4 +
>   4 files changed, 427 insertions(+)
> 
> diff --git a/drivers/raw/dpaa2_qdma/dpaa2_qdma.c b/drivers/raw/dpaa2_qdma/dpaa2_qdma.c
> index b5f6bd9..5cd18ec 100644
> --- a/drivers/raw/dpaa2_qdma/dpaa2_qdma.c
> +++ b/drivers/raw/dpaa2_qdma/dpaa2_qdma.c
> @@ -344,6 +344,338 @@
>   	return i;
>   }
>   
> +static void
> +dpaa2_qdma_populate_fle(struct qbman_fle *fle,
> +			uint64_t src, uint64_t dest,
> +			size_t len, uint32_t flags)
> +{
> +	struct qdma_sdd *sdd;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	sdd = (struct qdma_sdd *)((uint8_t *)(fle) +
> +		(DPAA2_QDMA_MAX_FLE * sizeof(struct qbman_fle)));
> +
> +	/* first frame list to source descriptor */
> +	DPAA2_SET_FLE_ADDR(fle, DPAA2_VADDR_TO_IOVA(sdd));
> +	DPAA2_SET_FLE_LEN(fle, (2 * (sizeof(struct qdma_sdd))));
> +
> +	/* source and destination descriptor */
> +	DPAA2_SET_SDD_RD_COHERENT(sdd); /* source descriptor CMD */
> +	sdd++;
> +	DPAA2_SET_SDD_WR_COHERENT(sdd); /* dest descriptor CMD */
> +
> +	fle++;
> +	/* source frame list to source buffer */
> +	if (flags & RTE_QDMA_JOB_SRC_PHY) {
> +		DPAA2_SET_FLE_ADDR(fle, src);
> +		DPAA2_SET_FLE_BMT(fle);
> +	} else {
> +		DPAA2_SET_FLE_ADDR(fle, DPAA2_VADDR_TO_IOVA(src));
> +	}
> +	DPAA2_SET_FLE_LEN(fle, len);
> +
> +	fle++;
> +	/* destination frame list to destination buffer */
> +	if (flags & RTE_QDMA_JOB_DEST_PHY) {
> +		DPAA2_SET_FLE_BMT(fle);
> +		DPAA2_SET_FLE_ADDR(fle, dest);
> +	} else {
> +		DPAA2_SET_FLE_ADDR(fle, DPAA2_VADDR_TO_IOVA(dest));
> +	}
> +	DPAA2_SET_FLE_LEN(fle, len);
> +
> +	/* Final bit: 1, for last frame list */
> +	DPAA2_SET_FLE_FIN(fle);
> +}
> +
> +static int
> +dpdmai_dev_enqueue(struct dpaa2_dpdmai_dev *dpdmai_dev,
> +		   uint16_t txq_id,
> +		   uint16_t vq_id,
> +		   struct rte_qdma_job *job)
> +{
> +	struct qdma_io_meta *io_meta;
> +	struct qbman_fd fd = {};
> +	struct dpaa2_queue *txq;
> +	struct qbman_fle *fle;
> +	struct qbman_eq_desc eqdesc;
> +	struct qbman_swp *swp;
> +	int ret;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	if (unlikely(!DPAA2_PER_LCORE_DPIO)) {
> +		ret = dpaa2_affine_qbman_swp();
> +		if (ret) {
> +			DPAA2_QDMA_ERR("Failure in affining portal\n");
> +			return 0;
> +		}
> +	}
> +	swp = DPAA2_PER_LCORE_PORTAL;
> +
> +	txq = &(dpdmai_dev->tx_queue[txq_id]);
> +
> +	/* Prepare enqueue descriptor */
> +	qbman_eq_desc_clear(&eqdesc);
> +	qbman_eq_desc_set_fq(&eqdesc, txq->fqid);
> +	qbman_eq_desc_set_no_orp(&eqdesc, 0);
> +	qbman_eq_desc_set_response(&eqdesc, 0, 0);
> +
> +	/*
> +	 * Get an FLE/SDD from FLE pool.
> +	 * Note: IO metadata is before the FLE and SDD memory.
> +	 */
> +	ret = rte_mempool_get(qdma_dev.fle_pool, (void **)(&io_meta));
> +	if (ret) {
> +		DPAA2_QDMA_ERR("Memory alloc failed for FLE\n");
> +		return ret;
> +	}
> +
> +	/* Set the metadata */
> +	io_meta->cnxt = (size_t)job;
> +	io_meta->id = vq_id;
> +
> +	fle = (struct qbman_fle *)(io_meta + 1);
> +
> +	/* populate Frame descriptor */
> +	DPAA2_SET_FD_ADDR(&fd, DPAA2_VADDR_TO_IOVA(fle));
> +	DPAA2_SET_FD_COMPOUND_FMT(&fd);
> +	DPAA2_SET_FD_FRC(&fd, QDMA_SER_CTX);
> +
> +	/* Populate FLE */
> +	memset(fle, 0, QDMA_FLE_POOL_SIZE);
> +	dpaa2_qdma_populate_fle(fle, job->src, job->dest, job->len, job->flags);
> +
> +	/* Enqueue the packet to the QBMAN */
> +	do {
> +		ret = qbman_swp_enqueue_multiple(swp, &eqdesc, &fd, NULL, 1);
> +		if (ret < 0 && ret != -EBUSY)
> +			DPAA2_QDMA_ERR("Transmit failure with err: %d\n", ret);

Similar comment to one of the other patch - your macro definition has 
'\n' already in it. So, double new lines.

> +	} while (ret == -EBUSY);
> +
> +	DPAA2_QDMA_DEBUG("Successfully transmitted a packet\n");

Macros like the above are compiled into the code unlike the DP_ 
variants. If this is I/O path and such conditionals impact performance, 
maybe DP_ variant would be more efficient.

> +
> +	return ret;
> +}
> +
> +int
> +rte_qdma_vq_enqueue_multi(uint16_t vq_id,
> +			  struct rte_qdma_job **job,
> +			  uint16_t nb_jobs)
> +{
> +	int i, ret;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	for (i = 0; i < nb_jobs; i++) {
> +		ret = rte_qdma_vq_enqueue(vq_id, job[i]);
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	return i;
> +}
> +
> +int
> +rte_qdma_vq_enqueue(uint16_t vq_id,
> +		    struct rte_qdma_job *job)
> +{
> +	struct qdma_virt_queue *qdma_vq = &qdma_vqs[vq_id];
> +	struct qdma_hw_queue *qdma_pq = qdma_vq->hw_queue;
> +	struct dpaa2_dpdmai_dev *dpdmai_dev = qdma_pq->dpdmai_dev;
> +	int ret;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	/* Return error in case of wrong lcore_id */
> +	if (rte_lcore_id() != qdma_vq->lcore_id) {
> +		DPAA2_QDMA_WARN("QDMA enqueue for vqid %d on wrong core",
                /^^^^^^^^^^^^^^^
               /
Should this be DPAA2_QDMA_ERR?

> +				vq_id);
> +		return -EINVAL;
> +	}
> +
> +	ret = dpdmai_dev_enqueue(dpdmai_dev, qdma_pq->queue_id, vq_id, job);
> +	if (ret < 0) {
> +		DPAA2_QDMA_ERR("DPDMAI device enqueue failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	qdma_vq->num_enqueues++;
> +
> +	return 1;
> +}
> +

[...]

> +
> +struct rte_qdma_job *
> +rte_qdma_vq_dequeue(uint16_t vq_id)
> +{
> +	struct qdma_virt_queue *qdma_vq = &qdma_vqs[vq_id];
> +	struct qdma_hw_queue *qdma_pq = qdma_vq->hw_queue;
> +	struct dpaa2_dpdmai_dev *dpdmai_dev = qdma_pq->dpdmai_dev;
> +	struct rte_qdma_job *job = NULL;
> +	struct qdma_virt_queue *temp_qdma_vq;
> +	int dequeue_budget = QDMA_DEQUEUE_BUDGET;
> +	int ring_count, ret, i;
> +	uint16_t temp_vq_id;
> +
> +	DPAA2_QDMA_FUNC_TRACE();
> +
> +	/* Return error in case of wrong lcore_id */
> +	if (rte_lcore_id() != (unsigned int)(qdma_vq->lcore_id)) {
> +		DPAA2_QDMA_WARN("QDMA dequeue for vqid %d on wrong core",
> +				vq_id);
> +		return NULL;
> +	}
> +
> +	/* Only dequeue when there are pending jobs on VQ */
> +	if (qdma_vq->num_enqueues == qdma_vq->num_dequeues)
> +		return NULL;
> +
> +	if (qdma_vq->exclusive_hw_queue) {
> +		/* In case of exclusine queue directly fetch from HW queue */

for above comment s/exclusine/exclusive/

[...]

-
Shreyansh