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: Friday, October 8, 2021 12:29 AM
> To: Nipun Gupta <nipun.gupta@nxp.com>; Akhil Goyal <gakhil@marvell.com>;
> dev@dpdk.org; 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 Nipun,
> 
> > -----Original Message-----
> > From: Nipun Gupta <nipun.gupta@nxp.com>
> > Sent: Thursday, October 7, 2021 9:49 AM
> > To: Chautru, Nicolas <nicolas.chautru@intel.com>; Akhil Goyal
> > <gakhil@marvell.com>; dev@dpdk.org; 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
> >
> >
> >
> > > -----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.
> 
> I want clarify that this would impact the application in case user wanted to
> switch between 2 such hw accelator.
> Ie. you cannot switch the 2 solutions, they are incompatible except if you
> explicitly do the byteswap in the application (as is done in bbdev-test).
> Not necessarily a problem in case they address 2 different ecosystems but
> capturing the implication to be explicit. Ie each device expose the assumptions
> expected by the application and it is up to the application the bbdev api to
> satisfy the relared assumptions.

Hi Nicolas,

Bbdev-test is one of a test application and it is using test vectors. Consider bbdev
example application, the packets are in the network order (i.e. big-endian) and no
swapping is being done in this application. It is probably assumed that the packets
which are arriving/going out at the network are in the endianness order which
are to be processed by the device.

Similarly in the real world usage/applications, the endianness handling would be done
on the other end (the originator/consumer of the data), which again could be done in
hw accelerators without impacting the real world applications.

Also, as standard is open for both, the current change makes sense and swapping in
any of the PMD does not seems suitable.

Regards,
Nipun

> 
> >
> > BTW Nicolos, my name is Nipun :)
> 
> My bad!
> 
> I am marking this patch as obsolete since you have included it in your serie.
> 
> 
> >
> > >
> > >
> > > >
> > > > >
> > > > >  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