From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 3D1EAA0579; Fri, 9 Apr 2021 12:03:00 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id B5B34407FF; Fri, 9 Apr 2021 12:02:59 +0200 (CEST) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by mails.dpdk.org (Postfix) with ESMTP id 2DB054014D for ; Fri, 9 Apr 2021 12:02:57 +0200 (CEST) IronPort-SDR: Wk1WMQKnpswkX/T/qiJoKgp7NfGIE+K6qWP+ACXHHcCjWw5BU17PwUI3Li9B+udLS/JZKIVPH3 sODf0s9jeQRw== X-IronPort-AV: E=McAfee;i="6000,8403,9948"; a="190531468" X-IronPort-AV: E=Sophos;i="5.82,209,1613462400"; d="scan'208";a="190531468" Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga102.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 09 Apr 2021 03:02:57 -0700 IronPort-SDR: SGrC4mUfFrWNIrZDcfDM+w0Ouml4rUxxVFViZqXIAqJeqtz5wqgazwq3uWcUV9w80DwZPMMWYF BN+njuCynhZw== X-IronPort-AV: E=Sophos;i="5.82,209,1613462400"; d="scan'208";a="459178853" Received: from bricha3-mobl.ger.corp.intel.com ([10.252.3.224]) by orsmga001-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 09 Apr 2021 03:02:54 -0700 Date: Fri, 9 Apr 2021 11:02:50 +0100 From: Bruce Richardson To: Juraj =?utf-8?Q?Linke=C5=A1?= Cc: Ruifeng.Wang@arm.com, Honnappa.Nagarahalli@arm.com, Phil.Yang@arm.com, vcchunga@amazon.com, Dharmik.Thakkar@arm.com, jerinjacobk@gmail.com, hemant.agrawal@nxp.com, ajit.khaparde@broadcom.com, ferruh.yigit@intel.com, aboyer@pensando.io, dev@dpdk.org Message-ID: <20210409100250.GA1367@bricha3-MOBL.ger.corp.intel.com> References: <1617950146-7307-1-git-send-email-juraj.linkes@pantheon.tech> <1617957679-7751-1-git-send-email-juraj.linkes@pantheon.tech> <1617957679-7751-2-git-send-email-juraj.linkes@pantheon.tech> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1617957679-7751-2-git-send-email-juraj.linkes@pantheon.tech> Subject: Re: [dpdk-dev] [PATCH v19 1/3] build: disable/enable drivers in Arm builds X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Fri, Apr 09, 2021 at 10:41:17AM +0200, Juraj Linkeš wrote: > Add support for enabling or disabling drivers for Arm cross build. Do > not implement any enable/disable lists yet. > > Enabling drivers is useful when building for an SoC where we only want > to build a few drivers. That way the list won't be too long. > > Similarly, disabling drivers is useful when we want to disable only a > few drivers. > > Both of these are advantageous mainly in aarch64 -> aarch64 (or arch -> > same arch) builds, where the build machine may have the required driver > dependencies, yet we don't want to build drivers for a specific SoC. > > By default, build all drivers for which dependencies are found. If > enabled_drivers is a non-empty list, build only those drivers. If > disabled_drivers is non-empty list, build all drivers except those in > disabled_drivers. Error out if both are specified (i.e. do not support > that case). > > There are two drivers, bus/pci and bus/vdev, which break the build if > not enabled. Address this by always enabling these if the user disables > them or doesn't specify in their allowlist. > > Also remove the old Makefile arm configuration options which don't do > anything in Meson. > > Signed-off-by: Juraj Linkeš > Acked-by: Bruce Richardson I think this patch has changed since I last acked it. Further review below. > Reviewed-by: Honnappa Nagarahalli > Reviewed-by: Ruifeng Wang > --- > config/arm/meson.build | 4 -- > .../linux_gsg/cross_build_dpdk_for_arm64.rst | 8 +++ > drivers/meson.build | 49 +++++++++++++++++-- > meson.build | 2 + > meson_options.txt | 2 + > 5 files changed, 56 insertions(+), 9 deletions(-) > > diff --git a/config/arm/meson.build b/config/arm/meson.build > index 00bc4610a3..a241c45d13 100644 > --- a/config/arm/meson.build > +++ b/config/arm/meson.build > @@ -16,9 +16,6 @@ flags_common = [ > # ['RTE_ARM64_MEMCPY_ALIGN_MASK', 0xF], > # ['RTE_ARM64_MEMCPY_STRICT_ALIGN', false], > > - ['RTE_NET_FM10K', false], > - ['RTE_NET_AVP', false], > - > ['RTE_SCHED_VECTOR', false], > ['RTE_ARM_USE_WFE', false], > ['RTE_ARCH_ARM64', true], > @@ -125,7 +122,6 @@ implementer_cavium = { > ['RTE_MACHINE', '"octeontx2"'], > ['RTE_ARM_FEATURE_ATOMICS', true], > ['RTE_USE_C11_MEM_MODEL', true], > - ['RTE_EAL_IGB_UIO', false], > ['RTE_MAX_LCORE', 36], > ['RTE_MAX_NUMA_NODES', 1] > ] > diff --git a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst > index faaf24b95b..1504dbfef0 100644 > --- a/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst > +++ b/doc/guides/linux_gsg/cross_build_dpdk_for_arm64.rst > @@ -234,3 +234,11 @@ There are other options you may specify in a cross file to tailor the build:: > numa = false # set to false to force building for a non-NUMA system > # if not set or set to true, the build system will build for a NUMA > # system only if libnuma is installed > + > + disabled_drivers = ['bus/dpaa', 'crypto/*'] # add disabled drivers > + # valid values are dir/subdirs in the drivers directory > + # wildcards are allowed > + > + enabled_drivers = ['common/*', 'bus/*'] # build only these drivers > + # valid values are dir/subdirs in the drivers directory > + # wildcards are allowed > diff --git a/drivers/meson.build b/drivers/meson.build > index 9c8eded697..be5fd2df88 100644 > --- a/drivers/meson.build > +++ b/drivers/meson.build > @@ -19,8 +19,39 @@ subdirs = [ > 'baseband', # depends on common and bus. > ] > > -disabled_drivers = run_command(list_dir_globs, get_option('disable_drivers'), > - ).stdout().split() > +always_enabled = ['bus/pci', 'bus/vdev'] > + > +if meson.is_cross_build() > + disabled_drivers += meson.get_cross_property('disabled_drivers', []) > + enabled_drivers += meson.get_cross_property('enabled_drivers', []) > +endif I don't think "+=" is correct here. This is the first use of the disabled_drivers variable. [Sorry, correction, I see you've added a new definition of it in the top-level meson.build. I think it's better to move that to this file] Also, would it not be better to have the cross-file option the same as that used in the parameter option? Right now you have the cross-file option as "disabled_drivers" vs cmdline option "disable_drivers" and the types being list and string respectively too. Why not have the cross-file option be a string called "disable_drivers" too? It would save some joining an array/string conversion below and simplify things. > + > +# add cmdline disabled drivers (comma separated string) > +# and meson disabled drivers (list) > +# together into a comma separated string > +disabled_drivers = ','.join([get_option('disable_drivers'), ','.join(disabled_drivers)]).strip(',') Do we need the string at the end here? I would think that join never adds a trailing comma? As stated above if these were both strings it might make things shorter and easier. > +if disabled_drivers != '' > + disabled_drivers = run_command(list_dir_globs, > + disabled_drivers).stdout().split() > +else > + disabled_drivers = [] > +endif Don't think we need the "if/else" here. The existing code works fine with taking an empty array. > + > +# add cmdline enabled drivers (comma separated string) > +# and meson enabled drivers (list) > +# together into a comma separated string > +enabled_drivers = ','.join([get_option('enable_drivers'), ','.join(enabled_drivers)]).strip(',') > +if enabled_drivers != '' > + enabled_drivers = run_command(list_dir_globs, > + enabled_drivers).stdout().split() > +else > + enabled_drivers = [] > +endif > + > +if disabled_drivers != [] and enabled_drivers != [] > + error('Simultaneous disabled drivers and enabled drivers ' + > + 'configuration is not supported.') > +endif For use in cross-files this makes sense, but I'm not sure it's the correct approach for when a cross-file specifies enable and the user specifies disable on the cmdline. In that case, the enable list should just have the disabled values removed from it. Therefore, I suggest having this check only in the cross-build section. > > default_cflags = machine_args > default_cflags += ['-DALLOW_EXPERIMENTAL_API'] > @@ -49,7 +80,7 @@ foreach subpath:subdirs > dpdk_driver_classes += class > endif > # get already enabled drivers of the same class > - enabled_drivers = get_variable(class + '_drivers', []) > + enabled_class_drivers = get_variable(class + '_drivers', []) > > foreach drv:drivers > drv_path = join_paths(class, drv) > @@ -77,11 +108,19 @@ foreach subpath:subdirs > if disabled_drivers.contains(drv_path) > build = false > reason = 'explicitly disabled via build config' > + elif enabled_drivers.length() > 0 and not enabled_drivers.contains(drv_path) > + build = false > + reason = 'not in enabled drivers build config' > else > # pull in driver directory which should update all the local variables > subdir(drv_path) > endif > > + if not build and always_enabled.contains(drv_path) > + build = true > + message('Driver @0@ cannot be disabled, enabling.'.format(drv_path)) > + endif > + > if build > # get dependency objs from strings > shared_deps = ext_deps > @@ -109,7 +148,7 @@ foreach subpath:subdirs > '_disable_reason', reason) > endif > else > - enabled_drivers += name > + enabled_class_drivers += name > lib_name = '_'.join(['rte', class, name]) > dpdk_conf.set(lib_name.to_upper(), 1) > > @@ -214,5 +253,5 @@ foreach subpath:subdirs > endif # build > endforeach > > - set_variable(class + '_drivers', enabled_drivers) > + set_variable(class + '_drivers', enabled_class_drivers) > endforeach > diff --git a/meson.build b/meson.build > index 7778e18200..38a2bd5416 100644 > --- a/meson.build > +++ b/meson.build > @@ -22,6 +22,8 @@ dpdk_drivers = [] > dpdk_extra_ldflags = [] > dpdk_libs_disabled = [] > dpdk_drvs_disabled = [] > +disabled_drivers = [] > +enabled_drivers = [] These variables don't need to be declared here. They are only used in drivers/meson.build and nowhere else. > abi_version_file = files('ABI_VERSION') > > if host_machine.cpu_family().startswith('x86') > diff --git a/meson_options.txt b/meson_options.txt > index 86bc6c88f6..d2d24a1424 100644 > --- a/meson_options.txt > +++ b/meson_options.txt > @@ -8,6 +8,8 @@ option('drivers_install_subdir', type: 'string', value: 'dpdk/pmds-', > description: 'Subdirectory of libdir where to install PMDs. Defaults to using a versioned subdirectory.') > option('enable_docs', type: 'boolean', value: false, > description: 'build documentation') > +option('enable_drivers', type: 'string', value: '', > + description: 'Comma-separated list of drivers to build. If unspecified, build all drivers.') > option('enable_driver_sdk', type: 'boolean', value: false, > description: 'Install headers to build drivers.') > option('enable_kmods', type: 'boolean', value: false, > -- > 2.20.1 >