[PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[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 <marcan@marcan.st>
---
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);
-- 
2.35.1

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Pekka Paalanen]

On Thu, 27 Oct 2022 22:57:11 +0900
Hector Martin <marcan@marcan.st> 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@vger.kernel.org
> Signed-off-by: Hector Martin <marcan@marcan.st>

Hi Hector,

I completely agree with all the rationale here.

Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>


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);

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 9420 bytes --]

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 <marcan@marcan.st>

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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?

Before removing the extra non-A formats, we first have to fix the fbdev 
console's format selection. So if users set video=-24 without native 
hardware support, simpledrm (and any other driver) falls back to a 
supported format. This worked with the old fbdev drivers, such as 
simplefb and efifb, and some users expect it.

If nothing else comes in, I'll merge your patch on Monday.

Best regards
Thomas

> 
> 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);

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 5388 bytes --]

On Fri, 28 Oct 2022 10:07:27 +0200
Thomas Zimmermann <tzimmermann@suse.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@vger.kernel.org
> > Signed-off-by: Hector Martin <marcan@marcan.st>  
> 
> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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.

You'd find this mostly in "embedded" display chips.


Thanks,
pq

> Before removing the extra non-A formats, we first have to fix the fbdev 
> console's format selection. So if users set video=-24 without native 
> hardware support, simpledrm (and any other driver) falls back to a 
> supported format. This worked with the old fbdev drivers, such as 
> simplefb and efifb, and some users expect it.
> 
> If nothing else comes in, I'll merge your patch on Monday.
> 
> Best regards
> Thomas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6214 bytes --]

Hi

Am 28.10.22 um 10:37 schrieb Pekka Paalanen:
> On Fri, 28 Oct 2022 10:07:27 +0200
> Thomas Zimmermann <tzimmermann@suse.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@vger.kernel.org
>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>
>> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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. But if we advertise XRGB and the scanout buffer is 
really ARGB, any garbage in the X filler byte would interfere.

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.

Best regards
Thomas

> 
> You'd find this mostly in "embedded" display chips.
> 
> 
> Thanks,
> pq
> 
>> Before removing the extra non-A formats, we first have to fix the fbdev
>> console's format selection. So if users set video=-24 without native
>> hardware support, simpledrm (and any other driver) falls back to a
>> supported format. This worked with the old fbdev drivers, such as
>> simplefb and efifb, and some users expect it.
>>
>> If nothing else comes in, I'll merge your patch on Monday.
>>
>> Best regards
>> Thomas

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 6590 bytes --]

On Fri, 28 Oct 2022 10:53:49 +0200
Thomas Zimmermann <tzimmermann@suse.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@suse.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@vger.kernel.org
> >>> Signed-off-by: Hector Martin <marcan@marcan.st>  
> >>
> >> Reviewed-by: Thomas Zimmermann <tzimmermann@suse.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.

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?

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.

> 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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 7330 bytes --]

Hi

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 7237 bytes --]

On Fri, 28 Oct 2022 11:34:34 +0200
Thomas Zimmermann <tzimmermann@suse.de> wrote:

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

I agree.

> > 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. ;)

I am kind of serious. See-through monitors would be cool if not
practical. They actually kind of exist already as projections through
semi-mirrors like in AR-goggles or car windscreens, but they don't yet
allow displaying opaque black (or do they?). OTOH, one could use a
traditional LCD and replace the mirror with another polarizing filter,
I guess. Maybe that's a way to implement controllable opacity. I've no
idea if anyone has combined these technologies.

Hmm, if alpha controls the opacity layer, then RGB values would behave
as if pre-multiplied in optical blending. The projected RGB primaries
can only add light on top of what passes through the LCD opacity mask.

Why don't I have one of those yet!


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Hector Martin]

On 28/10/2022 17.07, Thomas Zimmermann wrote:
> 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?

