-
-
Notifications
You must be signed in to change notification settings - Fork 14.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WIP: Mesa cache fix #93946
WIP: Mesa cache fix #93946
Conversation
The mesa patch should probably go upstream. I'll look into that when I get a chance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really related to #92807? Your case seems to differ since that issue was fixed by the update to Mesa 20.1.3 which didn't fix it for you. So you seem to be hitting another issue.
@@ -57,7 +57,7 @@ stdenv.mkDerivation { | |||
patches = [ | |||
./missing-includes.patch # dev_t needs sys/stat.h, time_t needs time.h, etc.-- fixes build w/musl | |||
./opencl-install-dir.patch | |||
./disk_cache-include-dri-driver-path-in-cache-key.patch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check carefully if this patch is indeed not required anymore. From a quick look at the source-code it looks ok, but I haven't checked all drivers yet. Some still refer to the parameter as *timestamp
(inconsistently even src/util/disk_cache.h
) while using the build-id.
I guess I'm mainly concerned since #91145 apparently didn't affect all drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a somewhat thorough check. All the dri and vulkan drivers have build-id.
lib/dri/radeonsi_drv_video.so
does not have build-id, and does contain si_disk_cache_create
. I'm not sure if that's a problem. None of the _drv_video.so
libs or vdpau libs have build-id.
The problem with the cache key patch is that it only affects the actual disk cache keys, not other uses of cache_uuid
, such as the one in radv.
I'd be happy just to merge a23c6888d399b291ba691e43028e1b295ad10efc while this gets investigated more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be the only driver that still uses timestamps directly:
https://gitlab.freedesktop.org/mesa/mesa/-/blob/master/src/freedreno/vulkan/tu_device.c#L61
I'm not sure if it's the same problem everyone in that thread was having, but this comment mentions |
Now that mesa uses build-id for cache keys, this shouldn't be necessary.
This is used by mesa drivers as a cache key. llvmPackages_9 is currently used by mesa in nixpkgs, so we just enable it there and in llvmPackages_10 so it doesn't get lost in future versions.
Ok, yeah it seems like multiple things got mixed up in that issue. I've gone ahead and pushed the important fix into staging 535a3e8 and rebased your branch. Regarding the rest of the commits:
|
How is this related to patchelf 0.11? ATM we're still using 0.9 for mesa drivers because of that bug (and I can't see this PR changing that). |
Well I guess the concern was patchelf 0.11 being using on libLLVM? I did build llvm 9 and 10, and the notes are intact. I also stepped through the uuid code to verify it gets used.
|
Yes, exactly.
Awesome, thanks :) @vcunat do you have an opinion / any comments regarding ff4481b? I think it's the way to go but I cannot really estimate the potential regressions. The main drivers should all be fine but as @corngood noted here not everything is using a build-id yet. If we encounter regressions we could then fix them properly (mesa shouldn't rely on any timestamps for reproducible builds and they seem to agree) but it might be difficult for users to track these regressions down (which is my main concern as I know how annoying graphics bugs can unfortunately be). But I don't have any objections if anyone would like to merge this right away, I just don't want to make that decision alone with my limited knowledge. (And I guess Mesa updates regularly come with some regressions/bugs anyway, so I might be just overthinking this.) |
I've never really looked into details around that patch. |
@primeos I marked this WIP because I'm also concerned about regressions in ff4481b.
|
This is used by mesa.drivers (still on LLVM 9) as a cache key. I've ported that change to LLVM 11 to test it and so that it doesn't get lost in future versions. Credit for the change goes to David McFarland. See #93946 for details. Co-Authored-By: David McFarland <corngood@gmail.com>
It's been a while, this PR can probably be closed as outdated since upstream has updated after merging the fix from here already. @corngood unless I missed something in the discussion? Admittedly I didn't follow it too closely. |
@jansol yeah, we don't need this PR. I'll close it. Thanks. |
This is used by mesa.drivers (still on LLVM 9) as a cache key. I've ported that change to LLVM 11 to test it and so that it doesn't get lost in future versions. Credit for the change goes to David McFarland. See NixOS#93946 for details. Co-Authored-By: David McFarland <corngood@gmail.com>
Motivation for this change
Fixes #92807
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)