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

["Chautru, Nicolas" <nicolas.chautru@intel.com>]

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

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

> 
> Tom
>