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

[Pekka Paalanen]

On Fri, 28 Oct 2022 11:34:34 +0200
Thomas Zimmermann <tzimmermann@xxxxxxx> wrote:

> 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.

I agree.

> > 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. ;)

I am kind of serious. See-through monitors would be cool if not
practical. They actually kind of exist already as projections through
semi-mirrors like in AR-goggles or car windscreens, but they don't yet
allow displaying opaque black (or do they?). OTOH, one could use a
traditional LCD and replace the mirror with another polarizing filter,
I guess. Maybe that's a way to implement controllable opacity. I've no
idea if anyone has combined these technologies.

Hmm, if alpha controls the opacity layer, then RGB values would behave
as if pre-multiplied in optical blending. The projected RGB primaries
can only add light on top of what passes through the LCD opacity mask.

Why don't I have one of those yet!


Thanks,
pq

Attachment: pgp7jwh1cNSrp.pgp
Description: OpenPGP digital signature