[PATCH v7 0/1] baseband/acc100: changes for 22.11

[PATCH v7 0/1] baseband/acc100: changes for 22.11

[Hernan Vargas]

v7: Not including dependency on SDK introduced in previous v3 for now due to lack of consensus yet.
  Still detecting the known corner case and flagging it.
v6: Fix commit message typo.
v5: Fix compilation error and squash documentation changes.
v4: Rebased code to use the latest ACC common API and implemented review comment changes.
v3: Code refactor based on comments and grouping fixes at beginning of series.
v2: Rebased code to use ACC common API.
v1: Upstreaming ACC100 changes for 22.11.
This patch series is dependant on series:
https://patches.dpdk.org/project/dpdk/list/?series=25191

Hernan Vargas (1):
  baseband/acc100: add detection for deRM corner cases

 drivers/baseband/acc/acc_common.h     |  8 ++++
 drivers/baseband/acc/rte_acc100_pmd.c | 55 +++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 3 deletions(-)

-- 
2.37.1

[PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Hernan Vargas]

Add function to detect if de-ratematch pre-processing is recommended for
SW corner cases.
Some specific 5GUL FEC corner cases may cause unintended back pressure
and in some cases a potential stability issue on the ACC100.
The PMD can detect such code block configuration and issue an info
message to the user.

Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
---
 drivers/baseband/acc/acc_common.h     |  8 ++++
 drivers/baseband/acc/rte_acc100_pmd.c | 55 +++++++++++++++++++++++++--
 2 files changed, 60 insertions(+), 3 deletions(-)

diff --git a/drivers/baseband/acc/acc_common.h b/drivers/baseband/acc/acc_common.h
index eae7eab4e9..6213b0b61e 100644
--- a/drivers/baseband/acc/acc_common.h
+++ b/drivers/baseband/acc/acc_common.h
@@ -123,6 +123,14 @@
 #define ACC_HARQ_ALIGN_64B          64
 #define ACC_MAX_ZC                  384
 
+/* De-ratematch code rate limitation for recommended operation */
+#define ACC_LIM_03 2  /* 0.03 */
+#define ACC_LIM_09 6  /* 0.09 */
+#define ACC_LIM_14 9  /* 0.14 */
+#define ACC_LIM_21 14 /* 0.21 */
+#define ACC_LIM_31 20 /* 0.31 */
+#define ACC_MAX_E (128 * 1024 - 2)
+
 /* Helper macro for logging */
 #define rte_acc_log(level, fmt, ...) \
 	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \
diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c
index 23bc5d25bb..47609f95b7 100644
--- a/drivers/baseband/acc/rte_acc100_pmd.c
+++ b/drivers/baseband/acc/rte_acc100_pmd.c
@@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
 		ret = -ENOMEM;
 		goto free_lb_out;
 	}
+	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
+			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
+			RTE_CACHE_LINE_SIZE, conf->socket);
+	if (q->derm_buffer == NULL) {
+		rte_bbdev_log(ERR, "Failed to allocate derm_buffer memory");
+		ret = -ENOMEM;
+		goto free_companion_ring_addr;
+	}
 
 	/*
 	 * Software queue ring wraps synchronously with the HW when it reaches
@@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
 	q_idx = acc100_find_free_queue_idx(dev, conf);
 	if (q_idx == -1) {
 		ret = -EINVAL;
-		goto free_companion_ring_addr;
+		goto free_derm_buffer;
 	}
 
 	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF;
@@ -804,6 +812,9 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
 	dev->data->queues[queue_id].queue_private = q;
 	return 0;
 
+free_derm_buffer:
+	rte_free(q->derm_buffer);
+	q->derm_buffer = NULL;
 free_companion_ring_addr:
 	rte_free(q->companion_ring_addr);
 	q->companion_ring_addr = NULL;
@@ -890,6 +901,7 @@ acc100_queue_release(struct rte_bbdev *dev, uint16_t q_id)
 		/* Mark the Queue as un-assigned */
 		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
 				(uint64_t) (1 << q->aq_id));
+		rte_free(q->derm_buffer);
 		rte_free(q->companion_ring_addr);
 		rte_free(q->lb_in);
 		rte_free(q->lb_out);
@@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	return 1;
 }
 
+/* Assess whether a work around is recommended for the deRM corner cases */
+static inline bool
+derm_workaround_recommended(struct rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q)
+{
+	if (!is_acc100(q))
+		return false;
+	int32_t e = ldpc_dec->cb_params.e;
+	int q_m = ldpc_dec->q_m;
+	int z_c = ldpc_dec->z_c;
+	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
+	bool recommended = false;
+
+	if (ldpc_dec->basegraph == 1) {
+		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
+			recommended = true;
+		else if ((e * ACC_LIM_21 > K * 64))
+			recommended = true;
+	} else {
+		if (q_m <= 2) {
+			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
+				recommended = true;
+			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
+				recommended = true;
+		} else if (e * ACC_LIM_14 > K * 64)
+			recommended = true;
+	}
+
+	return recommended;
+}
+
 /** Enqueue one decode operations for ACC100 device in CB mode */
 static inline int
 enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
