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

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

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