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