Hi Tom,
> -----Original Message-----
> From: Tom Rix <t...@redhat.com>
> Sent: Thursday, October 7, 2021 5:01 AM
> To: Chautru, Nicolas <nicolas.chau...@intel.com>; dev@dpdk.org;
> gak...@marvell.com; nipun.gu...@nxp.com
> Cc: tho...@monjalon.net; Zhang, Mingshan <mingshan.zh...@intel.com>;
> Joshi, Arun <arun.jo...@intel.com>; hemant.agra...@nxp.com;
> david.march...@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.chau...@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 */
