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

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

Hi Konstantin, Rosen, 
Replying to my own email. 
Can you confirm that the previous explanation below makes sense and can you ack this patch?

Thanks and regards, 
Nic

> From: Chautru, Nicolas
> 
> > 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