RE: [PATCH v1 2/2] test/bbdev: fix build issue with optional build flag

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

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Thursday, November 24, 2022 12:09 PM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; gakhil@marvell.com
> Cc: Vargas, Hernan <hernan.vargas@intel.com>; Vipin.Varghese@amd.com;
> Mcnamara, John <john.mcnamara@intel.com>; Ferruh.Yigit@amd.com;
> Clinton.France@amd.com
> Subject: Re: [PATCH v1 2/2] test/bbdev: fix build issue with optional build flag
> 
> Hi Nicolas,
> 
> On 11/24/22 17:06, Nicolas Chautru wrote:
> > Missing implementation for offload test with FFT.
> > Only build when the optional build flag RTE_BBDEV_OFFLOAD_COST is set.
> >
> > Fixes: 0acdb9866756 ("test/bbdev: add FFT operations cases")
> >
> > Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
> > ---
> >   app/test-bbdev/test_bbdev_perf.c | 82
> ++++++++++++++++++++++++++++++++
> >   1 file changed, 82 insertions(+)
> >
> > diff --git a/app/test-bbdev/test_bbdev_perf.c
> > b/app/test-bbdev/test_bbdev_perf.c
> > index 1859952901..b2e536b5e3 100644
> > --- a/app/test-bbdev/test_bbdev_perf.c
> > +++ b/app/test-bbdev/test_bbdev_perf.c
> > @@ -4940,6 +4940,88 @@ get_bbdev_queue_stats(uint16_t dev_id,
> uint16_t queue_id,
> >   	return 0;
> >   }
> >
> > +static int
> > +offload_latency_test_fft(struct rte_mempool *mempool, struct
> test_buffers *bufs,
> > +		struct rte_bbdev_fft_op *ref_op, uint16_t dev_id,
> > +		uint16_t queue_id, const uint16_t num_to_process,
> > +		uint16_t burst_sz, struct test_time_stats *time_st) {
> > +	int i, dequeued, ret;
> > +	struct rte_bbdev_fft_op *ops_enq[MAX_BURST],
> *ops_deq[MAX_BURST];
> > +	uint64_t enq_start_time, deq_start_time;
> > +	uint64_t enq_sw_last_time, deq_last_time;
> > +	struct rte_bbdev_stats stats;
> > +
> > +	for (i = 0, dequeued = 0; dequeued < num_to_process; ++i) {
> > +		uint16_t enq = 0, deq = 0;
> > +
> > +		if (unlikely(num_to_process - dequeued < burst_sz))
> > +			burst_sz = num_to_process - dequeued;
> > +
> > +		rte_bbdev_fft_op_alloc_bulk(mempool, ops_enq, burst_sz);
> 
> It might be safer to check for error.

We don’t check for error on the _op_alloc_bulk() for any of the other offload test functions. This could be changed through an independent patchset.
Are you okay to apply as is on the next-baseband subtree? Or you prefer a v2 with an additional commit now for all these functions?

Thanks
Nic

> 
> Given how late we are in the release, and also because it is in the test
> application, I'm fine if the fix is done in next release.
> 
> Other than that, it looks good to me:
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> 
> Thanks,
> Maxime