From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5656CA0032; Wed, 14 Sep 2022 12:35:25 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 46D1C4021D; Wed, 14 Sep 2022 12:35:25 +0200 (CEST) Received: from out4-smtp.messagingengine.com (out4-smtp.messagingengine.com [66.111.4.28]) by mails.dpdk.org (Postfix) with ESMTP id 76EF140156 for ; Wed, 14 Sep 2022 12:35:24 +0200 (CEST) Received: from compute4.internal (compute4.nyi.internal [10.202.2.44]) by mailout.nyi.internal (Postfix) with ESMTP id 0AE9F5C00C2; Wed, 14 Sep 2022 06:35:24 -0400 (EDT) Received: from mailfrontend2 ([10.202.2.163]) by compute4.internal (MEProxy); Wed, 14 Sep 2022 06:35:24 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=monjalon.net; h= cc:cc:content-transfer-encoding:content-type:date:date:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to; s=fm2; t=1663151724; x= 1663238124; bh=5tyuuec1Q0kkMIQqZrc+fQ+X4VZvmV2+Eu3jpFPvbKY=; b=Z hD43ECMbc1ZMJXoB91FfSnUtFQ6Np9ITnlEDLiMPx9WzZDOaDI/rsYD0x7CZUlmA gzMWAEBGta7jEfHggyaZGOtrlV2gHXACKyZWcnEUFSjRSUvmZmjbNPjY/UmqbS5M gKVPaeL/iCitFE5PxDE627jssypY99jI78d1UB6g9m2fli5ij8xaSUx15vmxkycR BGk1qTVgg2S/lQ1npf1+QhrysRk5DXTk/yG5KQ5sNYTccYqqxbf0tKLlRa7S8C+9 mLXUWKAPKRn8NYSMAje9YYFCARjmP/LEVD+/ddA2L4Mh8ZkIXKMf0qusuXi1OsJ4 oXO7IUCyVXWZ14o+3rcsg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d= messagingengine.com; h=cc:cc:content-transfer-encoding :content-type:date:date:feedback-id:feedback-id:from:from :in-reply-to:in-reply-to:message-id:mime-version:references :reply-to:sender:subject:subject:to:to:x-me-proxy:x-me-proxy :x-me-sender:x-me-sender:x-sasl-enc; s=fm2; t=1663151724; x= 1663238124; bh=5tyuuec1Q0kkMIQqZrc+fQ+X4VZvmV2+Eu3jpFPvbKY=; b=z Rf6AU5Zjvva06LQpEQyOhclsetb+kG79/GbQT/a1p6wXP95/9MIjMB7I4GtGb7wX C354MNgVvy1DryKvGJrHiinCT4Q5EcOCmyIgq2w8yxLyfmxam3KMFwyQXXddYMFv XiyFRW63YIO6LrwIXnvWOA5s/7chVZqjKtiIrMz+DhMNoPMAOlOS+3bFPJITY2c6 tE/aIhJLmlAgp2uSILy2bkE8k9X6ydEUdeFIqt5x8HZK61JFsoN+vvemYnz8iN8Z O+vA04lLzcUmXx7R2n6VGgMkFr/ushgF3+PUCeOci8650U/eekApR1mRPCQz+2mK jNyOo/h1t32PvP5DVY1qA== X-ME-Sender: X-ME-Received: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvfedrfeduiedgfedtucetufdoteggodetrfdotf fvucfrrhhofhhilhgvmecuhfgrshhtofgrihhlpdfqfgfvpdfurfetoffkrfgpnffqhgen uceurghilhhouhhtmecufedttdenucesvcftvggtihhpihgvnhhtshculddquddttddmne cujfgurhephffvvefufffkjghfggfgtgesthfuredttddtvdenucfhrhhomhepvfhhohhm rghsucfoohhnjhgrlhhonhcuoehthhhomhgrshesmhhonhhjrghlohhnrdhnvghtqeenuc ggtffrrghtthgvrhhnpedvfeekkefhhfeiffdvjeeludekleegieeghffhfeekheeuudei tdfhveehtefhieenucffohhmrghinhepughitghkghhruhhnvgdrtghomhenucevlhhush htvghrufhiiigvpedtnecurfgrrhgrmhepmhgrihhlfhhrohhmpehthhhomhgrshesmhho nhhjrghlohhnrdhnvght X-ME-Proxy: Feedback-ID: i47234305:Fastmail Received: by mail.messagingengine.com (Postfix) with ESMTPA; Wed, 14 Sep 2022 06:35:21 -0400 (EDT) From: Thomas Monjalon To: "Chautru, Nicolas" , Maxime Coquelin , "dev@dpdk.org" , "gakhil@marvell.com" , "hemant.agrawal@nxp.com" , "Vargas, Hernan" , Tom Rix Cc: "mdr@ashroe.eu" , "Richardson, Bruce" , "david.marchand@redhat.com" , "stephen@networkplumber.org" Subject: Re: [PATCH v1 00/10] baseband/acc200 Date: Wed, 14 Sep 2022 12:35:19 +0200 Message-ID: <16306674.geO5KgaWL5@thomas> In-Reply-To: References: <1657238503-143836-1-git-send-email-nicolas.chautru@intel.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 06/09/2022 14:51, Tom Rix: > On 9/1/22 1:34 PM, Chautru, Nicolas wrote: > > From: Tom Rix > >> On 8/31/22 6:26 PM, Chautru, Nicolas wrote: > >>> From: Tom Rix > >>>> 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/