On 9/14/22 21:57, Chautru, Nicolas wrote:
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.
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
