Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

From: Thomas Zimmermann
Date: Fri Oct 28 2022 - 05:34:46 EST


Hi

Am 28.10.22 um 11:17 schrieb Pekka Paalanen:
On Fri, 28 Oct 2022 10:53:49 +0200
Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:

Hi

Am 28.10.22 um 10:37 schrieb Pekka Paalanen:
On Fri, 28 Oct 2022 10:07:27 +0200
Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:
Hi

Am 27.10.22 um 15:57 schrieb Hector Martin:
drm_fb_build_fourcc_list() currently returns all emulated formats
unconditionally as long as the native format is among them, even though
not all combinations have conversion helpers. Although the list is
arguably provided to userspace in precedence order, userspace can pick
something out-of-order (and thus break when it shouldn't), or simply
only support a format that is unsupported (and thus think it can work,
which results in the appearance of a hang as FB blits fail later on,
instead of the initialization error you'd expect in this case).

Add checks to filter the list of emulated formats to only those
supported for conversion to the native format. This presumes that there
is a single native format (only the first is checked, if there are
multiple). Refactoring this API to drop the native list or support it
properly (by returning the appropriate emulated->native mapping table)
is left for a future patch.

The simpledrm driver is left as-is with a full table of emulated
formats. This keeps all currently working conversions available and
drops all the broken ones (i.e. this a strict bugfix patch, adding no
new supported formats nor removing any actually working ones). In order
to avoid proliferation of emulated formats, future drivers should
advertise only XRGB8888 as the sole emulated format (since some
userspace assumes its presence).

This fixes a real user regression where the ?RGB2101010 support commit
started advertising it unconditionally where not supported, and KWin
decided to start to use it over the native format and broke, but also
the fixes the spurious RGB565/RGB888 formats which have been wrongly
unconditionally advertised since the dawn of simpledrm.

Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Hector Martin <marcan@xxxxxxxxx>

Reviewed-by: Thomas Zimmermann <tzimmermann@xxxxxxx>

Thanks for your patch. I have verified that video=-{16,24} still works
with simpledrm.
---
I'm proposing this alternative approach after a heated discussion on
IRC. I'm out of ideas, if y'all don't like this one you can figure it
out for yourseves :-)

Changes since v1:
This v2 moves all the changes to the helper (so they will apply to
the upcoming ofdrm, though ofdrm also needs to be fixed to trim its
format table to only formats that should be emulated, probably only
XRGB8888, to avoid further proliferating the use of conversions),
and avoids touching more than one file. The API still needs cleanup
as mentioned (supporting more than one native format is fundamentally
broken, since the helper would need to tell the driver *what* native
format to use for *each* emulated format somehow), but all current and
planned users only pass in one native format, so this can (and should)
be fixed later.

Aside: After other IRC discussion, I'm testing nuking the
XRGB2101010 <-> ARGB2101010 advertisement (which does not involve
conversion) by removing those entries from simpledrm in the Asahi Linux
downstream tree. As far as I'm concerned, it can be removed if nobody
complains (by removing those entries from the simpledrm array), if
maintainers are generally okay with removing advertised formats at all.
If so, there might be other opportunities for further trimming the list
non-native formats advertised to userspace.

IMHO all of the extra A formats can immediately go. We have plenty of
simple drivers that only export XRGB8888 plus sometimes a few other
non-A formats. If anything in userspace had a hard dependency on an A
format, we'd probably heard about it.

In yesterday's discussion on IRC, it was said that several devices
advertise ARGB framebuffers when the hardware actually uses XRGB? Is
there hardware that supports transparent primary planes?

I'm fairly sure such hardware does exist, but I don't know if it's the
drivers in question here.

It's not uncommon to have extra hardware planes below the primary
plane, and then use alpha on primary plane to cut a hole to see through
to the "underlay" plane. This is a good setup for video playback, where
the video is on the underlay, and (a slow GPU or CPU renders) the
subtitles and UI on the primary plane.

I've heard that some hardware also has a separate background color
"plane" below all hardware planes, but I forget if upstream KMS exposes
that.

That's also what I heard of. It's not something we can control within
simpledrm or any other generic driver.

I'm worried that we advertise ARGB to userspace when the scanout buffer
is actually XRGB.

What would be the problem with that?
simpledrm would never expose more than only the primary plane, right?
Not even background color.

Right. My concerns are the proliferation of A format, and userspace that tries something fancy with that incorrect A byte, which leads to display artifacts. Like fading in/out the content of the primary plane.


That means that userspace cannot use the alpha channel for anything
anyway, there is nothing to show through. Or are you thinking about
transparent monitors?

I can't tell if you're serious, but I'm not going to rule this out. ;)


Of course, it would be best to advertise strictly what the hardware
does.

But if we advertise XRGB and the scanout buffer is
really ARGB, any garbage in the X filler byte would interfere.

Yes, probably. Garbage alpha being used would not hurt if a) userspace
thinks it's rendering XRGB which means that RGB values are all opaque,
and b) the hardware blending mode is pre-multiplied-alpha, and c)
whatever is behind the primary plane is all zeroes.

To my knowledge, there's no reliable way of detecting any of this. Especially not from within the hardware-agnostic code.

Best regards
Thomas


If we have a native ARGB scanout buffer, we could advertise XRGB to
userspace and set the filler byte unconditionally during the pageflip
step. That should be safe on all hardware.

Correct. Since you say "filler byte", I assume you are referring to
XRGB8888 only. That's good. Other formats should not be emulated.


Thanks,
pq

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature