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

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

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Tuesday, May 31, 2022 1:00 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
> 
> 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.

That function is not intended to be only used for debug purpose but more generally for other handling
when the code would differ from ACC100 vs ACC101, includes future changes targeting next release. 

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

This is due to difference in HW implementation in ACC100 and ACC101, ACC101 HW interface
would not require validation in driver of the input API. 

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