Re: [Buildroot] [PATCH/next v4 1/2] package/onevpl: new package

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

On Tue,  8 Aug 2023 08:41:06 +0200
Bernd Kuhls <bernd@kuhls.net> wrote:

> From: Bernd Kuhls <bernd.kuhls@t-online.de>
> 
> Signed-off-by: Bernd Kuhls <bernd@kuhls.net>

I've applied to next, but I have a few comments nevertheless, see below.

> diff --git a/package/onevpl/0001-Don-t-force-fstack-protector.patch b/package/onevpl/0001-Don-t-force-fstack-protector.patch
> new file mode 100644
> index 0000000000..14948e28cc
> --- /dev/null
> +++ b/package/onevpl/0001-Don-t-force-fstack-protector.patch
> @@ -0,0 +1,32 @@
> +From 682a9eabc71c2e33ebbbba2e75d0ba7caa08c7c8 Mon Sep 17 00:00:00 2001
> +From: Bernd Kuhls <bernd.kuhls@t-online.de>
> +Date: Mon, 8 May 2023 20:03:22 +0200
> +Subject: [PATCH] Don't force -fstack-protector
> +
> +This allows the environment to decide whether SSP should be used or
> +not, for example to support toolchains that don't have SSP support.
> +
> +Upstream: Not applicable

I'd like to challenge the fact that it's not applicable upstream. You
could add a CMake option that allows to enable/disable
-fstack-protector-strong, enabled by default so that it preserves
upstream behavior while allowing us to do what we think needs to be
done.

> diff --git a/package/onevpl/Config.in b/package/onevpl/Config.in
> new file mode 100644
> index 0000000000..65a34185e3
> --- /dev/null
> +++ b/package/onevpl/Config.in
> @@ -0,0 +1,26 @@
> +config BR2_PACKAGE_ONEVPL_ARCH_SUPPORTS
> +	bool
> +	default y if BR2_ARCH_IS_64
> +	default y if BR2_arm

Could you clarify where this architecture dependency comes from? Would
be good to add a comment about it (through a follow-up patch).

> +config BR2_PACKAGE_ONEVPL
> +	bool "onevpl"
> +	depends on BR2_PACKAGE_ONEVPL_ARCH_SUPPORTS
> +	depends on BR2_INSTALL_LIBSTDCPP
> +	depends on !BR2_STATIC_LIBS # dlfcn.h
> +	depends on BR2_TOOLCHAIN_GCC_AT_LEAST_7
> +	depends on BR2_TOOLCHAIN_HAS_THREADS
> +	select BR2_PACKAGE_WAYLAND_PROTOCOLS if BR2_PACKAGE_WAYLAND
> +	select BR2_PACKAGE_LIBDRM_INTEL if BR2_PACKAGE_LIBVA

I added a short comment above this select because it was not obvious
why it was needed, as we don't depend on libdrm. Turns out that for the
VA stuff in OneVPL to work, it needs libdrm to have been built with the
intel driver in.

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