Hi Tom, 

Back from time off, replying to that previous email.

-----Original Message-----
From: Tom Rix <trix@redhat.com>
Sent: Monday, July 18, 2022 6:21 AM
To: Chautru, Nicolas <redacted>; dev@dpdk.org;
thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
[off-list ref]; david.marchand@redhat.com;
stephen@networkplumber.org
Subject: Re: [PATCH v4 4/7] drivers/baseband: update PMDs to expose queue
per operation


On 7/7/22 10:19 AM, Chautru, Nicolas wrote:

quoted

Hi Tom,

quoted

-----Original Message-----
From: Tom Rix <trix@redhat.com>
Sent: Thursday, July 7, 2022 6:21 AM
To: Chautru, Nicolas <redacted>; dev@dpdk.org;
thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
[off-list ref]; david.marchand@redhat.com;
stephen@networkplumber.org
Subject: Re: [PATCH v4 4/7] drivers/baseband: update PMDs to expose
queue per operation


On 7/6/22 2:10 PM, Chautru, Nicolas wrote:

quoted

Hi Tom,

quoted

-----Original Message-----
From: Tom Rix <trix@redhat.com>
Sent: Wednesday, July 6, 2022 9:15 AM
To: Chautru, Nicolas <redacted>; dev@dpdk.org;
thomas@monjalon.net; gakhil@marvell.com;

hemant.agrawal@nxp.com

quoted

quoted

quoted

quoted

Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
[off-list ref]; david.marchand@redhat.com;
stephen@networkplumber.org
Subject: Re: [PATCH v4 4/7] drivers/baseband: update PMDs to expose
queue per operation


On 7/5/22 5:23 PM, Nicolas Chautru wrote:

quoted

Add support in existing bbdev PMDs for the explicit number of
queue and priority for each operation type configured on the device.

Signed-off-by: Nicolas Chautru <redacted>
---
    drivers/baseband/acc100/rte_acc100_pmd.c           | 29

+++++++++++++--

quoted

quoted

----

quoted

quoted

---

quoted

    drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c |  8 ++++++
    drivers/baseband/fpga_lte_fec/fpga_lte_fec.c       |  8 ++++++
    drivers/baseband/la12xx/bbdev_la12xx.c             |  7 ++++++
    drivers/baseband/turbo_sw/bbdev_turbo_software.c   | 11

++++++++

quoted

quoted

quoted

quoted

quoted

    5 files changed, 51 insertions(+), 12 deletions(-)

diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c

b/drivers/baseband/acc100/rte_acc100_pmd.c
index 17ba798..d568d0d 100644

--- a/drivers/baseband/acc100/rte_acc100_pmd.c
+++ b/drivers/baseband/acc100/rte_acc100_pmd.c

@@ -966,6 +966,7 @@
    		struct rte_bbdev_driver_info *dev_info)
    {
    	struct acc100_device *d = dev->data->dev_private;
+	int i;

    	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
    		{

@@ -1062,19 +1063,23 @@
    	fetch_acc100_config(dev);
    	dev_info->device_status =

RTE_BBDEV_DEV_NOT_SUPPORTED;

quoted

quoted

quoted

quoted

quoted

-	/* This isn't ideal because it reports the maximum number of queues

but

quoted

-	 * does not provide info on how many can be uplink/downlink or

different

quoted

-	 * priorities
-	 */
-	dev_info->max_num_queues =
-			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
-			d->acc100_conf.q_dl_5g.num_qgroups +
-			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
-			d->acc100_conf.q_ul_5g.num_qgroups +
-			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
-			d->acc100_conf.q_dl_4g.num_qgroups +
-			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
+	/* Expose number of queues */
+	dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_DEC] =
+d->acc100_conf.q_ul_4g.num_aqs_per_groups *
    			d->acc100_conf.q_ul_4g.num_qgroups;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_ENC] = d-
acc100_conf.q_dl_4g.num_aqs_per_groups *
+			d->acc100_conf.q_dl_4g.num_qgroups;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_DEC] = d-
acc100_conf.q_ul_5g.num_aqs_per_groups *
+			d->acc100_conf.q_ul_5g.num_qgroups;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_ENC] = d-
acc100_conf.q_dl_5g.num_aqs_per_groups *
+			d->acc100_conf.q_dl_5g.num_qgroups;
+	dev_info->queue_priority[RTE_BBDEV_OP_TURBO_DEC] =

d-

quoted

quoted

quoted

quoted

quoted

acc100_conf.q_ul_4g.num_qgroups;
+	dev_info->queue_priority[RTE_BBDEV_OP_TURBO_ENC] =

d-

quoted

quoted

quoted

quoted

quoted

acc100_conf.q_dl_4g.num_qgroups;
+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_DEC] = d-
acc100_conf.q_ul_5g.num_qgroups;
+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_ENC] = d-
acc100_conf.q_dl_5g.num_qgroups;
+	dev_info->max_num_queues = 0;
+	for (i = RTE_BBDEV_OP_TURBO_DEC; i <

RTE_BBDEV_OP_LDPC_ENC;

quoted

quoted

quoted

quoted

i++)

should this be i <=  ?

Thanks

quoted

quoted

+		dev_info->max_num_queues += dev_info-

