From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id EE6B94264F; Wed, 27 Sep 2023 09:32:19 +0200 (CEST) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9352640277; Wed, 27 Sep 2023 09:32:19 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id DE99940271 for ; Wed, 27 Sep 2023 09:32:17 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1695799937; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=kvwtsazHMShnh+PWS1bfErYMfzU4EISyxq2h40A+nvo=; b=Xx+fQ2ChKt2spI8hqNU1MyBX3Hn9TL3R5EzNHi4i7DbYFmyL7Ue9dpsB6TcrRm9sohwsUU ij2iSPzSSX+/yEYc9TUlicodsKYVmrtQawin27lZR1iAExY2G1QllHznx3D1Ge5S1TgPWF F7lkiIByeC2WTGFkMcx3I6+3cY17Fx4= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-643-NR0GsNuJM6a39cbqX80_qg-1; Wed, 27 Sep 2023 03:32:14 -0400 X-MC-Unique: NR0GsNuJM6a39cbqX80_qg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.rdu2.redhat.com [10.11.54.1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 979098015AB; Wed, 27 Sep 2023 07:32:13 +0000 (UTC) Received: from [10.39.208.19] (unknown [10.39.208.19]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2840640C2064; Wed, 27 Sep 2023 07:32:11 +0000 (UTC) Message-ID: <7e5ec641-885d-f66f-c122-ea19ce6181a5@redhat.com> Date: Wed, 27 Sep 2023 09:32:10 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Subject: Re: [PATCH v1 3/7] baseband/acc: remove the 4G SO capability for VRB1 From: Maxime Coquelin To: "Chautru, Nicolas" , David Marchand Cc: "dev@dpdk.org" , "hemant.agrawal@nxp.com" , "Vargas, Hernan" , Thomas Monjalon References: <20230919012136.2818396-1-nicolas.chautru@intel.com> <20230919012136.2818396-4-nicolas.chautru@intel.com> In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.1 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 >>> Sent: Thursday, September 21, 2023 12:13 AM >>> To: Chautru, Nicolas >>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; >>> hemant.agrawal@nxp.com; Vargas, Hernan ; >>> Thomas Monjalon >>> 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 >>> wrote: >>>> >>>> Hi David, >>>> >>>>> -----Original Message----- >>>>> From: David Marchand >>>>> Sent: Tuesday, September 19, 2023 8:20 AM >>>>> To: Chautru, Nicolas >>>>> Cc: dev@dpdk.org; maxime.coquelin@redhat.com; >>>>> hemant.agrawal@nxp.com; Vargas, Hernan >>>>> 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 >>>>> 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 >>>>>> --- >>>>>>   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 >>