Re: [PATCH v1 1/2] bbdev: add new function to dump debug information

[Hemant Agrawal <hemant.agrawal@oss.nxp.com>]

Hi Nicolas,

     Few comments inline.

On 02-07-2024 04:04, Nicolas Chautru wrote:
> This provides a new API to dump more debug information
> related to the status on a given bbdev queue.
> Some of this information is visible at bbdev level.
> This also provides a new option dev op, to print more
> information at the lower PMD level.
> This helps user to troubleshoot issues related to
> previous operations provided into a queue causing
> possible hard-to-debug negative scenarios.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   lib/bbdev/rte_bbdev.c     | 214 ++++++++++++++++++++++++++++++++++++++
>   lib/bbdev/rte_bbdev.h     |  41 ++++++++
>   lib/bbdev/rte_bbdev_pmd.h |   9 ++
>   lib/bbdev/version.map     |   4 +
>   4 files changed, 268 insertions(+)
>
> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
> index 13bde3c25b..81c031fc09 100644
> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -1190,3 +1190,217 @@ rte_bbdev_enqueue_status_str(enum rte_bbdev_enqueue_status status)
>   	rte_bbdev_log(ERR, "Invalid enqueue status");
>   	return NULL;
>   }
> +
> +
> +int
> +rte_bbdev_queue_ops_dump(uint16_t dev_id, uint16_t queue_id, FILE *f)
> +{
> +	struct rte_bbdev_queue_data *q_data;
> +	struct rte_bbdev_stats *stats;
> +	uint16_t i;
> +	struct rte_bbdev *dev = get_dev(dev_id);
> +
> +	VALID_DEV_OR_RET_ERR(dev, dev_id);
> +	VALID_QUEUE_OR_RET_ERR(queue_id, dev);
> +	VALID_DEV_OPS_OR_RET_ERR(dev, dev_id);
> +	VALID_FUNC_OR_RET_ERR(dev->dev_ops->queue_ops_dump, dev_id);
> +
> +	q_data = &dev->data->queues[queue_id];
> +
> +	if (f == NULL)
> +		return -EINVAL;
> +
> +	fprintf(f, "Dump of operations on %s queue %d\n",
> +			dev->data->name, queue_id);
> +	fprintf(f, "  Last Enqueue Status %s\n",
> +			rte_bbdev_enqueue_status_str(q_data->enqueue_status));
> +	for (i = 0; i < 4; i++)

It shall be RTE_BBDEV_ENQ_STATUS_SIZE_MAX instead of 4 ?


> +		if (q_data->queue_stats.enqueue_status_count[i] > 0)
> +			fprintf(f, "  Enqueue Status Counters %s %" PRIu64 "\n",
> +					rte_bbdev_enqueue_status_str(i),
> +					q_data->queue_stats.enqueue_status_count[i]);
> +	stats = &dev->data->queues[queue_id].queue_stats;
> +
> +	fprintf(f, "  Enqueue Count %" PRIu64 " Warning %" PRIu64 " Error %" PRIu64 "\n",
> +			stats->enqueued_count, stats->enqueue_warn_count,
> +			stats->enqueue_err_count);
> +	fprintf(f, "  Dequeue Count %" PRIu64 " Warning %" PRIu64 " Error %" PRIu64 "\n",
> +			stats->dequeued_count, stats->dequeue_warn_count,
> +			stats->dequeue_err_count);
> +
why not print acc_offload_cycles aas well?
> +	return dev->dev_ops->queue_ops_dump(dev, queue_id, f);
> +}
> +
> +char *
> +rte_bbdev_ops_param_string(void *op, enum rte_bbdev_op_type op_type)
> +{
> +	static char str[1024];
> +	static char partial[1024];
> +	struct rte_bbdev_dec_op *op_dec;
> +	struct rte_bbdev_enc_op *op_enc;
> +	struct rte_bbdev_fft_op *op_fft;
> +	struct rte_bbdev_mldts_op *op_mldts;
> +
> +	rte_iova_t add0 = 0, add1 = 0, add2 = 0, add3 = 0, add4 = 0;
> +
> +	if (op == NULL) {
> +		snprintf(str, sizeof(str), "Invalid Operation pointer\n");
> +		return str;
> +	}
> +
how about also covering mempool and opaque_data pointer - they may also 
be helpful in debugging?