On 2020-04-24, Brian Woodcox wrote:
> These patches add the panfrost graphics acceleration for the Pinebook
> Pro laptop.

Thanks! Been working with the pinebook pro for some months now running
guix, and it's great to see others making progress on it. :)


> You need to edit the /boot/extlinux/extlinux.conf file on the SD card and alter the FDTDIR line.
>
> I changed mine from
>
> FDTDIR /gnu/store/ls1byzmapi911cylh4s6044x0cmc61c8-linux-libre-pinebook-pro-5.6.0/lib/dtbs
>
> to
>
> FDTDIR /gnu/store/ls1byzmapi911cylh4s6044x0cmc61c8-linux-libre-pinebook-pro-5.6.0/lib/dtbs/rockchip

The u-boot-pinebook-pro-rk3399 on guix master works correctly as well as
the one from wip-pinebook-pro (should be the same).

This seems like your u-boot does not contain the correct value for
"fdtfile". It should be rockchip/rk3399-pinebook-pro.dtb. Are you
actually running an older u-boot? Did you at any point run saveenv from
u-boot, which saves the old u-boot configuration with an inappropriate
fdtfile variable?


It would be better to split up your patches into a separate patch
series, it is hard to review as one single large patch changing many
things.

A few targeted comments below...

> diff --git a/gnu/packages/gl.scm b/gnu/packages/gl.scm
> index 01241cd88e..65fe389927 100644
> --- a/gnu/packages/gl.scm
> +++ b/gnu/packages/gl.scm
> @@ -293,7 +294,7 @@ also known as DXTn or DXTC) for Mesa.")
>         '(,@(match (%current-system)
>               ((or "armhf-linux" "aarch64-linux")
>                ;; TODO: Fix svga driver for aarch64 and armhf.
> -              '("-Dgallium-drivers=etnaviv,freedreno,nouveau,r300,r600,swrast,tegra,v3d,vc4,virgl"))
> +              '("-Dgallium-drivers=etnaviv,freedreno,kmsro,lima,nouveau,panfrost,r300,r600,swrast,tegra,v3d,vc4,virgl"))
>               (_
>                '("-Dgallium-drivers=iris,nouveau,r300,r600,radeonsi,svga,swrast,virgl")))
>           ;; Enable various optional features.  TODO: opencl requires libclc,

This last part of your mesa patch is already on core-updates. Looking
forward to when the rest is properly supported upstream!


> diff --git a/gnu/packages/linux.scm b/gnu/packages/linux.scm
> index dd088ea24f..d4a36533ab 100644
> --- a/gnu/packages/linux.scm
> +++ b/gnu/packages/linux.scm
> @@ -326,7 +327,7 @@ corresponding UPSTREAM-SOURCE (an origin), using the given DEBLOB-SCRIPTS."
>                    (with-directory-excursion dir
>                      (setenv "PYTHON" (which "python"))
>                      (format #t "Running deblob script...~%")
> -                    (force-output)
> +                    (force-output))
>                      (invoke "/tmp/bin/deblob"))
> 
>                    (format #t "~%Packing new Linux-libre tarball...~%")

This looks like leftovers from your hack breaking linux-libre :P


> @@ -604,6 +605,7 @@ for ARCH and optionally VARIANT, or #f if there is no such configuration."
>      ("CONFIG_SECURITY_DMESG_RESTRICT" . #t)
>      ;; All kernels should have NAMESPACES options enabled
>      ("CONFIG_NAMESPACES" . #t)
> +    ("CONFIG_DRM_PANFROST" . #t)
>      ("CONFIG_UTS_NS" . #t)
>      ("CONFIG_IPC_NS" . #t)
>      ("CONFIG_USER_NS" . #t)

This obviously can't be enabled on all architectures. In the
linux-libre-arm64-generic and linux-libre-pinebook-pro kernels it's
already enabled as a module.

It obviously makes debugging easier to be available earlier, but it also
bloats platforms that do not use this driver.


> diff --git a/gnu/packages/patches/mesa-skip-disk-cache-test.patch b/gnu/packages/patches/mesa-skip-disk-cache-test.patch
> index 190f6b6ee1..585bf4f648 100644
> --- a/gnu/packages/patches/mesa-skip-disk-cache-test.patch
> +++ b/gnu/packages/patches/mesa-skip-disk-cache-test.patch
> @@ -1,11 +1,6 @@
> -disk_cache_create() here looks up the users home directory from <pwd.h>
> -which resolves to "/" in the build environment. I could not find an easy
> -way to set the home directory to something else, so we disable this test
> -for now.
> -
>  --- a/src/compiler/glsl/tests/cache_test.c
>  +++ b/src/compiler/glsl/tests/cache_test.c
> -@@ -170,11 +170,6 @@
> +@@ -219,11 +219,6 @@
>      unsetenv("MESA_GLSL_CACHE_DIR");
>      unsetenv("XDG_CACHE_HOME");
  
This removes a comment from the refreshed patch; I presume the comment
is still appropriate, though?


> diff --git a/gnu/packages/qt.scm b/gnu/packages/qt.scm
> index 8696dc4bb6..a1e7684964 100644
> --- a/gnu/packages/qt.scm
> +++ b/gnu/packages/qt.scm
> @@ -15,6 +15,7 @@
>  ;;; Copyright © 2018 John Soo <jsoo1@asu.edu>
>  ;;; Copyright © 2020 Mike Rosset <mike.rosset@gmail.com>
>  ;;; Copyright © 2020 Jakub Kądziołka <kuba@kadziolka.net>
> +;;; Copyright © 2020 Brian C. Woodcox <bw@InSkyData.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -485,6 +486,7 @@ developers using C++ or QML, a CSS & JavaScript like language.")
>                   "-no-compile-examples"
>                   ;; Most "-system-..." are automatic, but some use
>                   ;; the bundled copy by default.
> +                 "-opengl" "es2"
>                   "-system-sqlite"
>                   "-system-harfbuzz"
>                   "-system-pcre"

This might break some things where a different opengl is the default,
some architectures or platforms may require a different opengl
implementation.

I seem to recall some conversations in Debian about the complexities
around which opengl to enable per-architecture or per-platform or ... a
complicated matrix of concerns.


live well,
  vagrant