RE: [PATCH v3 01/13] baseband/acc100: refactor to segregate common code

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

Hi Maxime, 

> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, September 21, 2022 12:13 AM
> To: Chautru, Nicolas <nicolas.chautru@intel.com>; dev@dpdk.org;
> thomas@monjalon.net
> Cc: trix@redhat.com; mdr@ashroe.eu; Richardson, Bruce
> <bruce.richardson@intel.com>; hemant.agrawal@nxp.com;
> david.marchand@redhat.com; stephen@networkplumber.org; Vargas,
> Hernan <hernan.vargas@intel.com>
> Subject: Re: [PATCH v3 01/13] baseband/acc100: refactor to segregate
> common code
> 
> 
> 
> On 9/16/22 03:34, Nic Chautru wrote:
> > Refactoring all shareable common code to be used by future PMD
> > (including ACC200 in  this patchset as well as taking into account
> > following PMDs in roadmap) by gathering such structures or inline
> methods.
> > Cleaning up the enum files to remove un-used registers definitions.
> > No functionality change.
> >
> > Signed-off-by: Nic Chautru <nicolas.chautru@intel.com>
> 
> You usually sign-off with Nicolas, but some of the patches of this series are
> with Nic.
> 

OK

> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >   app/test-bbdev/test_bbdev_perf.c             |    6 +-
> >   drivers/baseband/acc100/acc100_pf_enum.h     |  939 -------------
> >   drivers/baseband/acc100/acc100_pmd.h         |  449 +------
> >   drivers/baseband/acc100/acc101_pmd.h         |   10 -
> >   drivers/baseband/acc100/acc_common.h         | 1388
> +++++++++++++++++++
> >   drivers/baseband/acc100/rte_acc100_cfg.h     |   70 +-
> >   drivers/baseband/acc100/rte_acc100_pmd.c     | 1856 ++++++++-----------
> -------
> >   drivers/baseband/acc100/rte_acc_common_cfg.h |  101 ++
> >   8 files changed, 2069 insertions(+), 2750 deletions(-)
> >   create mode 100644 drivers/baseband/acc100/acc_common.h
> >   create mode 100644 drivers/baseband/acc100/rte_acc_common_cfg.h
> 
> Overall, I think the patch should be split.
> For example:
> - One patch for rings rework as it introduces new helpers (and this patch
> should make use of them in ACC100).

Actually that ring rework is on the other serie specific to ACC100. Still I can remove the function from acc_common.h which are not used yet, and only add them for ACC200. 

> - One patch for the structs renaming and move to common file.

This is 90% of the content of that commit

> - One patch for registers

Can do. 

> - ...

What else you have in mind?

> 
> It will make easier for the reviewer to identify discrepancies, and also will
> help to identify regressions when using git bisect.
> 
> I have not done a full review of this patch, but something caught my eye wrt
> to available entries calculation in rings:
> 
> > diff --git a/drivers/baseband/acc100/acc100_pf_enum.h
> > b/drivers/baseband/acc100/acc100_pf_enum.h
> > index 2fba667..f4e5002 100644
> > --- a/drivers/baseband/acc100/acc100_pf_enum.h
> > +++ b/drivers/baseband/acc100/acc100_pf_enum.h
> > @@ -14,32 +14,6 @@
> 
> ...
> 
> > +
> > +/* Number of available descriptor in ring to enqueue */ static inline
> > +uint32_t acc_ring_avail_enq(struct acc_queue *q) {
> > +	return (q->sw_ring_depth - 1 + q->sw_ring_tail - q->sw_ring_head) %
> > +q->sw_ring_depth; }
> > +
> > +/* Number of available descriptor in ring to dequeue */ static inline
> > +uint32_t acc_ring_avail_deq(struct acc_queue *q) {
> > +	return (q->sw_ring_depth + q->sw_ring_head - q->sw_ring_tail) %
> > +q->sw_ring_depth; }
> 
> It is surprising to me that the number of available descriptors calculation for
> enqueue and dequeue are différent. Could you please explain why there a
> off-by-one delta between enc and dec?
> 
> If we look at other avail calculations in ACC100 enqueue, we get this:
> int32_t avail = q->sw_ring_depth + q->sw_ring_tail - q->sw_ring_head; It
> looks like there is indeed a off-by-one delta even for different avail enqueue
> calculation.
> 
> Also, these new helpers are introduced but are not used in the patch.

I can remove and only add when ACC200 uses them.
With regards to the code itself they are different number on purpose and not just off by one, there are the quasi complement of each other (note head - tail vs tail -head).
There was a bug indeed in the ACC100 implementation. But again this not addressed by this serie.

Thanks
Nic