Re: [PATCH v1 00/10] baseband/acc200

[Tom Rix]

On 9/1/22 1:34 PM, Chautru, Nicolas wrote:
Hi Tom,

-----Original Message-----
From: Tom Rix <t...@redhat.com>
Sent: Thursday, September 1, 2022 6:49 AM
To: Chautru, Nicolas <nicolas.chau...@intel.com>; Maxime Coquelin
<maxime.coque...@redhat.com>; dev@dpdk.org; tho...@monjalon.net;
gak...@marvell.com; hemant.agra...@nxp.com; Vargas, Hernan
<hernan.var...@intel.com>
Cc: m...@ashroe.eu; Richardson, Bruce <bruce.richard...@intel.com>;
david.march...@redhat.com; step...@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200


On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
Hi Tom,

-----Original Message-----
From: Tom Rix <t...@redhat.com>
Sent: Wednesday, August 31, 2022 5:28 PM
To: Chautru, Nicolas <nicolas.chau...@intel.com>; Maxime Coquelin
<maxime.coque...@redhat.com>; dev@dpdk.org;
tho...@monjalon.net;
gak...@marvell.com; hemant.agra...@nxp.com; Vargas, Hernan
<hernan.var...@intel.com>
Cc: m...@ashroe.eu; Richardson, Bruce <bruce.richard...@intel.com>;
david.march...@redhat.com; step...@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200


On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
Hi Thomas, Tom,

-----Original Message-----
From: Tom Rix <t...@redhat.com>
Sent: Wednesday, August 31, 2022 12:26 PM
To: Chautru, Nicolas <nicolas.chau...@intel.com>; Maxime Coquelin
<maxime.coque...@redhat.com>; dev@dpdk.org;
tho...@monjalon.net;
gak...@marvell.com; hemant.agra...@nxp.com; Vargas, Hernan
<hernan.var...@intel.com>
Cc: m...@ashroe.eu; Richardson, Bruce <bruce.richard...@intel.com>;
david.march...@redhat.com; step...@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200


On 8/30/22 12:45 PM, Chautru, Nicolas wrote:
Hi Maxime,

-----Original Message-----
From: Maxime Coquelin <maxime.coque...@redhat.com>
Sent: Tuesday, August 30, 2022 12:45 AM
To: Chautru, Nicolas <nicolas.chau...@intel.com>; dev@dpdk.org;
tho...@monjalon.net; gak...@marvell.com;
hemant.agra...@nxp.com;
t...@redhat.com; Vargas, Hernan <hernan.var...@intel.com>
Cc: m...@ashroe.eu; Richardson, Bruce
<bruce.richard...@intel.com>; david.march...@redhat.com;
step...@networkplumber.org
Subject: Re: [PATCH v1 00/10] baseband/acc200

Hi Nicolas,

On 7/12/22 15:48, Maxime Coquelin wrote:
Hi Nicolas, Hernan,

(Adding Hernan in the recipients list)

On 7/8/22 02:01, Nicolas Chautru wrote:
This is targeting 22.11 and includes the PMD for the integrated
accelerator on Intel Xeon SPR-EEC.
There is a dependency on that parallel serie still in-flight
which extends the bbdev api
https://patches.dpdk.org/project/dpdk/list/?series=23894

I will be offline for a few weeks for the summer break but
Hernan will cover for me during that time if required.

Thanks
Nic

