RE: [PATCH v1 1/2] baseband/acc: FFT support in VRB2 PRQ device

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

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Friday, October 18, 2024 12:54 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org
> Cc: hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>
> Subject: Re: [PATCH v1 1/2] baseband/acc: FFT support in VRB2 PRQ device
> 
> Hi Nicolas,
> 
> On 10/15/24 00:30, Nicolas Chautru wrote:
> > Supporting recent change in the device to extend FFT capability
> > processing in latest stepping.
> > Also including cosmetic change to VRB2 register definition.
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   drivers/baseband/acc/acc_common.h   |  2 +-
> >   drivers/baseband/acc/rte_vrb_pmd.c  | 30 +++++++++++++++++++++++++-
> ---
> >   drivers/baseband/acc/vrb2_vf_enum.h |  4 ++--
> >   3 files changed, 29 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/baseband/acc/acc_common.h
> > b/drivers/baseband/acc/acc_common.h
> > index 0c249d5b93..4c60b7896b 100644
> > --- a/drivers/baseband/acc/acc_common.h
> > +++ b/drivers/baseband/acc/acc_common.h
> > @@ -106,7 +106,7 @@
> >   #define ACC_MAX_FCW_SIZE              128
> >   #define ACC_IQ_SIZE                    4
> >
> > -#define ACC_FCW_FFT_BLEN_3             28
> > +#define ACC_FCW_FFT_BLEN_VRB2         128
> >
> >   /* 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 */ diff --git
> > a/drivers/baseband/acc/rte_vrb_pmd.c
> > b/drivers/baseband/acc/rte_vrb_pmd.c
> > index 0455320c2a..5eb3e8dd48 100644
> > --- a/drivers/baseband/acc/rte_vrb_pmd.c
> > +++ b/drivers/baseband/acc/rte_vrb_pmd.c
> > @@ -1006,7 +1006,7 @@ vrb_queue_setup(struct rte_bbdev *dev,
> uint16_t queue_id,
> >   	case RTE_BBDEV_OP_FFT:
> >   		fcw_len = ACC_FCW_FFT_BLEN;
> >   		if (q->d->device_variant == VRB2_VARIANT)
> > -			fcw_len = ACC_FCW_FFT_BLEN_3;
> > +			fcw_len = ACC_FCW_FFT_BLEN_VRB2;
> >   		break;
> >   	case RTE_BBDEV_OP_MLDTS:
> >   		fcw_len = ACC_FCW_MLDTS_BLEN;
> > @@ -1402,7 +1402,11 @@ vrb_dev_info_get(struct rte_bbdev *dev, struct
> rte_bbdev_driver_info *dev_info)
> >   						RTE_BBDEV_FFT_FP16_INPUT
> |
> >
> 	RTE_BBDEV_FFT_FP16_OUTPUT |
> >
> 	RTE_BBDEV_FFT_POWER_MEAS |
> > -
> 	RTE_BBDEV_FFT_WINDOWING_BYPASS,
> > +
> 	RTE_BBDEV_FFT_WINDOWING_BYPASS |
> > +
> 	RTE_BBDEV_FFT_TIMING_OFFSET_PER_CS |
> > +
> 	RTE_BBDEV_FFT_TIMING_ERROR |
> > +
> 	RTE_BBDEV_FFT_DEWINDOWING |
> > +
> 	RTE_BBDEV_FFT_FREQ_RESAMPLING,
> >   				.num_buffers_src = 1,
> >   				.num_buffers_dst = 1,
> >   				.fft_windows_num = ACC_MAX_FFT_WIN,
> @@ -3725,6 +3729,8 @@
> > vrb1_fcw_fft_fill(struct rte_bbdev_fft_op *op, struct acc_fcw_fft *fcw)
> >   static inline void
> >   vrb2_fcw_fft_fill(struct rte_bbdev_fft_op *op, struct acc_fcw_fft_3 *fcw)
> >   {
> > +	uint8_t cs;
> > +
> >   	fcw->in_frame_size = op->fft.input_sequence_size;
> >   	fcw->leading_pad_size = op->fft.input_leading_padding;
> >   	fcw->out_frame_size = op->fft.output_sequence_size; @@ -3760,6
> > +3766,16 @@ vrb2_fcw_fft_fill(struct rte_bbdev_fft_op *op, struct
> acc_fcw_fft_3 *fcw)
> >   		fcw->bypass = 3;
> >   	else
> >   		fcw->bypass = 0;
> > +
> > +	fcw->enable_dewin = check_bit(op->fft.op_flags,
> RTE_BBDEV_FFT_DEWINDOWING);
> > +	fcw->freq_resample_mode = op->fft.freq_resample_mode;
> > +	fcw->depad_output_size = fcw->freq_resample_mode == 0 ?
> > +			op->fft.output_sequence_size : op-
> >fft.output_depadded_size;
> > +	for (cs = 0; cs < RTE_BBDEV_MAX_CS; cs++) {
> > +		fcw->cs_theta_0[cs] = op->fft.cs_theta_0[cs];
> > +		fcw->cs_theta_d[cs] = op->fft.cs_theta_d[cs];
> > +		fcw->cs_time_offset[cs] = op->fft.time_offset[cs];
> > +	}
> >   }
> >
> >   static inline int
> > @@ -3782,8 +3798,14 @@ vrb_dma_desc_fft_fill(struct rte_bbdev_fft_op
> *op,
> >   	/* FCW already done */
> >   	acc_header_init(desc);
> >
> > -	RTE_SET_USED(win_input);
> > -	RTE_SET_USED(win_offset);
> > +	if (win_en && win_input) {
> > +		desc->data_ptrs[bd_idx].address =
> rte_pktmbuf_iova_offset(win_input, *win_offset);
> > +		desc->data_ptrs[bd_idx].blen = op->fft.output_depadded_size
> * 2;
> > +		desc->data_ptrs[bd_idx].blkid = ACC_DMA_BLKID_DEWIN_IN;
> > +		desc->data_ptrs[bd_idx].last = 0;
> > +		desc->data_ptrs[bd_idx].dma_ext = 0;
> > +		bd_idx++;
> > +	}
> >
> >   	desc->data_ptrs[bd_idx].address = rte_pktmbuf_iova_offset(input,
> *in_offset);
> >   	desc->data_ptrs[bd_idx].blen = op->fft.input_sequence_size *
> > ACC_IQ_SIZE; diff --git a/drivers/baseband/acc/vrb2_vf_enum.h
> > b/drivers/baseband/acc/vrb2_vf_enum.h
> > index 9c6e451010..1cc6986c67 100644
> > --- a/drivers/baseband/acc/vrb2_vf_enum.h
> > +++ b/drivers/baseband/acc/vrb2_vf_enum.h
> > @@ -18,8 +18,8 @@ enum {
> >   	VRB2_VfHiInfoRingIntWrEnVf       = 0x00000020,
> >   	VRB2_VfHiInfoRingPf2VfWrEnVf     = 0x00000024,
> >   	VRB2_VfHiMsixVectorMapperVf      = 0x00000060,
> > -	VRB2_VfHiDeviceStatus            = 0x00000068,
> > -	VRB2_VfHiInterruptSrc            = 0x00000070,
> > +	VRB2_VfHiDeviceStatus            = 0x00000064,
> > +	VRB2_VfHiInterruptSrc            = 0x00000068,
> 
> The offset of the registers change, is that what you describe as cosmetic
> change?
> 
> Does it have an impact on older DPDK versions? i.e. should it be backported?

Hi Maxime, These registers are currently not used in the code so it really has no impact. 
It is more for documentation purpose and in case we need to use them in the future,
basically not to carry such discrepancy with specs forward.
No actual impact and does not need to be back ported for the same reason.
Thanks
Nic

> 
> 
> Thanks,
> Maxime
> 
> >   	VRB2_VfDmaFec5GulDescBaseLoRegVf = 0x00000120,
> >   	VRB2_VfDmaFec5GulDescBaseHiRegVf = 0x00000124,
> >   	VRB2_VfDmaFec5GulRespPtrLoRegVf  = 0x00000128,