Re: [PATCH v4 13/14] baseband/acc: add PF configure companion function

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/24/22 02:20, Chautru, Nicolas wrote:
> Hi Maxime,
> Thanks will send an updated serie now. Much appreciated.
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, September 23, 2022 2:26 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> thomas@monjalon.net
>> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
>> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
>> david.marchand@redhat.com; stephen@networkplumber.org; Vargas,
>> Hernan <hernan.vargas@intel.com>
>> Subject: Re: [PATCH v4 13/14] baseband/acc: add PF configure companion
>> function
>>
>>
>>
>> On 9/22/22 02:27, Nic Chautru wrote:
>>> Add configure function notably to configure the device from the PF
>>> within DPDK and bbdev-test (without external dependency).
>>>
>>> Signed-off-by: Nic Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    app/test-bbdev/test_bbdev_perf.c      |  71 ++++++
>>>    drivers/baseband/acc/meson.build      |   2 +-
>>>    drivers/baseband/acc/rte_acc200_cfg.h |  48 ++++
>>>    drivers/baseband/acc/rte_acc200_pmd.c | 453
>> ++++++++++++++++++++++++++++++++++
>>>    drivers/baseband/acc/version.map      |   1 +
>>>    5 files changed, 574 insertions(+), 1 deletion(-)
>>>    create mode 100644 drivers/baseband/acc/rte_acc200_cfg.h
>>>
>>> diff --git a/app/test-bbdev/test_bbdev_perf.c
>>> b/app/test-bbdev/test_bbdev_perf.c
>>> index 41c78de..ec6ee2a 100644
>>> --- a/app/test-bbdev/test_bbdev_perf.c
>>> +++ b/app/test-bbdev/test_bbdev_perf.c
>>> @@ -62,6 +62,15 @@
>>>    #define ACC100_QMGR_INVALID_IDX -1
>>>    #define ACC100_QMGR_RR 1
>>>    #define ACC100_QOS_GBR 0
>>> +#include <rte_acc200_cfg.h>
>>> +#define ACC200PF_DRIVER_NAME   ("intel_acc200_pf")
>>> +#define ACC200VF_DRIVER_NAME   ("intel_acc200_vf")
>>> +#define ACC200_QMGR_NUM_AQS 16
>>> +#define ACC200_QMGR_NUM_QGS 2
>>> +#define ACC200_QMGR_AQ_DEPTH 5
>>> +#define ACC200_QMGR_INVALID_IDX -1
>>> +#define ACC200_QMGR_RR 1
>>> +#define ACC200_QOS_GBR 0
>>>    #endif
>>>
>>>    #define OPS_CACHE_SIZE 256U
>>> @@ -761,6 +770,68 @@ 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, ACC200PF_DRIVER_NAME)))
>> {
>>> +		struct rte_acc_conf conf;
>>> +		unsigned int i;
>>> +
>>> +		printf("Configure ACC200 FEC Driver %s with default
>> values\n",
>>> +				info->drv.driver_name);
>>> +
>>> +		/* clear default configuration before initialization */
>>> +		memset(&conf, 0, sizeof(struct rte_acc_conf));
>>> +
>>> +		/* Always set in PF mode for built-in configuration */
>>> +		conf.pf_mode_en = true;
>>> +		for (i = 0; i < RTE_ACC_NUM_VFS; ++i) {
>>> +			conf.arb_dl_4g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_dl_4g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_dl_4g[i].round_robin_weight =
>> ACC200_QMGR_RR;
>>> +			conf.arb_ul_4g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_ul_4g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_ul_4g[i].round_robin_weight =
>> ACC200_QMGR_RR;
>>> +			conf.arb_dl_5g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_dl_5g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_dl_5g[i].round_robin_weight =
>> ACC200_QMGR_RR;
>>> +			conf.arb_ul_5g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_ul_5g[i].gbr_threshold1 =
>> ACC200_QOS_GBR;
>>> +			conf.arb_ul_5g[i].round_robin_weight =
>> ACC200_QMGR_RR;
>>> +			conf.arb_fft[i].gbr_threshold1 = ACC200_QOS_GBR;
>>> +			conf.arb_fft[i].gbr_threshold1 = ACC200_QOS_GBR;
>>> +			conf.arb_fft[i].round_robin_weight =
>> ACC200_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 = ACC200_QMGR_NUM_QGS;
>>> +		conf.q_ul_4g.first_qgroup_index =
>> ACC200_QMGR_INVALID_IDX;
>>> +		conf.q_ul_4g.num_aqs_per_groups =
>> ACC200_QMGR_NUM_AQS;
>>> +		conf.q_ul_4g.aq_depth_log2 = ACC200_QMGR_AQ_DEPTH;
>>> +		conf.q_dl_4g.num_qgroups = ACC200_QMGR_NUM_QGS;
>>> +		conf.q_dl_4g.first_qgroup_index =
>> ACC200_QMGR_INVALID_IDX;
>>> +		conf.q_dl_4g.num_aqs_per_groups =
>> ACC200_QMGR_NUM_AQS;
>>> +		conf.q_dl_4g.aq_depth_log2 = ACC200_QMGR_AQ_DEPTH;
>>> +		conf.q_ul_5g.num_qgroups = ACC200_QMGR_NUM_QGS;
>>> +		conf.q_ul_5g.first_qgroup_index =
>> ACC200_QMGR_INVALID_IDX;
>>> +		conf.q_ul_5g.num_aqs_per_groups =
>> ACC200_QMGR_NUM_AQS;
>>> +		conf.q_ul_5g.aq_depth_log2 = ACC200_QMGR_AQ_DEPTH;
>>> +		conf.q_dl_5g.num_qgroups = ACC200_QMGR_NUM_QGS;
>>> +		conf.q_dl_5g.first_qgroup_index =
>> ACC200_QMGR_INVALID_IDX;
>>> +		conf.q_dl_5g.num_aqs_per_groups =
>> ACC200_QMGR_NUM_AQS;
>>> +		conf.q_dl_5g.aq_depth_log2 = ACC200_QMGR_AQ_DEPTH;
>>> +		conf.q_fft.num_qgroups = ACC200_QMGR_NUM_QGS;
>>> +		conf.q_fft.first_qgroup_index =
>> ACC200_QMGR_INVALID_IDX;
>>> +		conf.q_fft.num_aqs_per_groups =
>> ACC200_QMGR_NUM_AQS;
>>> +		conf.q_fft.aq_depth_log2 = ACC200_QMGR_AQ_DEPTH;
>>> +
>>> +		/* setup PF with configuration information */
>>> +		ret = rte_acc200_configure(info->dev_name, &conf);
>>> +		TEST_ASSERT_SUCCESS(ret,
>>> +				"Failed to configure ACC200 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/acc/meson.build
>>> b/drivers/baseband/acc/meson.build
>>> index 63912f0..7ae162a 100644
>>> --- a/drivers/baseband/acc/meson.build
>>> +++ b/drivers/baseband/acc/meson.build
>>> @@ -5,4 +5,4 @@ deps += ['bbdev', 'bus_vdev', 'ring', 'pci',
>>> 'bus_pci']
>>>
>>>    sources = files('rte_acc100_pmd.c', 'rte_acc200_pmd.c')
>>>
>>> -headers = files('rte_acc100_cfg.h')
>>> +headers = files('rte_acc100_cfg.h', 'rte_acc200_cfg.h')
>>> diff --git a/drivers/baseband/acc/rte_acc200_cfg.h
>>> b/drivers/baseband/acc/rte_acc200_cfg.h
>>> new file mode 100644
>>> index 0000000..8bd9b20
>>> --- /dev/null
>>> +++ b/drivers/baseband/acc/rte_acc200_cfg.h
>>> @@ -0,0 +1,48 @@
>>> +/* SPDX-License-Identifier: BSD-3-Clause
>>> + * Copyright(c) 2022 Intel Corporation  */
>>> +
>>> +#ifndef _RTE_ACC200_CFG_H_
>>> +#define _RTE_ACC200_CFG_H_
>>> +
>>> +/**
>>> + * @file rte_acc200_cfg.h
>>> + *
>>> + * Functions for configuring ACC200 HW, exposed directly to applications.
>>> + * Configuration related to encoding/decoding is done through the
>>> + * librte_bbdev library.
>>> + *
>>> + * @warning
>>> + * @b EXPERIMENTAL: this API may change without prior notice  */
>>> +
>>> +#include <stdint.h>
>>> +#include <stdbool.h>
>>> +#include "rte_acc_common_cfg.h"
>>> +
>>> +#ifdef __cplusplus
>>> +extern "C" {
>>> +#endif
>>> +
>>> +/**
>>> + * Configure a ACC200 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 ACC200 HW.
>>> + *
>>> + * @return
>>> + *   Zero on success, negative value on failure.
>>> + */
>>> +__rte_experimental
>>> +int
>>> +rte_acc200_configure(const char *dev_name, struct rte_acc_conf
>>> +*conf);
>>> +
>>> +#ifdef __cplusplus
>>> +}
>>> +#endif
>>> +
>>> +#endif /* _RTE_ACC200_CFG_H_ */
>>> diff --git a/drivers/baseband/acc/rte_acc200_pmd.c
>>> b/drivers/baseband/acc/rte_acc200_pmd.c
>>> index 3c2931b..21d614d 100644
>>> --- a/drivers/baseband/acc/rte_acc200_pmd.c
>>> +++ b/drivers/baseband/acc/rte_acc200_pmd.c
>>> @@ -43,6 +43,27 @@
>>>
>>>    enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, FFT, NUM_ACC};
>>>
>>> +/* Return the accelerator enum for a Queue Group Index */ static
>>> +inline int accFromQgid(int qg_idx, const struct rte_acc_conf
>>> +*acc_conf) {
>>> +	int accQg[ACC200_NUM_QGRPS];
>>> +	int NumQGroupsPerFn[NUM_ACC];
>>> +	int acc, qgIdx, qgIndex = 0;
>>> +	for (qgIdx = 0; qgIdx < ACC200_NUM_QGRPS; qgIdx++)
>>> +		accQg[qgIdx] = 0;
>>> +	NumQGroupsPerFn[UL_4G] = acc_conf->q_ul_4g.num_qgroups;
>>> +	NumQGroupsPerFn[UL_5G] = acc_conf->q_ul_5g.num_qgroups;
>>> +	NumQGroupsPerFn[DL_4G] = acc_conf->q_dl_4g.num_qgroups;
>>> +	NumQGroupsPerFn[DL_5G] = acc_conf->q_dl_5g.num_qgroups;
>>> +	NumQGroupsPerFn[FFT] = acc_conf->q_fft.num_qgroups;
>>> +	for (acc = UL_4G;  acc < NUM_ACC; acc++)
>>> +		for (qgIdx = 0; qgIdx < NumQGroupsPerFn[acc]; qgIdx++)
>>> +			accQg[qgIndex++] = acc;
>>> +	acc = accQg[qg_idx];
>>> +	return acc;
>>> +}
>>> +
>>>    /* Return the queue topology for a Queue Group Index */
>>>    static inline void
>>>    qtopFromAcc(struct rte_acc_queue_topology **qtop, int acc_enum, @@
>>> -75,6 +96,30 @@
>>>    	*qtop = p_qtop;
>>>    }
>>>
>>> +/* Return the AQ depth for a Queue Group Index */ static inline int
>>> +aqDepth(int qg_idx, struct rte_acc_conf *acc_conf) {
>>> +	struct rte_acc_queue_topology *q_top = NULL;
>>> +	int acc_enum = accFromQgid(qg_idx, acc_conf);
>>> +	qtopFromAcc(&q_top, acc_enum, acc_conf);
>>> +	if (unlikely(q_top == NULL))
>>> +		return 0;
>>> +	return q_top->aq_depth_log2;
>>> +}
>>> +
>>> +/* Return the AQ depth for a Queue Group Index */ static inline int
>>> +aqNum(int qg_idx, struct rte_acc_conf *acc_conf) {
>>> +	struct rte_acc_queue_topology *q_top = NULL;
>>> +	int acc_enum = accFromQgid(qg_idx, acc_conf);
>>> +	qtopFromAcc(&q_top, acc_enum, acc_conf);
>>> +	if (unlikely(q_top == NULL))
>>> +		return 0;
>>> +	return q_top->num_aqs_per_groups;
>>> +}
>>
>>
>> For the 3 functions above, please add new lines for clarity.
> 
> OK, guidelines would be good, as I am not fully convinced. But updating in new series.
> 

I have not checked whether it is in the contribution guidelines, but 
having new lines between the variables declarations and code is
a common rules that makes the code more readable. So is having new lines
before the returns.