DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Chautru, Nicolas" <nicolas.chautru@intel.com>
To: Akhil Goyal <gakhil@marvell.com>, "dev@dpdk.org" <dev@dpdk.org>,
	"nipun.gupta@nxp.com" <nipun.gupta@nxp.com>,
	"trix@redhat.com" <trix@redhat.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
	"Zhang, Mingshan" <mingshan.zhang@intel.com>,
	"Joshi, Arun" <arun.joshi@intel.com>,
	"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
	"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [EXT] [PATCH v9] bbdev: add device info related to data endianness assumption
Date: Thu, 7 Oct 2021 15:41:38 +0000	[thread overview]
Message-ID: <BY5PR11MB445117BCD90E09CEE2C01510F8B19@BY5PR11MB4451.namprd11.prod.outlook.com> (raw)
In-Reply-To: <CO6PR18MB44842B5BBC87DB7DEBA14D56D8B19@CO6PR18MB4484.namprd18.prod.outlook.com>

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


  reply	other threads:[~2021-10-07 15:41 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-06 20:58 [dpdk-dev] " 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 [this message]
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

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=BY5PR11MB445117BCD90E09CEE2C01510F8B19@BY5PR11MB4451.namprd11.prod.outlook.com \
    --to=nicolas.chautru@intel.com \
    --cc=arun.joshi@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=gakhil@marvell.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=mingshan.zhang@intel.com \
    --cc=nipun.gupta@nxp.com \
    --cc=thomas@monjalon.net \
    --cc=trix@redhat.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).