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

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

Hi Tom, 

> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Thursday, October 7, 2021 5:01 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> gakhil@marvell.com; nipun.gupta@nxp.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: [PATCH v9] bbdev: add device info related to data endianness
> assumption
> 
> 
> On 10/6/21 1:58 PM, Nicolas Chautru wrote:
> > 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 +
> 
> Missed bbdev_null.c
> 
> If this was intentional data_endianness is uninitialized or implicitly big endian.
> 
> It would be better to say it is unknown. which may mean another enum is
> needed.

I considered this but null driver doesn't touch data, so not relevant.
Still if preferred, Nipin feel free to set it in null_driver as well in your serie (with a comment that it is not relevant). 

> 
> >   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.
> >
> >   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 */ };
> 
> Could RTE_BIG|LITTLE_ENDIAN be reused ?

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. 

Thanks for the comments.

> 
> Tom
> 
> > +
> >   /**
> >    * 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;
> >   	/** Default queue configuration used if none is supplied  */
> >   	struct rte_bbdev_queue_conf default_queue_conf;
> >   	/** Device operation capabilities */