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 D0C73A00C5; Wed, 14 Sep 2022 22:09:57 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BF6424113C; Wed, 14 Sep 2022 22:09:57 +0200 (CEST) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id CE6B240156 for ; Wed, 14 Sep 2022 22:09:56 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1663186195; 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=P4c61lYL2WxJvRa9Pzgadqx/oxxuCo7ax/+oOm42pnI=; b=H5AWNsUS5asL8y88E82UbY5mTgsLnQ4O86rUvfri9gufYhcca8SAWFUyd7xm5tM4LVlsd+ 9cTtjV2aVJbXfViLZM00eoCdyczAXpbmhsXwgdW68LzNBijOQ8kQ5hHPnLkwW7prKGIw0k 90Dor28+Wc/kRtfVqneWBQUQLKOe2tc= 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-628-YPCloAeFPV6G-LyI_UUpNw-1; Wed, 14 Sep 2022 16:09:03 -0400 X-MC-Unique: YPCloAeFPV6G-LyI_UUpNw-1 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id BD502294EDE5; Wed, 14 Sep 2022 20:08:11 +0000 (UTC) Received: from [10.39.208.12] (unknown [10.39.208.12]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 1172E140EBF3; Wed, 14 Sep 2022 20:08:08 +0000 (UTC) Message-ID: <2c00baa1-4bf4-a09f-d0d2-1b060ac1b243@redhat.com> Date: Wed, 14 Sep 2022 22:08:07 +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: [EXT] Re: [PATCH v1 00/10] baseband/acc200 To: "Chautru, Nicolas" , Thomas Monjalon , "Richardson, Bruce" , Akhil Goyal Cc: "dev@dpdk.org" , "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> <2150057.1BCLMh4Saa@thomas> From: Maxime Coquelin In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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 21:57, Chautru, Nicolas wrote: > Hi Thomas, Akhil, Bruce, Maxime, > >> -----Original Message----- >> From: Thomas Monjalon >> Sent: Wednesday, September 14, 2022 7:23 AM >> To: Richardson, Bruce ; Maxime Coquelin >> ; Akhil Goyal ; >> Chautru, Nicolas >> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Vargas, Hernan >> ; Tom Rix ; mdr@ashroe.eu; >> david.marchand@redhat.com; stephen@networkplumber.org >> Subject: Re: [EXT] Re: [PATCH v1 00/10] baseband/acc200 >> >> 14/09/2022 15:44, Akhil Goyal: >>>>> 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: > >>>>>>>> >>>>>>>> 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. >>>> >>> It can be ok to have 2 separate directories, but >>> - is it not possible to have them in same directory say 'acc' for all affiliated >> devices. >>> Similar to other vendors' devices (cnxk, i40e, mlx). >>> - Can both the devices - acc100 and acc200 coexist? If not, same directory >> is good enough. >>> - there can be multiple files or directories in 'acc' which can be >>> named appropriately to denote the actual device(acc100/200). >>> >>> Having cross dependency across different drivers of same type looks a kind >> of hacking the meson. >>> This was a reason we moved to have a drivers/common/ for some of the >> drivers. >>> Also including "../acc100/abc.h" does not look appropriate to me. >>> >>> IMO, we should not add unnecessary directories when the code is common >> and can be managed in a single one. >>> >>> However, technically it is also ok to have 2 separate directories. >>> But, agreeing on this will set a precedence for future next generation >> devices from the same vendors. It may be a topic of discussion in techboard. >> >> Let me be frank, I don't trust Intel saying the hardware will be too much >> different in future. > > Thanks for the review and discussion. > > Let me clarify, this PMD segregation is specific to ACC1xx vs ACC2xxx. There is a clear intent to have a common PMD to encompass the future multiple integrated solutions VRAN accelerators on Xeon (based on ACC200 and future Xeon products in roadmap) but not for ACC1xx. > Here we are splitting the ACC1xx and the ACC2xx series (eASIC process with off-die PCIe device with on-card DDR vs a straight integrated Xeon accelerator) which are fundamentally different devices, and notably the ACC100 requiring a lot of SW workaround/mitigations/protections in the code which would not apply moving forward and would clutter the next generations which would be managed and optimized largely independently. Basically these are not just a few registers differences truly. > Again future integrated Xeon will shared common driver but always distinct from ACC1xx (only sharing some common code and structure when possible). > Here the refactoring effort was to gather all reusable code and structure together; which was useful indeed as there are several common functionalities and structures which could be superseded to be shared relatively seamlessly. > >> For mlx5, we manage to handle very different devices (like DPU and changing >> processors) in a single driver. >> So I agree with Maxime and Akhil that a single driver in a single directory >> should be enough. >> Having different registers in different devices is not enough to split. >> >> The worst case would be to have a common directory acc/ but it may be a bit >> disappointing. >> > > I believe that I hear 2 different options compatible with the 2 PMDs approach: > - The one suggested by Akhil and Maxime I think, is to put both ACC100 and ACC200 PMDs under ./baseband/acc/ similarly to what is done for cnxk for instance. In that case the common files are still all in same directory as the 2 PMDs so we don't have do the awkard "includes += include_directories('../acc100')" in meson which was frown upon, since everything in already under /drivers/baseband/acc. > - other option suggested by Thomas to put the shared code and structures under ./drivers/common/acc instead of being under ./drivers/acc/acc_common.h which also used for many drivers. > > My preference may probably be personally for the former option at the moment, but happy to get some form of consensus on this. I am fine with former option. drivers/common is especially useful when code is to be shared by different types of devices (e.g. net and crypto). Maxime > > Thanks and regards, > Nic >