[Mesa-dev] [PATCH] util/disk_cache: Fix disk_cache_get_function

Post by Bas Nieuwenhuizen
radv always needs it, so just check the header instead. Also
do not declare the function if the variable is not set, so we
get a nice compile error instead of failing to open a device
at runtime.

If that's the goal, why have any guards? Just #include the header,
that's your compilation error if it's missing :)

Post by Bas Nieuwenhuizen
Fixes: b87ef9e606a "util: fix MSVC build issue in disk_cache.h"
---
configure.ac | 1 +
meson.build | 2 +-
src/util/disk_cache.h | 8 +++-----
3 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/configure.ac b/configure.ac
index c946454cfae..ffb8424a07b 100644
--- a/configure.ac
+++ b/configure.ac
@@ -872,6 +872,7 @@ AC_HEADER_MAJOR
AC_CHECK_HEADER([xlocale.h], [DEFINES="$DEFINES -DHAVE_XLOCALE_H"])
AC_CHECK_HEADER([sys/sysctl.h], [DEFINES="$DEFINES -DHAVE_SYS_SYSCTL_H"])
AC_CHECK_HEADERS([endian.h])
+AC_CHECK_HEADER([dlfcn.h], [DEFINES="$DEFINES -DHAVE_DLFCN_H"])
AC_CHECK_FUNC([strtof], [DEFINES="$DEFINES -DHAVE_STRTOF"])
AC_CHECK_FUNC([mkostemp], [DEFINES="$DEFINES -DHAVE_MKOSTEMP"])
AC_CHECK_FUNC([timespec_get], [DEFINES="$DEFINES -DHAVE_TIMESPEC_GET"])
diff --git a/meson.build b/meson.build
index e05645cbf39..86a4a4ce6da 100644
--- a/meson.build
+++ b/meson.build
@@ -960,7 +960,7 @@ elif cc.has_header_symbol('sys/mkdev.h', 'major')
pre_args += '-DMAJOR_IN_MKDEV'
endif
-foreach h : ['xlocale.h', 'sys/sysctl.h', 'linux/futex.h', 'endian.h']
+foreach h : ['xlocale.h', 'sys/sysctl.h', 'linux/futex.h', 'endian.h', 'dlfcn.h']
endif
diff --git a/src/util/disk_cache.h b/src/util/disk_cache.h
index f84840fb5ca..50bd9f41ac4 100644
--- a/src/util/disk_cache.h
+++ b/src/util/disk_cache.h
@@ -24,7 +24,7 @@
#ifndef DISK_CACHE_H
#define DISK_CACHE_H
-#ifdef ENABLE_SHADER_CACHE
+#ifdef HAVE_DLFCN_H
#include <dlfcn.h>
#endif
#include <assert.h>
@@ -88,10 +88,10 @@ disk_cache_format_hex_id(char *buf, const uint8_t *hex_id, unsigned size)
return buf;
}
+#ifdef HAVE_DLFCN_H
static inline bool
disk_cache_get_function_timestamp(void *ptr, uint32_t* timestamp)
{
-#ifdef ENABLE_SHADER_CACHE
Dl_info info;
struct stat st;
if (!dladdr(ptr, &info) || !info.dli_fname) {
@@ -102,10 +102,8 @@ disk_cache_get_function_timestamp(void *ptr, uint32_t* timestamp)
}
*timestamp = st.st_mtime;
return true;
-#else
- return false;
-#endif
}
+#endif
/* Provide inlined stub functions if the shader cache is disabled. */
--
2.18.0
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Bas Nieuwenhuizen

2018-07-19 19:39:05 UTC

On Thu, Jul 19, 2018 at 6:48 PM, Eric Engestrom

If that's the goal, why have any guards? Just #include the header,
that's your compilation error if it's missing :)

Well the goal is only to introduce a compile error if we do not have
it, but any of the users of the function gets build. Notably this
function only gets used in radv,radeonsi,r600g and nouveau, while the
disk_cache.h gets included also from e.g. the glsl compiler which is
an issue for e.g. windows.

