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.
