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

[Tom Rix <trix@redhat.com>]

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.

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

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

Will we have this problem on acc120 or acc200 ?

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.

Tom