Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption

[Nipun Gupta <nipun.gupta@nxp.com>]

> -----Original Message-----
> From: Chautru, Nicolas <nicolas.chautru@intel.com>
> Sent: Thursday, October 7, 2021 9:12 PM
> To: Akhil Goyal <gakhil@marvell.com>; dev@dpdk.org; Nipun Gupta
> <nipun.gupta@nxp.com>; trix@redhat.com
> Cc: thomas@monjalon.net; Zhang, Mingshan <mingshan.zhang@intel.com>;
> Joshi, Arun <arun.joshi@intel.com>; Hemant Agrawal
> <hemant.agrawal@nxp.com>; david.marchand@redhat.com
> Subject: RE: [EXT] [PATCH v9] bbdev: add device info related to data endianness
> assumption
> 
> Hi Akhil,
> 
> 
> > -----Original Message-----
> > From: Akhil Goyal <gakhil@marvell.com>
> > Sent: Thursday, October 7, 2021 6:14 AM
> > To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> > nipun.gupta@nxp.com; trix@redhat.com
> > Cc: thomas@monjalon.net; Zhang, Mingshan <mingshan.zhang@intel.com>;
> > Joshi, Arun <arun.joshi@intel.com>; hemant.agrawal@nxp.com;
> > david.marchand@redhat.com
> > Subject: RE: [EXT] [PATCH v9] bbdev: add device info related to data
> > endianness assumption
> >
> > > Subject: [EXT] [PATCH v9] bbdev: add device info related to data
> > > endianness assumption
> > >
> > Title is too long.
> > bbdev: add dev info for data endianness
> 
> OK
> 
> >
> > > Adding device information to capture explicitly the assumption of the
> > > input/output data byte endianness being processed.
> > >
> > > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > > ---
> > >  doc/guides/rel_notes/release_21_11.rst             | 1 +
> > >  drivers/baseband/acc100/rte_acc100_pmd.c           | 1 +
> > >  drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c | 1 +
> > >  drivers/baseband/fpga_lte_fec/fpga_lte_fec.c       | 1 +
> > >  drivers/baseband/turbo_sw/bbdev_turbo_software.c   | 1 +
> > >  lib/bbdev/rte_bbdev.h                              | 8 ++++++++
> > >  6 files changed, 13 insertions(+)
> > >
> > > diff --git a/doc/guides/rel_notes/release_21_11.rst
> > > b/doc/guides/rel_notes/release_21_11.rst
> > > index a8900a3..f0b3006 100644
> > > --- a/doc/guides/rel_notes/release_21_11.rst
> > > +++ b/doc/guides/rel_notes/release_21_11.rst
> > > @@ -191,6 +191,7 @@ API Changes
> > >
> > >  * bbdev: Added capability related to more comprehensive CRC options.
> > >
> > > +* bbdev: Added device info related to data byte endianness processing
> > > assumption.
> >
> > It is not clear from the description or the release notes, what the application
> > is supposed to do based on the new dev_info field set and how the driver
> > determine what value to set?
> > Isn't there a standard from the application stand point that the input/output
> > data Should be in BE or in LE like in case of IP packets which are always in BE?
> > I mean why is it dependent on the PMD which is processing it?
> > Whatever application understands, PMD should comply with that and do
> > internal Swapping if it does not support it.
> > Am I missing something?
> 
> This is really to allow Nipin to add his own NXP la12xx PMD, which appears to
> have different assumption on endianness.
> All existing processing is done in LE by default by the existing PMDs and the
> existing ecosystem.
> I cannot comment on why they would want to do that for the la12xx specifically,
> I could only speculate but here trying to help to find the best way for the new
> PMD to be supported.
> So here this suggested change is purely about exposing different assumption for
> the PMDs, so that this new PMD can still be supported under this API even
> though this is in effect incompatible with existing ecosystem.
> In case the application has different assumption that what the PMD does, then
> byte swapping would have to be done in the application, more likely I assume
> that la12xx has its own ecosystem with different endianness required for other
> reasons.
> The option you are suggesting would be to put the burden on the PMD but I
> doubt there is an actual usecase for that. I assume they assume different
> endianness for other specific reason, not necessary to be compatible with
> existing ecosystem.
> Niping, Hemant, feel free to comment back, from previous discussion I believe
> this is what you wanted to do. Unsure of the reason, feel free to share more
> details or not.

Akhil/Nicolas,

As Hemant mentioned on v4 (previously asked by Dave)

"---
If we go back to the data providing source i.e. FAPI interface, it is 
implementation specific, as per SCF222.

Our customers do use BE data in network and at FAPI interface.

In LA12xx, at present, we use u8 Big-endian data for processing to FECA 
engine.  We do see that other drivers in DPDK are using Little Endian 
*(with u32 data)* but standards is open for both.
"---

