DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption
@ 2021-10-06 20:58 Nicolas Chautru
  2021-10-06 20:58 ` Nicolas Chautru
  2021-10-07  2:22 ` [dpdk-dev] " Nipun Gupta
  0 siblings, 2 replies; 10+ messages in thread
From: Nicolas Chautru @ 2021-10-06 20:58 UTC (permalink / raw)
  To: dev, gakhil, nipun.gupta, trix
  Cc: thomas, mingshan.zhang, arun.joshi, hemant.agrawal,
	david.marchand, Nicolas Chautru

Suggesting this bbdev api change attached to address the requirement for the new
bbdev PMD from this serie : https://patches.dpdk.org/project/dpdk/patch/20211006113112.11163-2-nipun.gupta@nxp.com/
Nipin, please review and you can use this one in case that is okay with you.
This is defined as an enum and default values set in existing PMDs.

Nicolas Chautru (1):
  bbdev: add device info related to data endianness assumption

 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(+)

-- 
1.8.3.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-06 20:58 [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption Nicolas Chautru
@ 2021-10-06 20:58 ` Nicolas Chautru
  2021-10-07 12:01   ` Tom Rix
  2021-10-07 13:13   ` [dpdk-dev] [EXT] " Akhil Goyal
  2021-10-07  2:22 ` [dpdk-dev] " Nipun Gupta
  1 sibling, 2 replies; 10+ messages in thread
From: Nicolas Chautru @ 2021-10-06 20:58 UTC (permalink / raw)
  To: dev, gakhil, nipun.gupta, trix
  Cc: thomas, mingshan.zhang, arun.joshi, hemant.agrawal,
	david.marchand, Nicolas Chautru

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-06 20:58 [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption Nicolas Chautru
  2021-10-06 20:58 ` Nicolas Chautru
@ 2021-10-07  2:22 ` Nipun Gupta
  1 sibling, 0 replies; 10+ messages in thread
From: Nipun Gupta @ 2021-10-07  2:22 UTC (permalink / raw)
  To: Nicolas Chautru, dev, gakhil, trix
  Cc: thomas, mingshan.zhang, arun.joshi, Hemant Agrawal, david.marchand

Sure Nicolas, this seems fine.
I'll use this change instead of existing patch 1/10 and send a re-spin.

Thanks,
Nipun

> -----Original Message-----
> From: Nicolas Chautru <nicolas.chautru@intel.com>
> Sent: Thursday, October 7, 2021 2:29 AM
> To: dev@dpdk.org; gakhil@marvell.com; Nipun Gupta <nipun.gupta@nxp.com>;
> trix@redhat.com
> Cc: thomas@monjalon.net; mingshan.zhang@intel.com; arun.joshi@intel.com;
> Hemant Agrawal <hemant.agrawal@nxp.com>; david.marchand@redhat.com;
> Nicolas Chautru <nicolas.chautru@intel.com>
> Subject: [PATCH v9] bbdev: add device info related to data endianness
> assumption
> 
> Suggesting this bbdev api change attached to address the requirement for the
> new
> bbdev PMD from this serie :
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatches.
> dpdk.org%2Fproject%2Fdpdk%2Fpatch%2F20211006113112.11163-2-
> nipun.gupta%40nxp.com%2F&amp;data=04%7C01%7Cnipun.gupta%40nxp.com
> %7Cd99e521071fc4162dfa508d9890c1b8a%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C637691507373998592%7CUnknown%7CTWFpbGZsb3d8eyJ
> WIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C
> 1000&amp;sdata=ogEvbxTKLnCAPs1cuIN76bXb1Mjty0crd3wotOmw%2Fcg%3D
> &amp;reserved=0
> Nipin, please review and you can use this one in case that is okay with you.
> This is defined as an enum and default values set in existing PMDs.
> 
> Nicolas Chautru (1):
>   bbdev: add device info related to data endianness assumption
> 
>  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(+)
> 
> --
> 1.8.3.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-06 20:58 ` Nicolas Chautru
@ 2021-10-07 12:01   ` Tom Rix
  2021-10-07 15:19     ` Chautru, Nicolas
  2021-10-07 13:13   ` [dpdk-dev] [EXT] " Akhil Goyal
  1 sibling, 1 reply; 10+ messages in thread
From: Tom Rix @ 2021-10-07 12:01 UTC (permalink / raw)
  To: Nicolas Chautru, dev, gakhil, nipun.gupta
  Cc: thomas, mingshan.zhang, arun.joshi, hemant.agrawal, david.marchand


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.

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

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-06 20:58 ` Nicolas Chautru
  2021-10-07 12:01   ` Tom Rix
@ 2021-10-07 13:13   ` Akhil Goyal
  2021-10-07 15:41     ` Chautru, Nicolas
  1 sibling, 1 reply; 10+ messages in thread
From: Akhil Goyal @ 2021-10-07 13:13 UTC (permalink / raw)
  To: Nicolas Chautru, dev, nipun.gupta, trix
  Cc: thomas, mingshan.zhang, arun.joshi, hemant.agrawal, david.marchand

> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-07 12:01   ` Tom Rix
@ 2021-10-07 15:19     ` Chautru, Nicolas
  0 siblings, 0 replies; 10+ messages in thread
From: Chautru, Nicolas @ 2021-10-07 15:19 UTC (permalink / raw)
  To: Tom Rix, dev, gakhil, nipun.gupta
  Cc: thomas, Zhang, Mingshan, Joshi, Arun, hemant.agrawal, david.marchand

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-07 13:13   ` [dpdk-dev] [EXT] " Akhil Goyal
@ 2021-10-07 15:41     ` Chautru, Nicolas
  2021-10-07 16:49       ` Nipun Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Chautru, Nicolas @ 2021-10-07 15:41 UTC (permalink / raw)
  To: Akhil Goyal, dev, nipun.gupta, trix
  Cc: thomas, Zhang, Mingshan, Joshi, Arun, hemant.agrawal, david.marchand

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-07 15:41     ` Chautru, Nicolas
@ 2021-10-07 16:49       ` Nipun Gupta
  2021-10-07 18:58         ` Chautru, Nicolas
  0 siblings, 1 reply; 10+ messages in thread
From: Nipun Gupta @ 2021-10-07 16:49 UTC (permalink / raw)
  To: Chautru, Nicolas, Akhil Goyal, dev, trix
  Cc: thomas, Zhang, Mingshan, Joshi, Arun, Hemant Agrawal, david.marchand



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

BTW Nicolos, my name is Nipun :)

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-07 16:49       ` Nipun Gupta
@ 2021-10-07 18:58         ` Chautru, Nicolas
  2021-10-08  4:34           ` Nipun Gupta
  0 siblings, 1 reply; 10+ messages in thread
From: Chautru, Nicolas @ 2021-10-07 18:58 UTC (permalink / raw)
  To: Nipun Gupta, Akhil Goyal, dev, trix
  Cc: thomas, Zhang, Mingshan, Joshi, Arun, Hemant Agrawal, david.marchand

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. 

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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
  2021-10-07 18:58         ` Chautru, Nicolas
@ 2021-10-08  4:34           ` Nipun Gupta
  0 siblings, 0 replies; 10+ messages in thread
From: Nipun Gupta @ 2021-10-08  4:34 UTC (permalink / raw)
  To: Chautru, Nicolas, Akhil Goyal, dev, trix
  Cc: thomas, Zhang, Mingshan, Joshi, Arun, Hemant Agrawal, david.marchand



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


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-10-08  4:34 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-06 20:58 [dpdk-dev] [PATCH v9] bbdev: add device info related to data endianness assumption Nicolas Chautru
2021-10-06 20:58 ` Nicolas Chautru
2021-10-07 12:01   ` Tom Rix
2021-10-07 15:19     ` Chautru, Nicolas
2021-10-07 13:13   ` [dpdk-dev] [EXT] " Akhil Goyal
2021-10-07 15:41     ` Chautru, Nicolas
2021-10-07 16:49       ` Nipun Gupta
2021-10-07 18:58         ` Chautru, Nicolas
2021-10-08  4:34           ` Nipun Gupta
2021-10-07  2:22 ` [dpdk-dev] " Nipun Gupta

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).