From: "Chautru, Nicolas" <nicolas.chautru@intel.com>
To: Thomas Monjalon <thomas@monjalon.net>,
"Richardson, Bruce" <bruce.richardson@intel.com>,
Maxime Coquelin <maxime.coquelin@redhat.com>,
Akhil Goyal <gakhil@marvell.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"hemant.agrawal@nxp.com" <hemant.agrawal@nxp.com>,
"Vargas, Hernan" <hernan.vargas@intel.com>,
Tom Rix <trix@redhat.com>, "mdr@ashroe.eu" <mdr@ashroe.eu>,
"david.marchand@redhat.com" <david.marchand@redhat.com>,
"stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: RE: [EXT] Re: [PATCH v1 00/10] baseband/acc200
Date: Wed, 14 Sep 2022 19:57:25 +0000 [thread overview]
Message-ID: <BY5PR11MB445133FD9A7014B660EC6DDFF8469@BY5PR11MB4451.namprd11.prod.outlook.com> (raw)
In-Reply-To: <2150057.1BCLMh4Saa@thomas>
Hi Thomas, Akhil, Bruce, Maxime,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, September 14, 2022 7:23 AM
> To: Richardson, Bruce <bruce.richardson@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Akhil Goyal <gakhil@marvell.com>;
> Chautru, Nicolas <nicolas.chautru@intel.com>
> Cc: dev@dpdk.org; hemant.agrawal@nxp.com; Vargas, Hernan
> <hernan.vargas@intel.com>; Tom Rix <trix@redhat.com>; 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 <trix@redhat.com>
> > > > > > > > On 8/31/22 6:26 PM, Chautru, Nicolas wrote:
> > > > > > > > > From: Tom Rix <trix@redhat.com>
> > > > > > > > > > On 8/31/22 3:37 PM, Chautru, Nicolas wrote:
<snip>
> > > > > > >
> > > > > > > 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.
Thanks and regards,
Nic
next prev parent reply other threads:[~2022-09-14 19:57 UTC|newest]
Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-08 0:01 Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 01/10] baseband/acc200: introduce PMD for ACC200 Nicolas Chautru
2022-09-12 1:08 ` [PATCH v2 00/11] baseband/acc200 Nic Chautru
2022-09-12 1:08 ` [PATCH v2 01/11] baseband/acc100: refactory to segregate common code Nic Chautru
2022-09-12 15:19 ` Bruce Richardson
2022-09-12 1:08 ` [PATCH v2 02/11] baseband/acc200: introduce PMD for ACC200 Nic Chautru
2022-09-12 15:41 ` Bruce Richardson
2022-09-12 1:08 ` [PATCH v2 03/11] baseband/acc200: add HW register definitions Nic Chautru
2022-09-12 1:08 ` [PATCH v2 04/11] baseband/acc200: add info get function Nic Chautru
2022-09-12 1:08 ` [PATCH v2 05/11] baseband/acc200: add queue configuration Nic Chautru
2022-09-12 1:08 ` [PATCH v2 06/11] baseband/acc200: add LDPC processing functions Nic Chautru
2022-09-12 1:08 ` [PATCH v2 07/11] baseband/acc200: add LTE " Nic Chautru
2022-09-12 1:08 ` [PATCH v2 08/11] baseband/acc200: add support for FFT operations Nic Chautru
2022-09-12 1:08 ` [PATCH v2 09/11] baseband/acc200: support interrupt Nic Chautru
2022-09-12 1:08 ` [PATCH v2 10/11] baseband/acc200: add device status and vf2pf comms Nic Chautru
2022-09-12 1:08 ` [PATCH v2 11/11] baseband/acc200: add PF configure companion function Nic Chautru
2022-07-08 0:01 ` [PATCH v1 02/10] baseband/acc200: add HW register definitions Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 03/10] baseband/acc200: add info get function Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 04/10] baseband/acc200: add queue configuration Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 05/10] baseband/acc200: add LDPC processing functions Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 06/10] baseband/acc200: add LTE " Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 07/10] baseband/acc200: add support for FFT operations Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 08/10] baseband/acc200: support interrupt Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 09/10] baseband/acc200: add device status and vf2pf comms Nicolas Chautru
2022-07-08 0:01 ` [PATCH v1 10/10] baseband/acc200: add PF configure companion function Nicolas Chautru
2022-07-12 13:48 ` [PATCH v1 00/10] baseband/acc200 Maxime Coquelin
2022-07-14 18:49 ` Vargas, Hernan
2022-07-17 13:08 ` Tom Rix
2022-07-22 18:29 ` Vargas, Hernan
2022-07-22 20:19 ` Tom Rix
2022-08-15 17:52 ` Chautru, Nicolas
2022-08-30 7:44 ` Maxime Coquelin
2022-08-30 19:45 ` Chautru, Nicolas
2022-08-31 16:43 ` Maxime Coquelin
2022-08-31 19:20 ` Thomas Monjalon
2022-08-31 19:26 ` Tom Rix
2022-08-31 22:37 ` Chautru, Nicolas
2022-09-01 0:28 ` Tom Rix
2022-09-01 1:26 ` Chautru, Nicolas
2022-09-01 13:49 ` Tom Rix
2022-09-01 20:34 ` Chautru, Nicolas
2022-09-06 12:51 ` Tom Rix
2022-09-14 10:35 ` Thomas Monjalon
2022-09-14 11:50 ` Maxime Coquelin
2022-09-14 13:19 ` Bruce Richardson
2022-09-14 13:27 ` Maxime Coquelin
2022-09-14 13:44 ` [EXT] " Akhil Goyal
2022-09-14 14:23 ` Thomas Monjalon
2022-09-14 19:57 ` Chautru, Nicolas [this message]
2022-09-14 20:08 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=BY5PR11MB445133FD9A7014B660EC6DDFF8469@BY5PR11MB4451.namprd11.prod.outlook.com \
--to=nicolas.chautru@intel.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=gakhil@marvell.com \
--cc=hemant.agrawal@nxp.com \
--cc=hernan.vargas@intel.com \
--cc=maxime.coquelin@redhat.com \
--cc=mdr@ashroe.eu \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
--cc=trix@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).