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

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

> From: Ananyev, Konstantin <konstantin.ananyev@intel.com>
> 
> 
> 
> > -----Original Message-----
> > From: dev <dev-bounces@dpdk.org> On Behalf Of Xu, Rosen
> > Sent: Thursday, September 3, 2020 3:34 AM
> > To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> > akhil.goyal@nxp.com
> > Cc: Richardson, Bruce <bruce.richardson@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC
> > processing functions
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Chautru, Nicolas <nicolas.chautru@intel.com>
> > > Sent: Sunday, August 30, 2020 2:01
> > > To: Xu, Rosen <rosen.xu@intel.com>; dev@dpdk.org;
> > > akhil.goyal@nxp.com
> > > Cc: Richardson, Bruce <bruce.richardson@intel.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC
> > > processing functions
> > >
> > > 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.
> >
> > What I mentioned is this changes about mbuf should move to librte_mbuf.
> > And it's better to align Olivier Matz.
> 
> There is already rte_pktmbuf_append() inside rte_mbuf.h.
> Wouldn't it suit?
> 

Hi Ananyev, Rosen, 
I agree that this can be confusing at first look and notably compared to packet processing. 
Note first that this same existing syntaxwhich  is already used in all bbdev PMDs when manipulating outbound mbufs in the context of base band signal processing (not really a packet as for NIC or other devices). 
Nothing new in that very PMD as this follows existing logic already in DPDK bbdev PMDs. 

This function basically differs from the typical rte_pktmbuf_append() as this is not appending data in the last mbuf but is used to potentially  update sequentially data for any mbufs in the middle from preallocated data hence it takes 2 arguments for both the head and the current mbuf segment in the list. 
There may be a more elegant way to do this down the line notably once there is a proposal to handle gracefully large mbufs (another usecase we have to handle in a slightly custom way). But I believe that is orthogonal to that very PMD serie which keeps on reling on using existing logic.  




> >
> > > 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