From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Chautru, Nicolas" <nicolas.chautru@intel.com>,
David Marchand <david.marchand@redhat.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"hemant.agrawal@nxp.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
Date: Wed, 27 Sep 2023 09:32:10 +0200 [thread overview]
Message-ID: <7e5ec641-885d-f66f-c122-ea19ce6181a5@redhat.com> (raw)
In-Reply-To: <d0dd8bea-aa76-0958-b3e9-9b6eb621a1ce@redhat.com>
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
>>
next prev parent reply other threads:[~2023-09-27 7:32 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-19 1:21 [PATCH v1 0/7] VRB2 BBDEV PMD introduction Nicolas Chautru
2023-09-19 1:21 ` [PATCH v1 1/7] bbdev: add FFT version member in driver info Nicolas Chautru
2023-09-19 9:55 ` Maxime Coquelin
2023-09-19 20:51 ` Chautru, Nicolas
2023-09-22 8:14 ` Maxime Coquelin
2023-09-22 16:41 ` Chautru, Nicolas
2023-09-26 10:00 ` Maxime Coquelin
2023-09-27 23:50 ` Chautru, Nicolas
2023-09-28 8:27 ` Maxime Coquelin
2023-09-28 16:33 ` Chautru, Nicolas
2023-10-03 11:46 ` Maxime Coquelin
2023-09-19 1:21 ` [PATCH v1 2/7] baseband/acc: add FFT version in the VRM PMD Nicolas Chautru
2023-09-19 1:21 ` [PATCH v1 3/7] baseband/acc: remove the 4G SO capability for VRB1 Nicolas Chautru
2023-09-19 15:20 ` David Marchand
2023-09-19 20:32 ` Chautru, Nicolas
2023-09-21 7:13 ` David Marchand
2023-09-21 17:18 ` Chautru, Nicolas
2023-09-27 7:08 ` Maxime Coquelin
2023-09-27 7:32 ` Maxime Coquelin [this message]
2023-09-19 1:21 ` [PATCH v1 4/7] baseband/acc: allocate FCW memory separately Nicolas Chautru
2023-09-19 1:21 ` [PATCH v1 5/7] baseband/acc: add support for MLD operation Nicolas Chautru
2023-09-19 1:21 ` [PATCH v1 6/7] baseband/acc: introduce the new VRB2 variant Nicolas Chautru
2023-09-19 1:21 ` [PATCH v1 7/7] baseband/acc: add configure helper for VRB2 Nicolas Chautru
2023-09-21 7:25 ` [PATCH v1 0/7] VRB2 BBDEV PMD introduction David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7e5ec641-885d-f66f-c122-ea19ce6181a5@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=hemant.agrawal@nxp.com \
--cc=hernan.vargas@intel.com \
--cc=nicolas.chautru@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).