Re: [dpdk-dev] [PATCH v3 04/14] baseband/turbo_sw: support large size code block

[Akhil Goyal <akhil.goyal@nxp.com>]

> 
> From: Nic Chautru <nicolas.chautru@intel.com>
> 
> This is to support cases when the input data for
> decoding a code block is larger than 64kB and would
> not fit as a contiguous block of data into one
> mbuf. In that case the length from the opearation
> supersedes the mbug default structure.
Spell check
Operation
Mbuf

> 
> Signed-off-by: Nic Chautru <nicolas.chautru@intel.com>
> ---
>  app/test-bbdev/test_bbdev_perf.c                 | 40 +++++++++++++++++++-----
>  doc/guides/rel_notes/release_20_05.rst           |  6 ++++
>  drivers/baseband/turbo_sw/bbdev_turbo_software.c | 11 ++++---
>  3 files changed, 45 insertions(+), 12 deletions(-)
> 
> diff --git a/app/test-bbdev/test_bbdev_perf.c b/app/test-
> bbdev/test_bbdev_perf.c
> index d8db58e..d46966d 100644
> --- a/app/test-bbdev/test_bbdev_perf.c
> +++ b/app/test-bbdev/test_bbdev_perf.c
> @@ -764,6 +764,7 @@ typedef int (test_case_function)(struct active_device
> *ad,
>  {
>  	int ret;
>  	unsigned int i, j;
> +	bool large_input = false;
> 
>  	for (i = 0; i < n; ++i) {
>  		char *data;
> @@ -774,24 +775,47 @@ typedef int (test_case_function)(struct active_device
> *ad,
>  				op_type, n * ref_entries->nb_segments,
>  				mbuf_pool->size);
> 
> -		TEST_ASSERT_SUCCESS(((seg->length +
> RTE_PKTMBUF_HEADROOM) >
> -				(uint32_t)UINT16_MAX),
> -				"Given data is bigger than allowed mbuf
> segment size");
> -
> +		if (seg->length > 64000) {
Shouldn't this be seg->length + RTE_PKTMBUF_HEADROOM and you don't need
A check again in assert.
64000 should be a MACRO

> +			/*
> +			 * Special case when DPDK mbuf cannot handle
> +			 * the required input size
> +			 */
> +			printf("Warning: Larger input size than DPDK
> mbuf %d\n",
> +					seg->length);
> +			large_input = true;
> +		} else {
> +			TEST_ASSERT_SUCCESS(
> +					((seg->length +
> RTE_PKTMBUF_HEADROOM)
> +					> (uint32_t)UINT16_MAX),
> +					"Given data is bigger than allowed
> mbuf segment size"
> +					);
> +		}
>  		bufs[i].data = m_head;
>  		bufs[i].offset = 0;
>  		bufs[i].length = 0;
> 
>  		if ((op_type == DATA_INPUT) || (op_type ==
> DATA_HARQ_INPUT)) {
> -			data = rte_pktmbuf_append(m_head, seg->length);
> -			TEST_ASSERT_NOT_NULL(data,
> +			if ((op_type == DATA_INPUT) && large_input) {
> +				/* Allocate a fake overused mbuf */
> +				data = rte_malloc(NULL, 128 * 1024, 0);

Again hard coding. Define some macros to know the basis of this value.

> +				memcpy(data, seg->addr, seg->length);
> +				m_head->buf_addr = data;
> +				m_head->buf_iova = rte_mem_virt2phy(data);
> +				m_head->data_off = 0;
> +				m_head->data_len = seg->length;
> +			} else {
> +				data = rte_pktmbuf_append(m_head, seg-
> >length);
> +				TEST_ASSERT_NOT_NULL(data,
>  					"Couldn't append %u bytes to mbuf
> from %d data type mbuf pool",
>  					seg->length, op_type);
> 
> -			TEST_ASSERT(data == RTE_PTR_ALIGN(data,
> min_alignment),
> +				TEST_ASSERT(data == RTE_PTR_ALIGN(
> +						data, min_alignment),
>  					"Data addr in mbuf (%p) is not aligned
> to device min alignment (%u)",
>  					data, min_alignment);
> -			rte_memcpy(data, seg->addr, seg->length);
> +				rte_memcpy(data, seg->addr, seg->length);
> +			}
> +
>  			bufs[i].length += seg->length;
> 
>  			for (j = 1; j < ref_entries->nb_segments; ++j) {
> diff --git a/doc/guides/rel_notes/release_20_05.rst
> b/doc/guides/rel_notes/release_20_05.rst
> index 2190eaf..d6c3dfb 100644
> --- a/doc/guides/rel_notes/release_20_05.rst
> +++ b/doc/guides/rel_notes/release_20_05.rst
> @@ -56,6 +56,12 @@ New Features
>       Also, make sure to start the actual text at the margin.
>       =========================================================
> 
> +* **Updated the TURBO_SW bbdev PMD.**
> +
> +  Updated the ``turbo_sw`` bbdev driver with changes including:
> +
> +  * Support for large size code block not fitting in one mbuf segment.
> +  * Exposes the accurate LLR decimal assumption.

Is it really worth to highlight this in release note? You even suggested not to
Backport it to stable. I am not sure if it is really that important to be highlighted
In release notes.
If at all, this patch is not changing the LLR decimal. This should be moved to
Appropriate patch.

> 
>  Removed Items
>  -------------
> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> index 5ca8ca1..ea3fecb 100644
> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> @@ -1335,7 +1335,7 @@ struct turbo_sw_queue {
> 
>  static inline void
>  process_ldpc_dec_cb(struct turbo_sw_queue *q, struct rte_bbdev_dec_op *op,
> -		uint8_t c, uint16_t out_length, uint16_t e,
> +		uint8_t c, uint16_t out_length, uint32_t e,
>  		struct rte_mbuf *m_in,
>  		struct rte_mbuf *m_out_head, struct rte_mbuf *m_out,
>  		struct rte_mbuf *m_harq_in,
> @@ -1617,8 +1617,8 @@ struct turbo_sw_queue {
>  		struct rte_bbdev_stats *queue_stats)
>  {
>  	uint8_t c, r = 0;
> -	uint16_t e, out_length;
> -	uint16_t crc24_overlap = 0;
> +	uint32_t e;
> +	uint16_t out_length, crc24_overlap = 0;
>  	struct rte_bbdev_op_ldpc_dec *dec = &op->ldpc_dec;
>  	struct rte_mbuf *m_in = dec->input.data;
>  	struct rte_mbuf *m_harq_in = dec->harq_combined_input.data;
> @@ -1661,7 +1661,10 @@ struct turbo_sw_queue {
>  			e = (r < dec->tb_params.cab) ?
>  				dec->tb_params.ea : dec->tb_params.eb;
> 
> -		seg_total_left = rte_pktmbuf_data_len(m_in) - in_offset;
> +		if (e < 64000) /* Special case handling when overusing mbuf */
Macro for 64000

> +			seg_total_left = rte_pktmbuf_data_len(m_in) -
> in_offset;
> +		else
> +			seg_total_left = e;
> 
>  		process_ldpc_dec_cb(q, op, c, out_length, e,
>  				m_in, m_out_head, m_out,
> --
> 1.8.3.1