Re: [PATCH] drm/simpledrm: Only advertise formats that are supported

[Hector Martin]

On 27/10/2022 19.13, Hector Martin wrote:
> Until now, simpledrm unconditionally advertised all formats that can be
> supported natively as conversions. However, we don't actually have a
> full conversion matrix of 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).
> 
> Split up the format table into separate ones for each required subset,
> and then pick one based on the native format. Also remove the
> native<->conversion overlap check from the helper (which doesn't make
> sense any more, since the native format is advertised anyway and this
> way RGB565/RGB888 can share a format table), and instead print the same
> message in simpledrm when the native format is not one for which we have
> conversions at all.
> 
> 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, but also the fixes
> the spurious RGB565/RGB888 formats which have been wrongly
> unconditionally advertised since the dawn of simpledrm.
> 
> Note: this patch is merged because splitting it into two patches, one
> for the helper and one for simpledrm, would regress at the midpoint
> regardless of the order. If simpledrm is changed first, that would break
> working conversions to RGB565/RGB888 (since those share a table that
> does not include the native formats). If the helper is changed first, it
> would start spuriously advertising all conversion formats when the
> native format doesn't have any supported conversions at all.
> 
> Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
> Fixes: 6ea966fca084 ("drm/simpledrm: Add [AX]RGB2101010 formats")
> Fixes: 11e8f5fd223b ("drm: Add simpledrm driver")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Hector Martin <marcan@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_format_helper.c | 15 -------
>  drivers/gpu/drm/tiny/simpledrm.c    | 62 +++++++++++++++++++++++++----
>  2 files changed, 55 insertions(+), 22 deletions(-)
> 

To answer some issues that came up on IRC:

Q: Why not move this logic / the tables to the helper?
A: Because simpledrm is the only user so far, and this patch is Cc:
stable because we have an actual regression that broke KDE. I'm going
for the minimal patch that keeps everything that worked to this day
working, and stops advertising things that never worked, no more, no
less. Future refactoring can always happen later (and is probably a good
idea when other drivers start using the helper).

Q: XRGB8888 is supposed to be the only canonical format. Why not just
drop everything but conversions to/from XRGB8888?
A: Because that would regress things that work today, and could break
existing userspace on some platforms. That may be a good idea, but I
think we should fix the bugs first, and leave the discussion of whether
we want to actually remove existing functionality for later.

Q: Why not just add a conversion from XRGB2101010 to XRGB8888?
A: Because that would only fix KDE, and would make it slower vs. not
advertising XRGB2101010 at all (double conversions, plus kernel
conversion can be slower). Plus, it doesn't make any sense as it only
fills in one entry in the conversion matrix. If we wanted to actually
fill out the conversion matrix, and thus support everything simpledrm
has advertised to day correctly, we would need helpers for:

rgb565->rgb888
rgb888->rgb565
rgb565->xrgb2101010
rgb888->xrgb2101010
xrgb2101010->rgb565
xrgb2101010->rgb888
xrgb2101010->xrgb8888

That seems like overkill and unlikely to actually help anyone, it'd just
give userspace more options to shoot itself in the foot with a
sub-optimal format choice. And it's a pile of code.

- Hector