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

[Akhil Goyal <gakhil@marvell.com>]

> Subject: [EXT] [PATCH v9] bbdev: add device info related to data endianness
> assumption
> 
Title is too long.
bbdev: add dev info for data endianness

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

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

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

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