Re: [PATCH v2 3/5] baseband/acc100: configuration of ACC101 from PF

[Tom Rix <trix@redhat.com>]

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
>>