[v4,2/7] bbdev: add device status info
Checks
Commit Message
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 | 24 +++++++++++++++
lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
lib/bbdev/version.map | 6 ++++
9 files changed, 69 insertions(+), 2 deletions(-)
Comments
On 7/5/22 5:23 PM, 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 | 24 +++++++++++++++
> lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
> lib/bbdev/version.map | 6 ++++
> 9 files changed, 69 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 22bd894..555bda9 100644
> --- a/lib/bbdev/rte_bbdev.c
> +++ b/lib/bbdev/rte_bbdev.c
> @@ -25,6 +25,8 @@
>
> /* Number of supported operation types */
> #define BBDEV_OP_TYPE_COUNT 5
> +/* Number of supported device status */
> +#define BBDEV_DEV_STATUS_COUNT 9
>
> /* BBDev library logging ID */
> RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE);
> @@ -1132,3 +1134,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 < BBDEV_DEV_STATUS_COUNT)
> + 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 */
If this was 0, you may not need to explicitly set.
> + 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 */
Last patch was padded, do something consistent here.
> +};
> +
> /** 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;
New elements should be added to the end to improve backward compatibility.
Tom
> 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;
> +};
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
>
>
> On 7/5/22 5:23 PM, 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 | 24 +++++++++++++++
> > lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
> > lib/bbdev/version.map | 6 ++++
> > 9 files changed, 69 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
> > 22bd894..555bda9 100644
> > --- a/lib/bbdev/rte_bbdev.c
> > +++ b/lib/bbdev/rte_bbdev.c
> > @@ -25,6 +25,8 @@
> >
> > /* Number of supported operation types */
> > #define BBDEV_OP_TYPE_COUNT 5
> > +/* Number of supported device status */ #define
> > +BBDEV_DEV_STATUS_COUNT 9
> >
> > /* BBDev library logging ID */
> > RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3
> +1134,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 < BBDEV_DEV_STATUS_COUNT)
> > + 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 */
> If this was 0, you may not need to explicitly set.
This helps to have the lack of status being equivalent to a cleared register.
> > + 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 */
> Last patch was padded, do something consistent here.
We only pad if we have to. Here there is no array whose size would be dimensioned by the size of that enum.
> > +};
> > +
> > /** 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;
>
> New elements should be added to the end to improve backward compatibility.
Same comment in different patch. I would like to know if there is a real recommendation from DPDK on this. I have heard opposite view as well.
In that very case we are breaking the ABI in that new serie for 22.11 (sizes and offsets are changing).
>
> Tom
>
> > 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;
> > +};
On 7/6/22 2:16 PM, Chautru, Nicolas wrote:
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
>>
>>
>> On 7/5/22 5:23 PM, 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 | 24 +++++++++++++++
>>> lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
>>> lib/bbdev/version.map | 6 ++++
>>> 9 files changed, 69 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
>>> 22bd894..555bda9 100644
>>> --- a/lib/bbdev/rte_bbdev.c
>>> +++ b/lib/bbdev/rte_bbdev.c
>>> @@ -25,6 +25,8 @@
>>>
>>> /* Number of supported operation types */
>>> #define BBDEV_OP_TYPE_COUNT 5
>>> +/* Number of supported device status */ #define
>>> +BBDEV_DEV_STATUS_COUNT 9
>>>
>>> /* BBDev library logging ID */
>>> RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3
>> +1134,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 < BBDEV_DEV_STATUS_COUNT)
>>> + 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 */
>> If this was 0, you may not need to explicitly set.
> This helps to have the lack of status being equivalent to a cleared register.
NOSTATUS is fine, just change
NOT_SUPPORTED = 0,
Tom
>
>>> + 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 */
>> Last patch was padded, do something consistent here.
> We only pad if we have to. Here there is no array whose size would be dimensioned by the size of that enum.
>
>>> +};
>>> +
>>> /** 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;
>> New elements should be added to the end to improve backward compatibility.
> Same comment in different patch. I would like to know if there is a real recommendation from DPDK on this. I have heard opposite view as well.
> In that very case we are breaking the ABI in that new serie for 22.11 (sizes and offsets are changing).
>
>> Tom
>>
>>> 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;
>>> +};
Hi Tom,
> -----Original Message-----
> From: Tom Rix <trix@redhat.com>
> Sent: Thursday, July 7, 2022 6:37 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
> Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; david.marchand@redhat.com;
> stephen@networkplumber.org
> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
>
>
> On 7/6/22 2:16 PM, Chautru, Nicolas wrote:
> >> -----Original Message-----
> >> From: Tom Rix <trix@redhat.com>
> >> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
> >>
> >>
> >> On 7/5/22 5:23 PM, 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 | 24 +++++++++++++++
> >>> lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
> >>> lib/bbdev/version.map | 6 ++++
> >>> 9 files changed, 69 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
> >>> 22bd894..555bda9 100644
> >>> --- a/lib/bbdev/rte_bbdev.c
> >>> +++ b/lib/bbdev/rte_bbdev.c
> >>> @@ -25,6 +25,8 @@
> >>>
> >>> /* Number of supported operation types */
> >>> #define BBDEV_OP_TYPE_COUNT 5
> >>> +/* Number of supported device status */ #define
> >>> +BBDEV_DEV_STATUS_COUNT 9
> >>>
> >>> /* BBDev library logging ID */
> >>> RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3
> >> +1134,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 < BBDEV_DEV_STATUS_COUNT)
> >>> + 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 */
> >> If this was 0, you may not need to explicitly set.
> > This helps to have the lack of status being equivalent to a cleared register.
>
> NOSTATUS is fine, just change
>
> NOT_SUPPORTED = 0,
Let me rephrase. Currently RTE_BBDEV_DEV_NOSTATUS is zero explicitly which can be valuable to match a clear register.
RTE_BBDEV_DEV_NOT_SUPPORTED would not be zero.
Are you suggesting I should put explictly a RTE_BBDEV_DEV_NOSTATUS = 0? Isn't it implicit for any compiler that the first enum starts from zero?
>
> Tom
>
> >
> >>> + 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 */
> >> Last patch was padded, do something consistent here.
> > We only pad if we have to. Here there is no array whose size would be
> dimensioned by the size of that enum.
> >
> >>> +};
> >>> +
> >>> /** 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;
> >> New elements should be added to the end to improve backward
> compatibility.
> > Same comment in different patch. I would like to know if there is a real
> recommendation from DPDK on this. I have heard opposite view as well.
> > In that very case we are breaking the ABI in that new serie for 22.11 (sizes
> and offsets are changing).
> >
> >> Tom
> >>
> >>> 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;
> >>> +};
On 7/7/22 10:15 AM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Thursday, July 7, 2022 6:37 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net; gakhil@marvell.com; hemant.agrawal@nxp.com
>> Cc: maxime.coquelin@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>> <bruce.richardson@intel.com>; david.marchand@redhat.com;
>> stephen@networkplumber.org
>> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
>>
>>
>> On 7/6/22 2:16 PM, Chautru, Nicolas wrote:
>>>> -----Original Message-----
>>>> From: Tom Rix <trix@redhat.com>
>>>> Subject: Re: [PATCH v4 2/7] bbdev: add device status info
>>>>
>>>>
>>>> On 7/5/22 5:23 PM, 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 | 24 +++++++++++++++
>>>>> lib/bbdev/rte_bbdev.h | 35 ++++++++++++++++++++--
>>>>> lib/bbdev/version.map | 6 ++++
>>>>> 9 files changed, 69 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
>>>>> 22bd894..555bda9 100644
>>>>> --- a/lib/bbdev/rte_bbdev.c
>>>>> +++ b/lib/bbdev/rte_bbdev.c
>>>>> @@ -25,6 +25,8 @@
>>>>>
>>>>> /* Number of supported operation types */
>>>>> #define BBDEV_OP_TYPE_COUNT 5
>>>>> +/* Number of supported device status */ #define
>>>>> +BBDEV_DEV_STATUS_COUNT 9
>>>>>
>>>>> /* BBDev library logging ID */
>>>>> RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE); @@ -1132,3
>>>> +1134,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 < BBDEV_DEV_STATUS_COUNT)
>>>>> + 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 */
>>>> If this was 0, you may not need to explicitly set.
>>> This helps to have the lack of status being equivalent to a cleared register.
>> NOSTATUS is fine, just change
>>
>> NOT_SUPPORTED = 0,
> Let me rephrase. Currently RTE_BBDEV_DEV_NOSTATUS is zero explicitly which can be valuable to match a clear register.
> RTE_BBDEV_DEV_NOT_SUPPORTED would not be zero.
> Are you suggesting I should put explictly a RTE_BBDEV_DEV_NOSTATUS = 0? Isn't it implicit for any compiler that the first enum starts from zero?
However you want to do it, try taking advantage of zero-ed memory. By
choosing for this enum to be non-zero, it has to be explicitly set. If
it was 0 it would be implicitly set, assuming dev is zero-ed.
Tom
>
>> Tom
>>
>>>>> + 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 */
>>>> Last patch was padded, do something consistent here.
>>> We only pad if we have to. Here there is no array whose size would be
>> dimensioned by the size of that enum.
>>>>> +};
>>>>> +
>>>>> /** 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;
>>>> New elements should be added to the end to improve backward
>> compatibility.
>>> Same comment in different patch. I would like to know if there is a real
>> recommendation from DPDK on this. I have heard opposite view as well.
>>> In that very case we are breaking the ABI in that new serie for 22.11 (sizes
>> and offsets are changing).
>>>> Tom
>>>>
>>>>> 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;
>>>>> +};
On 7/6/22 23:16, Chautru, Nicolas wrote:
>>> +};
>>> +
>>> /** 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;
>> New elements should be added to the end to improve backward compatibility.
> Same comment in different patch. I would like to know if there is a real recommendation from DPDK on this. I have heard opposite view as well.
> In that very case we are breaking the ABI in that new serie for 22.11 (sizes and offsets are changing).
>
Since we are breaking ABI anyways, I don't find it unreasonable to take
the opportunity to improve packing the struct.
Maxime
@@ -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
@@ -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;
@@ -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;
@@ -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);
}
@@ -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);
}
@@ -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);
}
@@ -25,6 +25,8 @@
/* Number of supported operation types */
#define BBDEV_OP_TYPE_COUNT 5
+/* Number of supported device status */
+#define BBDEV_DEV_STATUS_COUNT 9
/* BBDev library logging ID */
RTE_LOG_REGISTER_DEFAULT(bbdev_logtype, NOTICE);
@@ -1132,3 +1134,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 < BBDEV_DEV_STATUS_COUNT)
+ return dev_sta_string[status];
+
+ rte_bbdev_log(ERR, "Invalid device status");
+ return NULL;
+}
@@ -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
@@ -39,3 +39,9 @@ DPDK_22 {
local: *;
};
+
+EXPERIMENTAL {
+ global:
+
+ rte_bbdev_device_status_str;
+};