Nicolas Chautru (10):
       baseband/acc200: introduce PMD for ACC200
       baseband/acc200: add HW register definitions
       baseband/acc200: add info get function
       baseband/acc200: add queue configuration
       baseband/acc200: add LDPC processing functions
       baseband/acc200: add LTE processing functions
       baseband/acc200: add support for FFT operations
       baseband/acc200: support interrupt
       baseband/acc200: add device status and vf2pf comms
       baseband/acc200: add PF configure companion function

      MAINTAINERS                              |    3 +
      app/test-bbdev/meson.build               |    3 +
      app/test-bbdev/test_bbdev_perf.c         |   76 +
      doc/guides/bbdevs/acc200.rst             |  244 ++
      doc/guides/bbdevs/index.rst              |    1 +
      drivers/baseband/acc200/acc200_pf_enum.h |  468 +++
      drivers/baseband/acc200/acc200_pmd.h     |  690 ++++
      drivers/baseband/acc200/acc200_vf_enum.h |   89 +
      drivers/baseband/acc200/meson.build      |    8 +
      drivers/baseband/acc200/rte_acc200_cfg.h |  115 +
      drivers/baseband/acc200/rte_acc200_pmd.c | 5403
++++++++++++++++++++++++++++++
      drivers/baseband/acc200/version.map      |   10 +
      drivers/baseband/meson.build             |    1 +
      13 files changed, 7111 insertions(+)
      create mode 100644 doc/guides/bbdevs/acc200.rst
      create mode 100644
drivers/baseband/acc200/acc200_pf_enum.h
      create mode 100644 drivers/baseband/acc200/acc200_pmd.h
      create mode 100644
drivers/baseband/acc200/acc200_vf_enum.h
      create mode 100644 drivers/baseband/acc200/meson.build
      create mode 100644
drivers/baseband/acc200/rte_acc200_cfg.h
      create mode 100644
drivers/baseband/acc200/rte_acc200_pmd.c
      create mode 100644 drivers/baseband/acc200/version.map

Comparing ACC200 & ACC100 header files, I understand ACC200 is
an evolution of the ACC10x family. The FEC bits are really
close,
ACC200 main addition seems to be FFT acceleration which could be
handled in ACC10x driver based on device ID.

I think both drivers have to be merged in order to avoid code
duplication. That's how other families of devices (e.g. i40e)
are handled.
I haven't seen your reply on this point.
Do you confirm you are working on a single driver for ACC family
in order to avoid code duplication?

The implementation is based on distinct ACC100 and ACC200 drivers.
The 2
devices are fundamentally different generation, processes and IP.
MountBryce is an eASIC device over PCIe while ACC200 is an
integrated
accelerator on Xeon CPU.
The actual implementation are not the same, underlying IP are all
distinct
even if many of the descriptor format have similarities.
The actual capabilities of the acceleration are different and/or new.
The workaround and silicon errata are also different causing
different
limitation and implementation in the driver (see the serie with
ongoing changes for ACC100 in parallel).
This is fundamentally distinct from ACC101 which was a derivative
product
from ACC100 and where it made sense to share implementation
between
ACC100 and ACC101.
So in a nutshell these 2 devices and drivers are 2 different
beasts and the
intention is to keep them intentionally separate as in the serie.
Let me know if unclear, thanks!
Nic,

I used a similarity checker to compare acc100 and acc200

https://dickgrune.com/Programs/similarity_tester/

l=simum.log
if [ -f $l ]; then
        rm $l
fi

sim_c -s -R -o$l -R -p -P -a .

There results are

./acc200/acc200_pf_enum.h consists for 100 % of
./acc100/acc100_pf_enum.h material ./acc100/acc100_pf_enum.h
consists for 98 % of ./acc200/acc200_pf_enum.h material
./acc100/rte_acc100_pmd.h consists for
98 % of ./acc200/acc200_pmd.h material ./acc200/acc200_vf_enum.h
consists for 95 % of ./acc100/acc100_pf_enum.h material
./acc200/acc200_pmd.h consists for 92 % of
./acc100/rte_acc100_pmd.h material ./acc200/rte_acc200_cfg.h
consists for 92 % of ./acc100/rte_acc100_cfg.h material
./acc100/rte_acc100_pmd.c consists for 87 % of
./acc200/rte_acc200_pmd.c material ./acc100/acc100_vf_enum.h
consists for
80 % of ./acc200/acc200_pf_enum.h material
./acc200/rte_acc200_pmd.c consists for 78 % of
./acc100/rte_acc100_pmd.c material ./acc100/rte_acc100_cfg.h
consists for 75 % of ./acc200/rte_acc200_cfg.h material