radv works fine without the disk cache enabled, but still needs this
function or it fails device creation (a non-disk cache is part of the
vulkan API).

Eric Engestrom

2018-07-20 18:10:01 UTC

Post by Bas Nieuwenhuizen
On Thu, Jul 19, 2018 at 6:48 PM, Eric Engestrom

If that's the goal, why have any guards? Just #include the header,
that's your compilation error if it's missing :)

Well the goal is only to introduce a compile error if we do not have
it, but any of the users of the function gets build. Notably this
function only gets used in radv,radeonsi,r600g and nouveau, while the
disk_cache.h gets included also from e.g. the glsl compiler which is
an issue for e.g. windows.
radv works fine without the disk cache enabled, but still needs this
function or it fails device creation (a non-disk cache is part of the
vulkan API).

Oh right, you want to still be able to #include "disk_cache.h" in places
that don't need `disk_cache_get_function_timestamp()`.

Then this patch look correct to me; sorry for the confusion earlier.

Emil Velikov

2018-07-26 15:01:27 UTC

Hi Bas,

Pardon for joining so late.

AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
Back in the early days we had --enable-shader-cache which was removed
with d1efa09d342bff3e5def2978a0bef748d74f9c82

Apart from the reasons listed, we do not want to control this at
configure time since it will produce varying driver behaviour. Hence
multiple unhappy users, as they hit the codepath w/o cache.

Skimming through - I would imagine you're building with meson, which
has the explicit shader cache toggle.
I'm assuming Dylan had it based on early autconf builds?

We can drop that one and effectively revert this and the follow-up Android fix.

What do you think?
Emil

Bas Nieuwenhuizen

2018-07-26 15:15:56 UTC

Post by Emil Velikov
Hi Bas,
Pardon for joining so late.

AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
Back in the early days we had --enable-shader-cache which was removed
with d1efa09d342bff3e5def2978a0bef748d74f9c82
Apart from the reasons listed, we do not want to control this at
configure time since it will produce varying driver behaviour. Hence
multiple unhappy users, as they hit the codepath w/o cache.
Skimming through - I would imagine you're building with meson, which
has the explicit shader cache toggle.
I'm assuming Dylan had it based on early autconf builds?
We can drop that one and effectively revert this and the follow-up Android fix.
What do you think?

ChromeOS disables it for reasons I'm currently not sure about:

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/mesa-18.2_pre1.ebuild#197

And since the primary use case for vulkan at the moment seems to be
Android and I can conceive us not being able to use the cache there
for reasons, I'm not sure if disabling the cache is really that
unreasonable, though it is not ideal that they apparently override the
upstream configure.

Since most of that stuff seems to be driven by GL, this is pretty much
a "isolate radv from whatever people do on the GL side of things"
patch.

Bas Nieuwenhuizen

2018-07-26 15:25:22 UTC

On Thu, Jul 26, 2018 at 5:15 PM, Bas Nieuwenhuizen

Post by Emil Velikov
Hi Bas,
Pardon for joining so late.

AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
Back in the early days we had --enable-shader-cache which was removed
with d1efa09d342bff3e5def2978a0bef748d74f9c82
Apart from the reasons listed, we do not want to control this at
configure time since it will produce varying driver behaviour. Hence
multiple unhappy users, as they hit the codepath w/o cache.
Skimming through - I would imagine you're building with meson, which
has the explicit shader cache toggle.
I'm assuming Dylan had it based on early autconf builds?
We can drop that one and effectively revert this and the follow-up Android fix.
What do you think?

https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/master/media-libs/mesa/mesa-18.2_pre1.ebuild#197
And since the primary use case for vulkan at the moment seems to be
Android and I can conceive us not being able to use the cache there
for reasons, I'm not sure if disabling the cache is really that
unreasonable, though it is not ideal that they apparently override the
upstream configure.

To give a bit more context on why it was disabled:

"Starting with commit 96fe36f7 in mesa, the disk shader cache has been
enabled. This feature attempts to use multi-threading during GPU
initialization while Chromium enforces single threading during the
initialization process. The conflict causes the system to hang during
boot. This patchset includes the following line in the non-9999 ebuild
file, which disables the feature and allows platforms to boot:

append-flags "-UENABLE_SHADER_CACHE"
"
(https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1056288)

Post by Bas Nieuwenhuizen
Since most of that stuff seems to be driven by GL, this is pretty much
a "isolate radv from whatever people do on the GL side of things"
patch.

Emil Velikov

2018-07-26 15:55:57 UTC

Post by Bas Nieuwenhuizen
On Thu, Jul 26, 2018 at 5:15 PM, Bas Nieuwenhuizen

Post by Emil Velikov
Hi Bas,
Pardon for joining so late.

AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
Back in the early days we had --enable-shader-cache which was removed
with d1efa09d342bff3e5def2978a0bef748d74f9c82
Apart from the reasons listed, we do not want to control this at
configure time since it will produce varying driver behaviour. Hence
multiple unhappy users, as they hit the codepath w/o cache.
Skimming through - I would imagine you're building with meson, which
has the explicit shader cache toggle.
I'm assuming Dylan had it based on early autconf builds?
We can drop that one and effectively revert this and the follow-up Android fix.
What do you think?

"Starting with commit 96fe36f7 in mesa, the disk shader cache has been
enabled. This feature attempts to use multi-threading during GPU
initialization while Chromium enforces single threading during the
initialization process. The conflict causes the system to hang during
boot. This patchset includes the following line in the non-9999 ebuild
append-flags "-UENABLE_SHADER_CACHE"
"
(https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/1056288)

Huge thanks for the context. Did you get that from the bug report -
seems to be missing from gerrit.

Regardless, the solution seems like a good first step, although
something better is needed in the long run.

Perhaps, forcing the system to fail on thread_create and adding error
handling for util_queue_init() in src/util/disk_cache.c?
Thus anything spawning threads will get the same consistent treatment
and all of these workarounds can go.

HTH
Emil

Post by Bas Nieuwenhuizen
Since most of that stuff seems to be driven by GL, this is pretty much
a "isolate radv from whatever people do on the GL side of things"
patch.

Bas Nieuwenhuizen

2018-07-26 16:24:00 UTC

Post by Emil Velikov

Post by Bas Nieuwenhuizen
On Thu, Jul 26, 2018 at 5:15 PM, Bas Nieuwenhuizen

Post by Emil Velikov
Hi Bas,
Pardon for joining so late.

AFAICT the ENABLE_SHADER_CACHE define is set on all POSIX platforms.
Back in the early days we had --enable-shader-cache which was removed
with d1efa09d342bff3e5def2978a0bef748d74f9c82
Apart from the reasons listed, we do not want to control this at
configure time since it will produce varying driver behaviour. Hence
multiple unhappy users, as they hit the codepath w/o cache.
Skimming through - I would imagine you're building with meson, which
has the explicit shader cache toggle.
I'm assuming Dylan had it based on early autconf builds?
We can drop that one and effectively revert this and the follow-up Android fix.
What do you think?

Huge thanks for the context. Did you get that from the bug report -
seems to be missing from gerrit.

Nope, gerrit, just need to click on the "SHOW ALL 51 MESSAGES" button.

Post by Emil Velikov
Regardless, the solution seems like a good first step, although
something better is needed in the long run.
Perhaps, forcing the system to fail on thread_create and adding error
handling for util_queue_init() in src/util/disk_cache.c?
Thus anything spawning threads will get the same consistent treatment
and all of these workarounds can go.

It is my understanding that it does not fail thread creation, but if
at sandboxing time it detects multiple threads it kills the process.
Which unfortunately turns this into a game of "you are not allowed to
create any threads but we are not telling you", which is somewhat
annoying to fix without configuration.

Not familiar enough yet with the how and why of sandboxing this way or
why we open a GL context before that though.

Post by Emil Velikov
HTH
Emil

Post by Bas Nieuwenhuizen
Since most of that stuff seems to be driven by GL, this is pretty much
a "isolate radv from whatever people do on the GL side of things"
patch.