more drm format selection fixes #15797

Dudemanguy · 2025-02-03T17:52:05Z

Take 3? cc @mahkoh and @ikalco if you want to take a look real quick.

mahkoh · 2025-02-03T17:57:20Z

video/out/wayland_common.c

+                if (modifier != DRM_FORMAT_MOD_INVALID) {
                    return true;
+                } else {
+                    if (tranche->drm_plane_formats) {
+                        for (int j = 0; j < tranche->num_drm_plane_formats; ++j) {
+                            if (drm_format == tranche->drm_plane_formats[j])
+                                return true;
+                        }
+                    }
+                }


I don't yet understand the purpose of this section. If you are seeing issues with the invalid modifier, why does checking if a plane supports the format fix that issue?

My understanding is that support for a format with implicit modifiers will be advertised on a plane. From compositors, I get format + invalid modifier pairs but sometimes the format in question isn't advertised on the plane (e.g. NV12). Whenever this happens, I get broken rendering.

I might actually be just working around some bug somewhere and I shouldn't be getting that particular format + modifier pair in the first place. I get it from pretty much every compositor so it might be something deep in the stack.

I believe that this is just a coincidence. That being said, there is no harm in this check (at most some inefficiency) if you fall back to another format if it fails.

Yeah, it will select something else if the format in question fails.

My understanding is that support for a format with implicit modifiers will be advertised on a plane.

What? the supported compositor formats are the ones sent by the linux-dmabuf protocol for each individual tranche

the supported compositor formats are sent in 2 types of tranches,

renderer tranche (usually default), is based on supported formats from EGL (the renderer), and is for importing DMABUFs into EGL so that compositor can sample from the buffer as a texture, including external formats and implicit

direct-scanout tranche, is based on the supported formats of one the output's/gpu's DRM planes (could be primary or overlay), and is used for direct-scanout from client buf to DRM plane for an efficient zero-copy presentation

the client has no way of knowing which plane the direct-scanout tranche is for,
so it shouldn't be checking drm plane formats for comparability

I get format + invalid modifier pairs but sometimes the format in question isn't advertised on the plane (e.g. NV12). Whenever this happens, I get broken rendering.

if this is in reference to the mpv output looking red tinted, then that is Hyprland's fault
we are sampling mpv's NV12 DMABUFs as an RGB texture

hmm, well it depends how the compositors advertise NV12

if its in a renderer tranche, then it's probably supported such that the compositor's renderer will convert the nv12 to whatever format the output drm plane is actually using, usually rgbx or rgba

if its in a direct-scanout tranche, then that means that there exists a drm plane, for which this tranche is for, which can scanout NV12

if the direct-scanout tranche is used with NV12, and there appears to be a "rendering error" (its actually drm scanout but whatever), then that is a compositor issue or deeper driver issue like you said

edit: to clarify, I'm confused as to how mpv checking a drm planes formats could help because it could be any plane on that device that the tranche is representing

Currently only the first primary plane we find is checked. I suppose we could check all of them but I'm not sure if that's redundant or not. At least for me, the formats are always the same across the primary planes.

I think we shouldn't mix workarounds with other detection logic without a clear comment and explanation of why they are required and how they work. Preferably, an upstream issue should be linked as a reference. This feels similar to the "vc-1" case, where we were stubbornly working around an issue that "no one would ever fix" instead of properly reporting it upstream, where it was, in fact, fixed within a day.

I'm not saying this code has to be changed, but we need to ensure that it does not have adverse effects in some cases. Based on the comments here, it's unclear whether it behaves as intended or if it only coincidentally filters out certain formats. Regardless, some comments there would be nice to avoid discussing this again in v4 patch in half a year.

I will try to deep dive later and find out what precisely is happening on this device of mine before committing to this.

I am pretty confident that what I have been seeing all along on my end is actually just a mesa bug now. I've gone ahead and pushed the version of this change that should (hopefully) strip out all the unneeded code. Will follow up later with a more detailed explanation once I find the root cause.

github-actions · 2025-02-03T18:09:47Z

Download the artifacts for this pull request:

Windows

macOS

jeeb

Otherwise LGTM, but I feel like the reworked vo_wayland_valid_format could be made simpler to read by utilizing early exit :) .

jeeb · 2025-02-03T18:33:37Z

video/out/wayland_common.c

                    return true;
+                } else {


The nice thing about there being an early return in the if case is that you do not require this else here :)

jeeb · 2025-02-03T18:38:28Z

video/out/wayland_common.c

                    return true;
