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

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

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. 


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

> 
> > +
> >  /**
> >   * 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.

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