On Tue, Sep 19, 2023 at 10:32 PM Chautru, Nicolas <nicolas.chau...@intel.com> wrote: > > Hi David, > > > -----Original Message----- > > From: David Marchand <david.march...@redhat.com> > > Sent: Tuesday, September 19, 2023 8:20 AM > > To: Chautru, Nicolas <nicolas.chau...@intel.com> > > Cc: dev@dpdk.org; maxime.coque...@redhat.com; > > hemant.agra...@nxp.com; Vargas, Hernan <hernan.var...@intel.com> > > Subject: Re: [PATCH v1 3/7] baseband/acc: remove the 4G SO capability for > > VRB1 > > > > On Tue, Sep 19, 2023 at 3:25 AM Nicolas Chautru > > <nicolas.chau...@intel.com> wrote: > > > > > > This removes the specific capability and support of LTE Decoder Soft > > > Output option on the VRB1 PMD. > > > > Please explain why such support is removed for this hw. > > The decision is made to defeature this optional capability as under certain > race conditions enabling this may potentially cause reliability issues which > would not be acceptable. > Note that this is an optional additional output information (soft output > information) independent of the actual decoding operation. > More details below next to your other comments.
This must be explained in the commitlog. > > > > > > > > > > > Signed-off-by: Nicolas Chautru <nicolas.chau...@intel.com> > > > --- > > > drivers/baseband/acc/rte_vrb_pmd.c | 6 +++--- > > > 1 file changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/baseband/acc/rte_vrb_pmd.c > > > b/drivers/baseband/acc/rte_vrb_pmd.c > > > index 3c8f3409ed..e0f50460bd 100644 > > > --- a/drivers/baseband/acc/rte_vrb_pmd.c > > > +++ b/drivers/baseband/acc/rte_vrb_pmd.c > > > @@ -1019,14 +1019,11 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct > > rte_bbdev_driver_info *dev_info) > > > RTE_BBDEV_TURBO_CRC_TYPE_24B | > > > RTE_BBDEV_TURBO_DEC_CRC_24B_DROP | > > > RTE_BBDEV_TURBO_EQUALIZER | > > > - RTE_BBDEV_TURBO_SOFT_OUT_SATURATE > > > | > > > > > > RTE_BBDEV_TURBO_HALF_ITERATION_EVEN | > > > > > > RTE_BBDEV_TURBO_CONTINUE_CRC_MATCH | > > > - RTE_BBDEV_TURBO_SOFT_OUTPUT | > > > RTE_BBDEV_TURBO_EARLY_TERMINATION > > > | > > > RTE_BBDEV_TURBO_DEC_INTERRUPTS | > > > RTE_BBDEV_TURBO_NEG_LLR_1_BIT_IN | > > > - > > > RTE_BBDEV_TURBO_NEG_LLR_1_BIT_SOFT_OUT | > > > RTE_BBDEV_TURBO_MAP_DEC | > > > > > > RTE_BBDEV_TURBO_DEC_TB_CRC_24B_KEEP | > > > > > > RTE_BBDEV_TURBO_DEC_SCATTER_GATHER, > > > @@ -1975,6 +1972,9 @@ enqueue_dec_one_op_cb(struct acc_queue *q, > > struct rte_bbdev_dec_op *op, > > > struct rte_mbuf *input, *h_output_head, *h_output, > > > *s_output_head, *s_output; > > > > > > + /* Disable explictly SO for VRB 1. */ > > > + op->turbo_dec.op_flags &= ~RTE_BBDEV_TURBO_SOFT_OUTPUT; > > > > Can you explain why it is needed to filter this out? > > > > I did not find a clear description in the bbdev API. > > It would help if there were explicits references in doxygen of which > > capability > > is necessary for using flags/API. > > > > > > I was expecting that asking for RTE_BBDEV_TURBO_SOFT_OUTPUT to a driver > > is only allowed if rte_bbdev_op_cap contains it. > > With this assumption, it would be invalid for an application to request > > RTE_BBDEV_TURBO_SOFT_OUTPUT through rte_bbdev_enqueue_dec_ops. > > You may arguably expect this from a well behaved user application but still > there is nothing that enforces it explicitly, ie. notably under negative > scenario conditions which we still need to manage gracefully. If your application is buggy (not reading / complying with the device capabilities), fix it. > Here we want to make sure that in case the optional operational flag is > included, we fall back to default mode when using the VRB1 variant. > Keep in mind that the unified driver can support multiple HW variant (see > rest of the serie) and may support this option for other variants using same > code. Whatever the HW variant, the API should be respected: exposing capabilities is done on a per device basis. > > In term of documentation, I believe that capability/flag (ie. note that the > enum maps to a capability when retrieved from info_get, and to an operation > flag when provided to the bbdev api) is already captured explicitly for many > generations. Basically this an optional output of the LTE decoding > processing, to provide APP LLR which can be potentially be useful for the > user application (separate optional mbuf). It may or may not be supported by > a bb device, and it may or may not be requested to be provided through the > API. Typically this is not enabled. Being optional does not mean that a driver can ignore it. Otherwise, there is no point in exposing a capability. Thanks. -- David Marchand
