Re: [Buildroot] [PATCH 2/4] package/vkmark: add vmkark benchmarking tool

[Thomas Petazzoni via buildroot <buildroot@buildroot.org>]

Hello Alex,

Thanks for this patch! See below for some comments.

On Thu, 22 Aug 2024 14:02:18 +0100
Alex Bennée <alex.bennee@linaro.org> wrote:

> We build from the current master as we need fairly upto date bits for
> cross compile support.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  package/Config.in          |  1 +
>  package/vkmark/Config.in   | 39 ++++++++++++++++++++++++++++++++++++++
>  package/vkmark/vkmark.hash |  3 +++
>  package/vkmark/vkmark.mk   | 28 +++++++++++++++++++++++++++
>  4 files changed, 71 insertions(+)

This new package needs an entry in the DEVELOPERS file.


> diff --git a/package/vkmark/Config.in b/package/vkmark/Config.in
> new file mode 100644
> index 0000000000..15d8e380ed
> --- /dev/null
> +++ b/package/vkmark/Config.in
> @@ -0,0 +1,39 @@
> +config BR2_PACKAGE_VKMARK_FLAVOR_ANY
> +	bool
> +
> +config BR2_PACKAGE_VKMARK_FLAVOR_KMS
> +	bool
> +	default y
> +	depends on BR2_PACKAGE_HAS_LIBGBM
> +	select BR2_PACKAGE_LIBDRM
> +        select BR2_PACKAGE_VKMARK_FLAVOR_ANY

Indentation issue.

But more importantly, this option that you've added means that *ALL*
Buildroot configurations now have BR2_PACKAGE_LIBDRM forcefully
selected. Indeed, BR2_PACKAGE_VKMARK_FLAVOR_KMS is a blind option that
is default y (so always enabled) and it selects BR2_PACKAGE_LIBDRM.

> +config BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND
> +	bool
> +	default n

"default n" is useless, as an option is disabled by default.

> +	select BR2_PACKAGE_VKMARK_FLAVOR_ANY

This cannot have any impact, because BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND
is never enabled by anything.

> +config BR2_PACKAGE_VKMARK_FLAVOR_X11
> +	bool
> +	default n

Same comment as above.

> +	select BR2_PACKAGE_LIBXCB
> +        select BR2_PACKAGE_VKMARK_FLAVOR_ANY

Same comment as above: these selects are no-ops, as
BR2_PACKAGE_VKMARK_FLAVOR_X11 is never going to be "enabled", as it's a
blind option that nothing selects/enables.

If you want to do something like you did, you need to do something like
this:

config BR2_PACKAGE_VKMARK_FLAVOR_ANY
	bool

config BR2_PACKAGE_VKMARK_FLAVOR_KMS
	bool
	default y if BR2_PACKAGE_HAS_LIBGBM
        select BR2_PACKAGE_VKMARK_FLAVOR_ANY

config BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND
	bool
	default y if BR2_PACKAGE_WAYLAND
	select BR2_PACKAGE_VKMARK_FLAVOR_ANY

config BR2_PACKAGE_VKMARK_FLAVOR_X11
	bool
	default y if BR2_PACKAGE_XORG7
        select BR2_PACKAGE_VKMARK_FLAVOR_ANY

> +config BR2_PACKAGE_VKMARK
> +	bool "vkmark"
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_4_9 # C++14
> +	depends on BR2_PACKAGE_VKMARK_FLAVOR_ANY
> +	select BR2_PACKAGE_VULKAN_HEADERS
> +	select BR2_PACKAGE_VULKAN_LOADER

You need to replicate those dependencies:

        depends on !BR2_STATIC_LIBS # vulkan-loader
        depends on BR2_TOOLCHAIN_HAS_THREADS # vulkan-loader

> +	select BR2_PACKAGE_GLM
> +	select BR2_PACKAGE_ASSIMP

