06/09/2022 14:51, Tom Rix: > On 9/1/22 1:34 PM, Chautru, Nicolas wrote: > > From: Tom Rix <t...@redhat.com> > >> On 8/31/22 6:26 PM, Chautru, Nicolas wrote: > >>> From: Tom Rix <t...@redhat.com> > >>>> On 8/31/22 3:37 PM, Chautru, Nicolas wrote: > >>>>>>>>> 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.
+1 to have common files, and all in a single directory drivers/baseband/acc100/
