[dpdk-dev] [PATCH v11 03/10] baseband/acc100: add info get function

[Chautru, Nicolas]

Hi Tom

> From: Tom Rix <trix at redhat.com>> 
> 
> On 10/1/20 6:01 PM, Nicolas Chautru wrote:
> > Add in the "info_get" function to the driver, to allow us to query the
> > device.
> > No processing capability are available yet.
> > Linking bbdev-test to support the PMD with null capability.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru at intel.com>
> > Acked-by: Liu Tianjiao <Tianjiao.liu at intel.com>
> > ---
> >  app/test-bbdev/meson.build               |   3 +
> >  drivers/baseband/acc100/rte_acc100_cfg.h |  96 +++++++++++++
> > drivers/baseband/acc100/rte_acc100_pmd.c | 229
> > +++++++++++++++++++++++++++++++
> > drivers/baseband/acc100/rte_acc100_pmd.h |  10 ++
> >  4 files changed, 338 insertions(+)
> >  create mode 100644 drivers/baseband/acc100/rte_acc100_cfg.h
> >
> > diff --git a/app/test-bbdev/meson.build b/app/test-bbdev/meson.build
> > index 18ab6a8..fbd8ae3 100644
> > --- a/app/test-bbdev/meson.build
> > +++ b/app/test-bbdev/meson.build
> > @@ -12,3 +12,6 @@ endif
> >  if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_FPGA_5GNR_FEC')
> >  	deps += ['pmd_bbdev_fpga_5gnr_fec']
> >  endif
> > +if dpdk_conf.has('RTE_LIBRTE_PMD_BBDEV_ACC100')
> > +	deps += ['pmd_bbdev_acc100']
> > +endif
> > \ No newline at end of file
> > diff --git a/drivers/baseband/acc100/rte_acc100_cfg.h
> > b/drivers/baseband/acc100/rte_acc100_cfg.h
> > new file mode 100644
> > index 0000000..a1d43ef
> > --- /dev/null
> > +++ b/drivers/baseband/acc100/rte_acc100_cfg.h
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2020 Intel Corporation  */
> > +
> > +#ifndef _RTE_ACC100_CFG_H_
> > +#define _RTE_ACC100_CFG_H_
> > +
> > +/**
> > + * @file rte_acc100_cfg.h
> > + *
> > + * Functions for configuring ACC100 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>
> > +
> > +#ifdef __cplusplus
> > +extern "C" {
> > +#endif
> > +/**< Number of Virtual Functions ACC100 supports */ #define
> > +RTE_ACC100_NUM_VFS 16
> 
> I was expecting the definition of RTE_ACC100_NUM_VFS to be removed.
> 
> And its uses replaced with ACC100_NUM_VFS.
> 
> or
> 
> #define RTE_ACC100_NUM_VFS ACC100_NUM_VFS
> 

Yes it was actually on purpose to keep that piece of code portable outside of DPDK if required. 
One is related to the PMD generic function, the other one is used for the configuration function only. 
If you feel strongly about this I could change. 