-		uint16_t total_enqueued_cbs, bool same_op)
+		uint16_t total_enqueued_cbs, bool same_op,
+		struct rte_bbdev_queue_data *q_data)
 {
 	int ret;
 	if (unlikely(check_bit(op->ldpc_dec.op_flags,
@@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
 	} else {
 		struct acc_fcw_ld *fcw;
 		uint32_t seg_total_left;
+
+		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
+			RTE_SET_USED(q_data);
+			rte_bbdev_log(INFO, "Corner case may require deRM pre-processing");
+		}
+
 		fcw = &desc->req.fcw_ld;
 		q->d->fcw_ld_fill(op, fcw, harq_layout);
 
@@ -3721,7 +3770,7 @@ acc100_enqueue_ldpc_dec_cb(struct rte_bbdev_queue_data *q_data,
 			ops[i]->ldpc_dec.n_cb, ops[i]->ldpc_dec.q_m,
 			ops[i]->ldpc_dec.n_filler, ops[i]->ldpc_dec.cb_params.e,
 			same_op);
-		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op);
+		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op, q_data);
 		if (ret < 0) {
 			acc_enqueue_invalid(q_data);
 			break;
-- 
2.37.1

RE: [EXT] [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Akhil Goyal]

> Add function to detect if de-ratematch pre-processing is recommended for
> SW corner cases.
> Some specific 5GUL FEC corner cases may cause unintended back pressure
> and in some cases a potential stability issue on the ACC100.
> The PMD can detect such code block configuration and issue an info
> message to the user.
> 
> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
Hi Tom/Maxime,

Any further comments?

Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Maxime Coquelin]

On 11/1/22 05:13, Hernan Vargas wrote:
> Add function to detect if de-ratematch pre-processing is recommended for
> SW corner cases.
> Some specific 5GUL FEC corner cases may cause unintended back pressure
> and in some cases a potential stability issue on the ACC100.
> The PMD can detect such code block configuration and issue an info
> message to the user.
> 
> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> ---
>   drivers/baseband/acc/acc_common.h     |  8 ++++
>   drivers/baseband/acc/rte_acc100_pmd.c | 55 +++++++++++++++++++++++++--
>   2 files changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/baseband/acc/acc_common.h b/drivers/baseband/acc/acc_common.h
> index eae7eab4e9..6213b0b61e 100644
> --- a/drivers/baseband/acc/acc_common.h
> +++ b/drivers/baseband/acc/acc_common.h
> @@ -123,6 +123,14 @@
>   #define ACC_HARQ_ALIGN_64B          64
>   #define ACC_MAX_ZC                  384
>   
> +/* De-ratematch code rate limitation for recommended operation */
> +#define ACC_LIM_03 2  /* 0.03 */
> +#define ACC_LIM_09 6  /* 0.09 */
> +#define ACC_LIM_14 9  /* 0.14 */
> +#define ACC_LIM_21 14 /* 0.21 */
> +#define ACC_LIM_31 20 /* 0.31 */
> +#define ACC_MAX_E (128 * 1024 - 2)
> +
>   /* Helper macro for logging */
>   #define rte_acc_log(level, fmt, ...) \
>   	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \
> diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c
> index 23bc5d25bb..47609f95b7 100644
> --- a/drivers/baseband/acc/rte_acc100_pmd.c
> +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> @@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
>   		ret = -ENOMEM;
>   		goto free_lb_out;
>   	}
> +	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
> +			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
> +			RTE_CACHE_LINE_SIZE, conf->socket);
> +	if (q->derm_buffer == NULL) {
> +		rte_bbdev_log(ERR, "Failed to allocate derm_buffer memory");
> +		ret = -ENOMEM;
> +		goto free_companion_ring_addr;
> +	}
>   
>   	/*
>   	 * Software queue ring wraps synchronously with the HW when it reaches
> @@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
>   	q_idx = acc100_find_free_queue_idx(dev, conf);
>   	if (q_idx == -1) {
>   		ret = -EINVAL;
> -		goto free_companion_ring_addr;
> +		goto free_derm_buffer;
>   	}
>   
>   	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF;
> @@ -804,6 +812,9 @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
>   	dev->data->queues[queue_id].queue_private = q;
>   	return 0;
>   
> +free_derm_buffer:
> +	rte_free(q->derm_buffer);
> +	q->derm_buffer = NULL;
>   free_companion_ring_addr:
>   	rte_free(q->companion_ring_addr);
>   	q->companion_ring_addr = NULL;
> @@ -890,6 +901,7 @@ acc100_queue_release(struct rte_bbdev *dev, uint16_t q_id)
>   		/* Mark the Queue as un-assigned */
>   		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
>   				(uint64_t) (1 << q->aq_id));
> +		rte_free(q->derm_buffer);
>   		rte_free(q->companion_ring_addr);
>   		rte_free(q->lb_in);
>   		rte_free(q->lb_out);
> @@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	return 1;
>   }
>   
> +/* Assess whether a work around is recommended for the deRM corner cases */
> +static inline bool
> +derm_workaround_recommended(struct rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q)
> +{
> +	if (!is_acc100(q))
> +		return false;
> +	int32_t e = ldpc_dec->cb_params.e;
> +	int q_m = ldpc_dec->q_m;
> +	int z_c = ldpc_dec->z_c;
> +	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
> +	bool recommended = false;
> +
> +	if (ldpc_dec->basegraph == 1) {
> +		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
> +			recommended = true;
> +		else if ((e * ACC_LIM_21 > K * 64))
> +			recommended = true;
> +	} else {
> +		if (q_m <= 2) {
> +			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
> +				recommended = true;
> +			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
> +				recommended = true;
> +		} else if (e * ACC_LIM_14 > K * 64)
> +			recommended = true;
> +	}
> +
> +	return recommended;
> +}
> +
>   /** Enqueue one decode operations for ACC100 device in CB mode */
>   static inline int
>   enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
> -		uint16_t total_enqueued_cbs, bool same_op)
> +		uint16_t total_enqueued_cbs, bool same_op,
> +		struct rte_bbdev_queue_data *q_data)
>   {
>   	int ret;
>   	if (unlikely(check_bit(op->ldpc_dec.op_flags,
> @@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct rte_bbdev_dec_op *op,
>   	} else {
>   		struct acc_fcw_ld *fcw;
>   		uint32_t seg_total_left;
> +
> +		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
> +			RTE_SET_USED(q_data);
Why do we need q_data if it is not being used?

> +			rte_bbdev_log(INFO, "Corner case may require deRM pre-processing");
> +		}
> +
>   		fcw = &desc->req.fcw_ld;
>   		q->d->fcw_ld_fill(op, fcw, harq_layout);
>   
> @@ -3721,7 +3770,7 @@ acc100_enqueue_ldpc_dec_cb(struct rte_bbdev_queue_data *q_data,
>   			ops[i]->ldpc_dec.n_cb, ops[i]->ldpc_dec.q_m,
>   			ops[i]->ldpc_dec.n_filler, ops[i]->ldpc_dec.cb_params.e,
>   			same_op);
> -		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op);
> +		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op, q_data);
>   		if (ret < 0) {
>   			acc_enqueue_invalid(q_data);
>   			break;

RE: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Chautru, Nicolas]

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, November 3, 2022 7:10 AM
> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> gakhil@marvell.com; trix@redhat.com
> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner
> cases
> 
> 
> 
> On 11/1/22 05:13, Hernan Vargas wrote:
> > Add function to detect if de-ratematch pre-processing is recommended
> > for SW corner cases.
> > Some specific 5GUL FEC corner cases may cause unintended back pressure
> > and in some cases a potential stability issue on the ACC100.
> > The PMD can detect such code block configuration and issue an info
> > message to the user.
> >
> > Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> > ---
> >   drivers/baseband/acc/acc_common.h     |  8 ++++
> >   drivers/baseband/acc/rte_acc100_pmd.c | 55
> +++++++++++++++++++++++++--
> >   2 files changed, 60 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/baseband/acc/acc_common.h
> > b/drivers/baseband/acc/acc_common.h
> > index eae7eab4e9..6213b0b61e 100644
> > --- a/drivers/baseband/acc/acc_common.h
> > +++ b/drivers/baseband/acc/acc_common.h
> > @@ -123,6 +123,14 @@
> >   #define ACC_HARQ_ALIGN_64B          64
> >   #define ACC_MAX_ZC                  384
> >
> > +/* De-ratematch code rate limitation for recommended operation */
> > +#define ACC_LIM_03 2  /* 0.03 */ #define ACC_LIM_09 6  /* 0.09 */
> > +#define ACC_LIM_14 9  /* 0.14 */ #define ACC_LIM_21 14 /* 0.21 */
> > +#define ACC_LIM_31 20 /* 0.31 */ #define ACC_MAX_E (128 * 1024 - 2)
> > +
> >   /* Helper macro for logging */
> >   #define rte_acc_log(level, fmt, ...) \
> >   	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \ diff --git
> > a/drivers/baseband/acc/rte_acc100_pmd.c
> > b/drivers/baseband/acc/rte_acc100_pmd.c
> > index 23bc5d25bb..47609f95b7 100644
> > --- a/drivers/baseband/acc/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> > @@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev,
> uint16_t queue_id,
> >   		ret = -ENOMEM;
> >   		goto free_lb_out;
> >   	}
> > +	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
> > +			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
> > +			RTE_CACHE_LINE_SIZE, conf->socket);
> > +	if (q->derm_buffer == NULL) {
> > +		rte_bbdev_log(ERR, "Failed to allocate derm_buffer memory");
> > +		ret = -ENOMEM;
> > +		goto free_companion_ring_addr;
> > +	}
> >
> >   	/*
> >   	 * Software queue ring wraps synchronously with the HW when it
> > reaches @@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev *dev,
> uint16_t queue_id,
> >   	q_idx = acc100_find_free_queue_idx(dev, conf);
> >   	if (q_idx == -1) {
> >   		ret = -EINVAL;
> > -		goto free_companion_ring_addr;
> > +		goto free_derm_buffer;
> >   	}
> >
> >   	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF; @@ -804,6
> +812,9
> > @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
> >   	dev->data->queues[queue_id].queue_private = q;
> >   	return 0;
> >
> > +free_derm_buffer:
> > +	rte_free(q->derm_buffer);
> > +	q->derm_buffer = NULL;
> >   free_companion_ring_addr:
> >   	rte_free(q->companion_ring_addr);
> >   	q->companion_ring_addr = NULL;
> > @@ -890,6 +901,7 @@ acc100_queue_release(struct rte_bbdev *dev,
> uint16_t q_id)
> >   		/* Mark the Queue as un-assigned */
> >   		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
> >   				(uint64_t) (1 << q->aq_id));
> > +		rte_free(q->derm_buffer);
> >   		rte_free(q->companion_ring_addr);
> >   		rte_free(q->lb_in);
> >   		rte_free(q->lb_out);
> > @@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct
> rte_bbdev_dec_op *op,
> >   	return 1;
> >   }
> >
> > +/* Assess whether a work around is recommended for the deRM corner
> > +cases */ static inline bool derm_workaround_recommended(struct
> > +rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q) {
> > +	if (!is_acc100(q))
> > +		return false;
> > +	int32_t e = ldpc_dec->cb_params.e;
> > +	int q_m = ldpc_dec->q_m;
> > +	int z_c = ldpc_dec->z_c;
> > +	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
> > +	bool recommended = false;
> > +
> > +	if (ldpc_dec->basegraph == 1) {
> > +		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
> > +			recommended = true;
> > +		else if ((e * ACC_LIM_21 > K * 64))
> > +			recommended = true;
> > +	} else {
> > +		if (q_m <= 2) {
> > +			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
> > +				recommended = true;
> > +			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
> > +				recommended = true;
> > +		} else if (e * ACC_LIM_14 > K * 64)
> > +			recommended = true;
> > +	}
> > +
> > +	return recommended;
> > +}
> > +
> >   /** Enqueue one decode operations for ACC100 device in CB mode */
> >   static inline int
> >   enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct
> rte_bbdev_dec_op *op,
> > -		uint16_t total_enqueued_cbs, bool same_op)
> > +		uint16_t total_enqueued_cbs, bool same_op,
> > +		struct rte_bbdev_queue_data *q_data)
> >   {
> >   	int ret;
> >   	if (unlikely(check_bit(op->ldpc_dec.op_flags,
> > @@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct acc_queue
> *q, struct rte_bbdev_dec_op *op,
> >   	} else {
> >   		struct acc_fcw_ld *fcw;
> >   		uint32_t seg_total_left;
> > +
> > +		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
> > +			RTE_SET_USED(q_data);
> Why do we need q_data if it is not being used?

As you can guess this notably allows customer do consider running deRM from here as a local patch hence keeping the q_data accessible.
Basically we keep the prototype of the function enqueue_ldpc_dec_one_op_cb() compatible with such usage. 
I believe this is a good practice here. Let us know if you are not convinced. 
Thanks
Nic

> 
> > +			rte_bbdev_log(INFO, "Corner case may require deRM
> pre-processing");
> > +		}
> > +
> >   		fcw = &desc->req.fcw_ld;
> >   		q->d->fcw_ld_fill(op, fcw, harq_layout);
> >
> > @@ -3721,7 +3770,7 @@ acc100_enqueue_ldpc_dec_cb(struct
> rte_bbdev_queue_data *q_data,
> >   			ops[i]->ldpc_dec.n_cb, ops[i]->ldpc_dec.q_m,
> >   			ops[i]->ldpc_dec.n_filler, ops[i]-
> >ldpc_dec.cb_params.e,
> >   			same_op);
> > -		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op);
> > +		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op,
> q_data);
> >   		if (ret < 0) {
> >   			acc_enqueue_invalid(q_data);
> >   			break;

Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Maxime Coquelin]

Hi Nicolas,

On 11/3/22 15:59, Chautru, Nicolas wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, November 3, 2022 7:10 AM
>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com; trix@redhat.com
>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner
>> cases
>>
>>
>>
>> On 11/1/22 05:13, Hernan Vargas wrote:
>>> Add function to detect if de-ratematch pre-processing is recommended
>>> for SW corner cases.
>>> Some specific 5GUL FEC corner cases may cause unintended back pressure
>>> and in some cases a potential stability issue on the ACC100.
>>> The PMD can detect such code block configuration and issue an info
>>> message to the user.
>>>
>>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
>>> ---
>>>    drivers/baseband/acc/acc_common.h     |  8 ++++
>>>    drivers/baseband/acc/rte_acc100_pmd.c | 55
>> +++++++++++++++++++++++++--
>>>    2 files changed, 60 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/baseband/acc/acc_common.h
>>> b/drivers/baseband/acc/acc_common.h
>>> index eae7eab4e9..6213b0b61e 100644
>>> --- a/drivers/baseband/acc/acc_common.h
>>> +++ b/drivers/baseband/acc/acc_common.h
>>> @@ -123,6 +123,14 @@
>>>    #define ACC_HARQ_ALIGN_64B          64
>>>    #define ACC_MAX_ZC                  384
>>>
>>> +/* De-ratematch code rate limitation for recommended operation */
>>> +#define ACC_LIM_03 2  /* 0.03 */ #define ACC_LIM_09 6  /* 0.09 */
>>> +#define ACC_LIM_14 9  /* 0.14 */ #define ACC_LIM_21 14 /* 0.21 */
>>> +#define ACC_LIM_31 20 /* 0.31 */ #define ACC_MAX_E (128 * 1024 - 2)
>>> +
>>>    /* Helper macro for logging */
>>>    #define rte_acc_log(level, fmt, ...) \
>>>    	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \ diff --git
>>> a/drivers/baseband/acc/rte_acc100_pmd.c
>>> b/drivers/baseband/acc/rte_acc100_pmd.c
>>> index 23bc5d25bb..47609f95b7 100644
>>> --- a/drivers/baseband/acc/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc/rte_acc100_pmd.c
>>> @@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev,
>> uint16_t queue_id,
>>>    		ret = -ENOMEM;
>>>    		goto free_lb_out;
>>>    	}
>>> +	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
>>> +			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
>>> +			RTE_CACHE_LINE_SIZE, conf->socket);
>>> +	if (q->derm_buffer == NULL) {
>>> +		rte_bbdev_log(ERR, "Failed to allocate derm_buffer memory");
>>> +		ret = -ENOMEM;
>>> +		goto free_companion_ring_addr;
>>> +	}
>>>
>>>    	/*
>>>    	 * Software queue ring wraps synchronously with the HW when it
>>> reaches @@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev *dev,
>> uint16_t queue_id,
>>>    	q_idx = acc100_find_free_queue_idx(dev, conf);
>>>    	if (q_idx == -1) {
>>>    		ret = -EINVAL;
>>> -		goto free_companion_ring_addr;
>>> +		goto free_derm_buffer;
>>>    	}
>>>
>>>    	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF; @@ -804,6
>> +812,9
>>> @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
>>>    	dev->data->queues[queue_id].queue_private = q;
>>>    	return 0;
>>>
>>> +free_derm_buffer:
>>> +	rte_free(q->derm_buffer);
>>> +	q->derm_buffer = NULL;
>>>    free_companion_ring_addr:
>>>    	rte_free(q->companion_ring_addr);
>>>    	q->companion_ring_addr = NULL;
>>> @@ -890,6 +901,7 @@ acc100_queue_release(struct rte_bbdev *dev,
>> uint16_t q_id)
>>>    		/* Mark the Queue as un-assigned */
>>>    		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
>>>    				(uint64_t) (1 << q->aq_id));
>>> +		rte_free(q->derm_buffer);
>>>    		rte_free(q->companion_ring_addr);
>>>    		rte_free(q->lb_in);
>>>    		rte_free(q->lb_out);
>>> @@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct
>> rte_bbdev_dec_op *op,
>>>    	return 1;
>>>    }
>>>
>>> +/* Assess whether a work around is recommended for the deRM corner
>>> +cases */ static inline bool derm_workaround_recommended(struct
>>> +rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q) {
>>> +	if (!is_acc100(q))
>>> +		return false;
>>> +	int32_t e = ldpc_dec->cb_params.e;
>>> +	int q_m = ldpc_dec->q_m;
>>> +	int z_c = ldpc_dec->z_c;
>>> +	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
>>> +	bool recommended = false;
>>> +
>>> +	if (ldpc_dec->basegraph == 1) {
>>> +		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
>>> +			recommended = true;
>>> +		else if ((e * ACC_LIM_21 > K * 64))
>>> +			recommended = true;
>>> +	} else {
>>> +		if (q_m <= 2) {
>>> +			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
>>> +				recommended = true;
>>> +			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
>>> +				recommended = true;
>>> +		} else if (e * ACC_LIM_14 > K * 64)
>>> +			recommended = true;
>>> +	}
>>> +
>>> +	return recommended;
>>> +}
>>> +
>>>    /** Enqueue one decode operations for ACC100 device in CB mode */
>>>    static inline int
>>>    enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct
>> rte_bbdev_dec_op *op,
>>> -		uint16_t total_enqueued_cbs, bool same_op)
>>> +		uint16_t total_enqueued_cbs, bool same_op,
>>> +		struct rte_bbdev_queue_data *q_data)
>>>    {
>>>    	int ret;
>>>    	if (unlikely(check_bit(op->ldpc_dec.op_flags,
>>> @@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct acc_queue
>> *q, struct rte_bbdev_dec_op *op,
>>>    	} else {
>>>    		struct acc_fcw_ld *fcw;
>>>    		uint32_t seg_total_left;
>>> +
>>> +		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
>>> +			RTE_SET_USED(q_data);
>> Why do we need q_data if it is not being used?
> 
> As you can guess this notably allows customer do consider running deRM from here as a local patch hence keeping the q_data accessible.
> Basically we keep the prototype of the function enqueue_ldpc_dec_one_op_cb() compatible with such usage.
> I believe this is a good practice here. Let us know if you are not convinced.

I think it is better to just warn the user, without adding unused
parameters.

Thanks,
Maxime

> Thanks
> Nic
> 
>>
>>> +			rte_bbdev_log(INFO, "Corner case may require deRM
>> pre-processing");
>>> +		}
>>> +
>>>    		fcw = &desc->req.fcw_ld;
>>>    		q->d->fcw_ld_fill(op, fcw, harq_layout);
>>>
>>> @@ -3721,7 +3770,7 @@ acc100_enqueue_ldpc_dec_cb(struct
>> rte_bbdev_queue_data *q_data,
>>>    			ops[i]->ldpc_dec.n_cb, ops[i]->ldpc_dec.q_m,
>>>    			ops[i]->ldpc_dec.n_filler, ops[i]-
>>> ldpc_dec.cb_params.e,
>>>    			same_op);
>>> -		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op);
>>> +		ret = enqueue_ldpc_dec_one_op_cb(q, ops[i], i, same_op,
>> q_data);
>>>    		if (ret < 0) {
>>>    			acc_enqueue_invalid(q_data);
>>>    			break;
> 

RE: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Chautru, Nicolas]

Hi Maxime, Akhil, 


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, November 3, 2022 8:08 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas, Hernan
> <hernan.vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com;
> trix@redhat.com
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner
> cases
> 
> Hi Nicolas,
> 
> On 11/3/22 15:59, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, November 3, 2022 7:10 AM
> >> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> >> gakhil@marvell.com; trix@redhat.com
> >> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
> >> <qi.z.zhang@intel.com>
> >> Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM
> >> corner cases
> >>
> >>
> >>
> >> On 11/1/22 05:13, Hernan Vargas wrote:
> >>> Add function to detect if de-ratematch pre-processing is recommended
> >>> for SW corner cases.
> >>> Some specific 5GUL FEC corner cases may cause unintended back
> >>> pressure and in some cases a potential stability issue on the ACC100.
> >>> The PMD can detect such code block configuration and issue an info
> >>> message to the user.
> >>>
> >>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> >>> ---
> >>>    drivers/baseband/acc/acc_common.h     |  8 ++++
> >>>    drivers/baseband/acc/rte_acc100_pmd.c | 55
> >> +++++++++++++++++++++++++--
> >>>    2 files changed, 60 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/baseband/acc/acc_common.h
> >>> b/drivers/baseband/acc/acc_common.h
> >>> index eae7eab4e9..6213b0b61e 100644
> >>> --- a/drivers/baseband/acc/acc_common.h
> >>> +++ b/drivers/baseband/acc/acc_common.h
> >>> @@ -123,6 +123,14 @@
> >>>    #define ACC_HARQ_ALIGN_64B          64
> >>>    #define ACC_MAX_ZC                  384
> >>>
> >>> +/* De-ratematch code rate limitation for recommended operation */
> >>> +#define ACC_LIM_03 2  /* 0.03 */ #define ACC_LIM_09 6  /* 0.09 */
> >>> +#define ACC_LIM_14 9  /* 0.14 */ #define ACC_LIM_21 14 /* 0.21 */
> >>> +#define ACC_LIM_31 20 /* 0.31 */ #define ACC_MAX_E (128 * 1024 - 2)
> >>> +
> >>>    /* Helper macro for logging */
> >>>    #define rte_acc_log(level, fmt, ...) \
> >>>    	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \ diff
> >>> --git a/drivers/baseband/acc/rte_acc100_pmd.c
> >>> b/drivers/baseband/acc/rte_acc100_pmd.c
> >>> index 23bc5d25bb..47609f95b7 100644
> >>> --- a/drivers/baseband/acc/rte_acc100_pmd.c
> >>> +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> >>> @@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev,
> >> uint16_t queue_id,
> >>>    		ret = -ENOMEM;
> >>>    		goto free_lb_out;
> >>>    	}
> >>> +	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
> >>> +			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
> >>> +			RTE_CACHE_LINE_SIZE, conf->socket);
> >>> +	if (q->derm_buffer == NULL) {
> >>> +		rte_bbdev_log(ERR, "Failed to allocate derm_buffer memory");
> >>> +		ret = -ENOMEM;
> >>> +		goto free_companion_ring_addr;
> >>> +	}
> >>>
> >>>    	/*
> >>>    	 * Software queue ring wraps synchronously with the HW when it
> >>> reaches @@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev
> >>> *dev,
> >> uint16_t queue_id,
> >>>    	q_idx = acc100_find_free_queue_idx(dev, conf);
> >>>    	if (q_idx == -1) {
> >>>    		ret = -EINVAL;
> >>> -		goto free_companion_ring_addr;
> >>> +		goto free_derm_buffer;
> >>>    	}
> >>>
> >>>    	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF; @@ -804,6
> >> +812,9
> >>> @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
> >>>    	dev->data->queues[queue_id].queue_private = q;
> >>>    	return 0;
> >>>
> >>> +free_derm_buffer:
> >>> +	rte_free(q->derm_buffer);
> >>> +	q->derm_buffer = NULL;
> >>>    free_companion_ring_addr:
> >>>    	rte_free(q->companion_ring_addr);
> >>>    	q->companion_ring_addr = NULL;
> >>> @@ -890,6 +901,7 @@ acc100_queue_release(struct rte_bbdev *dev,
> >> uint16_t q_id)
> >>>    		/* Mark the Queue as un-assigned */
> >>>    		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
> >>>    				(uint64_t) (1 << q->aq_id));
> >>> +		rte_free(q->derm_buffer);
> >>>    		rte_free(q->companion_ring_addr);
> >>>    		rte_free(q->lb_in);
> >>>    		rte_free(q->lb_out);
> >>> @@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct
> >> rte_bbdev_dec_op *op,
> >>>    	return 1;
> >>>    }
> >>>
> >>> +/* Assess whether a work around is recommended for the deRM corner
> >>> +cases */ static inline bool derm_workaround_recommended(struct
> >>> +rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q) {
> >>> +	if (!is_acc100(q))
> >>> +		return false;
> >>> +	int32_t e = ldpc_dec->cb_params.e;
> >>> +	int q_m = ldpc_dec->q_m;
> >>> +	int z_c = ldpc_dec->z_c;
> >>> +	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
> >>> +	bool recommended = false;
> >>> +
> >>> +	if (ldpc_dec->basegraph == 1) {
> >>> +		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
> >>> +			recommended = true;
> >>> +		else if ((e * ACC_LIM_21 > K * 64))
> >>> +			recommended = true;
> >>> +	} else {
> >>> +		if (q_m <= 2) {
> >>> +			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
> >>> +				recommended = true;
> >>> +			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
> >>> +				recommended = true;
> >>> +		} else if (e * ACC_LIM_14 > K * 64)
> >>> +			recommended = true;
> >>> +	}
> >>> +
> >>> +	return recommended;
> >>> +}
> >>> +
> >>>    /** Enqueue one decode operations for ACC100 device in CB mode */
> >>>    static inline int
> >>>    enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct
> >> rte_bbdev_dec_op *op,
> >>> -		uint16_t total_enqueued_cbs, bool same_op)
> >>> +		uint16_t total_enqueued_cbs, bool same_op,
> >>> +		struct rte_bbdev_queue_data *q_data)
> >>>    {
> >>>    	int ret;
> >>>    	if (unlikely(check_bit(op->ldpc_dec.op_flags,
> >>> @@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct
> acc_queue
> >> *q, struct rte_bbdev_dec_op *op,
> >>>    	} else {
> >>>    		struct acc_fcw_ld *fcw;
> >>>    		uint32_t seg_total_left;
> >>> +
> >>> +		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
> >>> +			RTE_SET_USED(q_data);
> >> Why do we need q_data if it is not being used?
> >
> > As you can guess this notably allows customer do consider running deRM
> from here as a local patch hence keeping the q_data accessible.
> > Basically we keep the prototype of the function
> enqueue_ldpc_dec_one_op_cb() compatible with such usage.
> > I believe this is a good practice here. Let us know if you are not convinced.
> 
> I think it is better to just warn the user, without adding unused parameters.

Thanks Maxime. 
Akhil, any additional view on this? For the PMD usage and support this makes more sense to keep to prototype with q_data even with RTE_SET_USED().
I don't see any meaningful drawback justifying to push back. Still let us know. 

Thanks
Nic

RE: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner cases

[Chautru, Nicolas]

Given how close we are to 22.11, let's not argue over this. Hernan sending a new patch based on Maxime recommendation. 

> -----Original Message-----
> From: Chautru, Nicolas
> Sent: Thursday, November 3, 2022 9:21 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Vargas, Hernan
> <Hernan.Vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com;
> trix@redhat.com
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: RE: [PATCH v7 1/1] baseband/acc100: add detection for deRM corner
> cases
> 
> Hi Maxime, Akhil,
> 
> 
> > -----Original Message-----
> > From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > Sent: Thursday, November 3, 2022 8:08 AM
> > To: Chautru, Nicolas <nicolas.chautru@intel.com>; Vargas, Hernan
> > <hernan.vargas@intel.com>; dev@dpdk.org; gakhil@marvell.com;
> > trix@redhat.com
> > Cc: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM
> > corner cases
> >
> > Hi Nicolas,
> >
> > On 11/3/22 15:59, Chautru, Nicolas wrote:
> > > Hi Maxime,
> > >
> > >> -----Original Message-----
> > >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >> Sent: Thursday, November 3, 2022 7:10 AM
> > >> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
> > >> gakhil@marvell.com; trix@redhat.com
> > >> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
> > >> <qi.z.zhang@intel.com>
> > >> Subject: Re: [PATCH v7 1/1] baseband/acc100: add detection for deRM
> > >> corner cases
> > >>
> > >>
> > >>
> > >> On 11/1/22 05:13, Hernan Vargas wrote:
> > >>> Add function to detect if de-ratematch pre-processing is
> > >>> recommended for SW corner cases.
> > >>> Some specific 5GUL FEC corner cases may cause unintended back
> > >>> pressure and in some cases a potential stability issue on the ACC100.
> > >>> The PMD can detect such code block configuration and issue an info
> > >>> message to the user.
> > >>>
> > >>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
> > >>> ---
> > >>>    drivers/baseband/acc/acc_common.h     |  8 ++++
> > >>>    drivers/baseband/acc/rte_acc100_pmd.c | 55
> > >> +++++++++++++++++++++++++--
> > >>>    2 files changed, 60 insertions(+), 3 deletions(-)
> > >>>
> > >>> diff --git a/drivers/baseband/acc/acc_common.h
> > >>> b/drivers/baseband/acc/acc_common.h
> > >>> index eae7eab4e9..6213b0b61e 100644
> > >>> --- a/drivers/baseband/acc/acc_common.h
> > >>> +++ b/drivers/baseband/acc/acc_common.h
> > >>> @@ -123,6 +123,14 @@
> > >>>    #define ACC_HARQ_ALIGN_64B          64
> > >>>    #define ACC_MAX_ZC                  384
> > >>>
> > >>> +/* De-ratematch code rate limitation for recommended operation */
> > >>> +#define ACC_LIM_03 2  /* 0.03 */ #define ACC_LIM_09 6  /* 0.09 */
> > >>> +#define ACC_LIM_14 9  /* 0.14 */ #define ACC_LIM_21 14 /* 0.21 */
> > >>> +#define ACC_LIM_31 20 /* 0.31 */ #define ACC_MAX_E (128 * 1024 -
> > >>> +2)
> > >>> +
> > >>>    /* Helper macro for logging */
> > >>>    #define rte_acc_log(level, fmt, ...) \
> > >>>    	rte_log(RTE_LOG_ ## level, RTE_LOG_NOTICE, fmt "\n", \ diff
> > >>> --git a/drivers/baseband/acc/rte_acc100_pmd.c
> > >>> b/drivers/baseband/acc/rte_acc100_pmd.c
> > >>> index 23bc5d25bb..47609f95b7 100644
> > >>> --- a/drivers/baseband/acc/rte_acc100_pmd.c
> > >>> +++ b/drivers/baseband/acc/rte_acc100_pmd.c
> > >>> @@ -756,6 +756,14 @@ acc100_queue_setup(struct rte_bbdev *dev,
> > >> uint16_t queue_id,
> > >>>    		ret = -ENOMEM;
> > >>>    		goto free_lb_out;
> > >>>    	}
> > >>> +	q->derm_buffer = rte_zmalloc_socket(dev->device->driver->name,
> > >>> +			RTE_BBDEV_TURBO_MAX_CB_SIZE * 10,
> > >>> +			RTE_CACHE_LINE_SIZE, conf->socket);
> > >>> +	if (q->derm_buffer == NULL) {
> > >>> +		rte_bbdev_log(ERR, "Failed to allocate derm_buffer
> memory");
> > >>> +		ret = -ENOMEM;
> > >>> +		goto free_companion_ring_addr;
> > >>> +	}
> > >>>
> > >>>    	/*
> > >>>    	 * Software queue ring wraps synchronously with the HW when it
> > >>> reaches @@ -776,7 +784,7 @@ acc100_queue_setup(struct rte_bbdev
> > >>> *dev,
> > >> uint16_t queue_id,
> > >>>    	q_idx = acc100_find_free_queue_idx(dev, conf);
> > >>>    	if (q_idx == -1) {
> > >>>    		ret = -EINVAL;
> > >>> -		goto free_companion_ring_addr;
> > >>> +		goto free_derm_buffer;
> > >>>    	}
> > >>>
> > >>>    	q->qgrp_id = (q_idx >> ACC100_GRP_ID_SHIFT) & 0xF; @@ -804,6
> > >> +812,9
> > >>> @@ acc100_queue_setup(struct rte_bbdev *dev, uint16_t queue_id,
> > >>>    	dev->data->queues[queue_id].queue_private = q;
> > >>>    	return 0;
> > >>>
> > >>> +free_derm_buffer:
> > >>> +	rte_free(q->derm_buffer);
> > >>> +	q->derm_buffer = NULL;
> > >>>    free_companion_ring_addr:
> > >>>    	rte_free(q->companion_ring_addr);
> > >>>    	q->companion_ring_addr = NULL; @@ -890,6 +901,7 @@
> > >>> acc100_queue_release(struct rte_bbdev *dev,
> > >> uint16_t q_id)
> > >>>    		/* Mark the Queue as un-assigned */
> > >>>    		d->q_assigned_bit_map[q->qgrp_id] &= (0xFFFFFFFFFFFFFFFF -
> > >>>    				(uint64_t) (1 << q->aq_id));
> > >>> +		rte_free(q->derm_buffer);
> > >>>    		rte_free(q->companion_ring_addr);
> > >>>    		rte_free(q->lb_in);
> > >>>    		rte_free(q->lb_out);
> > >>> @@ -3111,10 +3123,41 @@ harq_loopback(struct acc_queue *q, struct
> > >> rte_bbdev_dec_op *op,
> > >>>    	return 1;
> > >>>    }
> > >>>
> > >>> +/* Assess whether a work around is recommended for the deRM
> > >>> +corner cases */ static inline bool
> > >>> +derm_workaround_recommended(struct
> > >>> +rte_bbdev_op_ldpc_dec *ldpc_dec, struct acc_queue *q) {
> > >>> +	if (!is_acc100(q))
> > >>> +		return false;
> > >>> +	int32_t e = ldpc_dec->cb_params.e;
> > >>> +	int q_m = ldpc_dec->q_m;
> > >>> +	int z_c = ldpc_dec->z_c;
> > >>> +	int K = (ldpc_dec->basegraph == 1 ? ACC_K_ZC_1 : ACC_K_ZC_2) * z_c;
> > >>> +	bool recommended = false;
> > >>> +
> > >>> +	if (ldpc_dec->basegraph == 1) {
> > >>> +		if ((q_m == 4) && (z_c >= 320) && (e * ACC_LIM_31 > K * 64))
> > >>> +			recommended = true;
> > >>> +		else if ((e * ACC_LIM_21 > K * 64))
> > >>> +			recommended = true;
> > >>> +	} else {
> > >>> +		if (q_m <= 2) {
> > >>> +			if ((z_c >= 208) && (e * ACC_LIM_09 > K * 64))
> > >>> +				recommended = true;
> > >>> +			else if ((z_c < 208) && (e * ACC_LIM_03 > K * 64))
> > >>> +				recommended = true;
> > >>> +		} else if (e * ACC_LIM_14 > K * 64)
> > >>> +			recommended = true;
> > >>> +	}
> > >>> +
> > >>> +	return recommended;
> > >>> +}
> > >>> +
> > >>>    /** Enqueue one decode operations for ACC100 device in CB mode */
> > >>>    static inline int
> > >>>    enqueue_ldpc_dec_one_op_cb(struct acc_queue *q, struct
> > >> rte_bbdev_dec_op *op,
> > >>> -		uint16_t total_enqueued_cbs, bool same_op)
> > >>> +		uint16_t total_enqueued_cbs, bool same_op,
> > >>> +		struct rte_bbdev_queue_data *q_data)
> > >>>    {
> > >>>    	int ret;
> > >>>    	if (unlikely(check_bit(op->ldpc_dec.op_flags,
> > >>> @@ -3168,6 +3211,12 @@ enqueue_ldpc_dec_one_op_cb(struct
> > acc_queue
> > >> *q, struct rte_bbdev_dec_op *op,
> > >>>    	} else {
> > >>>    		struct acc_fcw_ld *fcw;
> > >>>    		uint32_t seg_total_left;
> > >>> +
> > >>> +		if (derm_workaround_recommended(&op->ldpc_dec, q)) {
> > >>> +			RTE_SET_USED(q_data);
> > >> Why do we need q_data if it is not being used?
> > >
> > > As you can guess this notably allows customer do consider running
> > > deRM
> > from here as a local patch hence keeping the q_data accessible.
> > > Basically we keep the prototype of the function
> > enqueue_ldpc_dec_one_op_cb() compatible with such usage.
> > > I believe this is a good practice here. Let us know if you are not convinced.
> >
> > I think it is better to just warn the user, without adding unused parameters.
> 
> Thanks Maxime.
> Akhil, any additional view on this? For the PMD usage and support this makes
> more sense to keep to prototype with q_data even with RTE_SET_USED().
> I don't see any meaningful drawback justifying to push back. Still let us know.
> 
> Thanks
> Nic
>