[PATCH v1 3/7] baseband/acc: remove the 4G SO capability for VRB1

[Maxime Coquelin]

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 at redhat.com>
>>> Sent: Thursday, September 21, 2023 12:13 AM
>>> To: Chautru, Nicolas <nicolas.chautru at intel.com>
>>> Cc: dev at dpdk.org; maxime.coquelin at redhat.com;
>>> hemant.agrawal at nxp.com; Vargas, Hernan <hernan.vargas at intel.com>;
>>> Thomas Monjalon <thomas at 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 at intel.com> wrote:
>>>>
>>>> Hi David,
>>>>
>>>>> -----Original Message-----
>>>>> From: David Marchand <david.marchand at redhat.com>
>>>>> Sent: Tuesday, September 19, 2023 8:20 AM
>>>>> To: Chautru, Nicolas <nicolas.chautru at intel.com>
>>>>> Cc: dev at dpdk.org; maxime.coquelin at redhat.com;
>>>>> hemant.agrawal at nxp.com; Vargas, Hernan <hernan.vargas at 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 at 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 at 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
>>