On Thu, Oct 27, 2022 at 8:57 AM Hector Martin wrote:
>
> 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 XRGB 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]RGB210101
> Cc: sta...@vger.kernel.org
> Signed-off-by: Hector Martin
There is a CC for stable on here, but this patch does not apply in any
way on 6.0 or older kernels as the fourcc bits and considerable churn
came in with the 6.1 merge window. You don't happen to have a
backport of this to 6.0 do you?
Thanks,
Justin
> ---
> 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
> XRGB, 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.
>
> Tested with KWin-X11, KWin-Wayland, GNOME-X11, GNOME-Wayland, and Weston
> on both XRGB2101010 and RGB simpledrm framebuffers.
>
> drivers/gpu/drm/drm_format_helper.c | 66 -
> 1 file changed, 47 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_format_helper.c
> b/drivers/gpu/drm/drm_format_helper.c
> index e2f76621453c..3ee59bae9d2f 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -807,6 +807,38 @@ static bool is_listed_fourcc(const uint32_t *fourccs,
> size_t nfourccs, uint32_t
> return false;
> }
>
> +static const uint32_t conv_from_xrgb[] = {
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_ARGB,
> + DRM_FORMAT_XRGB2101010,
> + DRM_FORMAT_ARGB2101010,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_RGB888,
> +};
> +
> +static const uint32_t conv_from_rgb565_888[] = {
> + DRM_FORMAT_XRGB,
> + DRM_FORMAT_ARGB,
> +};
> +
> +static bool is_conversion_supported(uint32_t from, uint32_t to)
> +{
> + switch (from) {
> + case DRM_FORMAT_XRGB:
> + case DRM_FORMAT_ARGB:
> + return is_listed_fourcc(conv_from_xrgb,
> ARRAY_SIZE(conv_from_xrgb), to);
> + case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_RGB888:
> + return is_listed_fourcc(conv_from_rgb565_888,
>
