From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <dev-bounces@dpdk.org>
Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124])
	by inbox.dpdk.org (Postfix) with ESMTP id 38769A0093;
	Tue, 10 May 2022 14:02:14 +0200 (CEST)
Received: from [217.70.189.124] (localhost [127.0.0.1])
	by mails.dpdk.org (Postfix) with ESMTP id CDB85415D7;
	Tue, 10 May 2022 14:02:13 +0200 (CEST)
Received: from us-smtp-delivery-124.mimecast.com
 (us-smtp-delivery-124.mimecast.com [170.10.133.124])
 by mails.dpdk.org (Postfix) with ESMTP id 93ED44069D
 for <dev@dpdk.org>; Tue, 10 May 2022 14:02:12 +0200 (CEST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com;
 s=mimecast20190719; t=1652184132;
 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=KVol0ooXAc8S6AYCOckcBg2R+CAm+/Sx/3rCmVRiwB4=;
 b=ebSOwO8bibNb479cGrpgylHyteXo+a5UJzNLlFiUhp69e2GfAnZv6IBf/2w0+BXdmImXYX
 qkdK28KI26r9UOJZVo8QdxAdIMo/lshHo089jxvkThIul+glj/3cf0HJDwiMi3usLBIpGa
 ZbTdtjU95PBMaf4ePOv3yUszmdbhtEI=
Received: from mail-qk1-f197.google.com (mail-qk1-f197.google.com
 [209.85.222.197]) by relay.mimecast.com with ESMTP with STARTTLS
 (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id
 us-mta-642--zmw4ApOMkaq3L_ncw9khA-1; Tue, 10 May 2022 08:02:09 -0400
X-MC-Unique: -zmw4ApOMkaq3L_ncw9khA-1
Received: by mail-qk1-f197.google.com with SMTP id
 j12-20020ae9c20c000000b0069e8ac6b244so10542059qkg.1
 for <dev@dpdk.org>; Tue, 10 May 2022 05:02:09 -0700 (PDT)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
 d=1e100.net; s=20210112;
 h=x-gm-message-state:subject:to:cc:references:from:message-id:date
 :user-agent:mime-version:in-reply-to:content-transfer-encoding
 :content-language;
 bh=KVol0ooXAc8S6AYCOckcBg2R+CAm+/Sx/3rCmVRiwB4=;
 b=crEadJKkCnnYJJq7Q9mCQn12zBBBuSov1aBa0jTbwg+dDWARTDmH0FBGEMXJZfuvDi
 RBP8cXjxks7GSVdMm4tEMPJH1Pq1aULIsqFFrs8F5vrzu341Bf3PNujhML/3aBywrThm
 b6a9UTZAe1QMOT42Mtsq3VqTJOBOWXbNg23Ma0FPbu3Ivit2YAE+kLsFCoHravGjGdZ0
 x1mOzNgCwKG++C82ubgCL3anpG/b1X1HzdlYWGtmpHtqXLfLW3gmW/u2t+2HoBKqfKDg
 SfFPVR/1EA340q1Q7x42HSp9+5gOm64t9Yq6FmD1YV3XT+0qBk25fUkXrb3XJ6HmYP/2
 VW3A==
X-Gm-Message-State: AOAM530FapAkrNmUTunmmUIv/MRLtqEvEWpQ6Q39OwVbtkakOAEGDULT
 bp1cmYOGTjuEydW8XMvCqZjpmsEe6Ly/JkNKJSoasYf6jA7GItoNh0Y4Ckf4SafmZ86lnfn4uze
 DU7E=
X-Received: by 2002:a05:622a:284:b0:2f3:c6aa:6c96 with SMTP id
 z4-20020a05622a028400b002f3c6aa6c96mr18842881qtw.512.1652184128462; 
 Tue, 10 May 2022 05:02:08 -0700 (PDT)
X-Google-Smtp-Source: ABdhPJyYcr8/p5p2EwDhJuixmcnXEbKgrVVVNHuxRQEg7V4w0k+85F7KBjbyLqk5O3SINOhC40Vy7Q==
X-Received: by 2002:a05:622a:284:b0:2f3:c6aa:6c96 with SMTP id
 z4-20020a05622a028400b002f3c6aa6c96mr18842835qtw.512.1652184128047; 
 Tue, 10 May 2022 05:02:08 -0700 (PDT)
Received: from localhost.localdomain (024-205-208-113.res.spectrum.com.
 [24.205.208.113]) by smtp.gmail.com with ESMTPSA id
 a123-20020ae9e881000000b0069fc13ce227sm8395060qkg.88.2022.05.10.05.02.06
 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128);
 Tue, 10 May 2022 05:02:07 -0700 (PDT)
Subject: Re: [PATCH v2 3/5] baseband/acc100: configuration of ACC101 from PF
To: "Chautru, Nicolas" <nicolas.chautru@intel.com>,
 "dev@dpdk.org" <dev@dpdk.org>, "gakhil@marvell.com" <gakhil@marvell.com>
Cc: "thomas@monjalon.net" <thomas@monjalon.net>,
 "Kinsella, Ray" <ray.kinsella@intel.com>,
 "Richardson, Bruce" <bruce.richardson@intel.com>,
 "hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
 "Zhang, Mingshan" <mingshan.zhang@intel.com>,
 "david.marchand@redhat.com" <david.marchand@redhat.com>
References: <1651083423-33202-1-git-send-email-nicolas.chautru@intel.com>
 <1651083423-33202-4-git-send-email-nicolas.chautru@intel.com>
 <8384fb18-ce2d-a6e3-2930-c519f3b2139b@redhat.com>
 <BY5PR11MB44511BB19E72FB53C2838F64F8C69@BY5PR11MB4451.namprd11.prod.outlook.com>
From: Tom Rix <trix@redhat.com>
Message-ID: <b11a2b28-7242-5a6a-9825-336550034d04@redhat.com>
Date: Tue, 10 May 2022 05:02:04 -0700
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.10.1
MIME-Version: 1.0
In-Reply-To: <BY5PR11MB44511BB19E72FB53C2838F64F8C69@BY5PR11MB4451.namprd11.prod.outlook.com>
Authentication-Results: relay.mimecast.com;
 auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=trix@redhat.com
X-Mimecast-Spam-Score: 0
X-Mimecast-Originator: redhat.com
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
X-BeenThere: dev@dpdk.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: DPDK patches and discussions <dev.dpdk.org>
List-Unsubscribe: <https://mails.dpdk.org/options/dev>,
 <mailto:dev-request@dpdk.org?subject=unsubscribe>
List-Archive: <http://mails.dpdk.org/archives/dev/>
List-Post: <mailto:dev@dpdk.org>
List-Help: <mailto:dev-request@dpdk.org?subject=help>
List-Subscribe: <https://mails.dpdk.org/listinfo/dev>,
 <mailto:dev-request@dpdk.org?subject=subscribe>
Errors-To: dev-bounces@dpdk.org


On 5/9/22 2:36 PM, Chautru, Nicolas wrote:
>
>> -----Original Message-----
>> From: Tom Rix <trix@redhat.com>
>> Sent: Sunday, May 8, 2022 6:38 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com
>> Cc: thomas@monjalon.net; Kinsella, Ray <ray.kinsella@intel.com>; Richardson,
>> Bruce <bruce.richardson@intel.com>; hemant.agrawal@nxp.com; Zhang,
>> Mingshan <mingshan.zhang@intel.com>; david.marchand@redhat.com
>> Subject: Re: [PATCH v2 3/5] baseband/acc100: configuration of ACC101 from
>> PF
>>
>>
>> On 4/27/22 11:17 AM, Nicolas Chautru wrote:
>>> Adding companion function specific to ACC100 and it can be called from
>>> bbdev-test when running from PF.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    app/test-bbdev/test_bbdev_perf.c         |  57 ++++++
>>>    drivers/baseband/acc100/rte_acc100_cfg.h |  17 ++
>>>    drivers/baseband/acc100/rte_acc100_pmd.c | 302
>> +++++++++++++++++++++++++++++++
>>>    drivers/baseband/acc100/version.map      |   2 +-
>>>    4 files changed, 377 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/app/test-bbdev/test_bbdev_perf.c
>>> b/app/test-bbdev/test_bbdev_perf.c
>>> index 0fa119a..baf5f6d 100644
>>> --- a/app/test-bbdev/test_bbdev_perf.c
>>> +++ b/app/test-bbdev/test_bbdev_perf.c
>>> @@ -63,6 +63,8 @@
>>>    #define ACC100_QMGR_INVALID_IDX -1
>>>    #define ACC100_QMGR_RR 1
>>>    #define ACC100_QOS_GBR 0
>>> +#define ACC101PF_DRIVER_NAME   ("intel_acc101_pf")
>>> +#define ACC101VF_DRIVER_NAME   ("intel_acc101_vf")
>> A dup from patch 1
>>>    #endif
>>>
>>>    #define OPS_CACHE_SIZE 256U
>>> @@ -765,6 +767,61 @@ typedef int (test_case_function)(struct
>> active_device *ad,
>>>    				"Failed to configure ACC100 PF for bbdev %s",
>>>    				info->dev_name);
>>>    	}
>>> +	if ((get_init_device() == true) &&
>>> +		(!strcmp(info->drv.driver_name, ACC101PF_DRIVER_NAME)))
>> {
>>> +		struct rte_acc100_conf conf;
>> Mixing up acc100 and acc101 ?
>>
>> If this actually works, combine the two.
> The configuration file template is the same but not the configuration file. I can combine a bit more that part.
>
>>> +		unsigned int i;
>>> +
>>> +		printf("Configure ACC101 FEC Driver %s with default values\n",
>>> +				info->drv.driver_name);
>>> +
>>> +		/* clear default configuration before initialization */
>>> +		memset(&conf, 0, sizeof(struct rte_acc100_conf));
>>> +
>>> +		/* Always set in PF mode for built-in configuration */
>>> +		conf.pf_mode_en = true;
>>> +		for (i = 0; i < RTE_ACC100_NUM_VFS; ++i) {
>>> +			conf.arb_dl_4g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_dl_4g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_dl_4g[i].round_robin_weight =
>> ACC100_QMGR_RR;
>>> +			conf.arb_ul_4g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_ul_4g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_ul_4g[i].round_robin_weight =
>> ACC100_QMGR_RR;
>>> +			conf.arb_dl_5g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_dl_5g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_dl_5g[i].round_robin_weight =
>> ACC100_QMGR_RR;
>>> +			conf.arb_ul_5g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_ul_5g[i].gbr_threshold1 =
>> ACC100_QOS_GBR;
>>> +			conf.arb_ul_5g[i].round_robin_weight =
>> ACC100_QMGR_RR;
>>> +		}
>>> +
>>> +		conf.input_pos_llr_1_bit = true;
>>> +		conf.output_pos_llr_1_bit = true;
>>> +		conf.num_vf_bundles = 1; /**< Number of VF bundles to setup
>> */
>>> +
>>> +		conf.q_ul_4g.num_qgroups = ACC100_QMGR_NUM_QGS;
>>> +		conf.q_ul_4g.first_qgroup_index =
>> ACC100_QMGR_INVALID_IDX;
>>> +		conf.q_ul_4g.num_aqs_per_groups =
>> ACC100_QMGR_NUM_AQS;
>>> +		conf.q_ul_4g.aq_depth_log2 = ACC100_QMGR_AQ_DEPTH;
>>> +		conf.q_dl_4g.num_qgroups = ACC100_QMGR_NUM_QGS;
>>> +		conf.q_dl_4g.first_qgroup_index =
>> ACC100_QMGR_INVALID_IDX;
>>> +		conf.q_dl_4g.num_aqs_per_groups =
>> ACC100_QMGR_NUM_AQS;
>>> +		conf.q_dl_4g.aq_depth_log2 = ACC100_QMGR_AQ_DEPTH;
>>> +		conf.q_ul_5g.num_qgroups = ACC100_QMGR_NUM_QGS;
>>> +		conf.q_ul_5g.first_qgroup_index =
>> ACC100_QMGR_INVALID_IDX;
>>> +		conf.q_ul_5g.num_aqs_per_groups =
>> ACC100_QMGR_NUM_AQS;
>>> +		conf.q_ul_5g.aq_depth_log2 = ACC100_QMGR_AQ_DEPTH;
>>> +		conf.q_dl_5g.num_qgroups = ACC100_QMGR_NUM_QGS;
>>> +		conf.q_dl_5g.first_qgroup_index =
>> ACC100_QMGR_INVALID_IDX;
>>> +		conf.q_dl_5g.num_aqs_per_groups =
>> ACC100_QMGR_NUM_AQS;
>>> +		conf.q_dl_5g.aq_depth_log2 = ACC100_QMGR_AQ_DEPTH;
>>> +
>>> +		/* setup PF with configuration information */
>>> +		ret = rte_acc101_configure(info->dev_name, &conf);
>>> +		TEST_ASSERT_SUCCESS(ret,
>>> +				"Failed to configure ACC101 PF for bbdev %s",
>>> +				info->dev_name);
>>> +	}
>>>    #endif
>>>    	/* Let's refresh this now this is configured */
>>>    	rte_bbdev_info_get(dev_id, info);
>>> diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h
>>> b/drivers/baseband/acc100/rte_acc100_cfg.h
>>> index d233e42..2e3c43f 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_cfg.h
>>> +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
>> This file marks its API as experimental though the acc100 has been used in
>> production for some time.
>>
>> It is important that the API be stable.
>>
>> Is this an oversight ?
>>
>> Or what is needed to stabilize the API ?
> This is not part of the BBDEV-API, this is companion function to configure the device notably for bbdev-test. ie. would not be used in live production (ie. we would not run from the PF).
> It could be made non experimental through another patch if desired.
> With regards to the ACC101, this is the new function hence starting as experimental.

