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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

Hi Nicolas,

On 12/6/22 01:18, Chautru, Nicolas wrote:
> 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?

Both options would be fine to me as long as it is fixed for all the
similar functions.

Thanks,
Maxime

> 
> 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
>