Merge ~b-stolk/chromium-browser/+git/snap-from-source:checkdriverpath into ~chromium-team/chromium-browser/+git/snap-from-source:hwacc-beta

[Bram Stolk]

Avoid un-expanded DRI_DRIVER_DIR macro.

If DRI_DRIVER_DIR is not defined, as currently is the case in
our build, then the chrome binary with minigbm support will
attempt to load DRI_DRIVER_DIR/radeonsi_dri.so without expanding
the variable. This has severe consequences.
This patch will set it to /usr/lib/x86_64-linux-gnu/dri if the
macro is not defined.

Fixes LP:2004586

This change can be applied to all hwacc branches.

[Nathan Teodosio (nteodosio)]

This merge request patches amdgpu.c directly to "hard-define" DRI_DRIVER_DIR, which a single direct step away from the direct patch your partner proposed in the description of the linked bug report.

LP:2004586 --->
> But it is better to understand why this expansion failed.
> And it is possibly fixed in later chromium releases, if not, we should look how our snap building differs from the official Linux building instructions for chromium.
<---

If we want to better understand why the expansion failed, we need to actually understand why DRI_DRIVER_DIR is not defined, and thus why build/config/linux/pkg-config.py fails to find dridriverdir. Do you agree?

LP:2004586 --->
> I've verified that this can be fixed by adding a configuration to minigbm's BUILD.gn
<---

What would that fix be?

review: Needs Information

[Bram Stolk (b-stolk)]

Last time I tried, I had issues doing the proper fix.

There is a file called:

build/config/linux/dri/BUILD.gn

which should have taken care of the definition for DRI_DRIVER_DIR

But this file is not referenced.
I added a reference for the minigbm component, which did add a define, but it ended up being doubly-quoted somehow.

I will re-run that experiment, and log the results here.

Yes, I agree, a proper fix is better.

[Bram Stolk (b-stolk)]

Adding:
   configs += [ "//build/config/linux/dri" ]
To minigbm/BUILD.gn will cause DRI_DRIVER_DIR to be defined.

This ends up in the ninja defines:

-DDRI_DRIVER_DIR=\"/usr/lib/x86_64-linux-gnu/dri\"

Note the escaped quotes.

This leads to using the path

"/usr/lib/x86_64-linux-gnu/dri"/radeonsi_dri.so

..to end up in the binary, and used for loading.

There are two things wrong w that:

1) The quotes make it into the path.
2) The path is incorrect: we should be using /snap/gnome-3-38-2004/current/usr/lib/x86_64-linux-gnu/dri instead.

[Nathan Teodosio (nteodosio)]

Thanks for investigating this further, Bram.

> But this file is not referenced.

Ah, that was it, then!

> The path is incorrect: we should be using /snap/gnome-3-38-2004/current/usr/lib/x86_64-linux-gnu/dri instead.

But we do stage mesa-va-drivers, which provides /usr/lib/x86_64-linux-gnu/dri/radeonsi_drv_video.so. We haven't been using the GNOME SDK ones so far for our hwacc improvements.

[Nathan Teodosio (nteodosio)]

Let's close this as it was superseded by merge request 437819.

Unmerged commits

a291062... by Bram Stolk on 2023-02-17

Avoid un-expanded DRI_DRIVER_DIR macro.

If DRI_DRIVER_DIR is not defined, as currently is the case in
our build, then the chrome binary with minigbm support will
attempt to load DRI_DRIVER_DIR/radeonsi_dri.so without expanding
the variable. This has severe consequences.
This patch will set it to /usr/lib/x86_64-linux-gnu/dri if the
macro is not defined.

Fixes LP:2004586

diff --git a/build/chromium-patches/extra:885da2c:check-for-empty-define.diff b/build/chromium-patches/extra:885da2c:check-for-empty-define.diff
 new file mode 100644
index 0000000..0ca9c6a
--- /dev/null
+++ b/build/chromium-patches/extra:885da2c:check-for-empty-define.diff
@@ -0,0 +1,28 @@
+From 885da2c4dbeffd789bf81a4f609b9cf8d9b5d136 Mon Sep 17 00:00:00 2001
+From: Bram Stolk <[e-mail]>
+Date: Fri, 17 Feb 2023 09:20:03 -0800
+Subject: [PATCH] Make sure that DRI_DRIVER_DIR exists.
+
+When DRI_DRIVER_DIR is not defined, it cannot be expanded.
+This causes minigbm to load drivers from a non existing path.
+Instead of /usr/lib/x86_64-linux-gnu/dri being used, the library
+tries to load the file DRI_DRIVER_DIR/radeonsi_dri.so
+
+Change-Id: I8888b919057e375cf57d5ba488d75f7c4ea25df4
+---
+
+diff --git a/amdgpu.c b/amdgpu.c
+index 5c891de..d3ee1eb 100644
+--- a/amdgpu.c
++++ b/amdgpu.c
+@@ -23,6 +23,10 @@
+ #include "drv_priv.h"
+ #include "util.h"
+ 
++#if !defined(DRI_DRIVER_DIR)
++#	define DRI_DRIVER_DIR /usr/lib/x86_64-linux-gnu/dri
++#endif
++
+ // clang-format off
+ #define DRI_PATH STRINGIZE(DRI_DRIVER_DIR/radeonsi_dri.so)
+ // clang-format on
diff --git a/build/chromium-patches/series b/build/chromium-patches/series
index 9f7583b..b32895b 100644
--- a/build/chromium-patches/series
+++ b/build/chromium-patches/series
@@ -1,3 +1,4 @@
+extra:885da2c:check-for-empty-define.diff
 extra:fe38fdb:dynamic-check-nv12.diff
 guide-0.9.1:a45401f:vaapi-flag-ozone-wayland.diff
 guide-0.9.1:b49ca2f:one-copy.diff