And also:

        depends on BR2_USE_WCHAR # assimp
        depends on BR2_TOOLCHAIN_GCC_AT_LEAST_7 # assimp

and perhaps for C++ you need to:

	depends on BR2_INSTALL_LIBSTDCPP # vulkan-loader, glm, assimp, vkmark

> +	select BR2_PACKAGE_WAYLAND_PROTOCOLS if BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND

	select BR2_PACKAGE_LIBXCB if BR2_PACKAGE_VKMARK_FLAVOR_X11
	select BR2_PACKAGE_LIBDRM if BR2_PACKAGE_VKMARK_FLAVOR_KMS

> +	help
> +	  vmmark is an Vulkan GPU benchmark.
> +
> +	  https://github.com/vkmark/vkmark
> +
> +comment "vkmark needs a toolchain w/ C++, dynamic library, threads"
> +	depends on !BR2_INSTALL_LIBSTDCPP || BR2_STATIC_LIBS || \
> +		!BR2_TOOLCHAIN_HAS_THREADS

Update this comment to reflect all the dependencies above. Also, only
show the comment if BR2_PACKAGE_VKMARK_FLAVOR_ANY is true.

> diff --git a/package/vkmark/vkmark.hash b/package/vkmark/vkmark.hash
> new file mode 100644
> index 0000000000..494d0c4808
> --- /dev/null
> +++ b/package/vkmark/vkmark.hash
> @@ -0,0 +1,3 @@
> +# Locally computed
> +sha256  9f106a67ce1e2aa4140bbf9325cc5837157d64f945534bc9e57286d690b08346  vkmark-2017.08.tar.gz

Hash not needed.

> +sha256  d08143e8828d5b9ed005cb6dcef4d88a49df0ac4c9e1356ace739b449c165f54  vkmark-ab6e6f34077722d5ae33f6bd40b18ef9c0e99a15.tar.gz

We need the hash of the license file.

> diff --git a/package/vkmark/vkmark.mk b/package/vkmark/vkmark.mk
> new file mode 100644
> index 0000000000..19d796a9b0
> --- /dev/null
> +++ b/package/vkmark/vkmark.mk
> @@ -0,0 +1,28 @@
> +################################################################################
> +#
> +# vkmark
> +#
> +################################################################################
> +
> +VKMARK_VERSION = ab6e6f34077722d5ae33f6bd40b18ef9c0e99a15
> +VKMARK_SITE = $(call github,vkmark,vkmark,$(VKMARK_VERSION))
> +VKMARK_LICENSE = LGPL-2.1
> +VKMARK_LICENSE_FILES = COPYING-LGPL2.1
> +VKMARK_DEPENDENCIES = host-pkgconf vulkan-headers vulkan-loader glm

No build dependency on assimp? Then why do you select it?

> +ifeq ($(BR2_PACKAGE_VKMARK_FLAVOR_KMS),y)
> +VKMARK_DEPENDENCIES += libdrm libgbm
> +VKMARK_CONF_OPTS += -Dkms=true

else
VKMARK_CONF_OPTS += -Dkms=false


> +endif
> +
> +ifeq ($(BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND),y)
> +VKMARK_DEPENDENCIES += wayland wayland-protocols
> +VKMARK_CONF_OPTS += -Dwayland=true

else
VKMARK_CONF_OPTS += -Dwayland=false


> +endif
> +
> +ifeq ($(BR2_PACKAGE_VKMARK_FLAVOR_X11),y)
> +VKMARK_DEPENDENCIES += libxcb
> +VKMARK_CONF_OPTS += -Dxcb=true

else
VKMARK_CONF_OPTS += -Dxcb=false

> +endif
> +
> +$(eval $(meson-package))

-- 
Thomas Petazzoni, co-owner and CEO, Bootlin
Embedded Linux and Kernel engineering and training
https://bootlin.com
_______________________________________________
buildroot mailing list
buildroot@buildroot.org
https://lists.buildroot.org/mailman/listinfo/buildroot