[PATCH v3 01/13] baseband/acc100: refactor to segregate common code
Maxime Coquelin
maxime.coquelin at redhat.com
Wed Sep 21 09:13:10 CEST 2022
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 at 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 at 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.
More information about the dev
mailing list