On 9/29/20 5:25 PM, Chautru, Nicolas wrote:
> Hi Tom,
>
>> From: Tom Rix <t...@redhat.com>
>> On 9/28/20 5:29 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.chau...@intel.com>
>>> Acked-by: Liu Tianjiao <tianjiao....@intel.com>
>>> ---
>>> app/test-bbdev/meson.build | 3 +
>>> drivers/baseband/acc100/rte_acc100_cfg.h | 96 +++++++++++++
>>> drivers/baseband/acc100/rte_acc100_pmd.c | 225
>> +++++++++++++++++++++++++++++++
>>> drivers/baseband/acc100/rte_acc100_pmd.h | 3 +
>>> 4 files changed, 327 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..73bbe36
>>> --- /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
>> When will this experimental tag be removed ?
> I have pushed a patch to remove it. But the feedback from some of the
> community was to wait a bit more until next year.
ok, i am late to the party. i'll chip in when this issue comes around again.
>
>>> + */
>>> +
>>> +#include <stdint.h>
>>> +#include <stdbool.h>
>>> +
>>> +#ifdef __cplusplus
>>> +extern "C" {
>>> +#endif
>>> +/**< Number of Virtual Functions ACC100 supports */ #define
>>> +RTE_ACC100_NUM_VFS 16
>> This is already defined with ACC100_NUM_VFS
> Thanks.
>
>>> +
>>> +/**
>>> + * 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_q_topology_t {
>>> + /** 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_arbitration_t {
>>> + /** 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 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_q_topology_t q_ul_4g;
>>> + struct rte_q_topology_t q_dl_4g;
>>> + struct rte_q_topology_t q_ul_5g;
>>> + struct rte_q_topology_t q_dl_5g;
>>> + /** Arbitration configuration for each operation type */
>>> + struct rte_arbitration_t arb_ul_4g[RTE_ACC100_NUM_VFS];
>>> + struct rte_arbitration_t arb_dl_4g[RTE_ACC100_NUM_VFS];
>>> + struct rte_arbitration_t arb_ul_5g[RTE_ACC100_NUM_VFS];
>>> + struct rte_arbitration_t 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..7807a30 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -26,6 +26,184 @@
>>> 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);
>> Could you add *QmrIngressAq to the acc100_registry_addr and skip the if
>> (pf_device) check ?
> I am not convinced. That acc100_registry_addr is not kept with the driver, you
> still need to check pf_device flag to now what registers are to be used.
ok
The values of pf and vf regs are tantalizingly close
my first inclination is to abstract all of them.
>
>
>>> +}
>>> +
>>> +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_q_topology_t **qtop, int acc_enum,
>>> + struct acc100_conf *acc100_conf)
>>> +{
>>> + struct rte_q_topology_t *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");
>> Use in fetch_acc100_config does not check for NULL.
> Yes because it can't be null. This function is called explictly for supported
> values.
ok
>>> + break;
>>> + }
>>> + *qtop = p_qtop;
>>> +}
>>> +
>>> +static void
>>> +initQTop(struct 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 acc100_conf *acc100_conf,
>>> + struct acc100_device *d) {
>>> + uint32_t reg;
>>> + struct rte_q_topology_t *q_top = NULL;
>>> + qtopFromAcc(&q_top, acc, acc100_conf);
>>> + if (unlikely(q_top == NULL))
>>> + return;
>> as above, this error is not handled by caller fetch_acc100_config
> It cannot really fail for fetch_acc100_config. If you insist I can add.
An alternative to handing is to print a debug message.
could do this generally for the other checks i commented on.
>>> + 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 & 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 & 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 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;
>> Warn ?
> No this can genuinely happen on a regular basis, just no need to fetch it all
> again.
ok
Tom
>
>>> +
>>> + /* 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_q_topology_t *q_top = NULL;
>>> + int qman_func_id[5] = {0, 2, 1, 3, 4};
>> Do these magic numbers need #defines ?
> ok.
>
>>> + 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 & QUEUE_ENABLE) {
>>> + acc = qman_func_id[(reg >> (qg * 4)) & 0x7];
>> 0x7 and [5], this could overflow.
> ok thanks, I can add exception handling. Not clear to me right now why it did
> not trigger tool warning.
>
>>> + 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 == 2) ? 1 : 0;
>> 2 is a magic number, consider a #define
>>
> ok
>
>> Tom
>>
> Thanks
> Nic
>
>>> + }
>>> +
>>> + 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
>> +211,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 = 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 = 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 cd77570..662e2c8 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, ...) \ @@ -520,6 +521,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 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 */
>>> };
