Re: [Buildroot] [PATCH v2 3/4] package/vkmark: add vkmark benchmarking tool

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

Hello,

I was working on vkmark, but ended up looking at your v1 that was still
in patchwork. This version fixes a number of issues, but I still have
questions/topics.

On Thu,  5 Dec 2024 17:36:42 +0000
Alex Bennée <alex.bennee@linaro.org> wrote:

> We build from the current master as we need fairly upto date bits for
> diff --git a/package/vkmark/Config.in b/package/vkmark/Config.in
> new file mode 100644
> index 0000000000..3e899225ed
> --- /dev/null
> +++ b/package/vkmark/Config.in
> @@ -0,0 +1,46 @@

This file still has a whole bunch of tab vs. spaces issues. You can run
"make check-package" to figure out these issues. They are also visible
in the diff below.

> +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_PACKAGE_VKMARK_FLAVOR_ANY
> +	select BR2_PACKAGE_VULKAN_HEADERS
> +	select BR2_PACKAGE_VULKAN_LOADER
> +        depends on !BR2_STATIC_LIBS # vulkan-loader
> +        depends on BR2_TOOLCHAIN_HAS_THREADS # vulkan-loader
> +	select BR2_PACKAGE_GLM
> +        depends on BR2_USE_WCHAR # assimp
> +        depends on BR2_TOOLCHAIN_GCC_AT_LEAST_7 # assimp
> +	depends on BR2_INSTALL_LIBSTDCPP # vulkan-loader, glm, assimp, vkmark
> +	select BR2_PACKAGE_ASSIMP
> +	select BR2_PACKAGE_WAYLAND_PROTOCOLS if BR2_PACKAGE_VKMARK_FLAVOR_WAYLAND
> +	select BR2_PACKAGE_LIBXCB if BR2_PACKAGE_VKMARK_FLAVOR_X11
> +	select BR2_PACKAGE_XCB_UTIL_WM 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
> +
> +if BR2_PACKAGE_VKMARK_FLAVOR_ANY
> +
> +comment "vkmark needs a toolchain w/ C++, dynamic library, threads"
> +comment "and some form of backend (wayland, xcb or drm)"
> +	depends on !BR2_INSTALL_LIBSTDCPP || BR2_STATIC_LIBS || \
> +		!BR2_TOOLCHAIN_HAS_THREADS
> +endif

Please replace this with:

+comment "vkmark needs a toolchain w/ dynamic library, threads, wchar, gcc >= 7, C++"
+       depends on BR2_STATIC_LIBS || \
+               !BR2_TOOLCHAIN_HAS_THREADS || \
+               !BR2_USE_WCHAR || \
+               !BR2_TOOLCHAIN_GCC_AT_LEAST_7 || \
+               !BR2_INSTALL_LIBSTDCPP
+
+comment "vkmark needs a backend: gbm, wayland or xcb"
+       depends on !BR2_PACKAGE_VKMARK_FLAVOR_ANY

Indeed mentioning in your Config.in comment that it needs some form of
backend... and have the comment within a BR2_PACKAGE_VKMARK_FLAVOR_ANY
condition doesn't make much sense.

> diff --git a/package/vkmark/vkmark.hash b/package/vkmark/vkmark.hash
> new file mode 100644
> index 0000000000..ee8241d7de
> --- /dev/null
> +++ b/package/vkmark/vkmark.hash
> @@ -0,0 +1,2 @@
> +# Locally computed
> +sha256  d08143e8828d5b9ed005cb6dcef4d88a49df0ac4c9e1356ace739b449c165f54  vkmark-ab6e6f34077722d5ae33f6bd40b18ef9c0e99a15.tar.gz

The hash of the license file is missing.

> diff --git a/package/vkmark/vkmark.mk b/package/vkmark/vkmark.mk
> new file mode 100644
> index 0000000000..d82fb42e43
> --- /dev/null
> +++ b/package/vkmark/vkmark.mk
> @@ -0,0 +1,34 @@
> +################################################################################
> +#
> +# 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 assimp

Minor nit: alphabetic ordering is preferred.

> +ifeq ($(BR2_PACKAGE_VKMARK_FLAVOR_KMS),y)
> +VKMARK_DEPENDENCIES += libdrm libgbm

So here, I don't understand how you are able to get a configuration
that produces libgbm. Indeed, BR2_PACKAGE_MESA3D_GBM can only be enable
if BR2_PACKAGE_MESA3D_GALLIUM_DRIVER is true (but the vulkan VirtIO
driver is not a Gallium driver) and BR2_PACKAGE_MESA3D_OPENGL_EGL is
set (and we don't have it because we're using Vulkan).

Could you clarify?

Thanks!

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