num_queues[i];

quoted

quoted

quoted

quoted

    	dev_info->queue_size_lim = ACC100_MAX_QUEUE_DEPTH;
    	dev_info->hardware_accelerated = true;
    	dev_info->max_dl_queue_priority = diff --git
a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
index 57b12af..b4982af 100644

--- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
+++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c

@@ -379,6 +379,14 @@
    		if (hw_q_id != FPGA_INVALID_HW_QUEUE_ID)
    			dev_info->max_num_queues++;
    	}
+	/* Expose number of queue per operation type */
+	dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_DEC] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_ENC] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_DEC] =

dev_info-

quoted

quoted

quoted

quoted

quoted

max_num_queues / 2;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_ENC] =

dev_info-

quoted

quoted

quoted

quoted

quoted

max_num_queues / 2;
+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_DEC] = 1;
+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_ENC] = 1;
    }

    /**

diff --git a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c

b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
index 2a330c4..dc7f479 100644

--- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
+++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c

@@ -655,6 +655,14 @@ struct __rte_cache_aligned fpga_queue {
    		if (hw_q_id != FPGA_INVALID_HW_QUEUE_ID)
    			dev_info->max_num_queues++;
    	}
+	/* Expose number of queue per operation type */
+	dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_DEC] =

dev_info-

quoted

quoted

quoted

quoted

quoted

max_num_queues / 2;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_ENC] =

dev_info-

quoted

quoted

quoted

quoted

quoted

max_num_queues / 2;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_DEC] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_ENC] = 0;
+	dev_info->queue_priority[RTE_BBDEV_OP_TURBO_DEC] = 1;
+	dev_info->queue_priority[RTE_BBDEV_OP_TURBO_ENC] =

1;

quoted

quoted

quoted

quoted

quoted

    }

    /**

diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c

b/drivers/baseband/la12xx/bbdev_la12xx.c
index c1f88c6..e99ea9a 100644

--- a/drivers/baseband/la12xx/bbdev_la12xx.c
+++ b/drivers/baseband/la12xx/bbdev_la12xx.c

@@ -102,6 +102,13 @@ struct bbdev_la12xx_params {
    	dev_info->min_alignment = 64;
    	dev_info->device_status =

RTE_BBDEV_DEV_NOT_SUPPORTED;

quoted

quoted

quoted

quoted

quoted

+	dev_info->num_queues[RTE_BBDEV_OP_NONE] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_DEC] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_TURBO_ENC] = 0;
+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_DEC] =

LA12XX_MAX_QUEUES / 2;

quoted

+	dev_info->num_queues[RTE_BBDEV_OP_LDPC_ENC] =

LA12XX_MAX_QUEUES / 2;

quoted

+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_DEC] = 1;
+	dev_info->queue_priority[RTE_BBDEV_OP_LDPC_ENC] = 1;
    	rte_bbdev_log_debug("got device info from %u", dev->data-

dev_id);

quoted

quoted

quoted

quoted

    }

diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c

b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
index dbc5524..647e706 100644

--- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
+++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c

@@ -256,6 +256,17 @@ struct turbo_sw_queue {
    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
    	dev_info->device_status =

RTE_BBDEV_DEV_NOT_SUPPORTED;

quoted

quoted

quoted

quoted

quoted

+	const struct rte_bbdev_op_cap *op_cap =

bbdev_capabilities;

quoted

quoted

quoted

quoted

Should this be done through dev instead of assigning directly ?

I am not sure I follow your suggestion. Do you mind clarifying?

bbdev_capabilites is a const defined in this function, do you really
need to loop over it to find information that is constant ?

I still miss your point. Note that this constant is not always the same at

build time (based on what SDK it can links to).

quoted

What would suggest?

Operations that can be done at compile time, should be.  Useless there is a
good reason.

You need to provide a good reason or make the change.

I believe this is more graceful, scalable and maintainable this way. 
At build time we already define the list of capability then we just process that information without risk of disconnect between two pieces of code. 
The drawback is execution time but this function is not time sensititive (enumeration).
I could change it but the code would be poorer with risk of breaking stuff in the future (redundant information in the code hence bug prone).
Ie. defining the number of operations and queues using another serie of  #define. 
From my point of view that would be something that we would not do internally for the reasons above, but if you insist I will just change accordingly so that to move on. 
Not a big deal, let us know. 

Thanks
Nic

Tom

quoted

Thanks
Nic

quoted

Tom

quoted

quoted

Tom

quoted

+	int num_op_type = 0;
+	for (; op_cap->type != RTE_BBDEV_OP_NONE; ++op_cap)
+		num_op_type++;
+	op_cap = bbdev_capabilities;
+	if (num_op_type > 0) {
+		int num_queue_per_type = dev_info-

max_num_queues /

quoted

quoted

quoted

num_op_type;

quoted

+		for (; op_cap->type != RTE_BBDEV_OP_NONE;

++op_cap)

quoted

quoted

quoted

quoted

quoted

+			dev_info->num_queues[op_cap->type] =

num_queue_per_type;

quoted

+	}
+
    	rte_bbdev_log_debug("got device info from %u\n", dev-

data-

quoted

quoted

quoted

quoted

dev_id);
    }