On 10/4/23 23:28, Chautru, Nicolas wrote:
Hi Maxime,-----Original Message----- From: Maxime Coquelin <maxime.coque...@redhat.com> Sent: Wednesday, October 4, 2023 12:36 AM To: Chautru, Nicolas <nicolas.chau...@intel.com>; dev@dpdk.org Cc: hemant.agra...@nxp.com; david.march...@redhat.com; Vargas, Hernan <hernan.var...@intel.com> Subject: Re: [PATCH v3 06/12] baseband/acc: refactor to allow unified driver extension On 10/3/23 20:54, Chautru, Nicolas wrote:Hi Maxime,-----Original Message----- From: Maxime Coquelin <maxime.coque...@redhat.com> Sent: Tuesday, October 3, 2023 6:15 AM To: Chautru, Nicolas <nicolas.chau...@intel.com>; dev@dpdk.org Cc: hemant.agra...@nxp.com; david.march...@redhat.com; Vargas,Hernan<hernan.var...@intel.com> Subject: Re: [PATCH v3 06/12] baseband/acc: refactor to allow unified driver extension Thanks for doing the split, that will ease review. On 9/29/23 18:35, Nicolas Chautru wrote:Adding a few functions and common code prior to extending the VRB driver. Signed-off-by: Nicolas Chautru <nicolas.chau...@intel.com> --- drivers/baseband/acc/acc_common.h | 164+++++++++++++++++++++++---drivers/baseband/acc/rte_acc100_pmd.c | 4 +- drivers/baseband/acc/rte_vrb_pmd.c | 62 +++++----- 3 files changed, 184 insertions(+), 46 deletions(-) diff --git a/drivers/baseband/acc/acc_common.h b/drivers/baseband/acc/acc_common.h index 788abf1a3c..89893eae43 100644 --- a/drivers/baseband/acc/acc_common.h +++ b/drivers/baseband/acc/acc_common.h @@ -18,6 +18,7 @@ #define ACC_DMA_BLKID_OUT_HARQ 3 #define ACC_DMA_BLKID_IN_HARQ 3 #define ACC_DMA_BLKID_IN_MLD_R 3 +#define ACC_DMA_BLKID_DEWIN_IN 3 /* Values used in filling in decode FCWs */ #define ACC_FCW_TD_VER 1 @@ -103,6 +104,9 @@ #define ACC_MAX_NUM_QGRPS 32 #define ACC_RING_SIZE_GRANULARITY 64 #define ACC_MAX_FCW_SIZE 128 +#define ACC_IQ_SIZE 4 + +#define ACC_FCW_FFT_BLEN_3 28 /* Constants from K0 computation from 3GPP 38.212 Table 5.4.2.1-2 */ #define ACC_N_ZC_1 66 /* N = 66 Zc for BG 1 */ @@ -132,6 +136,17 @@ #define ACC_LIM_21 14 /* 0.21 */ #define ACC_LIM_31 20 /* 0.31 */ #define ACC_MAX_E (128 * 1024 - 2) +#define ACC_MAX_CS 12 + +#define ACC100_VARIANT 0 +#define VRB1_VARIANT 2 +#define VRB2_VARIANT 3 + +/* Queue Index Hierarchy */ +#define VRB1_GRP_ID_SHIFT 10 +#define VRB1_VF_ID_SHIFT 4 +#define VRB2_GRP_ID_SHIFT 12 +#define VRB2_VF_ID_SHIFT 6 /* Helper macro for logging */ #define rte_acc_log(level, fmt, ...) \ @@ -332,6 +347,37 @@ struct __rte_packed acc_fcw_fft { res:19; }; +/* FFT Frame Control Word. */ +struct __rte_packed acc_fcw_fft_3 { + uint32_t in_frame_size:16, + leading_pad_size:16; + uint32_t out_frame_size:16, + leading_depad_size:16; + uint32_t cs_window_sel; + uint32_t cs_window_sel2:16, + cs_enable_bmap:16; + uint32_t num_antennas:8, + idft_size:8, + dft_size:8, + cs_offset:8; + uint32_t idft_shift:8, + dft_shift:8, + cs_multiplier:16; + uint32_t bypass:2, + fp16_in:1, + fp16_out:1, + exp_adj:4, + power_shift:4, + power_en:1, + enable_dewin:1, + freq_resample_mode:2, + depad_output_size:16; + uint16_t cs_theta_0[ACC_MAX_CS]; + uint32_t cs_theta_d[ACC_MAX_CS]; + int8_t cs_time_offset[ACC_MAX_CS]; }; + + /* MLD-TS Frame Control Word */ struct __rte_packed acc_fcw_mldts { uint32_t fcw_version:4, @@ -473,14 +519,14 @@ union acc_info_ring_data { uint16_t valid: 1; }; struct { - uint32_t aq_id_3: 6; - uint32_t qg_id_3: 5; - uint32_t vf_id_3: 6; - uint32_t int_nb_3: 6; - uint32_t msi_0_3: 1; - uint32_t vf2pf_3: 6; - uint32_t loop_3: 1; - uint32_t valid_3: 1; + uint32_t aq_id_vrb2: 6; + uint32_t qg_id_vrb2: 5; + uint32_t vf_id_vrb2: 6; + uint32_t int_nb_vrb2: 6; + uint32_t msi_0_vrb2: 1; + uint32_t vf2pf_vrb2: 6; + uint32_t loop_vrb2: 1; + uint32_t valid_vrb2: 1; }; } __rte_packed; @@ -761,22 +807,105 @@ alloc_sw_rings_min_mem(struct rte_bbdev*dev,struct acc_device *d,free_base_addresses(base_addrs, i); } +/* Wrapper to provide VF index from ring data. */ static inline +uint16_t vf_from_ring(const union acc_info_ring_data ring_data, +uint16_t device_variant) {curly braces on a new line.Thanks.+ if (device_variant == VRB2_VARIANT) + return ring_data.vf_id_vrb2; + else + return ring_data.vf_id; +} + +/* Wrapper to provide QG index from ring data. */ static inline +uint16_t qg_from_ring(const union acc_info_ring_data ring_data, +uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return ring_data.qg_id_vrb2; + else + return ring_data.qg_id; +} + +/* Wrapper to provide AQ index from ring data. */ static inline +uint16_t aq_from_ring(const union acc_info_ring_data ring_data, +uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return ring_data.aq_id_vrb2; + else + return ring_data.aq_id; +} + +/* Wrapper to provide int index from ring data. */ static inline +uint16_t int_from_ring(const union acc_info_ring_data ring_data, +uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return ring_data.int_nb_vrb2; + else + return ring_data.int_nb; +} + +/* Wrapper to provide queue index from group and aq index. */ +static inline int queue_index(uint16_t group_idx, uint16_t aq_idx, +uint16_t +device_variant) { + if (device_variant == VRB2_VARIANT) + return (group_idx << VRB2_GRP_ID_SHIFT) + aq_idx; + else + return (group_idx << VRB1_GRP_ID_SHIFT) + aq_idx; } + +/* Wrapper to provide queue group from queue index. */ static +inline int qg_from_q(uint32_t q_idx, uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return (q_idx >> VRB2_GRP_ID_SHIFT) & 0x1F; + else + return (q_idx >> VRB1_GRP_ID_SHIFT) & 0xF; } + +/* Wrapper to provide vf from queue index. */ static inline int32_t +vf_from_q(uint32_t q_idx, uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return (q_idx >> VRB2_VF_ID_SHIFT) & 0x3F; + else + return (q_idx >> VRB1_VF_ID_SHIFT) & 0x3F; } + +/* Wrapper to provide aq index from queue index. */ static inline +int32_t aq_from_q(uint32_t q_idx, uint16_t device_variant) { + if (device_variant == VRB2_VARIANT) + return q_idx & 0x3F; + else + return q_idx & 0xF; +} + +/* Wrapper to set VF index in ring data. */ static inline int32_t +set_vf_in_ring(volatile union acc_info_ring_data *ring_data, + uint16_t device_variant, uint16_t value) { + if (device_variant == VRB2_VARIANT) + return ring_data->vf_id_vrb2 = value; + else + return ring_data->vf_id = value; +} + /* * Find queue_id of a device queue based on details from the Info Ring. * If a queue isn't found UINT16_MAX is returned. */ static inline uint16_t get_queue_id_from_ring_info(struct rte_bbdev_data *data, - const union acc_info_ring_data ring_data) + const union acc_info_ring_data ring_data, uint16_tdevice_variant) As I suggested on v2: get_queue_id_from_ring_info(struct rte_bbdev_data *data, const union acc_info_ring_data ring_data) { struct acc_device *d = data->dev_private; ... if (acc_q != NULL && acc_q->aq_id == aq_from_ring(d, ring_data) && ... } with /* Wrapper to provide AQ index from ring data. */ tatic inline uint16_t aq_from_ring(struct acc_device *d, const union acc_info_ring_data ring_data) { if (d->device_variant == VRB2_VARIANT) return ring_data.aq_id_vrb2; else return ring_data.aq_id; }I will change the get_queue_id_from_ring_info() to have a smaller prototype but I don’t plan on changing the other new underlying funs to use dev instead of the variant in prototype, I don’t see a reason to as these only need this very member.IMHO, reason is it cost nothing and is more future proof.Thanks, on that very case I believe it the prototype is cleaner with the device variant. I don’t see future proof concern.Also, my initial idea was to have an intermediate representation, like: struct acc_queue_info { // Not sure about the name uint16_t vf_id; uint8_t qgrp_id; uint16_t aq_id; }; Then we have a single callback for each variant static void vrb1_ring_data_to_queue_info(const union acc_info_ring_data ring_data, struct acc_queue_info *queue_info) { queue_info->vf_id = ring_data.vf_id; queue_info->qgrp_id = ... } static void vrb2_ring_data_to_queue_info(const union acc_info_ring_data ring_data, struct acc_queue_info *queue_info) { } The acc_queue_info struct can also be used in struct acc_queue, so we use same format everywhere. I think it will be less verbose, and quicker to add new variants without risking to miss adding "else if (d->device_variant == VRBx_VARIANT)" anywhere. What do you think?I think both would work. The intermediate structure may be a bit artificial, and it would have different members when getting info from queue or ring (ie. the int index). Also there is no reciprocal function, ie we set only the VF into the ring. And there is a location where we only need one of information not all of the other members. Again both are okay to me without super strong preference, so for now I would suggest to keep as is.
Ok, but please pass dev and not variant directly in the helpers.
{ uint16_t queue_id; + struct acc_queue *acc_q; for (queue_id = 0; queue_id < data->num_queues; ++queue_id) { - struct acc_queue *acc_q = - data->queues[queue_id].queue_private; - if (acc_q != NULL && acc_q->aq_id == ring_data.aq_id && - acc_q->qgrp_id == ring_data.qg_id && - acc_q->vf_id == ring_data.vf_id) + acc_q = data->queues[queue_id].queue_private; + + if (acc_q != NULL && acc_q->aq_id ==aq_from_ring(ring_data, device_variant) &&+ acc_q->qgrp_id == qg_from_ring(ring_data,device_variant) &&+ acc_q->vf_id == vf_from_ring(ring_data,device_variant))return queue_id; } @@ -1438,4 +1567,11 @@ get_num_cbs_in_tb_ldpc_enc(structrte_bbdev_op_ldpc_enc *ldpc_enc)return cbs_in_tb; } +static inline void +acc_reg_fast_write(struct acc_device *d, uint32_t offset, uint32_t +value) { + void *reg_addr = RTE_PTR_ADD(d->mmio_base, offset); + mmio_write(reg_addr, value); +} + #endif /* _ACC_COMMON_H_ */ diff --git a/drivers/baseband/acc/rte_acc100_pmd.c b/drivers/baseband/acc/rte_acc100_pmd.c index 5362d39c30..7f8d05b5a9 100644 --- a/drivers/baseband/acc/rte_acc100_pmd.c +++ b/drivers/baseband/acc/rte_acc100_pmd.c @@ -294,7 +294,7 @@ acc100_pf_interrupt_handler(struct rte_bbdev*dev)case ACC100_PF_INT_DMA_UL5G_DESC_IRQ: case ACC100_PF_INT_DMA_DL5G_DESC_IRQ: deq_intr_det.queue_id =get_queue_id_from_ring_info(- dev->data, *ring_data); + dev->data, *ring_data, acc100_dev- device_variant); if (deq_intr_det.queue_id == UINT16_MAX) { rte_bbdev_log(ERR, "Couldn't find queue: aq_id:%u, qg_id: %u, vf_id: %u", @@-348,7 +348,7 @@ acc100_vf_interrupt_handler(struct rte_bbdev *dev) */ ring_data->vf_id = 0; deq_intr_det.queue_id =get_queue_id_from_ring_info(- dev->data, *ring_data); + dev->data, *ring_data, acc100_dev- device_variant); if (deq_intr_det.queue_id == UINT16_MAX) { rte_bbdev_log(ERR, "Couldn't find queue: aq_id:%u, qg_id: %u", diff --gita/drivers/baseband/acc/rte_vrb_pmd.c b/drivers/baseband/acc/rte_vrb_pmd.c index a1de012b40..c89c26c59a 100644 --- a/drivers/baseband/acc/rte_vrb_pmd.c +++ b/drivers/baseband/acc/rte_vrb_pmd.c @@ -341,17 +341,18 @@ static inline void vrb_check_ir(struct acc_device *acc_dev) { volatile union acc_info_ring_data *ring_data; - uint16_t info_ring_head = acc_dev->info_ring_head; + uint16_t info_ring_head = acc_dev->info_ring_head, int_nb; if (unlikely(acc_dev->info_ring == NULL)) return; ring_data = acc_dev->info_ring + (acc_dev->info_ring_head & ACC_INFO_RING_MASK); while (ring_data->valid) { - if ((ring_data->int_nb < ACC_PF_INT_DMA_DL_DESC_IRQ) || ( - ring_data->int_nb >ACC_PF_INT_DMA_MLD_DESC_IRQ)) {+ int_nb = int_from_ring(*ring_data, acc_dev->device_variant); + if ((int_nb < ACC_PF_INT_DMA_DL_DESC_IRQ) || ( + int_nb > ACC_PF_INT_DMA_MLD_DESC_IRQ)){rte_bbdev_log(WARNING, "InfoRing: ITR:%dInfo:0x%x",- ring_data->int_nb, ring_data- detailed_info); + int_nb, ring_data->detailed_info); /* Initialize Info Ring entry and move forward. */ ring_data->val = 0; } @@ -368,16 +369,21 @@ vrb_dev_interrupt_handler(void *cb_arg) struct acc_device *acc_dev = dev->data->dev_private; volatile union acc_info_ring_data *ring_data; struct acc_deq_intr_details deq_intr_det; + uint16_t vf_id, aq_id, qg_id, int_nb; ring_data = acc_dev->info_ring + (acc_dev->info_ring_head & ACC_INFO_RING_MASK); while (ring_data->valid) { + vf_id = vf_from_ring(*ring_data, acc_dev->device_variant); + aq_id = aq_from_ring(*ring_data, acc_dev->device_variant); + qg_id = qg_from_ring(*ring_data, acc_dev->device_variant); + int_nb = int_from_ring(*ring_data, acc_dev->device_variant); if (acc_dev->pf_device) { rte_bbdev_log_debug( - "VRB1 PF Interrupt received, Info Ringdata: 0x%x -> %d",- ring_data->val, ring_data->int_nb); + "PF Interrupt received, Info Ring data:0x%x -> %d",+ ring_data->val, int_nb); - switch (ring_data->int_nb) { + switch (int_nb) { case ACC_PF_INT_DMA_DL_DESC_IRQ: case ACC_PF_INT_DMA_UL_DESC_IRQ: case ACC_PF_INT_DMA_FFT_DESC_IRQ: @@ -385,13 +391,11 @@ vrb_dev_interrupt_handler(void *cb_arg) case ACC_PF_INT_DMA_DL5G_DESC_IRQ: case ACC_PF_INT_DMA_MLD_DESC_IRQ: deq_intr_det.queue_id =get_queue_id_from_ring_info(- dev->data, *ring_data); + dev->data, *ring_data,acc_dev->device_variant);if (deq_intr_det.queue_id == UINT16_MAX) { rte_bbdev_log(ERR, "Couldn't find queue:aq_id: %u, qg_id: %u, vf_id: %u",- ring_data->aq_id, - ring_data->qg_id, - ring_data->vf_id); + aq_id, qg_id, vf_id); return; } rte_bbdev_pmd_callback_process(dev, @@ -403,9 +407,9 @@ vrb_dev_interrupt_handler(void *cb_arg) } } else { rte_bbdev_log_debug( - "VRB1 VF Interrupt received, Info Ringdata: 0x%x\n",+ "VRB VF Interrupt received, Info Ringdata: 0x%x\n",ring_data->val); - switch (ring_data->int_nb) { + switch (int_nb) { case ACC_VF_INT_DMA_DL_DESC_IRQ: case ACC_VF_INT_DMA_UL_DESC_IRQ: case ACC_VF_INT_DMA_FFT_DESC_IRQ: @@ -413,14 +417,13 @@ vrb_dev_interrupt_handler(void *cb_arg) case ACC_VF_INT_DMA_DL5G_DESC_IRQ: case ACC_VF_INT_DMA_MLD_DESC_IRQ: /* VFs are not aware of their vf_id - it's set to0. */- ring_data->vf_id = 0; + set_vf_in_ring(ring_data, acc_dev- device_variant, 0); deq_intr_det.queue_id =get_queue_id_from_ring_info(- dev->data, *ring_data); + dev->data, *ring_data,acc_dev->device_variant);if (deq_intr_det.queue_id == UINT16_MAX) { rte_bbdev_log(ERR, "Couldn't find queue:aq_id: %u, qg_id: %u",- ring_data->aq_id, - ring_data->qg_id); + aq_id, qg_id); return; } rte_bbdev_pmd_callback_process(dev, @@ -435,8 +438,7 @@ vrb_dev_interrupt_handler(void *cb_arg) /* Initialize Info Ring entry and move forward. */ ring_data->val = 0; ++acc_dev->info_ring_head; - ring_data = acc_dev->info_ring + - (acc_dev->info_ring_head &ACC_INFO_RING_MASK);+ ring_data = acc_dev->info_ring + (acc_dev->info_ring_head & +ACC_INFO_RING_MASK); } } @@ -556,8 +558,7 @@ vrb_setup_queues(struct rte_bbdev *dev, uint16_t num_queues, int socket_id) /* Configure tail pointer for use when SDONE enabled. */ if (d->tail_ptrs == NULL) - d->tail_ptrs = rte_zmalloc_socket( - dev->device->driver->name, + d->tail_ptrs = rte_zmalloc_socket(dev->device->driver->name, VRB_MAX_QGRPS * VRB_MAX_AQS *sizeof(uint32_t),RTE_CACHE_LINE_SIZE, socket_id); if (d->tail_ptrs == NULL) { @@ -783,7 +784,7 @@ vrb_find_free_queue_idx(struct rte_bbdev *dev, /* Mark the Queue as assigned. */ d->q_assigned_bit_map[group_idx] |= (1ULL <<aq_idx);/* Report the AQ Index. */ - return (group_idx << VRB1_GRP_ID_SHIFT) + aq_idx; + return queue_index(group_idx, aq_idx, d- device_variant); } } rte_bbdev_log(INFO, "Failed to find free queue on %s, priority %u", @@ -922,9 +923,10 @@ vrb_queue_setup(struct rte_bbdev *dev, uint16_tqueue_id,} } - q->qgrp_id = (q_idx >> VRB1_GRP_ID_SHIFT) & 0xF; - q->vf_id = (q_idx >> VRB1_VF_ID_SHIFT) & 0x3F; - q->aq_id = q_idx & 0xF; + q->qgrp_id = qg_from_q(q_idx, d->device_variant); + q->vf_id = vf_from_q(q_idx, d->device_variant); + q->aq_id = aq_from_q(q_idx, d->device_variant); + q->aq_depth = 0; if (conf->op_type == RTE_BBDEV_OP_TURBO_DEC) q->aq_depth = (1 << d->acc_conf.q_ul_4g.aq_depth_log2); @@ -1311,7 +1313,7 @@ vrb_fcw_td_fill(const struct rte_bbdev_dec_op*op, struct acc_fcw_td *fcw)fcw->bypass_teq = 0; } - fcw->code_block_mode = 1; /* FIXME */ + fcw->code_block_mode = 1;Could you remind me what was the issue?Historically there was the intention to use a difference format option in thefcw to help with the TB mode but that is not considered anymore. Ok.fcw->turbo_crc_type = check_bit(op->turbo_dec.op_flags, RTE_BBDEV_TURBO_CRC_TYPE_24B); @@ -1471,8 +1473,8 @@ vrb_dma_desc_td_fill(struct rte_bbdev_dec_op*op,if (op->turbo_dec.code_block_mode ==RTE_BBDEV_TRANSPORT_BLOCK) {k = op->turbo_dec.tb_params.k_pos; e = (r < op->turbo_dec.tb_params.cab) - ? op->turbo_dec.tb_params.ea - : op->turbo_dec.tb_params.eb; + ? op->turbo_dec.tb_params.ea + : op->turbo_dec.tb_params.eb; } else { k = op->turbo_dec.cb_params.k; e = op->turbo_dec.cb_params.e; @@ -1726,7 +1728,7 @@ vrb_dma_desc_ld_update(structrte_bbdev_dec_op *op,desc->op_addr = op; } -/* Enqueue one encode operations for device in CB mode */ +/* Enqueue one encode operations for device in CB mode. */ static inline int enqueue_enc_one_op_cb(struct acc_queue *q, struct rte_bbdev_enc_op*op,uint16_t total_enqueued_cbs) @@ -2263,7 +2265,7 @@ vrb_enqueue_ldpc_dec_one_op_tb(structacc_queue *q, struct rte_bbdev_dec_op *op,return current_enqueued_cbs; } -/* Enqueue one decode operations for device in TB mode */ +/* Enqueue one decode operations for device in TB mode. */ static inline int enqueue_dec_one_op_tb(struct acc_queue *q, struct rte_bbdev_dec_op*op,uint16_t total_enqueued_cbs, uint8_t cbs_in_tb)