ARGB hardware probably exists in the form of embedded systems with
preconfigured blending. For example, one could imagine an OSD-type setup
where there is a hardware video scaler controlled entirely outside of
DRM/KMS (probably by a horrible vendor driver), and the overlay
framebuffer is exposed via simpledrm as a dumb memory region, and
expects ARGB to work. So ideally, we wouldn't expose XRGB8888 on
ARGB8888 systems.

But there is this problem:

arch/arm64/boot/dts/qcom/msm8998-oneplus-common.dtsi:
   format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sdm630-sony-xperia-nile.dtsi:
   format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sdm660-xiaomi-lavender.dts:
   format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sdm845-shift-axolotl.dts:
format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sdm850-samsung-w737.dts:
format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm6125-sony-xperia-seine-pdx201.dts:
           format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm6350-sony-xperia-lena-pdx213.dts:
           format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm7225-fairphone-fp4.dts:
format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm8150-sony-xperia-kumano.dtsi:
   format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm8250-sony-xperia-edo.dtsi:
   format = "a8r8g8b8";
arch/arm64/boot/dts/qcom/sm8350-sony-xperia-sagami.dtsi:
   format = "a8r8g8b8";
arch/arm64/boot/dts/socionext/uniphier-ld20-akebi96.dts:
format = "a8r8g8b8";

I'm pretty sure those phones don't have transparent screens, nor
magically put video planes below the firmware framebuffer. If there are
12 device trees for phones in mainline which lie about having alpha
support, who knows how many more exist outside? If we stop advertising
pretend-XRGB8888 on them, I suspect we're going to break a lot of
software...

Of course, there is one "correct" solution here: have an actual
xrgb8888->argb8888 conversion helper that just clears the high byte.
Then those platforms lying about having alpha and using xrgb8888 from
userspace will take a performace hit, but they should arguably just fix
their device tree in that case. Maybe this is the way to go in this
case? Note that there would be no inverse conversion (no advertising
argb8888 on xrgb8888 backends), so that one would be dropped vs. what we
have today. This effectively keeps the "xrgb8888 helpers and nothing
else" rule while actually supporting it for argb8888 backend
framebuffers correctly. Any platforms actually wanting to use argb8888
framebuffers with meaningful alpha should be configuring their userspace
to preferentially render directly to argb8888 to avoid the perf hit anyway.

- Hector

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 8420 bytes --]

Merged into drm-misc-fixes

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 <marcan@marcan.st>
> ---
> 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);

-- 
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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Justin Forbes]

On Thu, Oct 27, 2022 at 8:57 AM Hector Martin <marcan@marcan.st> 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]RGB210101


> Cc: stable@vger.kernel.org
> Signed-off-by: Hector Martin <marcan@marcan.st>

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
> 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);
> --
> 2.35.1
>

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Hector Martin]

On 01/11/2022 01.15, Justin Forbes wrote:
> On Thu, Oct 27, 2022 at 8:57 AM Hector Martin <marcan@marcan.st> 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]RGB210101
> 
> 
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Hector Martin <marcan@marcan.st>
> 
> 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?

v1 is probably closer to such a backport, and I offered to figure it out
on Matrix but I heard you're already working on it ;)

- Hector

Re: [PATCH v2] drm/format-helper: Only advertise supported formats for conversion

[Justin Forbes]

On Mon, Oct 31, 2022 at 11:52 AM Hector Martin <marcan@marcan.st> wrote:
>
> On 01/11/2022 01.15, Justin Forbes wrote:
> > On Thu, Oct 27, 2022 at 8:57 AM Hector Martin <marcan@marcan.st> 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]RGB210101
> >
> >
> >> Cc: stable@vger.kernel.org
> >> Signed-off-by: Hector Martin <marcan@marcan.st>
> >
> > 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?
>
> v1 is probably closer to such a backport, and I offered to figure it out
> on Matrix but I heard you're already working on it ;)

i am, but I didn't want to be, so I thought I would ask.

Justin