Hi Maxime, > -----Original Message----- > From: Maxime Coquelin <[email protected]> > Sent: Tuesday, August 30, 2022 12:09 AM > To: Chautru, Nicolas <[email protected]>; [email protected]; > [email protected]; [email protected]; [email protected] > Cc: [email protected]; [email protected]; Richardson, Bruce > <[email protected]>; [email protected]; > [email protected] > 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 <[email protected]> > >> Sent: Friday, August 26, 2022 3:13 AM > >> To: Chautru, Nicolas <[email protected]>; [email protected]; > >> [email protected]; [email protected]; [email protected] > >> Cc: [email protected]; [email protected]; Richardson, Bruce > >> <[email protected]>; [email protected]; > >> [email protected] > >> 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 <[email protected]> > >>>> Sent: Thursday, August 25, 2022 7:19 AM > >>>> To: Chautru, Nicolas <[email protected]>; [email protected]; > >>>> [email protected]; [email protected]; > [email protected] > >>>> Cc: [email protected]; [email protected]; Richardson, Bruce > >>>> <[email protected]>; [email protected]; > >>>> [email protected] > >>>> 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 <[email protected]> > >>>>> --- > >>>>> 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