> > +
> > +/**
> > + * Definition of Queue Topology for ACC100 Configuration
> > + * Some level of details is abstracted out to expose a clean
> > +interface
> > + * given that comprehensive flexibility is not required  */ struct
> > +rte_acc100_queue_topology {
> > +	/** Number of QGroups in incremental order of priority */
> > +	uint16_t num_qgroups;
> > +	/**
> > +	 * All QGroups have the same number of AQs here.
> > +	 * Note : Could be made a 16-array if more flexibility is really
> > +	 * required
> > +	 */
> > +	uint16_t num_aqs_per_groups;
> > +	/**
> > +	 * Depth of the AQs is the same of all QGroups here. Log2 Enum : 2^N
> > +	 * Note : Could be made a 16-array if more flexibility is really
> > +	 * required
> > +	 */
> > +	uint16_t aq_depth_log2;
> > +	/**
> > +	 * Index of the first Queue Group Index - assuming contiguity
> > +	 * Initialized as -1
> > +	 */
> > +	int8_t first_qgroup_index;
> > +};
> > +
> > +/**
> > + * Definition of Arbitration related parameters for ACC100
> > +Configuration  */ struct rte_acc100_arbitration {
> > +	/** Default Weight for VF Fairness Arbitration */
> > +	uint16_t round_robin_weight;
> > +	uint32_t gbr_threshold1; /**< Guaranteed Bitrate Threshold 1 */
> > +	uint32_t gbr_threshold2; /**< Guaranteed Bitrate Threshold 2 */ };
> > +
> > +/**
> > + * Structure to pass ACC100 configuration.
> > + * Note: all VF Bundles will have the same configuration.
> > + */
> > +struct rte_acc100_conf {
> > +	bool pf_mode_en; /**< 1 if PF is used for dataplane, 0 for VFs */
> > +	/** 1 if input '1' bit is represented by a positive LLR value, 0 if '1'
> > +	 * bit is represented by a negative value.
> > +	 */
> > +	bool input_pos_llr_1_bit;
> > +	/** 1 if output '1' bit is represented by a positive value, 0 if '1'
> > +	 * bit is represented by a negative value.
> > +	 */
> > +	bool output_pos_llr_1_bit;
> > +	uint16_t num_vf_bundles; /**< Number of VF bundles to setup */
> > +	/** Queue topology for each operation type */
> > +	struct rte_acc100_queue_topology q_ul_4g;
> > +	struct rte_acc100_queue_topology q_dl_4g;
> > +	struct rte_acc100_queue_topology q_ul_5g;
> > +	struct rte_acc100_queue_topology q_dl_5g;
> > +	/** Arbitration configuration for each operation type */
> > +	struct rte_acc100_arbitration arb_ul_4g[RTE_ACC100_NUM_VFS];
> > +	struct rte_acc100_arbitration arb_dl_4g[RTE_ACC100_NUM_VFS];
> > +	struct rte_acc100_arbitration arb_ul_5g[RTE_ACC100_NUM_VFS];
> > +	struct rte_acc100_arbitration arb_dl_5g[RTE_ACC100_NUM_VFS]; };
> > +
> > +#ifdef __cplusplus
> > +}
> > +#endif
> > +
> > +#endif /* _RTE_ACC100_CFG_H_ */
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
> > b/drivers/baseband/acc100/rte_acc100_pmd.c
> > index 1b4cd13..fcba77e 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.c
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
> > @@ -26,6 +26,188 @@
> >  RTE_LOG_REGISTER(acc100_logtype, pmd.bb.acc100, NOTICE);  #endif
> >
> > +/* Read a register of a ACC100 device */ static inline uint32_t
> > +acc100_reg_read(struct acc100_device *d, uint32_t offset) {
> > +
> > +	void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset);
> > +	uint32_t ret = *((volatile uint32_t *)(reg_addr));
> > +	return rte_le_to_cpu_32(ret);
> > +}
> > +
> > +/* Calculate the offset of the enqueue register */ static inline
> > +uint32_t queue_offset(bool pf_device, uint8_t vf_id, uint8_t qgrp_id,
> > +uint16_t aq_id) {
> > +	if (pf_device)
> > +		return ((vf_id << 12) + (qgrp_id << 7) + (aq_id << 3) +
> > +				HWPfQmgrIngressAq);
> > +	else
> > +		return ((qgrp_id << 7) + (aq_id << 3) +
> > +				HWVfQmgrIngressAq);
> > +}
> > +
> > +enum {UL_4G = 0, UL_5G, DL_4G, DL_5G, NUM_ACC};
> > +
> > +/* Return the queue topology for a Queue Group Index */ static inline
> > +void qtopFromAcc(struct rte_acc100_queue_topology **qtop, int
> > +acc_enum,
> > +		struct rte_acc100_conf *acc100_conf) {
> > +	struct rte_acc100_queue_topology *p_qtop;
> > +	p_qtop = NULL;
> > +	switch (acc_enum) {
> > +	case UL_4G:
> > +		p_qtop = &(acc100_conf->q_ul_4g);
> > +		break;
> > +	case UL_5G:
> > +		p_qtop = &(acc100_conf->q_ul_5g);
> > +		break;
> > +	case DL_4G:
> > +		p_qtop = &(acc100_conf->q_dl_4g);
> > +		break;
> > +	case DL_5G:
> > +		p_qtop = &(acc100_conf->q_dl_5g);
> > +		break;
> > +	default:
> > +		/* NOTREACHED */
> > +		rte_bbdev_log(ERR, "Unexpected error evaluating
> qtopFromAcc");
> > +		break;
> > +	}
> > +	*qtop = p_qtop;
> > +}
> > +
> > +static void
> > +initQTop(struct rte_acc100_conf *acc100_conf) {
> > +	acc100_conf->q_ul_4g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_ul_4g.num_qgroups = 0;
> > +	acc100_conf->q_ul_4g.first_qgroup_index = -1;
> > +	acc100_conf->q_ul_5g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_ul_5g.num_qgroups = 0;
> > +	acc100_conf->q_ul_5g.first_qgroup_index = -1;
> > +	acc100_conf->q_dl_4g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_dl_4g.num_qgroups = 0;
> > +	acc100_conf->q_dl_4g.first_qgroup_index = -1;
> > +	acc100_conf->q_dl_5g.num_aqs_per_groups = 0;
> > +	acc100_conf->q_dl_5g.num_qgroups = 0;
> > +	acc100_conf->q_dl_5g.first_qgroup_index = -1; }
> > +
> > +static inline void
> > +updateQtop(uint8_t acc, uint8_t qg, struct rte_acc100_conf
> *acc100_conf,
> > +		struct acc100_device *d) {
> > +	uint32_t reg;
> > +	struct rte_acc100_queue_topology *q_top = NULL;
> > +	qtopFromAcc(&q_top, acc, acc100_conf);
> > +	if (unlikely(q_top == NULL))
> > +		return;
> > +	uint16_t aq;
> > +	q_top->num_qgroups++;
> > +	if (q_top->first_qgroup_index == -1) {
> > +		q_top->first_qgroup_index = qg;
> > +		/* Can be optimized to assume all are enabled by default */
> > +		reg = acc100_reg_read(d, queue_offset(d->pf_device,
> > +				0, qg, ACC100_NUM_AQS - 1));
> > +		if (reg & ACC100_QUEUE_ENABLE) {
> > +			q_top->num_aqs_per_groups = ACC100_NUM_AQS;
> > +			return;
> > +		}
> > +		q_top->num_aqs_per_groups = 0;
> > +		for (aq = 0; aq < ACC100_NUM_AQS; aq++) {
> > +			reg = acc100_reg_read(d, queue_offset(d-
> >pf_device,
> > +					0, qg, aq));
> > +			if (reg & ACC100_QUEUE_ENABLE)
> > +				q_top->num_aqs_per_groups++;
> > +		}
> > +	}
> > +}
> > +
> > +/* Fetch configuration enabled for the PF/VF using MMIO Read (slow)
> > +*/ static inline void fetch_acc100_config(struct rte_bbdev *dev) {
> > +	struct acc100_device *d = dev->data->dev_private;
> > +	struct rte_acc100_conf *acc100_conf = &d->acc100_conf;
> > +	const struct acc100_registry_addr *reg_addr;
> > +	uint8_t acc, qg;
> > +	uint32_t reg, reg_aq, reg_len0, reg_len1;
> > +	uint32_t reg_mode;
> > +
> > +	/* No need to retrieve the configuration is already done */
> > +	if (d->configured)
> > +		return;
> > +
> > +	/* Choose correct registry addresses for the device type */
> > +	if (d->pf_device)
> > +		reg_addr = &pf_reg_addr;
> > +	else
> > +		reg_addr = &vf_reg_addr;
> > +
> > +	d->ddr_size = (1 + acc100_reg_read(d, reg_addr->ddr_range)) << 10;
> > +
> > +	/* Single VF Bundle by VF */
> > +	acc100_conf->num_vf_bundles = 1;
> > +	initQTop(acc100_conf);
> > +
> > +	struct rte_acc100_queue_topology *q_top = NULL;
> > +	int qman_func_id[ACC100_NUM_ACCS] = {ACC100_ACCMAP_0,
> ACC100_ACCMAP_1,
> > +			ACC100_ACCMAP_2, ACC100_ACCMAP_3,
> ACC100_ACCMAP_4};
> > +	reg = acc100_reg_read(d, reg_addr->qman_group_func);
> > +	for (qg = 0; qg < ACC100_NUM_QGRPS_PER_WORD; qg++) {
> > +		reg_aq = acc100_reg_read(d,
> > +				queue_offset(d->pf_device, 0, qg, 0));
> > +		if (reg_aq & ACC100_QUEUE_ENABLE) {
> > +			uint32_t idx = (reg >> (qg * 4)) & 0x7;
> > +			if (idx >= ACC100_NUM_ACCS)
> > +				break;
> 
> a 'continue' would be better
> 
> or reverse the check
> 
> if (idx < ACC100_NUM_ACCS) {
> 
>     acc = qman_func_id ..
> 
> }

I can change. 
Please confirm what you think on the previous one. I would to get final by tomorrow latest.

Thanks
Nic


> 
> Tom
> 
> > +			acc = qman_func_id[idx];
> > +			updateQtop(acc, qg, acc100_conf, d);
> > +		}
> > +	}
> > +
> > +	/* Check the depth of the AQs*/
> > +	reg_len0 = acc100_reg_read(d, reg_addr->depth_log0_offset);
> > +	reg_len1 = acc100_reg_read(d, reg_addr->depth_log1_offset);
> > +	for (acc = 0; acc < NUM_ACC; acc++) {
> > +		qtopFromAcc(&q_top, acc, acc100_conf);
> > +		if (q_top->first_qgroup_index <
> ACC100_NUM_QGRPS_PER_WORD)
> > +			q_top->aq_depth_log2 = (reg_len0 >>
> > +					(q_top->first_qgroup_index * 4))
> > +					& 0xF;
> > +		else
> > +			q_top->aq_depth_log2 = (reg_len1 >>
> > +					((q_top->first_qgroup_index -
> > +					ACC100_NUM_QGRPS_PER_WORD) *
> 4))
> > +					& 0xF;
> > +	}
> > +
> > +	/* Read PF mode */
> > +	if (d->pf_device) {
> > +		reg_mode = acc100_reg_read(d, HWPfHiPfMode);
> > +		acc100_conf->pf_mode_en = (reg_mode == ACC100_PF_VAL)
> ? 1 : 0;
> > +	}
> > +
> > +	rte_bbdev_log_debug(
> > +			"%s Config LLR SIGN IN/OUT %s %s QG %u %u %u %u
> AQ %u %u %u %u Len %u %u %u %u\n",
> > +			(d->pf_device) ? "PF" : "VF",
> > +			(acc100_conf->input_pos_llr_1_bit) ? "POS" : "NEG",
> > +			(acc100_conf->output_pos_llr_1_bit) ? "POS" :
> "NEG",
> > +			acc100_conf->q_ul_4g.num_qgroups,
> > +			acc100_conf->q_dl_4g.num_qgroups,
> > +			acc100_conf->q_ul_5g.num_qgroups,
> > +			acc100_conf->q_dl_5g.num_qgroups,
> > +			acc100_conf->q_ul_4g.num_aqs_per_groups,
> > +			acc100_conf->q_dl_4g.num_aqs_per_groups,
> > +			acc100_conf->q_ul_5g.num_aqs_per_groups,
> > +			acc100_conf->q_dl_5g.num_aqs_per_groups,
> > +			acc100_conf->q_ul_4g.aq_depth_log2,
> > +			acc100_conf->q_dl_4g.aq_depth_log2,
> > +			acc100_conf->q_ul_5g.aq_depth_log2,
> > +			acc100_conf->q_dl_5g.aq_depth_log2);
> > +}
> > +
> >  /* Free 64MB memory used for software rings */  static int
> > acc100_dev_close(struct rte_bbdev *dev  __rte_unused) @@ -33,8
> +215,55
> > @@
> >  	return 0;
> >  }
> >
> > +/* Get ACC100 device info */
> > +static void
> > +acc100_dev_info_get(struct rte_bbdev *dev,
> > +		struct rte_bbdev_driver_info *dev_info) {
> > +	struct acc100_device *d = dev->data->dev_private;
> > +
> > +	static const struct rte_bbdev_op_cap bbdev_capabilities[] = {
> > +		RTE_BBDEV_END_OF_CAPABILITIES_LIST()
> > +	};
> > +
> > +	static struct rte_bbdev_queue_conf default_queue_conf;
> > +	default_queue_conf.socket = dev->data->socket_id;
> > +	default_queue_conf.queue_size = ACC100_MAX_QUEUE_DEPTH;
> > +
> > +	dev_info->driver_name = dev->device->driver->name;
> > +
> > +	/* Read and save the populated config from ACC100 registers */
> > +	fetch_acc100_config(dev);
> > +
> > +	/* This isn't ideal because it reports the maximum number of queues
> but
> > +	 * does not provide info on how many can be uplink/downlink or
> different
> > +	 * priorities
> > +	 */
> > +	dev_info->max_num_queues =
> > +			d->acc100_conf.q_dl_5g.num_aqs_per_groups *
> > +			d->acc100_conf.q_dl_5g.num_qgroups +
> > +			d->acc100_conf.q_ul_5g.num_aqs_per_groups *
> > +			d->acc100_conf.q_ul_5g.num_qgroups +
> > +			d->acc100_conf.q_dl_4g.num_aqs_per_groups *
> > +			d->acc100_conf.q_dl_4g.num_qgroups +
> > +			d->acc100_conf.q_ul_4g.num_aqs_per_groups *
> > +			d->acc100_conf.q_ul_4g.num_qgroups;
> > +	dev_info->queue_size_lim = ACC100_MAX_QUEUE_DEPTH;
> > +	dev_info->hardware_accelerated = true;
> > +	dev_info->max_dl_queue_priority =
> > +			d->acc100_conf.q_dl_4g.num_qgroups - 1;
> > +	dev_info->max_ul_queue_priority =
> > +			d->acc100_conf.q_ul_4g.num_qgroups - 1;
> > +	dev_info->default_queue_conf = default_queue_conf;
> > +	dev_info->cpu_flag_reqs = NULL;
> > +	dev_info->min_alignment = 64;
> > +	dev_info->capabilities = bbdev_capabilities;
> > +	dev_info->harq_buffer_size = d->ddr_size; }
> > +
> >  static const struct rte_bbdev_ops acc100_bbdev_ops = {
> >  	.close = acc100_dev_close,
> > +	.info_get = acc100_dev_info_get,
> >  };
> >
> >  /* ACC100 PCI PF address map */
> > diff --git a/drivers/baseband/acc100/rte_acc100_pmd.h
> > b/drivers/baseband/acc100/rte_acc100_pmd.h
> > index 6525d66..09965c8 100644
> > --- a/drivers/baseband/acc100/rte_acc100_pmd.h
> > +++ b/drivers/baseband/acc100/rte_acc100_pmd.h
> > @@ -7,6 +7,7 @@
> >
> >  #include "acc100_pf_enum.h"
> >  #include "acc100_vf_enum.h"
> > +#include "rte_acc100_cfg.h"
> >
> >  /* Helper macro for logging */
> >  #define rte_bbdev_log(level, fmt, ...) \ @@ -98,6 +99,13 @@  #define
> > ACC100_SIG_UL_4G_LAST 21
> >  #define ACC100_SIG_DL_4G      27
> >  #define ACC100_SIG_DL_4G_LAST 31
> > +#define ACC100_NUM_ACCS       5
> > +#define ACC100_ACCMAP_0       0
> > +#define ACC100_ACCMAP_1       2
> > +#define ACC100_ACCMAP_2       1
> > +#define ACC100_ACCMAP_3       3
> > +#define ACC100_ACCMAP_4       4
> > +#define ACC100_PF_VAL         2
> >
> >  /* max number of iterations to allocate memory block for all rings */
> > #define ACC100_SW_RING_MEM_ALLOC_ATTEMPTS 5 @@ -517,6 +525,8
> @@ struct
> > acc100_registry_addr {
> >  /* Private data structure for each ACC100 device */  struct
> > acc100_device {
> >  	void *mmio_base;  /**< Base address of MMIO registers (BAR0) */
> > +	uint32_t ddr_size; /* Size in kB */
> > +	struct rte_acc100_conf acc100_conf; /* ACC100 Initial configuration
> > +*/
> >  	bool pf_device; /**< True if this is a PF ACC100 device */
> >  	bool configured; /**< True if this ACC100 device is configured */
> > };