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 F015DA00C5; Wed, 14 Sep 2022 15:28:08 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 93F7C4021D; Wed, 14 Sep 2022 15:28:08 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mails.dpdk.org (Postfix) with ESMTP id E9B8040156 for ; Wed, 14 Sep 2022 15:28:06 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663162086; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=2+/blOAQy4SpPb9xEbtHYZN+lpK1y8gMsGM/IHlYH9g=; b=iQeyhH131VrbO/kD1I0wWMt+roBh5Sp0Ac8ogMAc3LL8lufEhFnJgn0brlVrKndQ08vn0V jCHOksSRWcgM7xOGRorxsqWnJ1L0jDuRSrzZnybQvOWhangHplq7/hwV1+1u4gbWdrZXPU o2P3ka5Phz+fcE5GAjzMhA+5xjvnb9I= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-665-9W2k0mb8NnS2lXb8ttIvFQ-1; Wed, 14 Sep 2022 09:28:03 -0400 X-MC-Unique: 9W2k0mb8NnS2lXb8ttIvFQ-1 Received: from smtp.corp.redhat.com (int-mx09.intmail.prod.int.rdu2.redhat.com [10.11.54.9]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 9FC853C1172D; Wed, 14 Sep 2022 13:28:02 +0000 (UTC) Received: from [10.39.208.12] (unknown [10.39.208.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id E0597492CA2; Wed, 14 Sep 2022 13:27:59 +0000 (UTC) Message-ID: <687a0f05-d6c1-a70d-fe41-c8bc186e44af@redhat.com> Date: Wed, 14 Sep 2022 15:27:57 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH v1 00/10] baseband/acc200 To: Bruce Richardson Cc: Thomas Monjalon , "Chautru, Nicolas" , "dev@dpdk.org" , "gakhil@marvell.com" , "hemant.agrawal@nxp.com" , "Vargas, Hernan" , Tom Rix , "mdr@ashroe.eu" , "david.marchand@redhat.com" , "stephen@networkplumber.org" References: <1657238503-143836-1-git-send-email-nicolas.chautru@intel.com> <16306674.geO5KgaWL5@thomas> <95130dc6-69ca-9be4-ccda-fabbc6c6c88a@redhat.com> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.9 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 On 9/14/22 15:19, Bruce Richardson wrote: > On Wed, Sep 14, 2022 at 01:50:05PM +0200, Maxime Coquelin wrote: >> >> >> On 9/14/22 12:35, Thomas Monjalon wrote: >>> 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/ >> >> Jus to be sure we are aligned, do you mean to have both drivers in the >> same directory, which will share some common files? That's the way I >> would go. >> > > I think the expectation is that the two drivers will diverge in future, so > having separate directories should be ok, even with common files placed in > one directory are shared with another. With meson include paths its pretty > trivial to manage if it's just header files, and even if there are common C > files, there is always the option of using drivers/common if we want to > split them out. As I understand it, right now it's only headers inluding > functions which can be static inline, so simple sharing via include paths > should work fine. Ok, then I prefer having the common parts in drivers/common/acc, in order to make it clear changes to these common files have impact on other drivers than ACC100. Is that good for you? Thanks, Maxime > /Bruce >