Standard is not specific to endianness and is open for implementation.
So it does not makes a reason to have one endianness as default and
other managed in the PMD, and the current change seems right.

Yes endianness assumption is taken in the test vector input/output data,
but this should be acceptable as it does not impact the PMD's and
end user applications in general.

BTW Nicolos, my name is Nipun :)

> 
> 
> >
> > >
> > >  ABI Changes
> > >  -----------
> > > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > > index 4e2feef..eb2c6c1 100644
> > > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > > @@ -1089,6 +1089,7 @@
> > >  #else
> > >  	dev_info->harq_buffer_size = 0;
> > >  #endif
> > > +	dev_info->data_endianness = RTE_BBDEV_LITTLE_ENDIAN;
> > >  	acc100_check_ir(d);
> > >  }
> > >
> > > diff --git a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > > b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > > index 6485cc8..c7f15c0 100644
> > > --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > > +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > > @@ -372,6 +372,7 @@
> > >  	dev_info->default_queue_conf = default_queue_conf;
> > >  	dev_info->capabilities = bbdev_capabilities;
> > >  	dev_info->cpu_flag_reqs = NULL;
> > > +	dev_info->data_endianness = RTE_BBDEV_LITTLE_ENDIAN;
> > >
> > >  	/* Calculates number of queues assigned to device */
> > >  	dev_info->max_num_queues = 0;
> > > diff --git a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > > b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > > index 350c424..72e213e 100644
> > > --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > > +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > > @@ -644,6 +644,7 @@ struct __rte_cache_aligned fpga_queue {
> > >  	dev_info->default_queue_conf = default_queue_conf;
> > >  	dev_info->capabilities = bbdev_capabilities;
> > >  	dev_info->cpu_flag_reqs = NULL;
> > > +	dev_info->data_endianness = RTE_BBDEV_LITTLE_ENDIAN;
> > >
> > >  	/* Calculates number of queues assigned to device */
> > >  	dev_info->max_num_queues = 0;
> > > diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > > b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > > index e1db2bf..0cab91a 100644
> > > --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > > +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > > @@ -253,6 +253,7 @@ struct turbo_sw_queue {
> > >  	dev_info->capabilities = bbdev_capabilities;
> > >  	dev_info->min_alignment = 64;
> > >  	dev_info->harq_buffer_size = 0;
> > > +	dev_info->data_endianness = RTE_BBDEV_LITTLE_ENDIAN;
> > >
> > >  	rte_bbdev_log_debug("got device info from %u\n", dev->data-
> > > >dev_id);
> > >  }
> > > diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> > > 3ebf62e..b3f3000 100644
> > > --- a/lib/bbdev/rte_bbdev.h
> > > +++ b/lib/bbdev/rte_bbdev.h
> > > @@ -49,6 +49,12 @@ enum rte_bbdev_state {
> > >  	RTE_BBDEV_INITIALIZED
> > >  };
> > >
> > > +/** Definitions of device data byte endianness types */ enum
> > > +rte_bbdev_endianness {
> > > +	RTE_BBDEV_BIG_ENDIAN,    /**< Data with byte-endianness BE */
> > > +	RTE_BBDEV_LITTLE_ENDIAN, /**< Data with byte-endianness LE */ };
> > If at all be need this dev_info field,
> > as Tom suggested we should use RTE_BIG/LITTLE_ENDIAN.
> 
> See separate comment on my reply to Tom:
> I considered this but the usage is different, these are build time #define, and
> really would bring confusion here.
> Note that there are not really the endianness of the system itself but specific to
> the bbdev data output going through signal processing.
> I thought it was more explicit and less confusing this way, feel free to comment
> back.
> NXP would know best why a different endianness would be required in the PMD.

Please see previous comment for endianness support.
I agree with the RTE_ prefix we can add it as it is for the application interface.

> 
> >
> > > +
> > >  /**
> > >   * Get the total number of devices that have been successfully initialised.
> > >   *
> > > @@ -309,6 +315,8 @@ struct rte_bbdev_driver_info {
> > >  	uint16_t min_alignment;
> > >  	/** HARQ memory available in kB */
> > >  	uint32_t harq_buffer_size;
> > > +	/** Byte endianness assumption for input/output data */
> > > +	enum rte_bbdev_endianness data_endianness;
> >
> > We should define how the input and output data are expected from the app.
> > If need be, we can define a simple ``bool swap`` instead of an enum.
> 
> This could be done as well. Default no swap, and swap required for the new
> PMD.
> I will let Nipin/Hemant comment back.

Again endianness is implementation specific and not standard for 5G processing,
unlike it is for network packet.

Regards,
Nipun

> 
> >
> > >  	/** Default queue configuration used if none is supplied  */
> > >  	struct rte_bbdev_queue_conf default_queue_conf;
> > >  	/** Device operation capabilities */
> > > --
> > > 1.8.3.1