Message-ID: <98ec2c54-cf99-6465-31a1-b63e3b4d021d@suse.de>
Date:   Fri, 28 Oct 2022 11:34:34 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Pekka Paalanen <ppaalanen@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, Hector Martin <marcan@...can.st>,
        Javier Martinez Canillas <javierm@...hat.com>,
        stable@...r.kernel.org, linux-kernel@...r.kernel.org,
        asahi@...ts.linux.dev
Subject: Re: [PATCH v2] drm/format-helper: Only advertise supported formats
 for conversion

Hi

Am 28.10.22 um 11:17 schrieb Pekka Paalanen:
> On Fri, 28 Oct 2022 10:53:49 +0200
> Thomas Zimmermann <tzimmermann@...e.de> wrote:
> 
>> Hi
>>
>> Am 28.10.22 um 10:37 schrieb Pekka Paalanen:
>>> On Fri, 28 Oct 2022 10:07:27 +0200
>>> Thomas Zimmermann <tzimmermann@...e.de> 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@...r.kernel.org
>>>>> Signed-off-by: Hector Martin <marcan@...can.st>
>>>>
>>>> Reviewed-by: Thomas Zimmermann <tzimmermann@...e.de>
>>>>
>>>> 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

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)

Powered by blists - more mailing lists