Re: [PATCH v5 4/5] baseband/acc100: modify validation code for ACC101

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

Hi Nicolas,

On 5/26/22 00:15, Chautru, Nicolas wrote:
> 
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Wednesday, May 25, 2022 7:33 AM
>> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
>> gakhil@marvell.com; trix@redhat.com
>> Cc: thomas@monjalon.net; Kinsella, Ray <ray.kinsella@intel.com>;
>> Richardson, Bruce <bruce.richardson@intel.com>;
>> hemant.agrawal@nxp.com; Vargas, Hernan <hernan.vargas@intel.com>;
>> david.marchand@redhat.com
>> Subject: Re: [PATCH v5 4/5] baseband/acc100: modify validation code for
>> ACC101
>>
>>
>>
>> On 5/24/22 02:08, Nicolas Chautru wrote:
>>> The validation requirement is different for the two devices.
>>>
>>> Signed-off-by: Nicolas Chautru <nicolas.chautru@intel.com>
>>> ---
>>>    drivers/baseband/acc100/rte_acc100_pmd.c | 47
>> ++++++++++++++++++++++++--------
>>>    1 file changed, 35 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> index 41475b2..e3706e0 100644
>>> --- a/drivers/baseband/acc100/rte_acc100_pmd.c
>>> +++ b/drivers/baseband/acc100/rte_acc100_pmd.c
>>> @@ -1289,6 +1289,21 @@
>>>    			RTE_BBDEV_TURBO_HALF_ITERATION_EVEN);
>>>    }
>>>
>>> +#ifdef RTE_LIBRTE_BBDEV_DEBUG
>>> +
>>> +static inline bool
>>> +is_acc100(struct acc100_queue *q)
>>> +{
>>> +	return (q->d->device_variant == ACC100_VARIANT);
>>
>> I keep insisting, but please rely on the PCI device ID, there is no need to
>> introduce a new field.
> 
> Thanks. I have a couple of concerns changing this:
> 1) the device id is not accessible from the structures currently passed by the functions which would rely on this. Ie. device_id is accessible from rte_bbdev/rte_device but not from acc100_device or acc100_queue. Would be convoluted to have to carry forward this structure when needed instead of using directly acc100_device structure.
> 2) These call would be done as part of the workload operation where performance matters, best to keep the check as trivial as possible within the PMD.

I think it is better to have a pointer to the rte_bbdev/rte_device than
introducing a new ID.

Regarding performance, is_acc100 is only defined in
RTE_LIBRTE_BBDEV_DEBUG, so it should not be that critical.

Regarding that, I have hard time to understand why we need to validate
encoder/decoder parameters in ACC100 case, but not in ACC101 one. For
example, I guess having a valid mempool pointer is required in both
ACC100 and ACC101 cases.

> Will aim for new patch in next few days based on the other refactory suggestions and unified driver.
> 
> Thanks
> Nic

Thanks,
Maxime