diff mbox series

[v5,2/7] bbdev: add device status info

Message ID 1657150110-69957-3-git-send-email-nicolas.chautru@intel.com (mailing list archive)
State Superseded, archived
Delegated to: akhil goyal
Headers
Series bbdev changes for 22.11 |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Chautru, Nicolas July 6, 2022, 11:28 p.m. UTC 
  Added device status information, so that the PMD can
expose information related to the underlying accelerator device status.
Minor order change in structure to fit into padding hole.

Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
 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/la12xx/bbdev_la12xx.c             |  1 +
 drivers/baseband/null/bbdev_null.c                 |  1 +
 drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
 lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
 lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
 lib/bbdev/version.map                              |  6 ++++
 9 files changed, 67 insertions(+), 2 deletions(-)

Comments

Maxime Coquelin Aug. 25, 2022, 2:18 p.m. UTC | #1 
On 7/7/22 01:28, Nicolas Chautru wrote:
> Added device status information, so that the PMD can
> expose information related to the underlying accelerator device status.
> Minor order change in structure to fit into padding hole.
> 
> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> ---
>   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/la12xx/bbdev_la12xx.c             |  1 +
>   drivers/baseband/null/bbdev_null.c                 |  1 +
>   drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
>   lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
>   lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
>   lib/bbdev/version.map                              |  6 ++++
>   9 files changed, 67 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c b/drivers/baseband/acc100/rte_acc100_pmd.c
> index de7e4bc..17ba798 100644
> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> @@ -1060,6 +1060,7 @@
>   
>   	/* Read and save the populated config from ACC100 registers */
>   	fetch_acc100_config(dev);
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	/* This isn't ideal because it reports the maximum number of queues but
>   	 * does not provide info on how many can be uplink/downlink or different
> 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 82ae6ba..57b12af 100644
> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> @@ -369,6 +369,7 @@
>   	dev_info->capabilities = bbdev_capabilities;
>   	dev_info->cpu_flag_reqs = NULL;
>   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	/* 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 21d3529..2a330c4 100644
> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
>   	dev_info->capabilities = bbdev_capabilities;
>   	dev_info->cpu_flag_reqs = NULL;
>   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	/* Calculates number of queues assigned to device */
>   	dev_info->max_num_queues = 0;
> diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c b/drivers/baseband/la12xx/bbdev_la12xx.c
> index 4d1bd16..c1f88c6 100644
> --- a/drivers/baseband/la12xx/bbdev_la12xx.c
> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
>   	dev_info->capabilities = bbdev_capabilities;
>   	dev_info->cpu_flag_reqs = NULL;
>   	dev_info->min_alignment = 64;
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
>   }
> diff --git a/drivers/baseband/null/bbdev_null.c b/drivers/baseband/null/bbdev_null.c
> index 248e129..94a1976 100644
> --- a/drivers/baseband/null/bbdev_null.c
> +++ b/drivers/baseband/null/bbdev_null.c
> @@ -82,6 +82,7 @@ struct bbdev_queue {
>   	 * here for code completeness.
>   	 */
>   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
>   }
> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> index af7bc41..dbc5524 100644
> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> @@ -254,6 +254,7 @@ struct turbo_sw_queue {
>   	dev_info->min_alignment = 64;
>   	dev_info->harq_buffer_size = 0;
>   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>   
>   	rte_bbdev_log_debug("got device info from %u\n", dev->data->dev_id);
>   }
> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
> index 4da8047..38630a2 100644
> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -1133,3 +1133,25 @@ struct rte_mempool *
>   	rte_bbdev_log(ERR, "Invalid operation type");
>   	return NULL;
>   }
> +
> +const char *
> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status)
> +{
> +	static const char * const dev_sta_string[] = {
> +		"RTE_BBDEV_DEV_NOSTATUS",
> +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
> +		"RTE_BBDEV_DEV_RESET",
> +		"RTE_BBDEV_DEV_CONFIGURED",
> +		"RTE_BBDEV_DEV_ACTIVE",
> +		"RTE_BBDEV_DEV_FATAL_ERR",
> +		"RTE_BBDEV_DEV_RESTART_REQ",
> +		"RTE_BBDEV_DEV_RECONFIG_REQ",
> +		"RTE_BBDEV_DEV_CORRECT_ERR",
> +	};
> +
> +	if (status < sizeof(dev_sta_string) / sizeof(char *))
> +		return dev_sta_string[status];
> +
> +	rte_bbdev_log(ERR, "Invalid device status");
> +	return NULL;
> +}
> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h
> index b88c881..9b1ffa4 100644
> --- a/lib/bbdev/rte_bbdev.h
> +++ b/lib/bbdev/rte_bbdev.h
> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
>   int
>   rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
>   
> +/**
> + * Flags indicate the status of the device
> + */
> +enum rte_bbdev_device_status {
> +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
> +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not supported on the PMD */
> +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-configured state */
> +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and ready to use */
> +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is being used */
> +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal uncorrectable error */
> +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application to restart */
> +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires application to reconfigure queues */
> +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable error event happened */
> +};

I don't have a strong opinion on this, but I think NOT_SUPPORTED should 
be a special value. If you want to keep 0 value for NOSTATUS, maybe you 
could do:

