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

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

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.

> 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).
- One patch for the structs renaming and move to common file.
- One patch for registers
- ...

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.