Re: [PATCH v1 13/13] test/bbdev: remove iteration count check

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

Hi Nicolas,

On 2/9/23 17:59, Chautru, Nicolas wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Thursday, February 9, 2023 1:11 AM
>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com; Rix, Tom <trix@redhat.com>
>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
>> <qi.z.zhang@intel.com>
>> Subject: Re: [PATCH v1 13/13] test/bbdev: remove iteration count check
>>
>>
>>
>> On 2/8/23 21:38, Vargas, Hernan wrote:
>>> Hi Maxime,
>>>
>>> We would like to keep the same signature for validate_dec_op because there
>> are functions such as latency_test_dec that have vector_mask on their
>> signatures and they pass it to validate_dec_op.
>>> Let me know if you'd like to discuss more.
>>
>> I think this is not a valid reason, just simplify latency_test_dec too.
> 
> The principle is that all these functions may or may not use that generic operation masks, but we still use a stable (future proof) and consistent prototype for these
> test functions.

I would agree that it would be necessary if these were callbacks, but
that's not the case.

> I believe this is valid and better practice for the test functions, but again if you really want to push back, this could be changed.

I prefer we do not bloat the code with things that could be useful in an
hypothetical future.

Thanks,
Maxime

> Thanks!!
> Nic
> 
>>
>> Thanks,
>> Maxime
>>
>>> Thanks,
>>> Hernan
>>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Tuesday, January 31, 2023 6:36 AM
>>> To: Vargas, Hernan <hernan.vargas@intel.com>; dev@dpdk.org;
>>> gakhil@marvell.com; Rix, Tom <trix@redhat.com>
>>> Cc: Chautru, Nicolas <nicolas.chautru@intel.com>; Zhang, Qi Z
>>> <qi.z.zhang@intel.com>
>>> Subject: Re: [PATCH v1 13/13] test/bbdev: remove iteration count check
>>>
>>>
>>>
>>> On 1/17/23 17:50, Hernan Vargas wrote:
>>>> To make the test compatible with devices that do not support early
>>>> termination, the iteration count assert can be removed.
>>>>
>>>> Signed-off-by: Hernan Vargas <hernan.vargas@intel.com>
>>>> ---
>>>>     app/test-bbdev/test_bbdev_perf.c | 6 +-----
>>>>     1 file changed, 1 insertion(+), 5 deletions(-)
>>>>
>>>> diff --git a/app/test-bbdev/test_bbdev_perf.c
>>>> b/app/test-bbdev/test_bbdev_perf.c
>>>> index 81bf2c8b60..c68d79cf29 100644
>>>> --- a/app/test-bbdev/test_bbdev_perf.c
>>>> +++ b/app/test-bbdev/test_bbdev_perf.c
>>>> @@ -2290,6 +2290,7 @@ static int
>>>>     validate_dec_op(struct rte_bbdev_dec_op **ops, const uint16_t n,
>>>>     		struct rte_bbdev_dec_op *ref_op, const int vector_mask)
>>>>     {
>>>> +	RTE_SET_USED(vector_mask);
>>>
>>> Why not just remove vector_mask if it isn't of any use instead of hiding the
>> warning?
>>>
>>>>     	unsigned int i;
>>>>     	int ret;
>>>>     	struct op_data_entries *hard_data_orig = @@ -2299,17 +2300,12
>> @@
>>>> validate_dec_op(struct rte_bbdev_dec_op **ops, const uint16_t n,
>>>>     	struct rte_bbdev_op_turbo_dec *ops_td;
>>>>     	struct rte_bbdev_op_data *hard_output;
>>>>     	struct rte_bbdev_op_data *soft_output;
>>>> -	struct rte_bbdev_op_turbo_dec *ref_td = &ref_op->turbo_dec;
>>>>
>>>>     	for (i = 0; i < n; ++i) {
>>>>     		ops_td = &ops[i]->turbo_dec;
>>>>     		hard_output = &ops_td->hard_output;
>>>>     		soft_output = &ops_td->soft_output;
>>>>
>>>> -		if (vector_mask & TEST_BBDEV_VF_EXPECTED_ITER_COUNT)
>>>> -			TEST_ASSERT(ops_td->iter_count <= ref_td-
>>> iter_count,
>>>> -					"Returned iter_count (%d) > expected
>> iter_count (%d)",
>>>> -					ops_td->iter_count, ref_td-
>>> iter_count);
>>>>     		ret = check_dec_status_and_ordering(ops[i], i, ref_op-
>>> status);
>>>>     		TEST_ASSERT_SUCCESS(ret,
>>>>     				"Checking status and ordering for decoder
>> failed");
>>>
>>> Maxime
>>>
>