If this is not part of the bbdev-api at all and is just test code, 
please remove all of the experimental tags.

Such tags are reviewed and would be misinterpreted that the source is 
not production ready.

>
>>> @@ -106,6 +106,23 @@ struct rte_acc100_conf {
>>>    int
>>>    rte_acc100_configure(const char *dev_name, struct rte_acc100_conf
>>> *conf);
>>>
>>> +/**
>>> + * Configure a ACC101 device
>>> + *
>>> + * @param dev_name
>>> + *   The name of the device. This is the short form of PCI BDF, e.g. 00:01.0.
>>> + *   It can also be retrieved for a bbdev device from the dev_name field in the
>>> + *   rte_bbdev_info structure returned by rte_bbdev_info_get().
>>> + * @param conf
>>> + *   Configuration to apply to ACC101 HW.
>>> + *
>>> + * @return
>>> + *   Zero on success, negative value on failure.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_acc101_configure(const char *dev_name, struct rte_acc100_conf
>>> +*conf);
>> I am finding seeing acc100* structs in acc101 function parameters confusing.
>>
>> Maybe a general renaming of acc100 -> acc10x for the common parts.
> Again this is just a companion function to configure the device.
>
>> Will we have this problem on acc120 or acc200 ?
> There is a plan for ACC200 but that is a complete different product and distinct PMD.
>
>> Maybe shorten everything now to acc
>>
>>> +
>>>    #ifdef __cplusplus
>>>    }
>>>    #endif
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> index daf2ce0..b03cedc 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -4921,3 +4921,305 @@ static int acc100_pci_remove(struct
>> rte_pci_device *pci_dev)
>>>    	rte_bbdev_log_debug("PF Tip configuration complete for %s",
>> dev_name);
>>>    	return 0;
>>>    }
>>> +
>>> +
>>> +/* Initial configuration of a ACC101 device prior to running
>>> +configure() */ int rte_acc101_configure(const char *dev_name, struct
>>> +rte_acc100_conf *conf) {
>> This is very similar to the acc100 configure function.
>>
>> It would be good if these could be combined.
> These should not be combined. The device configuration is distinct and would be artificial to make that function support non compatible register interface.
> Note that this functional is again is not part of PMD.

ok

Tom

>
>> Tom
>>