[dpdk-dev] [PATCH v2 01/22] event/dlb2: add documentation and meson build infrastructure

[Bruce Richardson]

On Tue, Oct 20, 2020 at 04:17:13PM +0100, McDaniel, Timothy wrote:
> 
> 
> > -----Original Message-----
> > From: Bruce Richardson <bruce.richardson at intel.com>
> > Sent: Monday, October 19, 2020 3:34 AM
> > To: Jerin Jacob <jerinjacobk at gmail.com>
> > Cc: McDaniel, Timothy <timothy.mcdaniel at intel.com>; Mcnamara, John
> > <john.mcnamara at intel.com>; Kovacevic, Marko
> > <Marko.Kovacevic at intel.com>; Ray Kinsella <mdr at ashroe.eu>; Neil Horman
> > <nhorman at tuxdriver.com>; dpdk-dev <dev at dpdk.org>; Carrillo, Erik G
> > <Erik.G.Carrillo at intel.com>; Eads, Gage <gage.eads at intel.com>; Van Haaren,
> > Harry <harry.van.haaren at intel.com>; Jerin Jacob <jerinj at marvell.com>;
> > Thomas Monjalon <thomas at 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.mcdaniel at 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.mcdaniel at intel.com>
> > > > Reviewed-by: Gage Eads <gage.eads at 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