On Tue, Oct 20, 2020 at 04:17:13PM +0100, McDaniel, Timothy wrote:
>
>
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richard...@intel.com>
> > Sent: Monday, October 19, 2020 3:34 AM
> > To: Jerin Jacob <jerinjac...@gmail.com>
> > Cc: McDaniel, Timothy <timothy.mcdan...@intel.com>; Mcnamara, John
> > <john.mcnam...@intel.com>; Kovacevic, Marko
> > <marko.kovace...@intel.com>; Ray Kinsella <m...@ashroe.eu>; Neil Horman
> > <nhor...@tuxdriver.com>; dpdk-dev <dev@dpdk.org>; Carrillo, Erik G
> > <erik.g.carri...@intel.com>; Eads, Gage <gage.e...@intel.com>; Van Haaren,
> > Harry <harry.van.haa...@intel.com>; Jerin Jacob <jer...@marvell.com>;
> > Thomas Monjalon <tho...@monjalon.net>
> > Subject: Re: [dpdk-dev] [PATCH v2 01/22] event/dlb2: add documentation and
> > meson build infrastructure
> >
> > On Sun, Oct 18, 2020 at 02:18:32PM +0530, Jerin Jacob wrote:
> > > On Sat, Oct 17, 2020 at 11:50 PM Timothy McDaniel
> > > <timothy.mcdan...@intel.com> wrote:
> > > >
> > > > Adds the meson build infrastructure, which includes
> > > > compile-time constants in rte_config.h. DLB2 is
> > > > only supported on Linux X86 platforms at this time.
> > > >
> > > > Signed-off-by: Timothy McDaniel <timothy.mcdan...@intel.com>
> > > > Reviewed-by: Gage Eads <gage.e...@intel.com>
> > > > ---
> > > > config/rte_config.h | 7 +
> > > > doc/guides/eventdevs/dlb2.rst | 330
> > > > ++++++++++++++++++++++
> > > > doc/guides/eventdevs/index.rst | 1 +
> > > > drivers/event/dlb2/meson.build | 7 +
> > > > drivers/event/dlb2/rte_pmd_dlb2_event_version.map | 3 +
> > > > drivers/event/meson.build | 3 +
> > > > 6 files changed, 351 insertions(+)
> > > > create mode 100644 doc/guides/eventdevs/dlb2.rst
> > > > create mode 100644 drivers/event/dlb2/meson.build
> > > > create mode 100644 drivers/event/dlb2/rte_pmd_dlb2_event_version.map
> > > >
> > > > diff --git a/config/rte_config.h b/config/rte_config.h
> > > > index 0bae630..fd1b3c3 100644
> > > > --- a/config/rte_config.h
> > > > +++ b/config/rte_config.h
> > > > @@ -131,4 +131,11 @@
> > > > /* QEDE PMD defines */
> > > > #define RTE_LIBRTE_QEDE_FW ""
> > > >
> > > > +/* DLB2 defines */
> > > > +#define RTE_LIBRTE_PMD_DLB2_POLL_INTERVAL 1000
> > > > +#define RTE_LIBRTE_PMD_DLB2_UMWAIT_CTL_STATE 0
> > > > +#undef RTE_LIBRTE_PMD_DLB2_QUELL_STATS
> > > > +#define RTE_LIBRTE_PMD_DLB2_SW_CREDIT_QUANTA 32
> > > > +#define RTE_PMD_DLB2_DEFAULT_DEPTH_THRESH 256
> > >
> > > We are going way with GLOBAL compile time options for drivers.
> > > I think, we should move to driver-specific folder not pollute top
> > > level config file.
> > > @Richardson, Bruce @Thomas Monjalon Thoughts?
> > >
> > Yes, we do need a better way to pass driver-specific build config options.
> > How likely is it that the user may want to tweak these values? If it's not
> > likely having them just as regular defines in the driver folder may be
> > better, though they are not particularly problematic in rte_config.h given
> > the number of other values already there.
> >
> > >
> > > > +
> > <snip>
> > > > --- a/drivers/event/meson.build
> > > > +++ b/drivers/event/meson.build
> > > > @@ -10,6 +10,9 @@ if not (toolchain == 'gcc' and
> > cc.version().version_compare('<4.8.6') and
> > > > dpdk_conf.has('RTE_ARCH_ARM64'))
> > > > drivers += 'octeontx'
> > > > endif
> > > > +if (dpdk_conf.has('RTE_ARCH_X86_64') and is_linux)
> > > > + drivers += 'dlb2'
> > > > +endif
> > >
> > > Please add the message in "Content Skipped" section,
> > > Reference: grep "reason" in drivers/vdpa/mlx5/meson.build
> > >
> >
> > The octeontx case is also wrong in this file, IMHO. Rather than checking
> > things in the event level and adding things to the list, the list should
> > just be static. If something should be optionally compiled, then check the
> > conditions in the driver meson.build file itself and add "build=false" to
> > disable, setting "reason" to the cause of it being disabled. This keeps all
> > the logic about a driver in the other file, rather than someone having to
> > look in multiple places for why something is or isn't getting built
> > properly.
> >
> > Regards,
> > /Bruce
>
> Due to time constraints, I would prefer to take these issues up in a future
> release.
>
I'm not suggesting you need to fix the existing octeon case, but it's easy
enough to add dlb2 to the existing list and add :
if not dpdk_conf.has('RTE_ARCH_X86_64') or not is_linux
build = false
reason = "..."
endif
to the dlb2 meson.build file.
Regards,
/Bruce
