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

[McDaniel, Timothy]

> -----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.