On Tue, Oct 20, 2020 at 04:33:42PM +0100, McDaniel, Timothy wrote:
>
>
> > -----Original Message-----
> > From: Thomas Monjalon <tho...@monjalon.net>
> > Sent: Tuesday, October 20, 2020 10:21 AM
> > To: McDaniel, Timothy <timothy.mcdan...@intel.com>; Eads, Gage
> > <gage.e...@intel.com>
> > Cc: Richardson, Bruce <bruce.richard...@intel.com>; Jerin Jacob
> > <jerinjac...@gmail.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>; Van Haaren,
> > Harry
> > <harry.van.haa...@intel.com>; Jerin Jacob <jer...@marvell.com>;
> > david.march...@redhat.com
> > Subject: Re: [dpdk-dev] [PATCH v2 01/22] event/dlb2: add documentation and
> > meson build infrastructure
> >
> > 20/10/2020 17:17, McDaniel, Timothy:
> > > From: Bruce Richardson <bruce.richard...@intel.com>
> > > > 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>
> > > > > > ---
> > > > > > --- 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.
> >
> > If you have strong time constraints to complete everything,
> > then it is better to postpone the feature to the next release.
> >
>
> You read too much into my comment.
> My point is that there does not seem to be community consensus on this issue,
There is consensus here, and the way the octeon event driver is doing
things is wrong, and was just missed in previous reviews.
> so why are we debating it at this point in the release cycle. There are
> plenty of examples
> doing exactly what I am doing.
>
Looking through the driver/*/meson.build files this appears to be the only
incidence where a driver is optionally adding itself to the list. All other
drivers are always processed so that when enabled/disabled they can be
properly listed in the configuration summary and a reason given.
Regards,
/Bruce
