[v1,3/7] baseband/acc: remove the 4G SO capability for VRB1
Checks
Commit Message
This removes the specific capability and support of LTE Decoder
Soft Output option on the VRB1 PMD.
Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
---
drivers/baseband/acc/rte_vrb_pmd.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
Comments
On Tue, Sep 19, 2023 at 3:25 AM Nicolas Chautru
<nicolas.chautru@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.
>
> Signed-off-by: Nicolas Chautru <nicolas.chautru@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.
> +
> desc = acc_desc(q, total_enqueued_cbs);
> vrb_fcw_td_fill(op, &desc->req.fcw_td);
>
> --
> 2.34.1
>
At this point of the series, the documentation still references
RTE_BBDEV_TURBO_SOFT_OUTPUT as something supported by the vrb1 driver.
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, September 19, 2023 8:20 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@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.chautru@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.
>
>
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@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.
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.
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.
In that commit we are defeaturing this optional capability for VRB1, we no longer expose it to the application, and in case the application was requesting it, we would ignore it (as we do for any other flags that is not supported, they become don't care flags which are ignored).
Kindly let me know if still unclear.
Thanks
Nic
>
>
> > +
> > desc = acc_desc(q, total_enqueued_cbs);
> > vrb_fcw_td_fill(op, &desc->req.fcw_td);
> >
> > --
> > 2.34.1
> >
>
> At this point of the series, the documentation still references
> RTE_BBDEV_TURBO_SOFT_OUTPUT as something supported by the vrb1
> driver.
>
>
> --
> David Marchand
On Tue, Sep 19, 2023 at 10:32 PM Chautru, Nicolas
<nicolas.chautru@intel.com> wrote:
>
> Hi David,
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, September 19, 2023 8:20 AM
> > To: Chautru, Nicolas <nicolas.chautru@intel.com>
> > Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> > hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@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.chautru@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.chautru@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.
Hi David,
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Thursday, September 21, 2023 12:13 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>;
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH v1 3/7] baseband/acc: remove the 4G SO capability for
> VRB1
>
> On Tue, Sep 19, 2023 at 10:32 PM Chautru, Nicolas
> <nicolas.chautru@intel.com> wrote:
> >
> > Hi David,
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Tuesday, September 19, 2023 8:20 AM
> > > To: Chautru, Nicolas <nicolas.chautru@intel.com>
> > > Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
> > > hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@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.chautru@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.
OK will add now.
>
> >
> > >
> > >
> > > >
> > > > Signed-off-by: Nicolas Chautru <nicolas.chautru@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.
Supporting negative scenario is within the scope of the PMD, whatever the application throws at us in cannot cause any HW issue.
Fixing application issues is outside of DPDK control obviously.
>
>
> > 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.
>
It should be ideally, but in practice in case this is not done for whatever reason (negative scenario, bug in user application)
then we want the PMD to still avoid misbehaving.
>
> >
> > 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.
I am not sure I follow your concern. Capability are critical for application to enumerate what the underlying device can do.
Here we are only stating that this is valuable to harden the PMD so that it can operate even if an unexpected API is provided, notably to guarantee the unified code is not used in an unintended manner.
Note that no PMD to my knowledge enforces checking explicitly the op_flag matches with the capability (like a bitmask check),
and I don’t really think we have to, these other flags are just meant to have effect since not supported.
>
>
>
> Thanks.
>
> --
> David Marchand
On 9/21/23 19:18, Chautru, Nicolas wrote:
> Hi David,
>
>> -----Original Message-----
>> From: David Marchand <david.marchand@redhat.com>
>> Sent: Thursday, September 21, 2023 12:13 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>
>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
>> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>;
>> Thomas Monjalon <thomas@monjalon.net>
>> Subject: Re: [PATCH v1 3/7] baseband/acc: remove the 4G SO capability for
>> VRB1
>>
>> On Tue, Sep 19, 2023 at 10:32 PM Chautru, Nicolas
>> <nicolas.chautru@intel.com> wrote:
>>>
>>> Hi David,
>>>
>>>> -----Original Message-----
>>>> From: David Marchand <david.marchand@redhat.com>
>>>> Sent: Tuesday, September 19, 2023 8:20 AM
>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>
>>>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
>>>> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@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.chautru@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.
>
> OK will add now.
>
>>
>>>
>>>>
>>>>
>>>>>
>>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@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.
>
> Supporting negative scenario is within the scope of the PMD, whatever the application throws at us in cannot cause any HW issue.
> Fixing application issues is outside of DPDK control obviously.
I don't think it is not the role of the PMD to workaround application
bugs.
The PMD driver reports capabilities for a given device variant. The
application ignores that and forces the capability, the PMD driver
should fail. It is better for the application the PMD driver let it know
it is misbehaving than trying to hide 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.
>>
>
> It should be ideally, but in practice in case this is not done for whatever reason (negative scenario, bug in user application)
> then we want the PMD to still avoid misbehaving.
>
>>
>>>
>>> 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.
>
> I am not sure I follow your concern. Capability are critical for application to enumerate what the underlying device can do.
> Here we are only stating that this is valuable to harden the PMD so that it can operate even if an unexpected API is provided, notably to guarantee the unified code is not used in an unintended manner.
> Note that no PMD to my knowledge enforces checking explicitly the op_flag matches with the capability (like a bitmask check),
> and I don’t really think we have to, these other flags are just meant to have effect since not supported.
>
>>
>>
>>
>> Thanks.
>>
>> --
>> David Marchand
>
On 9/27/23 09:08, Maxime Coquelin wrote:
>
>
> On 9/21/23 19:18, Chautru, Nicolas wrote:
>> Hi David,
>>
>>> -----Original Message-----
>>> From: David Marchand <david.marchand@redhat.com>
>>> Sent: Thursday, September 21, 2023 12:13 AM
>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>
>>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
>>> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>;
>>> Thomas Monjalon <thomas@monjalon.net>
>>> Subject: Re: [PATCH v1 3/7] baseband/acc: remove the 4G SO capability
>>> for
>>> VRB1
>>>
>>> On Tue, Sep 19, 2023 at 10:32 PM Chautru, Nicolas
>>> <nicolas.chautru@intel.com> wrote:
>>>>
>>>> Hi David,
>>>>
>>>>> -----Original Message-----
>>>>> From: David Marchand <david.marchand@redhat.com>
>>>>> Sent: Tuesday, September 19, 2023 8:20 AM
>>>>> To: Chautru, Nicolas <nicolas.chautru@intel.com>
>>>>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com;
>>>>> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@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.chautru@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.
>>
>> OK will add now.
>>
>>>
>>>>
>>>>>
>>>>>
>>>>>>
>>>>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@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.
>>
>> Supporting negative scenario is within the scope of the PMD, whatever
>> the application throws at us in cannot cause any HW issue.
>> Fixing application issues is outside of DPDK control obviously.
>
> I don't think it is not the role of the PMD to workaround application
> bugs.
Of course I meant:
I don't think it is the role of the PMD to workaround application bugs.
>
> The PMD driver reports capabilities for a given device variant. The
> application ignores that and forces the capability, the PMD driver
> should fail. It is better for the application the PMD driver let it know
> it is misbehaving than trying to hide 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.
>>>
>>
>> It should be ideally, but in practice in case this is not done for
>> whatever reason (negative scenario, bug in user application)
>> then we want the PMD to still avoid misbehaving.
>>
>>>
>>>>
>>>> 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.
>>
>> I am not sure I follow your concern. Capability are critical for
>> application to enumerate what the underlying device can do.
>> Here we are only stating that this is valuable to harden the PMD so
>> that it can operate even if an unexpected API is provided, notably to
>> guarantee the unified code is not used in an unintended manner.
>> Note that no PMD to my knowledge enforces checking explicitly the
>> op_flag matches with the capability (like a bitmask check),
>> and I don’t really think we have to, these other flags are just meant
>> to have effect since not supported.
>>
>>>
>>>
>>>
>>> Thanks.
>>>
>>> --
>>> David Marchand
>>
@@ -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;
+
desc = acc_desc(q, total_enqueued_cbs);
vrb_fcw_td_fill(op, &desc->req.fcw_td);