enum rte_bbdev_device_status {
	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not supported 
on the PMD */
	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported */
	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-configured 
state */
...


> +
>   /** Device statistics. */
>   struct rte_bbdev_stats {
>   	uint64_t enqueued_count;  /**< Count of all operations enqueued */
> @@ -285,12 +300,14 @@ struct rte_bbdev_driver_info {
>   	/** Set if device supports per-queue interrupts */
>   	bool queue_intr_supported;
>   	/** Minimum alignment of buffers, in bytes */
> -	uint16_t min_alignment;
> -	/** HARQ memory available in kB */
> +	/** Device Status */
> +	enum rte_bbdev_device_status device_status;
>   	uint32_t harq_buffer_size;
>   	/** Byte endianness (RTE_BIG_ENDIAN/RTE_LITTLE_ENDIAN) supported
>   	 *  for input/output data
>   	 */
> +	uint16_t min_alignment;
> +	/** HARQ memory available in kB */
>   	uint8_t data_endianness;
>   	/** Default queue configuration used if none is supplied  */
>   	struct rte_bbdev_queue_conf default_queue_conf;
> @@ -827,6 +844,20 @@ typedef void (*rte_bbdev_cb_fn)(uint16_t dev_id,
>   rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t queue_id, int epfd, int op,
>   		void *data);
>   
> +/**
> + * Converts device status from enum to string
> + *
> + * @param status
> + *   Device status as enum
> + *
> + * @returns
> + *   Operation type as string or NULL if op_type is invalid
> + *
> + */
> +__rte_experimental
> +const char*
> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/bbdev/version.map b/lib/bbdev/version.map
> index cce3f3c..9ac3643 100644
> --- a/lib/bbdev/version.map
> +++ b/lib/bbdev/version.map
> @@ -39,3 +39,9 @@ DPDK_22 {
>   
>   	local: *;
>   };
> +
> +EXPERIMENTAL {
> +	global:
> +

We now add the version the new API was introduced in as a comment:

         # added in 22.11
> +	rte_bbdev_device_status_str;
> +};
Chautru, Nicolas Aug. 25, 2022, 6:30 p.m. UTC | #2 
Thanks Maxime,

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, August 25, 2022 7:19 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org
> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> 
> 
> 
> On 7/7/22 01:28, Nicolas Chautru wrote:
> > Added device status information, so that the PMD can expose
> > information related to the underlying accelerator device status.
> > Minor order change in structure to fit into padding hole.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   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/la12xx/bbdev_la12xx.c             |  1 +
> >   drivers/baseband/null/bbdev_null.c                 |  1 +
> >   drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
> >   lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
> >   lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
> >   lib/bbdev/version.map                              |  6 ++++
> >   9 files changed, 67 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index de7e4bc..17ba798 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -1060,6 +1060,7 @@
> >
> >   	/* Read and save the populated config from ACC100 registers */
> >   	fetch_acc100_config(dev);
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* This isn't ideal because it reports the maximum number of queues
> but
> >   	 * does not provide info on how many can be uplink/downlink or
> > different 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 82ae6ba..57b12af 100644
> > --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> > @@ -369,6 +369,7 @@
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* 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 21d3529..2a330c4 100644
> > --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> > @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	/* Calculates number of queues assigned to device */
> >   	dev_info->max_num_queues = 0;
> > diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c
> > b/drivers/baseband/la12xx/bbdev_la12xx.c
> > index 4d1bd16..c1f88c6 100644
> > --- a/drivers/baseband/la12xx/bbdev_la12xx.c
> > +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
> > @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
> >   	dev_info->capabilities = bbdev_capabilities;
> >   	dev_info->cpu_flag_reqs = NULL;
> >   	dev_info->min_alignment = 64;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u", dev->data-
> >dev_id);
> >   }
> > diff --git a/drivers/baseband/null/bbdev_null.c
> > b/drivers/baseband/null/bbdev_null.c
> > index 248e129..94a1976 100644
> > --- a/drivers/baseband/null/bbdev_null.c
> > +++ b/drivers/baseband/null/bbdev_null.c
> > @@ -82,6 +82,7 @@ struct bbdev_queue {
> >   	 * here for code completeness.
> >   	 */
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u", dev->data-
> >dev_id);
> >   }
> > diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > index af7bc41..dbc5524 100644
> > --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> > @@ -254,6 +254,7 @@ struct turbo_sw_queue {
> >   	dev_info->min_alignment = 64;
> >   	dev_info->harq_buffer_size = 0;
> >   	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> > +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >
> >   	rte_bbdev_log_debug("got device info from %u\n", dev->data-
> >dev_id);
> >   }
> > diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> > 4da8047..38630a2 100644
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -1133,3 +1133,25 @@ struct rte_mempool *
> >   	rte_bbdev_log(ERR, "Invalid operation type");
> >   	return NULL;
> >   }
> > +
> > +const char *
> > +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) {
> > +	static const char * const dev_sta_string[] = {
> > +		"RTE_BBDEV_DEV_NOSTATUS",
> > +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
> > +		"RTE_BBDEV_DEV_RESET",
> > +		"RTE_BBDEV_DEV_CONFIGURED",
> > +		"RTE_BBDEV_DEV_ACTIVE",
> > +		"RTE_BBDEV_DEV_FATAL_ERR",
> > +		"RTE_BBDEV_DEV_RESTART_REQ",
> > +		"RTE_BBDEV_DEV_RECONFIG_REQ",
> > +		"RTE_BBDEV_DEV_CORRECT_ERR",
> > +	};
> > +
> > +	if (status < sizeof(dev_sta_string) / sizeof(char *))
> > +		return dev_sta_string[status];
> > +
> > +	rte_bbdev_log(ERR, "Invalid device status");
> > +	return NULL;
> > +}
> > diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> > b88c881..9b1ffa4 100644
> > --- a/lib/bbdev/rte_bbdev.h
> > +++ b/lib/bbdev/rte_bbdev.h
> > @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
> >   int
> >   rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
> >
> > +/**
> > + * Flags indicate the status of the device  */ enum
> > +rte_bbdev_device_status {
> > +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
> > +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not
> supported on the PMD */
> > +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> configured state */
> > +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and
> ready to use */
> > +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is
> being used */
> > +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
> uncorrectable error */
> > +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application
> to restart */
> > +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires
> application to reconfigure queues */
> > +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable
> error event happened */
> > +};
> 
> I don't have a strong opinion on this, but I think NOT_SUPPORTED should be
> a special value. If you want to keep 0 value for NOSTATUS, maybe you could
> do:
> 
> enum rte_bbdev_device_status {
> 	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not
> supported
> on the PMD */
> 	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported
> */
> 	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> configured
> state */
> ...

Thanks Maxime. My concern is that I am upstreaming in parallel in pf_bb_config in parallel hence would like to keep it unchanged if possible.
Given you don’t have a strong opinion is that okay to keep as is? Or I can force special value 1 for NOT_SUPPORTED so that this is explicitly defined. But really enum should always be used.


> 
> 
> > +
> >   /** Device statistics. */
> >   struct rte_bbdev_stats {
> >   	uint64_t enqueued_count;  /**< Count of all operations enqueued */
> > @@ -285,12 +300,14 @@ struct rte_bbdev_driver_info {
> >   	/** Set if device supports per-queue interrupts */
> >   	bool queue_intr_supported;
> >   	/** Minimum alignment of buffers, in bytes */
> > -	uint16_t min_alignment;
> > -	/** HARQ memory available in kB */
> > +	/** Device Status */
> > +	enum rte_bbdev_device_status device_status;
> >   	uint32_t harq_buffer_size;
> >   	/** Byte endianness (RTE_BIG_ENDIAN/RTE_LITTLE_ENDIAN)
> supported
> >   	 *  for input/output data
> >   	 */
> > +	uint16_t min_alignment;
> > +	/** HARQ memory available in kB */
> >   	uint8_t data_endianness;
> >   	/** Default queue configuration used if none is supplied  */
> >   	struct rte_bbdev_queue_conf default_queue_conf; @@ -827,6
> +844,20
> > @@ typedef void (*rte_bbdev_cb_fn)(uint16_t dev_id,
> >   rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t queue_id, int epfd, int
> op,
> >   		void *data);
> >
> > +/**
> > + * Converts device status from enum to string
> > + *
> > + * @param status
> > + *   Device status as enum
> > + *
> > + * @returns
> > + *   Operation type as string or NULL if op_type is invalid
> > + *
> > + */
> > +__rte_experimental
> > +const char*
> > +rte_bbdev_device_status_str(enum rte_bbdev_device_status status);
> > +
> >   #ifdef __cplusplus
> >   }
> >   #endif
> > diff --git a/lib/bbdev/version.map b/lib/bbdev/version.map index
> > cce3f3c..9ac3643 100644
> > --- a/lib/bbdev/version.map
> > +++ b/lib/bbdev/version.map
> > @@ -39,3 +39,9 @@ DPDK_22 {
> >
> >   	local: *;
> >   };
> > +
> > +EXPERIMENTAL {
> > +	global:
> > +
> 
> We now add the version the new API was introduced in as a comment:
> 
>          # added in 22.11

Thanks for this feedback, I will update this

> > +	rte_bbdev_device_status_str;
> > +};
Maxime Coquelin Aug. 26, 2022, 10:12 a.m. UTC | #3 
Hi,

On 8/25/22 20:30, Chautru, Nicolas wrote:
> Thanks Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, August 25, 2022 7:19 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
>> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>> stephen@networkplumber.org
>> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
>>
>>
>>
>> On 7/7/22 01:28, Nicolas Chautru wrote:
>>> Added device status information, so that the PMD can expose
>>> information related to the underlying accelerator device status.
>>> Minor order change in structure to fit into padding hole.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    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/la12xx/bbdev_la12xx.c             |  1 +
>>>    drivers/baseband/null/bbdev_null.c                 |  1 +
>>>    drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
>>>    lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
>>>    lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
>>>    lib/bbdev/version.map                              |  6 ++++
>>>    9 files changed, 67 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> index de7e4bc..17ba798 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -1060,6 +1060,7 @@
>>>
>>>    	/* Read and save the populated config from ACC100 registers */
>>>    	fetch_acc100_config(dev);
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	/* This isn't ideal because it reports the maximum number of queues
>> but
>>>    	 * does not provide info on how many can be uplink/downlink or
>>> different 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 82ae6ba..57b12af 100644
>>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>> @@ -369,6 +369,7 @@
>>>    	dev_info->capabilities = bbdev_capabilities;
>>>    	dev_info->cpu_flag_reqs = NULL;
>>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	/* 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 21d3529..2a330c4 100644
>>> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>>> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>>> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
>>>    	dev_info->capabilities = bbdev_capabilities;
>>>    	dev_info->cpu_flag_reqs = NULL;
>>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	/* Calculates number of queues assigned to device */
>>>    	dev_info->max_num_queues = 0;
>>> diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c
>>> b/drivers/baseband/la12xx/bbdev_la12xx.c
>>> index 4d1bd16..c1f88c6 100644
>>> --- a/drivers/baseband/la12xx/bbdev_la12xx.c
>>> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
>>> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
>>>    	dev_info->capabilities = bbdev_capabilities;
>>>    	dev_info->cpu_flag_reqs = NULL;
>>>    	dev_info->min_alignment = 64;
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	rte_bbdev_log_debug("got device info from %u", dev->data-
>>> dev_id);
>>>    }
>>> diff --git a/drivers/baseband/null/bbdev_null.c
>>> b/drivers/baseband/null/bbdev_null.c
>>> index 248e129..94a1976 100644
>>> --- a/drivers/baseband/null/bbdev_null.c
>>> +++ b/drivers/baseband/null/bbdev_null.c
>>> @@ -82,6 +82,7 @@ struct bbdev_queue {
>>>    	 * here for code completeness.
>>>    	 */
>>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	rte_bbdev_log_debug("got device info from %u", dev->data-
>>> dev_id);
>>>    }
>>> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>> index af7bc41..dbc5524 100644
>>> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>> @@ -254,6 +254,7 @@ struct turbo_sw_queue {
>>>    	dev_info->min_alignment = 64;
>>>    	dev_info->harq_buffer_size = 0;
>>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>
>>>    	rte_bbdev_log_debug("got device info from %u\n", dev->data-
>>> dev_id);
>>>    }
>>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
>>> 4da8047..38630a2 100644
>>> --- a/lib/bbdev/rte_bbdev.c
>>> +++ b/lib/bbdev/rte_bbdev.c
>>> @@ -1133,3 +1133,25 @@ struct rte_mempool *
>>>    	rte_bbdev_log(ERR, "Invalid operation type");
>>>    	return NULL;
>>>    }
>>> +
>>> +const char *
>>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) {
>>> +	static const char * const dev_sta_string[] = {
>>> +		"RTE_BBDEV_DEV_NOSTATUS",
>>> +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
>>> +		"RTE_BBDEV_DEV_RESET",
>>> +		"RTE_BBDEV_DEV_CONFIGURED",
>>> +		"RTE_BBDEV_DEV_ACTIVE",
>>> +		"RTE_BBDEV_DEV_FATAL_ERR",
>>> +		"RTE_BBDEV_DEV_RESTART_REQ",
>>> +		"RTE_BBDEV_DEV_RECONFIG_REQ",
>>> +		"RTE_BBDEV_DEV_CORRECT_ERR",
>>> +	};
>>> +
>>> +	if (status < sizeof(dev_sta_string) / sizeof(char *))
>>> +		return dev_sta_string[status];
>>> +
>>> +	rte_bbdev_log(ERR, "Invalid device status");
>>> +	return NULL;
>>> +}
>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>> b88c881..9b1ffa4 100644
>>> --- a/lib/bbdev/rte_bbdev.h
>>> +++ b/lib/bbdev/rte_bbdev.h
>>> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
>>>    int
>>>    rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
>>>
>>> +/**
>>> + * Flags indicate the status of the device  */ enum
>>> +rte_bbdev_device_status {
>>> +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
>>> +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not
>> supported on the PMD */
>>> +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
>> configured state */
>>> +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and
>> ready to use */
>>> +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is
>> being used */
>>> +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
>> uncorrectable error */
>>> +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application
>> to restart */
>>> +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires
>> application to reconfigure queues */
>>> +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable
>> error event happened */
>>> +};
>>
>> I don't have a strong opinion on this, but I think NOT_SUPPORTED should be
>> a special value. If you want to keep 0 value for NOSTATUS, maybe you could
>> do:
>>
>> enum rte_bbdev_device_status {
>> 	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not
>> supported
>> on the PMD */
>> 	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported
>> */
>> 	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
>> configured
>> state */
>> ...
> 
> Thanks Maxime. My concern is that I am upstreaming in parallel in pf_bb_config in parallel hence would like to keep it unchanged if possible.
> Given you don’t have a strong opinion is that okay to keep as is? Or I can force special value 1 for NOT_SUPPORTED so that this is explicitly defined. But really enum should always be used.

I don't understand. It should not have any impact on pf_bb_config, given
pf_bb_config does not use DPDK.

Maxime
Chautru, Nicolas Aug. 29, 2022, 4:10 p.m. UTC | #4 
Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, August 26, 2022 3:13 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org
> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> 
> Hi,
> 
> On 8/25/22 20:30, Chautru, Nicolas wrote:
> > Thanks Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Thursday, August 25, 2022 7:19 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> >> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> >> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> >> stephen@networkplumber.org
> >> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> >>
> >>
> >>
> >> On 7/7/22 01:28, Nicolas Chautru wrote:
> >>> Added device status information, so that the PMD can expose
> >>> information related to the underlying accelerator device status.
> >>> Minor order change in structure to fit into padding hole.
> >>>
> >>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >>> ---
> >>>    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/la12xx/bbdev_la12xx.c             |  1 +
> >>>    drivers/baseband/null/bbdev_null.c                 |  1 +
> >>>    drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
> >>>    lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
> >>>    lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
> >>>    lib/bbdev/version.map                              |  6 ++++
> >>>    9 files changed, 67 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> >>> b/drivers/baseband/acc100/rte_acc100_pmd.c
> >>> index de7e4bc..17ba798 100644
> >>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> >>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> >>> @@ -1060,6 +1060,7 @@
> >>>
> >>>    	/* Read and save the populated config from ACC100 registers */
> >>>    	fetch_acc100_config(dev);
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	/* This isn't ideal because it reports the maximum number of
> >>> queues
> >> but
> >>>    	 * does not provide info on how many can be uplink/downlink or
> >>> different 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 82ae6ba..57b12af 100644
> >>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >>> @@ -369,6 +369,7 @@
> >>>    	dev_info->capabilities = bbdev_capabilities;
> >>>    	dev_info->cpu_flag_reqs = NULL;
> >>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	/* 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 21d3529..2a330c4 100644
> >>> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> >>> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> >>> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
> >>>    	dev_info->capabilities = bbdev_capabilities;
> >>>    	dev_info->cpu_flag_reqs = NULL;
> >>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	/* Calculates number of queues assigned to device */
> >>>    	dev_info->max_num_queues = 0;
> >>> diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c
> >>> b/drivers/baseband/la12xx/bbdev_la12xx.c
> >>> index 4d1bd16..c1f88c6 100644
> >>> --- a/drivers/baseband/la12xx/bbdev_la12xx.c
> >>> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
> >>> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
> >>>    	dev_info->capabilities = bbdev_capabilities;
> >>>    	dev_info->cpu_flag_reqs = NULL;
> >>>    	dev_info->min_alignment = 64;
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	rte_bbdev_log_debug("got device info from %u", dev->data-
> >>> dev_id);
> >>>    }
> >>> diff --git a/drivers/baseband/null/bbdev_null.c
> >>> b/drivers/baseband/null/bbdev_null.c
> >>> index 248e129..94a1976 100644
> >>> --- a/drivers/baseband/null/bbdev_null.c
> >>> +++ b/drivers/baseband/null/bbdev_null.c
> >>> @@ -82,6 +82,7 @@ struct bbdev_queue {
> >>>    	 * here for code completeness.
> >>>    	 */
> >>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	rte_bbdev_log_debug("got device info from %u", dev->data-
> >>> dev_id);
> >>>    }
> >>> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>> index af7bc41..dbc5524 100644
> >>> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>> @@ -254,6 +254,7 @@ struct turbo_sw_queue {
> >>>    	dev_info->min_alignment = 64;
> >>>    	dev_info->harq_buffer_size = 0;
> >>>    	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>
> >>>    	rte_bbdev_log_debug("got device info from %u\n", dev->data-
> >>> dev_id);
> >>>    }
> >>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> >>> 4da8047..38630a2 100644
> >>> --- a/lib/bbdev/rte_bbdev.c
> >>> +++ b/lib/bbdev/rte_bbdev.c
> >>> @@ -1133,3 +1133,25 @@ struct rte_mempool *
> >>>    	rte_bbdev_log(ERR, "Invalid operation type");
> >>>    	return NULL;
> >>>    }
> >>> +
> >>> +const char *
> >>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) {
> >>> +	static const char * const dev_sta_string[] = {
> >>> +		"RTE_BBDEV_DEV_NOSTATUS",
> >>> +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
> >>> +		"RTE_BBDEV_DEV_RESET",
> >>> +		"RTE_BBDEV_DEV_CONFIGURED",
> >>> +		"RTE_BBDEV_DEV_ACTIVE",
> >>> +		"RTE_BBDEV_DEV_FATAL_ERR",
> >>> +		"RTE_BBDEV_DEV_RESTART_REQ",
> >>> +		"RTE_BBDEV_DEV_RECONFIG_REQ",
> >>> +		"RTE_BBDEV_DEV_CORRECT_ERR",
> >>> +	};
> >>> +
> >>> +	if (status < sizeof(dev_sta_string) / sizeof(char *))
> >>> +		return dev_sta_string[status];
> >>> +
> >>> +	rte_bbdev_log(ERR, "Invalid device status");
> >>> +	return NULL;
> >>> +}
> >>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> >>> b88c881..9b1ffa4 100644
> >>> --- a/lib/bbdev/rte_bbdev.h
> >>> +++ b/lib/bbdev/rte_bbdev.h
> >>> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
> >>>    int
> >>>    rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
> >>>
> >>> +/**
> >>> + * Flags indicate the status of the device  */ enum
> >>> +rte_bbdev_device_status {
> >>> +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
> >>> +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not
> >> supported on the PMD */
> >>> +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> >> configured state */
> >>> +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and
> >> ready to use */
> >>> +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is
> >> being used */
> >>> +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
> >> uncorrectable error */
> >>> +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application
> >> to restart */
> >>> +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires
> >> application to reconfigure queues */
> >>> +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable
> >> error event happened */
> >>> +};
> >>
> >> I don't have a strong opinion on this, but I think NOT_SUPPORTED
> >> should be a special value. If you want to keep 0 value for NOSTATUS,
> >> maybe you could
> >> do:
> >>
> >> enum rte_bbdev_device_status {
> >> 	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not
> >> supported
> >> on the PMD */
> >> 	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported
> >> */
> >> 	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> >> configured
> >> state */
> >> ...
> >
> > Thanks Maxime. My concern is that I am upstreaming in parallel in
> pf_bb_config in parallel hence would like to keep it unchanged if possible.
> > Given you don’t have a strong opinion is that okay to keep as is? Or I can
> force special value 1 for NOT_SUPPORTED so that this is explicitly defined. But
> really enum should always be used.
> 
> I don't understand. It should not have any impact on pf_bb_config, given
> pf_bb_config does not use DPDK.
> 
> Maxime

That device status is being shared from pf_bb_config to the bbdev PMD through PF2VF communications, hence they share that same enum.
Maxime Coquelin Aug. 30, 2022, 7:08 a.m. UTC | #5 
On 8/29/22 18:10, Chautru, Nicolas wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, August 26, 2022 3:13 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
>> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>> stephen@networkplumber.org
>> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
>>
>> Hi,
>>
>> On 8/25/22 20:30, Chautru, Nicolas wrote:
>>> Thanks Maxime,
>>>
>>>> -----Original Message-----
>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>> Sent: Thursday, August 25, 2022 7:19 AM
>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>>>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
>>>> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>>>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>>>> stephen@networkplumber.org
>>>> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
>>>>
>>>>
>>>>
>>>> On 7/7/22 01:28, Nicolas Chautru wrote:
>>>>> Added device status information, so that the PMD can expose
>>>>> information related to the underlying accelerator device status.
>>>>> Minor order change in structure to fit into padding hole.
>>>>>
>>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>>>> ---
>>>>>     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/la12xx/bbdev_la12xx.c             |  1 +
>>>>>     drivers/baseband/null/bbdev_null.c                 |  1 +
>>>>>     drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
>>>>>     lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
>>>>>     lib/bbdev/rte_bbdev.h                              | 35 ++++++++++++++++++++--
>>>>>     lib/bbdev/version.map                              |  6 ++++
>>>>>     9 files changed, 67 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>>>> index de7e4bc..17ba798 100644
>>>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>>>> @@ -1060,6 +1060,7 @@
>>>>>
>>>>>     	/* Read and save the populated config from ACC100 registers */
>>>>>     	fetch_acc100_config(dev);
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	/* This isn't ideal because it reports the maximum number of
>>>>> queues
>>>> but
>>>>>     	 * does not provide info on how many can be uplink/downlink or
>>>>> different 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 82ae6ba..57b12af 100644
>>>>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>>>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
>>>>> @@ -369,6 +369,7 @@
>>>>>     	dev_info->capabilities = bbdev_capabilities;
>>>>>     	dev_info->cpu_flag_reqs = NULL;
>>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	/* 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 21d3529..2a330c4 100644
>>>>> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>>>>> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
>>>>> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
>>>>>     	dev_info->capabilities = bbdev_capabilities;
>>>>>     	dev_info->cpu_flag_reqs = NULL;
>>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	/* Calculates number of queues assigned to device */
>>>>>     	dev_info->max_num_queues = 0;
>>>>> diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c
>>>>> b/drivers/baseband/la12xx/bbdev_la12xx.c
>>>>> index 4d1bd16..c1f88c6 100644
>>>>> --- a/drivers/baseband/la12xx/bbdev_la12xx.c
>>>>> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
>>>>> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
>>>>>     	dev_info->capabilities = bbdev_capabilities;
>>>>>     	dev_info->cpu_flag_reqs = NULL;
>>>>>     	dev_info->min_alignment = 64;
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	rte_bbdev_log_debug("got device info from %u", dev->data-
>>>>> dev_id);
>>>>>     }
>>>>> diff --git a/drivers/baseband/null/bbdev_null.c
>>>>> b/drivers/baseband/null/bbdev_null.c
>>>>> index 248e129..94a1976 100644
>>>>> --- a/drivers/baseband/null/bbdev_null.c
>>>>> +++ b/drivers/baseband/null/bbdev_null.c
>>>>> @@ -82,6 +82,7 @@ struct bbdev_queue {
>>>>>     	 * here for code completeness.
>>>>>     	 */
>>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	rte_bbdev_log_debug("got device info from %u", dev->data-
>>>>> dev_id);
>>>>>     }
>>>>> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>>>> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>>>> index af7bc41..dbc5524 100644
>>>>> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>>>> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
>>>>> @@ -254,6 +254,7 @@ struct turbo_sw_queue {
>>>>>     	dev_info->min_alignment = 64;
>>>>>     	dev_info->harq_buffer_size = 0;
>>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
>>>>> +	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
>>>>>
>>>>>     	rte_bbdev_log_debug("got device info from %u\n", dev->data-
>>>>> dev_id);
>>>>>     }
>>>>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
>>>>> 4da8047..38630a2 100644
>>>>> --- a/lib/bbdev/rte_bbdev.c
>>>>> +++ b/lib/bbdev/rte_bbdev.c
>>>>> @@ -1133,3 +1133,25 @@ struct rte_mempool *
>>>>>     	rte_bbdev_log(ERR, "Invalid operation type");
>>>>>     	return NULL;
>>>>>     }
>>>>> +
>>>>> +const char *
>>>>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status) {
>>>>> +	static const char * const dev_sta_string[] = {
>>>>> +		"RTE_BBDEV_DEV_NOSTATUS",
>>>>> +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
>>>>> +		"RTE_BBDEV_DEV_RESET",
>>>>> +		"RTE_BBDEV_DEV_CONFIGURED",
>>>>> +		"RTE_BBDEV_DEV_ACTIVE",
>>>>> +		"RTE_BBDEV_DEV_FATAL_ERR",
>>>>> +		"RTE_BBDEV_DEV_RESTART_REQ",
>>>>> +		"RTE_BBDEV_DEV_RECONFIG_REQ",
>>>>> +		"RTE_BBDEV_DEV_CORRECT_ERR",
>>>>> +	};
>>>>> +
>>>>> +	if (status < sizeof(dev_sta_string) / sizeof(char *))
>>>>> +		return dev_sta_string[status];
>>>>> +
>>>>> +	rte_bbdev_log(ERR, "Invalid device status");
>>>>> +	return NULL;
>>>>> +}
>>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
>>>>> b88c881..9b1ffa4 100644
>>>>> --- a/lib/bbdev/rte_bbdev.h
>>>>> +++ b/lib/bbdev/rte_bbdev.h
>>>>> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
>>>>>     int
>>>>>     rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
>>>>>
>>>>> +/**
>>>>> + * Flags indicate the status of the device  */ enum
>>>>> +rte_bbdev_device_status {
>>>>> +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
>>>>> +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not
>>>> supported on the PMD */
>>>>> +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
>>>> configured state */
>>>>> +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and
>>>> ready to use */
>>>>> +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is
>>>> being used */
>>>>> +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
>>>> uncorrectable error */
>>>>> +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application
>>>> to restart */
>>>>> +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires
>>>> application to reconfigure queues */
>>>>> +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable
>>>> error event happened */
>>>>> +};
>>>>
>>>> I don't have a strong opinion on this, but I think NOT_SUPPORTED
>>>> should be a special value. If you want to keep 0 value for NOSTATUS,
>>>> maybe you could
>>>> do:
>>>>
>>>> enum rte_bbdev_device_status {
>>>> 	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not
>>>> supported
>>>> on the PMD */
>>>> 	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported
>>>> */
>>>> 	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
>>>> configured
>>>> state */
>>>> ...
>>>
>>> Thanks Maxime. My concern is that I am upstreaming in parallel in
>> pf_bb_config in parallel hence would like to keep it unchanged if possible.
>>> Given you don’t have a strong opinion is that okay to keep as is? Or I can
>> force special value 1 for NOT_SUPPORTED so that this is explicitly defined. But
>> really enum should always be used.
>>
>> I don't understand. It should not have any impact on pf_bb_config, given
>> pf_bb_config does not use DPDK.
>>
>> Maxime
> 
> That device status is being shared from pf_bb_config to the bbdev PMD through PF2VF communications, hence they share that same enum.
> 

Ok, but generic DPDK ABI should not be dependent on a vendor internal
implementation IMHO.

Maxime
Chautru, Nicolas Aug. 30, 2022, 7:38 p.m. UTC | #6 
Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, August 30, 2022 12:09 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org
> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> 
> 
> 
> On 8/29/22 18:10, Chautru, Nicolas wrote:
> > Hi Maxime,
> >
> >> -----Original Message-----
> >> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >> Sent: Friday, August 26, 2022 3:13 AM
> >> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> >> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> >> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> >> stephen@networkplumber.org
> >> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> >>
> >> Hi,
> >>
> >> On 8/25/22 20:30, Chautru, Nicolas wrote:
> >>> Thanks Maxime,
> >>>
> >>>> -----Original Message-----
> >>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> >>>> Sent: Thursday, August 25, 2022 7:19 AM
> >>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> >>>> thomas@monjalon.net; gakhil@marvell.com;
> hemant.agrawal@nxp.com
> >>>> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> >>>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> >>>> stephen@networkplumber.org
> >>>> Subject: Re: [PATCH v5 2/7] bbdev: add device status info
> >>>>
> >>>>
> >>>>
> >>>> On 7/7/22 01:28, Nicolas Chautru wrote:
> >>>>> Added device status information, so that the PMD can expose
> >>>>> information related to the underlying accelerator device status.
> >>>>> Minor order change in structure to fit into padding hole.
> >>>>>
> >>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> >>>>> ---
> >>>>>     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/la12xx/bbdev_la12xx.c             |  1 +
> >>>>>     drivers/baseband/null/bbdev_null.c                 |  1 +
> >>>>>     drivers/baseband/turbo_sw/bbdev_turbo_software.c   |  1 +
> >>>>>     lib/bbdev/rte_bbdev.c                              | 22 ++++++++++++++
> >>>>>     lib/bbdev/rte_bbdev.h                              | 35
> ++++++++++++++++++++--
> >>>>>     lib/bbdev/version.map                              |  6 ++++
> >>>>>     9 files changed, 67 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> >>>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
> >>>>> index de7e4bc..17ba798 100644
> >>>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> >>>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> >>>>> @@ -1060,6 +1060,7 @@
> >>>>>
> >>>>>     	/* Read and save the populated config from ACC100
> registers */
> >>>>>     	fetch_acc100_config(dev);
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	/* This isn't ideal because it reports the maximum number of
> >>>>> queues
> >>>> but
> >>>>>     	 * does not provide info on how many can be
> uplink/downlink
> >>>>> or different 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 82ae6ba..57b12af 100644
> >>>>> --- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >>>>> +++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
> >>>>> @@ -369,6 +369,7 @@
> >>>>>     	dev_info->capabilities = bbdev_capabilities;
> >>>>>     	dev_info->cpu_flag_reqs = NULL;
> >>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	/* 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 21d3529..2a330c4 100644
> >>>>> --- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> >>>>> +++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
> >>>>> @@ -645,6 +645,7 @@ struct __rte_cache_aligned fpga_queue {
> >>>>>     	dev_info->capabilities = bbdev_capabilities;
> >>>>>     	dev_info->cpu_flag_reqs = NULL;
> >>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	/* Calculates number of queues assigned to device */
> >>>>>     	dev_info->max_num_queues = 0; diff --git
> >>>>> a/drivers/baseband/la12xx/bbdev_la12xx.c
> >>>>> b/drivers/baseband/la12xx/bbdev_la12xx.c
> >>>>> index 4d1bd16..c1f88c6 100644
> >>>>> --- a/drivers/baseband/la12xx/bbdev_la12xx.c
> >>>>> +++ b/drivers/baseband/la12xx/bbdev_la12xx.c
> >>>>> @@ -100,6 +100,7 @@ struct bbdev_la12xx_params {
> >>>>>     	dev_info->capabilities = bbdev_capabilities;
> >>>>>     	dev_info->cpu_flag_reqs = NULL;
> >>>>>     	dev_info->min_alignment = 64;
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	rte_bbdev_log_debug("got device info from %u", dev->data-
> >>>>> dev_id);
> >>>>>     }
> >>>>> diff --git a/drivers/baseband/null/bbdev_null.c
> >>>>> b/drivers/baseband/null/bbdev_null.c
> >>>>> index 248e129..94a1976 100644
> >>>>> --- a/drivers/baseband/null/bbdev_null.c
> >>>>> +++ b/drivers/baseband/null/bbdev_null.c
> >>>>> @@ -82,6 +82,7 @@ struct bbdev_queue {
> >>>>>     	 * here for code completeness.
> >>>>>     	 */
> >>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	rte_bbdev_log_debug("got device info from %u", dev->data-
> >>>>> dev_id);
> >>>>>     }
> >>>>> diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>>>> b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>>>> index af7bc41..dbc5524 100644
> >>>>> --- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>>>> +++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
> >>>>> @@ -254,6 +254,7 @@ struct turbo_sw_queue {
> >>>>>     	dev_info->min_alignment = 64;
> >>>>>     	dev_info->harq_buffer_size = 0;
> >>>>>     	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
> >>>>> +	dev_info->device_status =
> RTE_BBDEV_DEV_NOT_SUPPORTED;
> >>>>>
> >>>>>     	rte_bbdev_log_debug("got device info from %u\n", dev-
> >data-
> >>>>> dev_id);
> >>>>>     }
> >>>>> diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c index
> >>>>> 4da8047..38630a2 100644
> >>>>> --- a/lib/bbdev/rte_bbdev.c
> >>>>> +++ b/lib/bbdev/rte_bbdev.c
> >>>>> @@ -1133,3 +1133,25 @@ struct rte_mempool *
> >>>>>     	rte_bbdev_log(ERR, "Invalid operation type");
> >>>>>     	return NULL;
> >>>>>     }
> >>>>> +
> >>>>> +const char *
> >>>>> +rte_bbdev_device_status_str(enum rte_bbdev_device_status status)
> {
> >>>>> +	static const char * const dev_sta_string[] = {
> >>>>> +		"RTE_BBDEV_DEV_NOSTATUS",
> >>>>> +		"RTE_BBDEV_DEV_NOT_SUPPORTED",
> >>>>> +		"RTE_BBDEV_DEV_RESET",
> >>>>> +		"RTE_BBDEV_DEV_CONFIGURED",
> >>>>> +		"RTE_BBDEV_DEV_ACTIVE",
> >>>>> +		"RTE_BBDEV_DEV_FATAL_ERR",
> >>>>> +		"RTE_BBDEV_DEV_RESTART_REQ",
> >>>>> +		"RTE_BBDEV_DEV_RECONFIG_REQ",
> >>>>> +		"RTE_BBDEV_DEV_CORRECT_ERR",
> >>>>> +	};
> >>>>> +
> >>>>> +	if (status < sizeof(dev_sta_string) / sizeof(char *))
> >>>>> +		return dev_sta_string[status];
> >>>>> +
> >>>>> +	rte_bbdev_log(ERR, "Invalid device status");
> >>>>> +	return NULL;
> >>>>> +}
> >>>>> diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h index
> >>>>> b88c881..9b1ffa4 100644
> >>>>> --- a/lib/bbdev/rte_bbdev.h
> >>>>> +++ b/lib/bbdev/rte_bbdev.h
> >>>>> @@ -223,6 +223,21 @@ struct rte_bbdev_queue_conf {
> >>>>>     int
> >>>>>     rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
> >>>>>
> >>>>> +/**
> >>>>> + * Flags indicate the status of the device  */ enum
> >>>>> +rte_bbdev_device_status {
> >>>>> +	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being
> reported */
> >>>>> +	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is
> not
> >>>> supported on the PMD */
> >>>>> +	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> >>>> configured state */
> >>>>> +	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is
> configured and
> >>>> ready to use */
> >>>>> +	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured
> and VF is
> >>>> being used */
> >>>>> +	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal
> >>>> uncorrectable error */
> >>>>> +	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires
> application
> >>>> to restart */
> >>>>> +	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires
> >>>> application to reconfigure queues */
> >>>>> +	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a
> correctable
> >>>> error event happened */
> >>>>> +};
> >>>>
> >>>> I don't have a strong opinion on this, but I think NOT_SUPPORTED
> >>>> should be a special value. If you want to keep 0 value for
> >>>> NOSTATUS, maybe you could
> >>>> do:
> >>>>
> >>>> enum rte_bbdev_device_status {
> >>>> 	RTE_BBDEV_DEV_NOT_SUPPORTED = -1,   /**< Device status is not
> >>>> supported
> >>>> on the PMD */
> >>>> 	RTE_BBDEV_DEV_NOSTATUS = 0,        /**< Nothing being reported
> >>>> */
> >>>> 	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-
> >>>> configured
> >>>> state */
> >>>> ...
> >>>
> >>> Thanks Maxime. My concern is that I am upstreaming in parallel in
> >> pf_bb_config in parallel hence would like to keep it unchanged if possible.
> >>> Given you don’t have a strong opinion is that okay to keep as is? Or
> >>> I can
> >> force special value 1 for NOT_SUPPORTED so that this is explicitly
> >> defined. But really enum should always be used.
> >>
> >> I don't understand. It should not have any impact on pf_bb_config,
> >> given pf_bb_config does not use DPDK.
> >>
> >> Maxime
> >
> > That device status is being shared from pf_bb_config to the bbdev PMD
> through PF2VF communications, hence they share that same enum.
> >
> 
> Ok, but generic DPDK ABI should not be dependent on a vendor internal
> implementation IMHO.
> 

I agree. This is the opposite direction, pf_bb_config is reusing the same API enumeration for now the assumption is that enum is being applied. 
In case we change here (for a fairly cosmetic reason) that would cause to change as well on the other ingredient, which is doable but has overhead.
If you really believe that there is a strong reason to do such a change let me know now, we would need to impact pf_bb_config release to have similar change and match that API change which is possible but not ideal. 

Thanks
Nic
diff mbox series

Patch

diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c b/drivers/baseband/acc100/rte_acc100_pmd.c
index de7e4bc..17ba798 100644
--- a/drivers/baseband/acc100/rte_acc100_pmd.c
+++ b/drivers/baseband/acc100/rte_acc100_pmd.c
@@ -1060,6 +1060,7 @@ 
 
 	/* Read and save the populated config from ACC100 registers */
 	fetch_acc100_config(dev);
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	/* This isn't ideal because it reports the maximum number of queues but
 	 * does not provide info on how many can be uplink/downlink or different
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 82ae6ba..57b12af 100644
--- a/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
+++ b/drivers/baseband/fpga_5gnr_fec/rte_fpga_5gnr_fec.c
@@ -369,6 +369,7 @@ 
 	dev_info->capabilities = bbdev_capabilities;
 	dev_info->cpu_flag_reqs = NULL;
 	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	/* 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 21d3529..2a330c4 100644
--- a/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
+++ b/drivers/baseband/fpga_lte_fec/fpga_lte_fec.c
@@ -645,6 +645,7 @@  struct __rte_cache_aligned fpga_queue {
 	dev_info->capabilities = bbdev_capabilities;
 	dev_info->cpu_flag_reqs = NULL;
 	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	/* Calculates number of queues assigned to device */
 	dev_info->max_num_queues = 0;
diff --git a/drivers/baseband/la12xx/bbdev_la12xx.c b/drivers/baseband/la12xx/bbdev_la12xx.c
index 4d1bd16..c1f88c6 100644
--- a/drivers/baseband/la12xx/bbdev_la12xx.c
+++ b/drivers/baseband/la12xx/bbdev_la12xx.c
@@ -100,6 +100,7 @@  struct bbdev_la12xx_params {
 	dev_info->capabilities = bbdev_capabilities;
 	dev_info->cpu_flag_reqs = NULL;
 	dev_info->min_alignment = 64;
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
 }
diff --git a/drivers/baseband/null/bbdev_null.c b/drivers/baseband/null/bbdev_null.c
index 248e129..94a1976 100644
--- a/drivers/baseband/null/bbdev_null.c
+++ b/drivers/baseband/null/bbdev_null.c
@@ -82,6 +82,7 @@  struct bbdev_queue {
 	 * here for code completeness.
 	 */
 	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	rte_bbdev_log_debug("got device info from %u", dev->data->dev_id);
 }
diff --git a/drivers/baseband/turbo_sw/bbdev_turbo_software.c b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
index af7bc41..dbc5524 100644
--- a/drivers/baseband/turbo_sw/bbdev_turbo_software.c
+++ b/drivers/baseband/turbo_sw/bbdev_turbo_software.c
@@ -254,6 +254,7 @@  struct turbo_sw_queue {
 	dev_info->min_alignment = 64;
 	dev_info->harq_buffer_size = 0;
 	dev_info->data_endianness = RTE_LITTLE_ENDIAN;
+	dev_info->device_status = RTE_BBDEV_DEV_NOT_SUPPORTED;
 
 	rte_bbdev_log_debug("got device info from %u\n", dev->data->dev_id);
 }
diff --git a/lib/bbdev/rte_bbdev.c b/lib/bbdev/rte_bbdev.c
index 4da8047..38630a2 100644
--- a/lib/bbdev/rte_bbdev.c
+++ b/lib/bbdev/rte_bbdev.c
@@ -1133,3 +1133,25 @@  struct rte_mempool *
 	rte_bbdev_log(ERR, "Invalid operation type");
 	return NULL;
 }
+
+const char *
+rte_bbdev_device_status_str(enum rte_bbdev_device_status status)
+{
+	static const char * const dev_sta_string[] = {
+		"RTE_BBDEV_DEV_NOSTATUS",
+		"RTE_BBDEV_DEV_NOT_SUPPORTED",
+		"RTE_BBDEV_DEV_RESET",
+		"RTE_BBDEV_DEV_CONFIGURED",
+		"RTE_BBDEV_DEV_ACTIVE",
+		"RTE_BBDEV_DEV_FATAL_ERR",
+		"RTE_BBDEV_DEV_RESTART_REQ",
+		"RTE_BBDEV_DEV_RECONFIG_REQ",
+		"RTE_BBDEV_DEV_CORRECT_ERR",
+	};
+
+	if (status < sizeof(dev_sta_string) / sizeof(char *))
+		return dev_sta_string[status];
+
+	rte_bbdev_log(ERR, "Invalid device status");
+	return NULL;
+}
diff --git a/lib/bbdev/rte_bbdev.h b/lib/bbdev/rte_bbdev.h
index b88c881..9b1ffa4 100644
--- a/lib/bbdev/rte_bbdev.h
+++ b/lib/bbdev/rte_bbdev.h
@@ -223,6 +223,21 @@  struct rte_bbdev_queue_conf {
 int
 rte_bbdev_queue_stop(uint16_t dev_id, uint16_t queue_id);
 
+/**
+ * Flags indicate the status of the device
+ */
+enum rte_bbdev_device_status {
+	RTE_BBDEV_DEV_NOSTATUS,        /**< Nothing being reported */
+	RTE_BBDEV_DEV_NOT_SUPPORTED,   /**< Device status is not supported on the PMD */
+	RTE_BBDEV_DEV_RESET,           /**< Device in reset and un-configured state */
+	RTE_BBDEV_DEV_CONFIGURED,      /**< Device is configured and ready to use */
+	RTE_BBDEV_DEV_ACTIVE,          /**< Device is configured and VF is being used */
+	RTE_BBDEV_DEV_FATAL_ERR,       /**< Device has hit a fatal uncorrectable error */
+	RTE_BBDEV_DEV_RESTART_REQ,     /**< Device requires application to restart */
+	RTE_BBDEV_DEV_RECONFIG_REQ,    /**< Device requires application to reconfigure queues */
+	RTE_BBDEV_DEV_CORRECT_ERR,     /**< Warning of a correctable error event happened */
+};
+
 /** Device statistics. */
 struct rte_bbdev_stats {
 	uint64_t enqueued_count;  /**< Count of all operations enqueued */
@@ -285,12 +300,14 @@  struct rte_bbdev_driver_info {
 	/** Set if device supports per-queue interrupts */
 	bool queue_intr_supported;
 	/** Minimum alignment of buffers, in bytes */
-	uint16_t min_alignment;
-	/** HARQ memory available in kB */
+	/** Device Status */
+	enum rte_bbdev_device_status device_status;
 	uint32_t harq_buffer_size;
 	/** Byte endianness (RTE_BIG_ENDIAN/RTE_LITTLE_ENDIAN) supported
 	 *  for input/output data
 	 */
+	uint16_t min_alignment;
+	/** HARQ memory available in kB */
 	uint8_t data_endianness;
 	/** Default queue configuration used if none is supplied  */
 	struct rte_bbdev_queue_conf default_queue_conf;
@@ -827,6 +844,20 @@  typedef void (*rte_bbdev_cb_fn)(uint16_t dev_id,
 rte_bbdev_queue_intr_ctl(uint16_t dev_id, uint16_t queue_id, int epfd, int op,
 		void *data);
 
+/**
+ * Converts device status from enum to string
+ *
+ * @param status
+ *   Device status as enum
+ *
+ * @returns
+ *   Operation type as string or NULL if op_type is invalid
+ *
+ */
+__rte_experimental
+const char*
+rte_bbdev_device_status_str(enum rte_bbdev_device_status status);
+
 #ifdef __cplusplus
 }
 #endif
diff --git a/lib/bbdev/version.map b/lib/bbdev/version.map
index cce3f3c..9ac3643 100644
--- a/lib/bbdev/version.map
+++ b/lib/bbdev/version.map
@@ -39,3 +39,9 @@  DPDK_22 {
 
 	local: *;
 };
+
+EXPERIMENTAL {
+	global:
+
+	rte_bbdev_device_status_str;
+};