Spot checking the first *pf_enum.h at 100%, these are the devices'
registers, they are the same.

I raised this similarity issue with 100 vs 101.

Having multiple copies is difficult to support and should be avoided.

For the end user, they should have to use only one driver.

There are really different IP and do not have the same interface
(PCIe/DDR vs
integrated) and there is big serie of changes which are specific to
ACC100 coming in parallel. Any workaround, optimization would be
different.
I agree that for the coming serie of integrated accelerator we will
use a
unified driver approach but for that very case that would be quite
messy to artificially put them within the same PMD.

How is the IP different when 100% of the registers are the same ?

These are 2 different HW aspects. The base toplevel configuration registers
are kept similar on purpose but the underlying IP are totally different design
and implementation.
Even the registers have differences but not visible here, the actual RDL file
would define more specifically these registers bitfields and implementation
including which ones are not implemented (but that is proprietary
information), and at bbdev level the interface is not some much register
based than processing based on data from DMA.
Basically even if there was a common driver, all these would be duplicated
and they are indeed different IP (including different vendors)..
But I agree with the general intent and to have a common driver for the
integrated driver serie (ACC200, ACC300...) now that we are moving away
from PCIe/DDR lookaside acceleration and eASIC/FPGA implementation
(ACC100/AC101).

Looking a little deeper, at how the driver is lays out some of its bitfields and
private data by reviewing the

./acc200/acc200_pmd.h consists for 92 % of ./acc100/rte_acc100_pmd.h

There are some minor changes to existing reserved bitfields.
A new structure for fft.
The acc200_device, the private data for the driver, is an exact copy of
acc100_device.

acc200_pmd.h is the superset and could be used with little changes as a
common acc_pmd.h.
acc200 is doing everything the acc100 did in a very similar if not exact way,
adding the fft feature.

Can you point to some portion of this patchset that is so unique that it could
not be abstracted to an if-check or function and so requiring this separate,
nearly identical driver ?

You used a similarity checker really, there are actually way more relevent 
differences than what you imply here.
With regards to the 2 pf_enum.h file, there are many registers that have same 
or similar names but have now different values being mapped hence you just 
cannot use one for the other.
Saying that "./acc200/acc200_pmd.h consists for 92 % of 
./acc100/rte_acc100_pmd.h" is just not correct and really irrelevant.
Just do a diff side by side please and check, that should be extremely obvious, 
that metrics tells more about the similarity checker limitation than anything 
else.
Even when using a common driver for ACC200/300 they will have distinct register 
enum files being auto-generated and coming from distinct RDL.
Again just do a diff of these 2 files. I believe you will agree that is not 
relevant for these files to try to artificially merged these together.

With regards to the pmd.h, some structure/defines are indeed common and could 
be moved to a common file (for instance turboencoder and LDPC encoder which are 
more vanilla and unlikely to change for future product unlike the decoders 
which have different feature set and behaviour; or some 3GPP constant that can 
be defined once).
We can definitely change these to put together shared structures/defines, but 
not intending to try to artificially put things together with spaghetti code.
We would like to keep 3 parallel versions of these PMD for 3 different product 
lines which are indeed fundamentally different designs (including different 
workaround required as can be seen on the parallel ACC100 serie under review).
- one version for FPGA implementation (support for N3000, N6000, ...)
- one version for eASIC lookaside card implementation (ACC100, ACC101, ...)
- one version for the integrated Xeon accelerators (ACC200, ACC300, ...)

Some suggestions on refactoring,

For the registers, have a common file.

For the shared functionality, ex/ ldpc encoder, break these out to its 
own shared file.

The public interface, see my earlier comments on the documentation, 
should be have the same interfaces and the few differences highlighted.

Tom


Let me know if unclear
Nic







Tom



Tom