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

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev <dev-bounces at dpdk.org> On Behalf Of Xu, Rosen
> Sent: Thursday, September 3, 2020 3:34 AM
> To: Chautru, Nicolas <nicolas.chautru at intel.com>; dev at dpdk.org; akhil.goyal at nxp.com
> Cc: Richardson, Bruce <bruce.richardson at intel.com>
> Subject: Re: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC processing functions
> 
> Hi,
> 
> > -----Original Message-----
> > From: Chautru, Nicolas <nicolas.chautru at intel.com>
> > Sent: Sunday, August 30, 2020 2:01
> > To: Xu, Rosen <rosen.xu at intel.com>; dev at dpdk.org; akhil.goyal at nxp.com
> > Cc: Richardson, Bruce <bruce.richardson at intel.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 05/11] baseband/acc100: add LDPC
> > processing functions
> >
> > Hi Rosen,
> >
> > > From: Xu, Rosen <rosen.xu at intel.com>
> > >
> > > Hi,
> > >
> > > > -----Original Message-----
> > > > From: dev <dev-bounces at dpdk.org> On Behalf Of Nicolas Chautru
> > > > Sent: Wednesday, August 19, 2020 8:25
> > > > To: dev at dpdk.org; akhil.goyal at nxp.com
> > > > Cc: Richardson, Bruce <bruce.richardson at intel.com>; Chautru, Nicolas
> > > > <nicolas.chautru at 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 at 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?

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