On Thu, 27 Oct 2022 22:57:11 +0900
Hector Martin <marcan@xxxxxxxxx> 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 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>
Hi Hector,
I completely agree with all the rationale here.
Acked-by: Pekka Paalanen <pekka.paalanen@xxxxxxxxxxxxx>
Thanks,
pq
> ---
> 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.
>
> Tested with KWin-X11, KWin-Wayland, GNOME-X11, GNOME-Wayland, and Weston
> on both XRGB2101010 and RGB8888 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_xrgb8888[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> + DRM_FORMAT_XRGB2101010,
> + DRM_FORMAT_ARGB2101010,
> + DRM_FORMAT_RGB565,
> + DRM_FORMAT_RGB888,
> +};
> +
> +static const uint32_t conv_from_rgb565_888[] = {
> + DRM_FORMAT_XRGB8888,
> + DRM_FORMAT_ARGB8888,
> +};
> +
> +static bool is_conversion_supported(uint32_t from, uint32_t to)
> +{
> + switch (from) {
> + case DRM_FORMAT_XRGB8888:
> + case DRM_FORMAT_ARGB8888:
> + return is_listed_fourcc(conv_from_xrgb8888, ARRAY_SIZE(conv_from_xrgb8888), to);
> + case DRM_FORMAT_RGB565:
> + case DRM_FORMAT_RGB888:
> + return is_listed_fourcc(conv_from_rgb565_888, ARRAY_SIZE(conv_from_rgb565_888), to);
> + case DRM_FORMAT_XRGB2101010:
> + return to == DRM_FORMAT_ARGB2101010;
> + case DRM_FORMAT_ARGB2101010:
> + return to == DRM_FORMAT_XRGB2101010;
> + default:
> + return false;
> + }
> +}
> +
> /**
> * drm_fb_build_fourcc_list - Filters a list of supported color formats against
> * the device's native formats
> @@ -827,7 +859,9 @@ static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t
> * be handed over to drm_universal_plane_init() et al. Native formats
> * will go before emulated formats. Other heuristics might be applied
> * to optimize the order. Formats near the beginning of the list are
> - * usually preferred over formats near the end of the list.
> + * usually preferred over formats near the end of the list. Formats
> + * without conversion helpers will be skipped. New drivers should only
> + * pass in XRGB8888 and avoid exposing additional emulated formats.
> *
> * Returns:
> * The number of color-formats 4CC codes returned in @fourccs_out.
> @@ -839,7 +873,7 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> {
> u32 *fourccs = fourccs_out;
> const u32 *fourccs_end = fourccs_out + nfourccs_out;
> - bool found_native = false;
> + uint32_t native_format = 0;
> size_t i;
>
> /*
> @@ -858,26 +892,18 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
>
> drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
>
> - if (!found_native)
> - found_native = is_listed_fourcc(driver_fourccs, driver_nfourccs, fourcc);
> + /*
> + * There should only be one native format with the current API.
> + * This API needs to be refactored to correctly support arbitrary
> + * sets of native formats, since it needs to report which native
> + * format to use for each emulated format.
> + */
> + if (!native_format)
> + native_format = fourcc;
> *fourccs = fourcc;
> ++fourccs;
> }
>
> - /*
> - * The plane's atomic_update helper converts the framebuffer's color format
> - * to a native format when copying to device memory.
> - *
> - * If there is not a single format supported by both, device and
> - * driver, the native formats are likely not supported by the conversion
> - * helpers. Therefore *only* support the native formats and add a
> - * conversion helper ASAP.
> - */
> - if (!found_native) {
> - drm_warn(dev, "Format conversion helpers required to add extra formats.\n");
> - goto out;
> - }
> -
> /*
> * The extra formats, emulated by the driver, go second.
> */
> @@ -890,6 +916,9 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> } else if (fourccs == fourccs_end) {
> drm_warn(dev, "Ignoring emulated format %p4cc\n", &fourcc);
> continue; /* end of available output buffer */
> + } else if (!is_conversion_supported(fourcc, native_format)) {
> + drm_dbg_kms(dev, "Unsupported emulated format %p4cc\n", &fourcc);
> + continue; /* format is not supported for conversion */
> }
>
> drm_dbg_kms(dev, "adding emulated format %p4cc\n", &fourcc);
> @@ -898,7 +927,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
> ++fourccs;
> }
>
> -out:
> return fourccs - fourccs_out;
> }
> EXPORT_SYMBOL(drm_fb_build_fourcc_list);
