gallium: deduplicate etnaviv, freedreno & lima `screen_lookup_or_create` fun...

We really need to make this behavior common somehow.

Thanks a lot for looking into this @eric! Would the same approach work for panfrost as well, and is there a chance of talking you into expanding this MR? :P

P.S.: in case of Panfrost I could help testing and debugging the Mutter side.

Yeah, once I get it to work I'm going to de-duplicate this with existing implementations, at which point it will hopefully be easy to add it to the drivers that don't have it yet.

Speaking of getting it to work, would you be available on IRC to have a live conversation (perhaps on Monday, as it's already late here)?
I'm not sure I quite understand all the parts and how they interact.

Sure, feel free to ping me (CET here). @daenzer is probably an even better address though.

Made it into a common helper, which etnaviv, freedreno & lima now use, and added the functionality to v3d, vc4 & panfrost.

@rmader you mentioned you can test it on panfrost; I would love that :)

When testing on rpi4 the flickering (that I can reproduce by dragging the mouse in glmark2-wayland --fullscreen) is still there, but I think it might be because v3d actually uses parts of vc4 (specifically it uses vc4_drm_screen_create() + v3d_drm_screen_create_renderonly()) and things might not be deduplicated the right way between the two (but also I'm not sure what the "right way" would be). I expect things should work on other drivers, but unfortunately I don't have the hardware to test them.

This MR is a draft because of all of the above, but if you have the hardware please do test it (and report back 😜)

I think it might be because v3d actually uses parts of vc4 (specifically it uses vc4_drm_screen_create() + v3d_drm_screen_create_renderonly()) and things might not be deduplicated the right way between the two (but also I'm not sure what the "right way" would be).

In some cases like this, deduplication of screens isn't really possible. Instead, if the BO was allocated for a different DRM file description than the one referenced by the fd passed to gbm_create_device, the BO needs to be imported into that DRM file description (e.g. by exporting and re-importing a dma-buf) and the GEM handle of that re-imported BO be returned for gbm_bo_get_handle(_for_plane). See e.g. iris_bo_export_gem_handle_for_device.

Ah, that makes sense, thanks for the explanation!

I'm off work today, but I'll try to do that tomorrow :)

panfrost changes look ok to me, but I don't understand any of the WSI details (and I think I like it that way ;) ) so not going to try to review in detail.

I think if you use ro->kms_fd as your hash key, that will get the right deduping for the kmsro case too. As is, each kmsro screen create from the same display device will open a new gpu_fd

(thankyouthankyouthankyou for deduping this code!)

Ah, thanks, so basically int index_fd = ro ? ro->kms_fd : gpu_fd; and use that in util_hash_table_get() and _mesa_hash_table_insert()?

That actually brings back something I forgot to look into: in _mesa_hash_table_insert(), isn't the fd we get from the screen always going to be gpu_fd (or index_fd after the above)? I wanted to look into why the previous code was getting the fd back from the screen here instead of using the fd already there, as I expect it would always be the same one (or a dup thereof) stored in there.

I'll look into all that tomorrow, asking this both as a note to self and in case you already know the answer off the top of your head :)

Sorry, no off-the-top-of-head answers here :)

So, I checked the drivers affected by this MR: asahi, etnaviv, lima, panfrost, v3d & vc4 all store the fd itself, while freedreno stores a dup of the fd (which makes no difference in the hash table, thanks to hash_file_description()).

Dropping the fd read-back, and doing that int key_fd = ro ? ro->kms_fd : gpu_fd;.

Do we have any test that does multiple eglInitialize() (that's when screen_create() is called, right?), other than egl_khr_display_reference that we don't yet support (it's the only one I could find)? The more change I do, the less confident I get that "just run mutter and glmark on it" is enough to validate it.

(Is that a sign I should finish that MR? 😅)

I can't figure out how to properly test this, short of calling u_pipe_screen_lookup_or_create() twice in *_drm_screen_create_renderonly(), in which case I can see that the hash table works fine and the existing screen is returned the second time:

I'm calling this MR ready, but I'd be grateful if someone could point out a non-hacky way to test this (ie. without having to change the driver code) 🙂

(The latest rebase is just for panfrost & asahi's clang-format changes, no functional change.)

Ping 🙂

I would love some reviews 🔍

Subject: [PATCH 01/10] gallium/u_screen.h: add missing stdint.h include

Subject: [PATCH 02/10] gallium: move etnaviv screen_lookup_or_create function

A-b, didn't actually check the original logic though.

Subject: [PATCH 07/10] panfrost: use u_pipe_screen_lookup_or_create() to keep

nit: indentation after the clang-format rebase, other r-b

Subject: [PATCH 08/10] asahi: use u_pipe_screen_lookup_or_create() to keep

Subject: [PATCH 09/10] u_pipe_screen_lookup_or_create: avoid re-querying the fd to have a consistent hash key

This really needs an extneded commit message. What was wrong before? How does this fix it? Or is this just an optimization?

Subject: [PATCH 10/10] u_pipe_screen_lookup_or_create: use ro->kms_fd as the

Likewise, this needs an extended commit message because I can't tell what issue this is addressing without reading the gitlab discussion that prompted it.

I just saw that this is causing a crash in piglit glx-close-display:

I haven't figured out what's going on exactly, but the fact it's happening on the second run of the test which has the following comment:

Run a second to exercise reloading the *_dri.so driver.

makes me guess that this memory belongs to the previous memory block for the driver, so I'm guessing something like this:

1. test creates screen
2. screen is allocated by X, with the vtbl pointing to the driver loaded
3. test unloads the driver, reloads it
4. test creates screen
5. screen is found by u_pipe_screen_lookup_or_create() because it still exists in X's memory and is returned, with its vtbl pointing to now-unloaded memory

I'm not sure how to fix this; is there a way to test in u_pipe_screen_lookup_or_create() if the screen's vtbl is within the loaded driver's memory? I couldn't find any way to do that, but plenty of people saying it's impossible (but it's only impossible until someone does it).

@robclark @anholt could I ask you to review the freedreno commit? ff819a9b

@anarsoul @enunes could I ask you to review the lima commit? 9171a84e

For both of those, you'll want to also look at the commit that moves the etnaviv implementation to common code: 871819f8

Thanks :)

a-b for the concept.. but I'll take a closer look a bit later today when I have some free minutes

a-b for lima changes. I'm not an expert in WSI though

a-b for me too for the lima changes, other than CI I gave it some tests locally and there doesn't seem to be any changes. Thanks!

If the meson bit in the new util/kcmp patch does what I expect it to, this looks like it should work (and probably fix #5312). I don't have a setup for testing on android atm, but I'll test that for good measure on Monday.

But the patch ordering should probably change to move the util patch first (or at least before the freedreno conversion)

I had to make a small change to get this working:

I thought kcmp was available on android on chromeos, but not on "plain" android?

Anyway, I integrated your patch and moved the commit to the beginning 👍

thanks

jfwiw, I think kcmp was not blocked by seccomp with arc++ (android P), but is with arcvm (android R and later)

Thanks for the explanation, and for the r-b; does it apply to both this meson change and the freedreno commit, or only the former?

Awesome, thanks for your help!

With that, everything is reviewed or acked, so -> merging 🙂

* grumble *

windows is unhappy about _MTX_INITIALIZER_NP not existing there.

@jenatali do you know if this is something that windows could easily support/provide, or should I just put all this new common code in a big #if !windows? (pushed a fixup commit for what that would look like, if someone has an opinion)