Re: [Buildroot] [PATCH] package/nvidia-driver: bump version to 550.78

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

Hello Nick,

Thanks for your patch, a number of comments/questions below.

On Thu,  9 May 2024 15:00:27 -0400
Nick Whitlock <nick.whitlock@eizo.com> wrote:

> A configuration value was also added to check if the user wants
> to install GTK integration for the NVIDIA driver.
> 
> This has been confirmed to work on kernel 6.14.

Kernel 6.14 ? You're ahead of your time, there's no such thing as Linux
6.14.

We need your patch to have your Signed-off-by line, otherwise we
unfortunately cannot apply it.

> ---
>  package/nvidia-driver/Config.in          |  4 ++
>  package/nvidia-driver/nvidia-driver.hash |  8 +--
>  package/nvidia-driver/nvidia-driver.mk   | 72 +++++++++++++-----------
>  3 files changed, 47 insertions(+), 37 deletions(-)
> 
> diff --git a/package/nvidia-driver/Config.in b/package/nvidia-driver/Config.in
> index a8617a939b..996f2f6eea 100644
> --- a/package/nvidia-driver/Config.in
> +++ b/package/nvidia-driver/Config.in
> @@ -90,4 +90,8 @@ config BR2_PACKAGE_NVIDIA_DRIVER_MODULE
>  	  provides Unified Memory access to the GPU and CPU memories for
>  	  CUDA programs.
>  
> +config BR2_PACKAGE_NVIDIA_DRIVER_GTK
> +	bool "GTK support"
> +	depends on BR2_PACKAGE_NVIDIA_DRIVER_MODULE

Why does this have a depends on BR2_PACKAGE_NVIDIA_DRIVER_MODULE ? From
what I can see in your patch, this option is merely installing more
user-space libraries, so why do you make this depend on the driver
module being enabled?

> diff --git a/package/nvidia-driver/nvidia-driver.hash b/package/nvidia-driver/nvidia-driver.hash
> index 620112e6c8..163a168dae 100644
> --- a/package/nvidia-driver/nvidia-driver.hash
> +++ b/package/nvidia-driver/nvidia-driver.hash
> @@ -1,4 +1,4 @@
> -# Locally computed
> -sha256  94e399b459659c12b1344e8c8f4f5eee1ed5915ff459fc8bb831c9e1d44677db  NVIDIA-Linux-x86-390.151.run
> -sha256  6e4fd2258465f5d5527fe80abd46de925a30348b4a84658498a6d75caf42c47c  NVIDIA-Linux-x86_64-390.151-no-compat32.run
> -sha256  bd28b0c5aeeb00eb11d3ec6f6f3449d4b3a40100914258332734a53527997526  LICENSE
> +# Locally computed:
> +md5 c289987ebda8e9419a73e7e8e3409244  NVIDIA-Linux-x86_64-550.78.run
> +sha1 4e33f1e9d6274fdef630a3a1b903901c09149e40  NVIDIA-Linux-x86_64-550.78.run
> +sha256 34070434527ec9d575483e7f11ca078e467e73f6defc54366ecfbdcfe4a3bf73 NVIDIA-Linux-x86_64-550.78.run

A number of things are not correct:

- You should keep the license file hashes

- You should keep the spacing: 2 spaces between each field

- Are these hashes provided by some authoritative source upstream? If
  so, indicate where. Otherwise, keeping the locally calculated sha256
  is sufficient.

> -NVIDIA_DRIVER_VERSION = 390.151
> -NVIDIA_DRIVER_SUFFIX = $(if $(BR2_x86_64),_64)
> -NVIDIA_DRIVER_SITE = http://download.nvidia.com/XFree86/Linux-x86$(NVIDIA_DRIVER_SUFFIX)/$(NVIDIA_DRIVER_VERSION)
> -NVIDIA_DRIVER_SOURCE = NVIDIA-Linux-x86$(NVIDIA_DRIVER_SUFFIX)-$(NVIDIA_DRIVER_VERSION)$(if $(BR2_x86_64),-no-compat32).run
> +# NVIDIA drivers now are bundled 64-bit and 32-bit together in one .run file.

This should be in the commit log, not in a comment here.

> +NVIDIA_DRIVER_VERSION = 550.78
> +NVIDIA_DRIVER_SITE = http://download.nvidia.com/XFree86/Linux-x86_64/$(NVIDIA_DRIVER_VERSION)
> +NVIDIA_DRIVER_SOURCE = NVIDIA-Linux-x86_64-$(NVIDIA_DRIVER_VERSION).run
>  NVIDIA_DRIVER_LICENSE = NVIDIA Software License
>  NVIDIA_DRIVER_LICENSE_FILES = LICENSE
>  NVIDIA_DRIVER_REDISTRIBUTE = NO
> @@ -23,30 +23,15 @@ ifeq ($(BR2_PACKAGE_NVIDIA_DRIVER_XORG),y)
>  NVIDIA_DRIVER_DEPENDENCIES += mesa3d-headers xlib_libX11 xlib_libXext
>  NVIDIA_DRIVER_PROVIDES += libgl libegl libgles
>  
> -# libGL.so.$(NVIDIA_DRIVER_VERSION) is the legacy libGL.so library; it
> -# has been replaced with libGL.so.1.0.0. Installing both is technically
> -# possible, but great care must be taken to ensure they do not conflict,
> -# so that EGL still works. The legacy library exposes an NVidia-specific
> -# API, so it should not be needed, except for legacy, binary-only
> -# applications (in other words: we don't care).
> -#
> -# libGL.so.1.0.0 is the new vendor-neutral library, aimed at replacing
> -# the old libGL.so.$(NVIDIA_DRIVER_VERSION) library. The latter contains
> -# NVidia extensions (which is deemed bad now), while the former follows
> -# the newly-introduced vendor-neutral "dispatching" API/ABI:
> -#   https://github.com/aritger/linux-opengl-abi-proposal/blob/master/linux-opengl-abi-proposal.txt
> -# However, this is not very useful to us, as we don't support multiple
> -# GL providers at the same time on the system, which this proposal is
> -# aimed at supporting.
> -#
> -# So we only install the legacy library for now.

You probably need to explain in the commit log why this is all going
away.


>  NVIDIA_DRIVER_LIBS += \
>  	$(NVIDIA_DRIVER_LIBS_GL) \
> @@ -87,13 +77,26 @@ endef
>  # wants to run a third-party program developed under such an agreement).
>  ifeq ($(BR2_PACKAGE_NVIDIA_DRIVER_PRIVATE_LIBS),y)
>  NVIDIA_DRIVER_LIBS += \
> -	libnvidia-ifr.so.$(NVIDIA_DRIVER_VERSION) \
> -	libnvidia-fbc.so.$(NVIDIA_DRIVER_VERSION)
> +	libnvidia-fbc.so.$(NVIDIA_DRIVER_VERSION) /
> +	libnvidia-ifr.so.$(NVIDIA_DRIVER_VERSION) /
> +	libnvidia-ngx.so.$(NVIDIA_DRIVER_VERSION) /
> +	libnvidia-opticalflow.so.$(NVIDIA_DRIVER_VERSION) /

These lines end with slashes instead of backslashes, not sure how this
can work. Did you test this part?

Could you fix those different issues, and send a v2 of this patch?

Thanks a lot!

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