+                } else {
+                    if (tranche->planar_formats) {


If the case is that you only care about one specific format, which then ends up having DRM_FORMAT_MOD_INVALID and no planar formats, it seems like you could early exit with a if (!tranche->planar_formats) { return false; }, and then just move onto the for loop?

if (!tranche->planar_formats) continue;

jeeb · 2025-02-03T18:50:57Z

video/out/wayland_common.c

-        if (supported_compositor_format && tranche->planar_formats) {
-            for (int i = 0; i < tranche->num_planar_formats; i++) {
-                if (drm_format == tranche->planar_formats[i])
+            if (modifier == formats[i].modifier) {


Switching to early exit with if (modifier != formats[i].modifier) continue; would enable you to simplify further code. This also clarifies that the loop continuation is on purpose (same format may be in the tranche multiple times with different modifiers).

ikalco · 2025-02-03T19:48:24Z

does vaapi support outputting implicit modifiers? if it doesn't then you can't use implicit through linux-dmabuf
cause you can't import an explicit modifier buffer from vaapi into the compositor as an implicit modifier buffer

from the kernel docs, https://docs.kernel.org/userspace-api/dma-buf-alloc-exchange.html

It follows from this that for any single buffer, the complete chain of operations formed by the producer and all the consumers must be either fully implicit or fully explicit. For example, if a user wishes to allocate a buffer for use between GPU, display, and media, but the media API does not support modifiers, then the user must not allocate the buffer with explicit modifiers and attempt to import the buffer into the media API with no modifier, but either perform the allocation using implicit modifiers, or allocate the buffer for media use separately and copy between the two buffers.

edit:
im only putting this here in case you know of mpv trying to do the above, in which case it could help fix an issue
otherwise this lgtm, except the drm plane format checking ofcourse

Dudemanguy · 2025-02-03T20:38:23Z

does vaapi support outputting implicit modifiers?

I believe so. I did a test on sway while hardcoding the invalid mod in zwp_linux_buffer_params_v1_add and it output correctly.

llyyr · 2025-02-05T14:28:42Z

Can also revert 32d103c now

No longer needed since the relevant code was removed in the previous commit. This reverts commit 32d103c.

dmabuf_interop_wl: simplify modifier logging

b57e176

mahkoh reviewed Feb 3, 2025

View reviewed changes

jeeb requested changes Feb 3, 2025

View reviewed changes

Dudemanguy force-pushed the more-drm-format-selection-fixes branch 2 times, most recently from 12081a3 to 6472732 Compare February 3, 2025 19:40

wayland: fix implicit modifier selecting mess (again)

57bdc4c

Dudemanguy force-pushed the more-drm-format-selection-fixes branch from 6472732 to 57bdc4c Compare February 5, 2025 04:54

Revert "meson: bump libdrm version to 2.4.109"

6936479

No longer needed since the relevant code was removed in the previous commit. This reverts commit 32d103c.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more drm format selection fixes #15797

more drm format selection fixes #15797

Dudemanguy commented Feb 3, 2025

mahkoh Feb 3, 2025

Dudemanguy Feb 3, 2025 •

edited

Loading

mahkoh Feb 3, 2025

Dudemanguy Feb 3, 2025

ikalco Feb 3, 2025

ikalco Feb 3, 2025 •

edited

Loading

Dudemanguy Feb 3, 2025

kasper93 Feb 3, 2025

Dudemanguy Feb 3, 2025

Dudemanguy Feb 5, 2025

github-actions bot commented Feb 3, 2025 •

edited

Loading

jeeb left a comment

jeeb Feb 3, 2025

jeeb Feb 3, 2025

kasper93 Feb 3, 2025

jeeb Feb 3, 2025

ikalco commented Feb 3, 2025 •

edited

Loading

Dudemanguy commented Feb 3, 2025

llyyr commented Feb 5, 2025

more drm format selection fixes #15797

Are you sure you want to change the base?

more drm format selection fixes #15797

Conversation

Dudemanguy commented Feb 3, 2025

Choose a reason for hiding this comment

Dudemanguy Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikalco Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 3, 2025 • edited Loading

jeeb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikalco commented Feb 3, 2025 • edited Loading

Dudemanguy commented Feb 3, 2025

llyyr commented Feb 5, 2025

Dudemanguy Feb 3, 2025 •

edited

Loading

ikalco Feb 3, 2025 •

edited

Loading

github-actions bot commented Feb 3, 2025 •

edited

Loading

ikalco commented Feb 3, 2025 •

edited

Loading