Hi Thomas, Akhil, Bruce, Maxime,
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Wednesday, September 14, 2022 7:23 AM
> To: Richardson, Bruce <bruce.richard...@intel.com>; Maxime Coquelin
> <maxime.coque...@redhat.com>; Akhil Goyal <gak...@marvell.com>;
> Chautru, Nicolas <nicolas.chau...@intel.com>
> Cc: dev@dpdk.org; hemant.agra...@nxp.com; Vargas, Hernan
> <hernan.var...@intel.com>; Tom Rix <t...@redhat.com>; m...@ashroe.eu;
> david.march...@redhat.com; step...@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 <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:
<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
