Re: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC processing functions

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

Hi Rosen, 

> From: Xu, Rosen <rosen.xu@intel.com>
> 
> Hi,
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Nicolas Chautru
> > Sent: Wednesday, August 19, 2020 8:25
> > To: dev@dpdk.org; akhil.goyal@nxp.com
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>; Chautru, Nicolas
> > <nicolas.chautru@intel.com>
> > Subject: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC
> > processing functions
> >
> > Adding LDPC decode and encode processing operations
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >  drivers/baseband/acc100/rte_acc100_pmd.c | 1625
> > +++++++++++++++++++++++++++++-
> >  drivers/baseband/acc100/rte_acc100_pmd.h |    3 +
> >  2 files changed, 1626 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index 7a21c57..5f32813 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -15,6 +15,9 @@
> >  #include <rte_hexdump.h>
> >  #include <rte_pci.h>
> >  #include <rte_bus_pci.h>
> > +#ifdef RTE_BBDEV_OFFLOAD_COST
> > +#include <rte_cycles.h>
> > +#endif
> >
> >  #include <rte_bbdev.h>
> >  #include <rte_bbdev_pmd.h>
> > @@ -449,7 +452,6 @@
> >  	return 0;
> >  }
> >
> > -
> >  /**
> >   * Report a ACC100 queue index which is free
> >   * Return 0 to 16k for a valid queue_idx or -1 when no queue is available
> > @@ -634,6 +636,46 @@
> >  	struct acc100_device *d = dev->data->dev_private;
> >
> >  	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
> > +		{
> > +			.type   = RTE_BBDEV_OP_LDPC_ENC,
> > +			.cap.ldpc_enc = {
> > +				.capability_flags =
> > +					RTE_BBDEV_LDPC_RATE_MATCH |
> > +					RTE_BBDEV_LDPC_CRC_24B_ATTACH
> > |
> > +
> > 	RTE_BBDEV_LDPC_INTERLEAVER_BYPASS,
> > +				.num_buffers_src =
> > +
> > 	RTE_BBDEV_LDPC_MAX_CODE_BLOCKS,
> > +				.num_buffers_dst =
> > +
> > 	RTE_BBDEV_LDPC_MAX_CODE_BLOCKS,
> > +			}
> > +		},
> > +		{
> > +			.type   = RTE_BBDEV_OP_LDPC_DEC,
> > +			.cap.ldpc_dec = {
> > +			.capability_flags =
> > +				RTE_BBDEV_LDPC_CRC_TYPE_24B_CHECK |
> > +				RTE_BBDEV_LDPC_CRC_TYPE_24B_DROP |
> > +
> > 	RTE_BBDEV_LDPC_HQ_COMBINE_IN_ENABLE |
> > +
> > 	RTE_BBDEV_LDPC_HQ_COMBINE_OUT_ENABLE |
> > +#ifdef ACC100_EXT_MEM
> > +
> > 	RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_IN_ENABLE |
> > +
> > 	RTE_BBDEV_LDPC_INTERNAL_HARQ_MEMORY_OUT_ENABLE |
> > +#endif
> > +
> > 	RTE_BBDEV_LDPC_ITERATION_STOP_ENABLE |
> > +				RTE_BBDEV_LDPC_DEINTERLEAVER_BYPASS
> > |
> > +				RTE_BBDEV_LDPC_DECODE_BYPASS |
> > +				RTE_BBDEV_LDPC_DEC_SCATTER_GATHER |
> > +
> > 	RTE_BBDEV_LDPC_HARQ_6BIT_COMPRESSION |
> > +				RTE_BBDEV_LDPC_LLR_COMPRESSION,
> > +			.llr_size = 8,
> > +			.llr_decimals = 1,
> > +			.num_buffers_src =
> > +
> > 	RTE_BBDEV_LDPC_MAX_CODE_BLOCKS,
> > +			.num_buffers_hard_out =
> > +
> > 	RTE_BBDEV_LDPC_MAX_CODE_BLOCKS,
> > +			.num_buffers_soft_out = 0,
> > +			}
> > +		},
> >  		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
> >  	};
> >
> > @@ -669,9 +711,14 @@
> >  	dev_info->cpu_flag_reqs = NULL;
> >  	dev_info->min_alignment = 64;
> >  	dev_info->capabilities = bbdev_capabilities;
> > +#ifdef ACC100_EXT_MEM
> >  	dev_info->harq_buffer_size = d->ddr_size;
> > +#else
> > +	dev_info->harq_buffer_size = 0;
> > +#endif
> >  }
> >
> > +
> >  static const struct rte_bbdev_ops acc100_bbdev_ops = {
> >  	.setup_queues = acc100_setup_queues,
> >  	.close = acc100_dev_close,
> > @@ -696,6 +743,1577 @@
> >  	{.device_id = 0},
> >  };
> >
> > +/* Read flag value 0/1 from bitmap */
> > +static inline bool
> > +check_bit(uint32_t bitmap, uint32_t bitmask)
> > +{
> > +	return bitmap & bitmask;
> > +}
> > +
> > +static inline char *
> > +mbuf_append(struct rte_mbuf *m_head, struct rte_mbuf *m, uint16_t len)
> > +{
> > +	if (unlikely(len > rte_pktmbuf_tailroom(m)))
> > +		return NULL;
> > +
> > +	char *tail = (char *)m->buf_addr + m->data_off + m->data_len;
> > +	m->data_len = (uint16_t)(m->data_len + len);
> > +	m_head->pkt_len  = (m_head->pkt_len + len);
> > +	return tail;
> > +}
> 
> Is it reasonable to direct add data_len of rte_mbuf?
> 

Do you suggest to add directly without checking there is enough room in the mbuf? We cannot rely on the application providing mbuf with enough tailroom.
In case you ask about the 2 mbufs, this is because this function is used to also support segmented memory made of multiple mbufs segments. 
Note that this function is also used in other existing bbdev PMDs. In case you believe there is a better way to do this, we can certainly discuss and change these in several PMDs through another serie. 

Thanks for all the reviews and useful comments.
Nic