[PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Raphael GALLAIS-POU - foss]

Hello,

This series aims to add a generic background color property for CRTC as
discussed in the conversation:
https://patchwork.freedesktop.org/patch/439585/

This patch "drm: add crtc background color property" is strongly inspired
of Matthew Roper's.  Please see:
https://patchwork.freedesktop.org/patch/333632/

Raphael Gallais-Pou (2):
  drm: add crtc background color property
  drm/stm: ltdc: add crtc background color property support

 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c         |  4 ++
 drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++-
 drivers/gpu/drm/drm_mode_config.c         |  6 +++
 drivers/gpu/drm/stm/ltdc.c                | 48 ++++++++++++++++++++---
 include/drm/drm_blend.h                   |  1 +
 include/drm/drm_crtc.h                    | 12 ++++++
 include/drm/drm_mode_config.h             |  5 +++
 include/uapi/drm/drm_mode.h               | 28 +++++++++++++
 9 files changed, 132 insertions(+), 7 deletions(-)

-- 
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/2] drm: add crtc background color property

[Raphael GALLAIS-POU - foss]

Some display controllers can be programmed to present non-black colors
for pixels not covered by any plane (or pixels covered by the
transparent regions of higher planes).  Compositors that want a UI with
a solid color background can potentially save memory bandwidth by
setting the CRTC background property and using smaller planes to display
the rest of the content.

To avoid confusion between different ways of encoding RGB data, we
define a standard 64-bit format that should be used for this property's
value.  Helper functions and macros are provided to generate and dissect
values in this standard format with varying component precision values.

Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
 drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
 drivers/gpu/drm/drm_mode_config.c         |  6 ++++
 include/drm/drm_blend.h                   |  1 +
 include/drm/drm_crtc.h                    | 12 ++++++++
 include/drm/drm_mode_config.h             |  5 ++++
 include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
 8 files changed, 89 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index ddcf5c2c8e6a..1b95a4ecdb2b 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
 				     struct drm_crtc *crtc)
 {
 	crtc_state->crtc = crtc;
+	crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
 }
 EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
 
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 438e9585b225..fea68f8f17f8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
 		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
 	} else if (property == crtc->scaling_filter_property) {
 		state->scaling_filter = val;
+	} else if (property == config->bgcolor_property) {
+		state->bgcolor = val;
 	} else if (crtc->funcs->atomic_set_property) {
 		return crtc->funcs->atomic_set_property(crtc, state, property, val);
 	} else {
@@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
 		*val = 0;
 	else if (property == crtc->scaling_filter_property)
 		*val = state->scaling_filter;
+	else if (property == config->bgcolor_property)
+		*val = state->bgcolor;
 	else if (crtc->funcs->atomic_get_property)
 		return crtc->funcs->atomic_get_property(crtc, state, property, val);
 	else
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index ec37cbfabb50..6692d6a6db22 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -186,8 +186,7 @@
  *		 assumed to be 1.0
  *
  * Note that all the property extensions described here apply either to the
- * plane or the CRTC (e.g. for the background color, which currently is not
- * exposed and assumed to be black).
+ * plane or the CRTC.
  *
  * SCALING_FILTER:
  *     Indicates scaling filter to be used for plane scaler
@@ -201,6 +200,21 @@
  *
  * Drivers can set up this property for a plane by calling
  * drm_plane_create_scaling_filter_property
+ *
+ * BACKGROUND_COLOR:
+ *	Defines the ARGB color of a full-screen layer that exists below all
+ *	planes.  This color will be used for pixels not covered by any plane
+ *	and may also be blended with plane contents as allowed by a plane's
+ *	alpha values.  The background color defaults to black, and is assumed
+ *	to be black for drivers that do not expose this property.  Although
+ *	background color isn't a plane, it is assumed that the color provided
+ *	here undergoes the same pipe-level degamma/CSC/gamma transformations
+ *	that planes undergo.  Note that the color value provided here includes
+ *	an alpha channel...non-opaque background color values are allowed,
+ *	but are generally only honored in special cases (e.g., when a memory
+ *	writeback connector is in use).
+ *
+ *	This property is setup with drm_crtc_add_bgcolor_property().
  */
 
 /**
@@ -616,3 +630,19 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 	return 0;
 }
 EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
+
+/**
+ * drm_crtc_add_bgcolor_property - add background color property
+ * @crtc: drm crtc
+ *
+ * Adds the background color property to @crtc.  The property defaults to
+ * solid black and will accept 64-bit ARGB values in the format generated by
+ * drm_argb().
+ */
+void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc)
+{
+	drm_object_attach_property(&crtc->base,
+				   crtc->dev->mode_config.bgcolor_property,
+				   drm_argb(16, 0xffff, 0, 0, 0));
+}
+EXPORT_SYMBOL(drm_crtc_add_bgcolor_property);
diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
index 37b4b9f0e468..d62d6585399b 100644
--- a/drivers/gpu/drm/drm_mode_config.c
+++ b/drivers/gpu/drm/drm_mode_config.c
@@ -371,6 +371,12 @@ static int drm_mode_create_standard_properties(struct drm_device *dev)
 		return -ENOMEM;
 	dev->mode_config.modifiers_property = prop;
 
+	prop = drm_property_create_range(dev, 0, "BACKGROUND_COLOR",
+					 0, GENMASK_ULL(63, 0));
+	if (!prop)
+		return -ENOMEM;
+	dev->mode_config.bgcolor_property = prop;
+
 	return 0;
 }
 
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 88bdfec3bd88..9e2538dd7b9a 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -58,4 +58,5 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
 			      struct drm_atomic_state *state);
 int drm_plane_create_blend_mode_property(struct drm_plane *plane,
 					 unsigned int supported_modes);
+void drm_crtc_add_bgcolor_property(struct drm_crtc *crtc);
 #endif
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 13eeba2a750a..12601eb63c45 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -288,6 +288,18 @@ struct drm_crtc_state {
 	 */
 	struct drm_property_blob *gamma_lut;
 
+	/**
+	 * @bgcolor:
+	 *
+	 * RGB value representing the pipe's background color.  The background
+	 * color (aka "canvas color") of a pipe is the color that will be used
+	 * for pixels not covered by a plane, or covered by transparent pixels
+	 * of a plane.  The value here should be built via drm_argb();
+	 * individual color components can be extracted with desired precision
+	 * via the DRM_ARGB_*() macros.
+	 */
+	u64 bgcolor;
+
 	/**
 	 * @target_vblank:
 	 *
diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
index 1ddf7783fdf7..76c491d10d8d 100644
--- a/include/drm/drm_mode_config.h
+++ b/include/drm/drm_mode_config.h
@@ -867,6 +867,11 @@ struct drm_mode_config {
 	 */
 	struct drm_property *hdcp_content_type_property;
 
+	/**
+	 * @bgcolor_property: RGB background color for CRTC.
+	 */
+	struct drm_property *bgcolor_property;
+
 	/* dumb ioctl parameters */
 	uint32_t preferred_depth, prefer_shadow;
 
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 98bf130feda5..035f06c6750e 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -1154,6 +1154,34 @@ struct drm_mode_rect {
 	__s32 y2;
 };
 
+/*
+ * Put ARGB values into a standard 64-bit representation that can be used
+ * for ioctl parameters, inter-driver commmunication, etc.  If the component
+ * values being provided contain less than 16 bits of precision, they'll
+ * be shifted into the most significant bits.
+ */
+static inline __u64
+drm_argb(__u8 bpc, __u16 alpha, __u16 red, __u16 green, __u16 blue)
+{
+	int msb_shift = 16 - bpc;
+
+	return (__u64)alpha << msb_shift << 48 |
+	       (__u64)red   << msb_shift << 32 |
+	       (__u64)green << msb_shift << 16 |
+	       (__u64)blue  << msb_shift;
+}
+
+/*
+ * Extract the specified number of bits of a specific color component from a
+ * standard 64-bit ARGB value.
+ */
+#define DRM_ARGB_COMP(c, shift, numbits) \
+	((__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits))))
+#define DRM_ARGB_BLUE(c, numbits)  DRM_ARGB_COMP(c, 0, numbits)
+#define DRM_ARGB_GREEN(c, numbits) DRM_ARGB_COMP(c, 16, numbits)
+#define DRM_ARGB_RED(c, numbits)   DRM_ARGB_COMP(c, 32, numbits)
+#define DRM_ARGB_ALPHA(c, numbits) DRM_ARGB_COMP(c, 48, numbits)
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Pekka Paalanen]

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

On Wed, 7 Jul 2021 08:48:47 +0000
Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:

> Some display controllers can be programmed to present non-black colors
> for pixels not covered by any plane (or pixels covered by the
> transparent regions of higher planes).  Compositors that want a UI with
> a solid color background can potentially save memory bandwidth by
> setting the CRTC background property and using smaller planes to display
> the rest of the content.
> 
> To avoid confusion between different ways of encoding RGB data, we
> define a standard 64-bit format that should be used for this property's
> value.  Helper functions and macros are provided to generate and dissect
> values in this standard format with varying component precision values.
> 
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>  drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>  drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>  include/drm/drm_blend.h                   |  1 +
>  include/drm/drm_crtc.h                    | 12 ++++++++
>  include/drm/drm_mode_config.h             |  5 ++++
>  include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>  8 files changed, 89 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> index ddcf5c2c8e6a..1b95a4ecdb2b 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
>  				     struct drm_crtc *crtc)
>  {
>  	crtc_state->crtc = crtc;
> +	crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
>  
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> index 438e9585b225..fea68f8f17f8 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>  		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>  	} else if (property == crtc->scaling_filter_property) {
>  		state->scaling_filter = val;
> +	} else if (property == config->bgcolor_property) {
> +		state->bgcolor = val;
>  	} else if (crtc->funcs->atomic_set_property) {
>  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>  	} else {
> @@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>  		*val = 0;
>  	else if (property == crtc->scaling_filter_property)
>  		*val = state->scaling_filter;
> +	else if (property == config->bgcolor_property)
> +		*val = state->bgcolor;
>  	else if (crtc->funcs->atomic_get_property)
>  		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>  	else
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index ec37cbfabb50..6692d6a6db22 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -186,8 +186,7 @@
>   *		 assumed to be 1.0
>   *
>   * Note that all the property extensions described here apply either to the
> - * plane or the CRTC (e.g. for the background color, which currently is not
> - * exposed and assumed to be black).
> + * plane or the CRTC.
>   *
>   * SCALING_FILTER:
>   *     Indicates scaling filter to be used for plane scaler
> @@ -201,6 +200,21 @@
>   *
>   * Drivers can set up this property for a plane by calling
>   * drm_plane_create_scaling_filter_property
> + *

Hi,

I assume the below block is the UAPI documentation of this new property
and that it would appear with the other UAPI docs.

> + * BACKGROUND_COLOR:
> + *	Defines the ARGB color of a full-screen layer that exists below all
> + *	planes.  This color will be used for pixels not covered by any plane
> + *	and may also be blended with plane contents as allowed by a plane's
> + *	alpha values.  The background color defaults to black, and is assumed
> + *	to be black for drivers that do not expose this property.

All good up to here.

>  Although
> + *	background color isn't a plane, it is assumed that the color provided
> + *	here undergoes the same pipe-level degamma/CSC/gamma transformations
> + *	that planes undergo.

This sounds to me like it refers to the per-plane degamma/csc/gamma
which are new properties in development. I believe you do not mean
that, but you mean the CRTC degamma/csc/gamma and everything else which
apply *after* the blending of planes. So the wording here would need
clarification.

>  Note that the color value provided here includes
> + *	an alpha channel...non-opaque background color values are allowed,
> + *	but are generally only honored in special cases (e.g., when a memory
> + *	writeback connector is in use).

This could be read as: if you use a non-opaque color value, it will
usually be completely ignored (and the background will be e.g. black
instead). Is that your intention?

I think a more useful definition would be that the alpha is used in
blending as always, but because we do not yet have physically
transparent monitors, the final alpha value may not reach the video
sink or the video sink may ignore it.

> + *
> + *	This property is setup with drm_crtc_add_bgcolor_property().

You forgot to document the value format of this property. The ARGB color
format needs to be defined at least to the same detail as all pixel
formats in drm_fourcc.h are. If there is a suitable DRM_FORMAT_*
definition already, simply saying the color is in that format would be
enough.

Another thing to document is whether this color value is alpha
pre-multiplied or not. Planes can have the property "pixel blend mode",
but because the background color is not on a plane, that property
cannot apply here.

The difference it makes is that if background color is both non-opaque
and pre-multiplied, then the question arises what pixel values will
actually be transmitted to video sink for the background. Will the
alpha pre-multiplication be undone?

Btw. note that "pixel blend mode" property does not document the use of
background alpha at all. So if the background color can have non-opaque
alpha, then you need to document the behavior in "pixel blend mode". It
also does not document what alpha value will result from blending, for
blending the next plane.

The question about full vs. limited range seems unnecessary to me, as
the background color will be used as-is in the blending stage, so
userspace can just program the correct value that fits the pipeline it
is setting up.

One more question is, as HDR exists, could we need background colors
with component values greater than 1.0?

Scanout of FP16 formats exists.

>   */


Thanks,
pq

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Raphael Gallais-Pou]

On 7/9/21 10:04 AM, Pekka Paalanen wrote:
> On Wed, 7 Jul 2021 08:48:47 +0000
> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
>
>> Some display controllers can be programmed to present non-black colors
>> for pixels not covered by any plane (or pixels covered by the
>> transparent regions of higher planes).  Compositors that want a UI with
>> a solid color background can potentially save memory bandwidth by
>> setting the CRTC background property and using smaller planes to display
>> the rest of the content.
>>
>> To avoid confusion between different ways of encoding RGB data, we
>> define a standard 64-bit format that should be used for this property's
>> value.  Helper functions and macros are provided to generate and dissect
>> values in this standard format with varying component precision values.
>>
>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>> ---
>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>>   include/drm/drm_blend.h                   |  1 +
>>   include/drm/drm_crtc.h                    | 12 ++++++++
>>   include/drm/drm_mode_config.h             |  5 ++++
>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>>   8 files changed, 89 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>> index ddcf5c2c8e6a..1b95a4ecdb2b 100644
>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>> @@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
>>   				     struct drm_crtc *crtc)
>>   {
>>   	crtc_state->crtc = crtc;
>> +	crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
>>   }
>>   EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
>>   
>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>> index 438e9585b225..fea68f8f17f8 100644
>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>> @@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>   		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>>   	} else if (property == crtc->scaling_filter_property) {
>>   		state->scaling_filter = val;
>> +	} else if (property == config->bgcolor_property) {
>> +		state->bgcolor = val;
>>   	} else if (crtc->funcs->atomic_set_property) {
>>   		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>>   	} else {
>> @@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>   		*val = 0;
>>   	else if (property == crtc->scaling_filter_property)
>>   		*val = state->scaling_filter;
>> +	else if (property == config->bgcolor_property)
>> +		*val = state->bgcolor;
>>   	else if (crtc->funcs->atomic_get_property)
>>   		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>>   	else
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index ec37cbfabb50..6692d6a6db22 100644
>> --- a/drivers/gpu/drm/drm_blend.c
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -186,8 +186,7 @@
>>    *		 assumed to be 1.0
>>    *
>>    * Note that all the property extensions described here apply either to the
>> - * plane or the CRTC (e.g. for the background color, which currently is not
>> - * exposed and assumed to be black).
>> + * plane or the CRTC.
>>    *
>>    * SCALING_FILTER:
>>    *     Indicates scaling filter to be used for plane scaler
>> @@ -201,6 +200,21 @@
>>    *
>>    * Drivers can set up this property for a plane by calling
>>    * drm_plane_create_scaling_filter_property
>> + *
> Hi,


Hi Pekka,


Many thanks for your feedback, your comments are taken into account for 
a v2.


>
> I assume the below block is the UAPI documentation of this new property
> and that it would appear with the other UAPI docs.
>
>> + * BACKGROUND_COLOR:
>> + *	Defines the ARGB color of a full-screen layer that exists below all
>> + *	planes.  This color will be used for pixels not covered by any plane
>> + *	and may also be blended with plane contents as allowed by a plane's
>> + *	alpha values.  The background color defaults to black, and is assumed
>> + *	to be black for drivers that do not expose this property.
> All good up to here.
>
>>   Although
>> + *	background color isn't a plane, it is assumed that the color provided
>> + *	here undergoes the same pipe-level degamma/CSC/gamma transformations
>> + *	that planes undergo.
> This sounds to me like it refers to the per-plane degamma/csc/gamma
> which are new properties in development. I believe you do not mean
> that, but you mean the CRTC degamma/csc/gamma and everything else which
> apply *after* the blending of planes. So the wording here would need
> clarification.


Yes, I was not sure about this, but it is effectively the general CRTC 
color correction which is applicable to the background color.

>
>>   Note that the color value provided here includes
>> + *	an alpha channel...non-opaque background color values are allowed,
>> + *	but are generally only honored in special cases (e.g., when a memory
>> + *	writeback connector is in use).
> This could be read as: if you use a non-opaque color value, it will
> usually be completely ignored (and the background will be e.g. black
> instead). Is that your intention?
>
> I think a more useful definition would be that the alpha is used in
> blending as always, but because we do not yet have physically
> transparent monitors, the final alpha value may not reach the video
> sink or the video sink may ignore it.

In our case, the hardware does not support alpha channel (as you can see 
the DRM_ARGB_TO_LTDC_RGB24 macro in the second patch).

For chip vendors who does support this feature, the video sink would get 
this transparency parameter. In the case where it is not, alpha channel 
would be ignored.


>> + *
>> + *	This property is setup with drm_crtc_add_bgcolor_property().
> You forgot to document the value format of this property. The ARGB color
> format needs to be defined at least to the same detail as all pixel
> formats in drm_fourcc.h are. If there is a suitable DRM_FORMAT_*
> definition already, simply saying the color is in that format would be
> enough.


Will do ! :)

I was thinking about the FourCC AR4H format. Have you something else in 
mind ?

>
> Another thing to document is whether this color value is alpha
> pre-multiplied or not. Planes can have the property "pixel blend mode",
> but because the background color is not on a plane, that property
> cannot apply here.
>
> The difference it makes is that if background color is both non-opaque
> and pre-multiplied, then the question arises what pixel values will
> actually be transmitted to video sink for the background. Will the
> alpha pre-multiplication be undone?
>
> Btw. note that "pixel blend mode" property does not document the use of
> background alpha at all. So if the background color can have non-opaque
> alpha, then you need to document the behavior in "pixel blend mode". It
> also does not document what alpha value will result from blending, for
> blending the next plane.

Those are questions that did not crossed my mind at all.

What would you suggest ? Instinctively I would say that in the case 
where there is a non-opaque background color,

alpha pre-multiplication would not be taken into account, although it is 
maybe not the best solution.

As I am not quite sure, I will lookup for this.


>
> The question about full vs. limited range seems unnecessary to me, as
> the background color will be used as-is in the blending stage, so
> userspace can just program the correct value that fits the pipeline it
> is setting up.
>
> One more question is, as HDR exists, could we need background colors
> with component values greater than 1.0?

AR4H color format should cover that case, isn't it ?



Regards,

Raphaël

>
> Scanout of FP16 formats exists.
>
>>    */
>
> Thanks,
> pq

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Pekka Paalanen]

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

On Fri, 9 Jul 2021 18:23:26 +0200
Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:

> On 7/9/21 10:04 AM, Pekka Paalanen wrote:
> > On Wed, 7 Jul 2021 08:48:47 +0000
> > Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
> >  
> >> Some display controllers can be programmed to present non-black colors
> >> for pixels not covered by any plane (or pixels covered by the
> >> transparent regions of higher planes).  Compositors that want a UI with
> >> a solid color background can potentially save memory bandwidth by
> >> setting the CRTC background property and using smaller planes to display
> >> the rest of the content.
> >>
> >> To avoid confusion between different ways of encoding RGB data, we
> >> define a standard 64-bit format that should be used for this property's
> >> value.  Helper functions and macros are provided to generate and dissect
> >> values in this standard format with varying component precision values.
> >>
> >> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> >> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >> ---
> >>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
> >>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
> >>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
> >>   include/drm/drm_blend.h                   |  1 +
> >>   include/drm/drm_crtc.h                    | 12 ++++++++
> >>   include/drm/drm_mode_config.h             |  5 ++++
> >>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
> >>   8 files changed, 89 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> index ddcf5c2c8e6a..1b95a4ecdb2b 100644
> >> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> >> @@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
> >>   				     struct drm_crtc *crtc)
> >>   {
> >>   	crtc_state->crtc = crtc;
> >> +	crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
> >>   }
> >>   EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
> >>   
> >> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> >> index 438e9585b225..fea68f8f17f8 100644
> >> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> >> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> >> @@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >>   		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
> >>   	} else if (property == crtc->scaling_filter_property) {
> >>   		state->scaling_filter = val;
> >> +	} else if (property == config->bgcolor_property) {
> >> +		state->bgcolor = val;
> >>   	} else if (crtc->funcs->atomic_set_property) {
> >>   		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> >>   	} else {
> >> @@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >>   		*val = 0;
> >>   	else if (property == crtc->scaling_filter_property)
> >>   		*val = state->scaling_filter;
> >> +	else if (property == config->bgcolor_property)
> >> +		*val = state->bgcolor;
> >>   	else if (crtc->funcs->atomic_get_property)
> >>   		return crtc->funcs->atomic_get_property(crtc, state, property, val);
> >>   	else
> >> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >> index ec37cbfabb50..6692d6a6db22 100644
> >> --- a/drivers/gpu/drm/drm_blend.c
> >> +++ b/drivers/gpu/drm/drm_blend.c
> >> @@ -186,8 +186,7 @@
> >>    *		 assumed to be 1.0
> >>    *
> >>    * Note that all the property extensions described here apply either to the
> >> - * plane or the CRTC (e.g. for the background color, which currently is not
> >> - * exposed and assumed to be black).
> >> + * plane or the CRTC.
> >>    *
> >>    * SCALING_FILTER:
> >>    *     Indicates scaling filter to be used for plane scaler
> >> @@ -201,6 +200,21 @@
> >>    *
> >>    * Drivers can set up this property for a plane by calling
> >>    * drm_plane_create_scaling_filter_property
> >> + *  
> > Hi,  
> 
> 
> Hi Pekka,
> 
> 
> Many thanks for your feedback, your comments are taken into account for 
> a v2.
> 
> 
> >
> > I assume the below block is the UAPI documentation of this new property
> > and that it would appear with the other UAPI docs.
> >  
> >> + * BACKGROUND_COLOR:
> >> + *	Defines the ARGB color of a full-screen layer that exists below all
> >> + *	planes.  This color will be used for pixels not covered by any plane
> >> + *	and may also be blended with plane contents as allowed by a plane's
> >> + *	alpha values.  The background color defaults to black, and is assumed
> >> + *	to be black for drivers that do not expose this property.  
> > All good up to here.
> >  
> >>   Although
> >> + *	background color isn't a plane, it is assumed that the color provided
> >> + *	here undergoes the same pipe-level degamma/CSC/gamma transformations
> >> + *	that planes undergo.  
> > This sounds to me like it refers to the per-plane degamma/csc/gamma
> > which are new properties in development. I believe you do not mean
> > that, but you mean the CRTC degamma/csc/gamma and everything else which
> > apply *after* the blending of planes. So the wording here would need
> > clarification.  
> 
> 
> Yes, I was not sure about this, but it is effectively the general CRTC 
> color correction which is applicable to the background color.
> 
> >  
> >>   Note that the color value provided here includes
> >> + *	an alpha channel...non-opaque background color values are allowed,
> >> + *	but are generally only honored in special cases (e.g., when a memory
> >> + *	writeback connector is in use).  
> > This could be read as: if you use a non-opaque color value, it will
> > usually be completely ignored (and the background will be e.g. black
> > instead). Is that your intention?
> >
> > I think a more useful definition would be that the alpha is used in
> > blending as always, but because we do not yet have physically
> > transparent monitors, the final alpha value may not reach the video
> > sink or the video sink may ignore it.  
> 
> In our case, the hardware does not support alpha channel (as you can see 
> the DRM_ARGB_TO_LTDC_RGB24 macro in the second patch).
> 
> For chip vendors who does support this feature, the video sink would get 
> this transparency parameter. In the case where it is not, alpha channel 
> would be ignored.
> 
> 
> >> + *
> >> + *	This property is setup with drm_crtc_add_bgcolor_property().  
> > You forgot to document the value format of this property. The ARGB color
> > format needs to be defined at least to the same detail as all pixel
> > formats in drm_fourcc.h are. If there is a suitable DRM_FORMAT_*
> > definition already, simply saying the color is in that format would be
> > enough.  
> 
> 
> Will do ! :)
> 
> I was thinking about the FourCC AR4H format. Have you something else in 
> mind ?

Hi,

if you mean DRM_FORMAT_ARGB16161616F then that is not what you are
using right now. That is a floating-point format using 16-bit floats
(half float). It has only 10 bits precision IIRC.

As C compilers do not(?) have built-in support for halfs, using this
format would be inconvenient for userspace (and the kernel?). Since
it's just for one pixel value, I think using a format that is
convenient to craft would be good.


> > Another thing to document is whether this color value is alpha
> > pre-multiplied or not. Planes can have the property "pixel blend mode",
> > but because the background color is not on a plane, that property
> > cannot apply here.
> >
> > The difference it makes is that if background color is both non-opaque
> > and pre-multiplied, then the question arises what pixel values will
> > actually be transmitted to video sink for the background. Will the
> > alpha pre-multiplication be undone?
> >
> > Btw. note that "pixel blend mode" property does not document the use of
> > background alpha at all. So if the background color can have non-opaque
> > alpha, then you need to document the behavior in "pixel blend mode". It
> > also does not document what alpha value will result from blending, for
> > blending the next plane.  
> 
> Those are questions that did not crossed my mind at all.
> 
> What would you suggest ? Instinctively I would say that in the case 
> where there is a non-opaque background color,
> 
> alpha pre-multiplication would not be taken into account, although it is 
> maybe not the best solution.
> 
> As I am not quite sure, I will lookup for this.

Right now, I would suggest to just dodge the whole question: define the
background color to be opaque. Either drop the alpha channel, or
specify that alpha must be 1.0 for now (fail ioctl if not).

Let the people who actually need alpha in the background color figure
out all the details. They would know what they want, while we don't. We
also can't come up with a proper userspace for non-opaque alpha to
prove that the design works.

If you specify that alpha channel exists but must be 1.0, then someone
else could later add another property that defines how the alpha would
be used if it was less than 1.0. The existence of that other property
would then tell userspace that non-1.0 alpha is supported and also
define what it does. Userspace that does not understand that will just
keep using alpha 1.0, meaning it doesn't matter what value the other
new property has. So this seems quite future-proof to me.

> > The question about full vs. limited range seems unnecessary to me, as
> > the background color will be used as-is in the blending stage, so
> > userspace can just program the correct value that fits the pipeline it
> > is setting up.
> >
> > One more question is, as HDR exists, could we need background colors
> > with component values greater than 1.0?  
> 
> AR4H color format should cover that case, isn't it ?

Yes, but with the inconvenience I mentioned.

This is a genuine question though, would anyone actually need
background color values > 1.0. I don't know of any case yet where it
would be required. It would imply that plane blending happens in a
color space where >1.0 values are meaningful. I'm not even sure if any
hardware supporting that exists.

Maybe it would be best to assume that only [0.0, 1.0] pixel value range
is useful, and mention in the commit message that if someone really
needs values outside of that, they should create another background
color property. Then, you can pick a simple unsigned integer pixel
format, too. (I didn't see any 16 bit-per-channel formats like that in
drm_fourcc.h though.)


Thanks,
pq

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Harry Wentland]

On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:
> On Fri, 9 Jul 2021 18:23:26 +0200
> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
> 
>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:
>>> On Wed, 7 Jul 2021 08:48:47 +0000
>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
>>>  
>>>> Some display controllers can be programmed to present non-black colors
>>>> for pixels not covered by any plane (or pixels covered by the
>>>> transparent regions of higher planes).  Compositors that want a UI with
>>>> a solid color background can potentially save memory bandwidth by
>>>> setting the CRTC background property and using smaller planes to display
>>>> the rest of the content.
>>>>
>>>> To avoid confusion between different ways of encoding RGB data, we
>>>> define a standard 64-bit format that should be used for this property's
>>>> value.  Helper functions and macros are provided to generate and dissect
>>>> values in this standard format with varying component precision values.
>>>>
>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>> ---
>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>>>>   include/drm/drm_blend.h                   |  1 +
>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
>>>>   include/drm/drm_mode_config.h             |  5 ++++
>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>>>>   8 files changed, 89 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> index ddcf5c2c8e6a..1b95a4ecdb2b 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>>>> @@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
>>>>   				     struct drm_crtc *crtc)
>>>>   {
>>>>   	crtc_state->crtc = crtc;
>>>> +	crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
>>>>   }
>>>>   EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
>>>>   
>>>> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> index 438e9585b225..fea68f8f17f8 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_uapi.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
>>>> @@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
>>>>   		set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>>>>   	} else if (property == crtc->scaling_filter_property) {
>>>>   		state->scaling_filter = val;
>>>> +	} else if (property == config->bgcolor_property) {
>>>> +		state->bgcolor = val;
>>>>   	} else if (crtc->funcs->atomic_set_property) {
>>>>   		return crtc->funcs->atomic_set_property(crtc, state, property, val);
>>>>   	} else {
>>>> @@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>>>>   		*val = 0;
>>>>   	else if (property == crtc->scaling_filter_property)
>>>>   		*val = state->scaling_filter;
>>>> +	else if (property == config->bgcolor_property)
>>>> +		*val = state->bgcolor;
>>>>   	else if (crtc->funcs->atomic_get_property)
>>>>   		return crtc->funcs->atomic_get_property(crtc, state, property, val);
>>>>   	else
>>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>>> index ec37cbfabb50..6692d6a6db22 100644
>>>> --- a/drivers/gpu/drm/drm_blend.c
>>>> +++ b/drivers/gpu/drm/drm_blend.c
>>>> @@ -186,8 +186,7 @@
>>>>    *		 assumed to be 1.0
>>>>    *
>>>>    * Note that all the property extensions described here apply either to the
>>>> - * plane or the CRTC (e.g. for the background color, which currently is not
>>>> - * exposed and assumed to be black).
>>>> + * plane or the CRTC.
>>>>    *
>>>>    * SCALING_FILTER:
>>>>    *     Indicates scaling filter to be used for plane scaler
>>>> @@ -201,6 +200,21 @@
>>>>    *
>>>>    * Drivers can set up this property for a plane by calling
>>>>    * drm_plane_create_scaling_filter_property
>>>> + *  
>>> Hi,  
>>
>>
>> Hi Pekka,
>>
>>
>> Many thanks for your feedback, your comments are taken into account for 
>> a v2.
>>
>>
>>>
>>> I assume the below block is the UAPI documentation of this new property
>>> and that it would appear with the other UAPI docs.
>>>  
>>>> + * BACKGROUND_COLOR:
>>>> + *	Defines the ARGB color of a full-screen layer that exists below all
>>>> + *	planes.  This color will be used for pixels not covered by any plane
>>>> + *	and may also be blended with plane contents as allowed by a plane's
>>>> + *	alpha values.  The background color defaults to black, and is assumed
>>>> + *	to be black for drivers that do not expose this property.  
>>> All good up to here.
>>>  
>>>>   Although
>>>> + *	background color isn't a plane, it is assumed that the color provided
>>>> + *	here undergoes the same pipe-level degamma/CSC/gamma transformations
>>>> + *	that planes undergo.  
>>> This sounds to me like it refers to the per-plane degamma/csc/gamma
>>> which are new properties in development. I believe you do not mean
>>> that, but you mean the CRTC degamma/csc/gamma and everything else which
>>> apply *after* the blending of planes. So the wording here would need
>>> clarification.  
>>
>>
>> Yes, I was not sure about this, but it is effectively the general CRTC 
>> color correction which is applicable to the background color.
>>
>>>  
>>>>   Note that the color value provided here includes
>>>> + *	an alpha channel...non-opaque background color values are allowed,
>>>> + *	but are generally only honored in special cases (e.g., when a memory
>>>> + *	writeback connector is in use).  
>>> This could be read as: if you use a non-opaque color value, it will
>>> usually be completely ignored (and the background will be e.g. black
>>> instead). Is that your intention?
>>>
>>> I think a more useful definition would be that the alpha is used in
>>> blending as always, but because we do not yet have physically
>>> transparent monitors, the final alpha value may not reach the video
>>> sink or the video sink may ignore it.  
>>
>> In our case, the hardware does not support alpha channel (as you can see 
>> the DRM_ARGB_TO_LTDC_RGB24 macro in the second patch).
>>
>> For chip vendors who does support this feature, the video sink would get 
>> this transparency parameter. In the case where it is not, alpha channel 
>> would be ignored.
>>
>>
>>>> + *
>>>> + *	This property is setup with drm_crtc_add_bgcolor_property().  
>>> You forgot to document the value format of this property. The ARGB color
>>> format needs to be defined at least to the same detail as all pixel
>>> formats in drm_fourcc.h are. If there is a suitable DRM_FORMAT_*
>>> definition already, simply saying the color is in that format would be
>>> enough.  
>>
>>
>> Will do ! :)
>>
>> I was thinking about the FourCC AR4H format. Have you something else in 
>> mind ?
> 
> Hi,
> 
> if you mean DRM_FORMAT_ARGB16161616F then that is not what you are
> using right now. That is a floating-point format using 16-bit floats
> (half float). It has only 10 bits precision IIRC.
> 
> As C compilers do not(?) have built-in support for halfs, using this
> format would be inconvenient for userspace (and the kernel?). Since
> it's just for one pixel value, I think using a format that is
> convenient to craft would be good.
> 
> 
>>> Another thing to document is whether this color value is alpha
>>> pre-multiplied or not. Planes can have the property "pixel blend mode",
>>> but because the background color is not on a plane, that property
>>> cannot apply here.
>>>
>>> The difference it makes is that if background color is both non-opaque
>>> and pre-multiplied, then the question arises what pixel values will
>>> actually be transmitted to video sink for the background. Will the
>>> alpha pre-multiplication be undone?
>>>
>>> Btw. note that "pixel blend mode" property does not document the use of
>>> background alpha at all. So if the background color can have non-opaque
>>> alpha, then you need to document the behavior in "pixel blend mode". It
>>> also does not document what alpha value will result from blending, for
>>> blending the next plane.  
>>
>> Those are questions that did not crossed my mind at all.
>>
>> What would you suggest ? Instinctively I would say that in the case 
>> where there is a non-opaque background color,
>>
>> alpha pre-multiplication would not be taken into account, although it is 
>> maybe not the best solution.
>>
>> As I am not quite sure, I will lookup for this.
> 
> Right now, I would suggest to just dodge the whole question: define the
> background color to be opaque. Either drop the alpha channel, or
> specify that alpha must be 1.0 for now (fail ioctl if not).
> 
> Let the people who actually need alpha in the background color figure
> out all the details. They would know what they want, while we don't. We
> also can't come up with a proper userspace for non-opaque alpha to
> prove that the design works.
> 
> If you specify that alpha channel exists but must be 1.0, then someone
> else could later add another property that defines how the alpha would
> be used if it was less than 1.0. The existence of that other property
> would then tell userspace that non-1.0 alpha is supported and also
> define what it does. Userspace that does not understand that will just
> keep using alpha 1.0, meaning it doesn't matter what value the other
> new property has. So this seems quite future-proof to me.
> 
>>> The question about full vs. limited range seems unnecessary to me, as
>>> the background color will be used as-is in the blending stage, so
>>> userspace can just program the correct value that fits the pipeline it
>>> is setting up.
>>>
>>> One more question is, as HDR exists, could we need background colors
>>> with component values greater than 1.0?  
>>
>> AR4H color format should cover that case, isn't it ?
> 
> Yes, but with the inconvenience I mentioned.
> 
> This is a genuine question though, would anyone actually need
> background color values > 1.0. I don't know of any case yet where it
> would be required. It would imply that plane blending happens in a
> color space where >1.0 values are meaningful. I'm not even sure if any
> hardware supporting that exists.
> 
> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
> is useful, and mention in the commit message that if someone really
> needs values outside of that, they should create another background
> color property. Then, you can pick a simple unsigned integer pixel
> format, too. (I didn't see any 16 bit-per-channel formats like that in
> drm_fourcc.h though.)
> 

I don't think we should artificially limit this to [0.0, 1.0]. As you
mentioned above when talking about full vs limited, the userspace
understands what's the correct value that fits the pipeline. If that
pipeline is FP16 with > 1.0 values then it would make sense that the
background color can be > 1.0.

Harry

> 
> Thanks,
> pq
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Pekka Paalanen]

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

On Mon, 12 Jul 2021 12:15:59 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:
> > On Fri, 9 Jul 2021 18:23:26 +0200
> > Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
> >   
> >> On 7/9/21 10:04 AM, Pekka Paalanen wrote:  
> >>> On Wed, 7 Jul 2021 08:48:47 +0000
> >>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
> >>>    
> >>>> Some display controllers can be programmed to present non-black colors
> >>>> for pixels not covered by any plane (or pixels covered by the
> >>>> transparent regions of higher planes).  Compositors that want a UI with
> >>>> a solid color background can potentially save memory bandwidth by
> >>>> setting the CRTC background property and using smaller planes to display
> >>>> the rest of the content.
> >>>>
> >>>> To avoid confusion between different ways of encoding RGB data, we
> >>>> define a standard 64-bit format that should be used for this property's
> >>>> value.  Helper functions and macros are provided to generate and dissect
> >>>> values in this standard format with varying component precision values.
> >>>>
> >>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> >>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>>> ---
> >>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
> >>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
> >>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
> >>>>   include/drm/drm_blend.h                   |  1 +
> >>>>   include/drm/drm_crtc.h                    | 12 ++++++++
> >>>>   include/drm/drm_mode_config.h             |  5 ++++
> >>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
> >>>>   8 files changed, 89 insertions(+), 2 deletions(-)

...

> >>> The question about full vs. limited range seems unnecessary to me, as
> >>> the background color will be used as-is in the blending stage, so
> >>> userspace can just program the correct value that fits the pipeline it
> >>> is setting up.
> >>>
> >>> One more question is, as HDR exists, could we need background colors
> >>> with component values greater than 1.0?    
> >>
> >> AR4H color format should cover that case, isn't it ?  
> > 
> > Yes, but with the inconvenience I mentioned.
> > 
> > This is a genuine question though, would anyone actually need
> > background color values > 1.0. I don't know of any case yet where it
> > would be required. It would imply that plane blending happens in a
> > color space where >1.0 values are meaningful. I'm not even sure if any
> > hardware supporting that exists.
> > 
> > Maybe it would be best to assume that only [0.0, 1.0] pixel value range
> > is useful, and mention in the commit message that if someone really
> > needs values outside of that, they should create another background
> > color property. Then, you can pick a simple unsigned integer pixel
> > format, too. (I didn't see any 16 bit-per-channel formats like that in
> > drm_fourcc.h though.)
> >   
> 
> I don't think we should artificially limit this to [0.0, 1.0]. As you
> mentioned above when talking about full vs limited, the userspace
> understands what's the correct value that fits the pipeline. If that
> pipeline is FP16 with > 1.0 values then it would make sense that the
> background color can be > 1.0.

Ok. The standard FP32 format then for ease of use and guaranteed enough
range and precision for far into the future?

Or do you want to keep it in 64 bits total, so the UABI can pack
everything into a u64 instead of needing to create a blob?

I don't mind as long as it's clearly documented what it is and how it
works, and it carries enough precision.

But FP16 with its 10 bits of precision might be too little for integer
12-16 bpc pipelines and sinks?

If the values can go beyond [0.0, 1.0] range, then does the blending
hardware and the degamma/ctm/gamma coming afterwards cope with them, or
do they get clamped anyway?


Thanks,
pq

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Harry Wentland]

On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:
> On Mon, 12 Jul 2021 12:15:59 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:
>>> On Fri, 9 Jul 2021 18:23:26 +0200
>>> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
>>>   
>>>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:  
>>>>> On Wed, 7 Jul 2021 08:48:47 +0000
>>>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
>>>>>    
>>>>>> Some display controllers can be programmed to present non-black colors
>>>>>> for pixels not covered by any plane (or pixels covered by the
>>>>>> transparent regions of higher planes).  Compositors that want a UI with
>>>>>> a solid color background can potentially save memory bandwidth by
>>>>>> setting the CRTC background property and using smaller planes to display
>>>>>> the rest of the content.
>>>>>>
>>>>>> To avoid confusion between different ways of encoding RGB data, we
>>>>>> define a standard 64-bit format that should be used for this property's
>>>>>> value.  Helper functions and macros are provided to generate and dissect
>>>>>> values in this standard format with varying component precision values.
>>>>>>
>>>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
>>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>>>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>>>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>>>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>>>>>>   include/drm/drm_blend.h                   |  1 +
>>>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
>>>>>>   include/drm/drm_mode_config.h             |  5 ++++
>>>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>>>>>>   8 files changed, 89 insertions(+), 2 deletions(-)
> 
> ...
> 
>>>>> The question about full vs. limited range seems unnecessary to me, as
>>>>> the background color will be used as-is in the blending stage, so
>>>>> userspace can just program the correct value that fits the pipeline it
>>>>> is setting up.
>>>>>
>>>>> One more question is, as HDR exists, could we need background colors
>>>>> with component values greater than 1.0?    
>>>>
>>>> AR4H color format should cover that case, isn't it ?  
>>>
>>> Yes, but with the inconvenience I mentioned.
>>>
>>> This is a genuine question though, would anyone actually need
>>> background color values > 1.0. I don't know of any case yet where it
>>> would be required. It would imply that plane blending happens in a
>>> color space where >1.0 values are meaningful. I'm not even sure if any
>>> hardware supporting that exists.
>>>
>>> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
>>> is useful, and mention in the commit message that if someone really
>>> needs values outside of that, they should create another background
>>> color property. Then, you can pick a simple unsigned integer pixel
>>> format, too. (I didn't see any 16 bit-per-channel formats like that in
>>> drm_fourcc.h though.)
>>>   
>>
>> I don't think we should artificially limit this to [0.0, 1.0]. As you
>> mentioned above when talking about full vs limited, the userspace
>> understands what's the correct value that fits the pipeline. If that
>> pipeline is FP16 with > 1.0 values then it would make sense that the
>> background color can be > 1.0.
> 
> Ok. The standard FP32 format then for ease of use and guaranteed enough
> range and precision for far into the future?
> 

I don't have a strong preference for FP16 vs FP32. My understanding is
that FP16 is enough to represent linearly encoded data in a way that
looks smooth to humans.

scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].

> Or do you want to keep it in 64 bits total, so the UABI can pack
> everything into a u64 instead of needing to create a blob?
> 
> I don't mind as long as it's clearly documented what it is and how it
> works, and it carries enough precision.
> 
> But FP16 with its 10 bits of precision might be too little for integer
> 12-16 bpc pipelines and sinks?
> 
> If the values can go beyond [0.0, 1.0] range, then does the blending
> hardware and the degamma/ctm/gamma coming afterwards cope with them, or
> do they get clamped anyway?
> 

That probably depends on the HW and how it's configured. AMD HW can handle
values above and below [0.0, 1.0].

Harry

> 
> Thanks,
> pq
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Pekka Paalanen]

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

On Tue, 13 Jul 2021 09:54:35 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:
> > On Mon, 12 Jul 2021 12:15:59 -0400
> > Harry Wentland <harry.wentland@amd.com> wrote:
> >   
> >> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:  
> >>> On Fri, 9 Jul 2021 18:23:26 +0200
> >>> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
> >>>     
> >>>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:    
> >>>>> On Wed, 7 Jul 2021 08:48:47 +0000
> >>>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
> >>>>>      
> >>>>>> Some display controllers can be programmed to present non-black colors
> >>>>>> for pixels not covered by any plane (or pixels covered by the
> >>>>>> transparent regions of higher planes).  Compositors that want a UI with
> >>>>>> a solid color background can potentially save memory bandwidth by
> >>>>>> setting the CRTC background property and using smaller planes to display
> >>>>>> the rest of the content.
> >>>>>>
> >>>>>> To avoid confusion between different ways of encoding RGB data, we
> >>>>>> define a standard 64-bit format that should be used for this property's
> >>>>>> value.  Helper functions and macros are provided to generate and dissect
> >>>>>> values in this standard format with varying component precision values.
> >>>>>>
> >>>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> >>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>>>>> ---
> >>>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >>>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
> >>>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
> >>>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
> >>>>>>   include/drm/drm_blend.h                   |  1 +
> >>>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
> >>>>>>   include/drm/drm_mode_config.h             |  5 ++++
> >>>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
> >>>>>>   8 files changed, 89 insertions(+), 2 deletions(-)  
> > 
> > ...
> >   
> >>>>> The question about full vs. limited range seems unnecessary to me, as
> >>>>> the background color will be used as-is in the blending stage, so
> >>>>> userspace can just program the correct value that fits the pipeline it
> >>>>> is setting up.
> >>>>>
> >>>>> One more question is, as HDR exists, could we need background colors
> >>>>> with component values greater than 1.0?      
> >>>>
> >>>> AR4H color format should cover that case, isn't it ?    
> >>>
> >>> Yes, but with the inconvenience I mentioned.
> >>>
> >>> This is a genuine question though, would anyone actually need
> >>> background color values > 1.0. I don't know of any case yet where it
> >>> would be required. It would imply that plane blending happens in a
> >>> color space where >1.0 values are meaningful. I'm not even sure if any
> >>> hardware supporting that exists.
> >>>
> >>> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
> >>> is useful, and mention in the commit message that if someone really
> >>> needs values outside of that, they should create another background
> >>> color property. Then, you can pick a simple unsigned integer pixel
> >>> format, too. (I didn't see any 16 bit-per-channel formats like that in
> >>> drm_fourcc.h though.)
> >>>     
> >>
> >> I don't think we should artificially limit this to [0.0, 1.0]. As you
> >> mentioned above when talking about full vs limited, the userspace
> >> understands what's the correct value that fits the pipeline. If that
> >> pipeline is FP16 with > 1.0 values then it would make sense that the
> >> background color can be > 1.0.  
> > 
> > Ok. The standard FP32 format then for ease of use and guaranteed enough
> > range and precision for far into the future?
> >   
> 
> I don't have a strong preference for FP16 vs FP32. My understanding is
> that FP16 is enough to represent linearly encoded data in a way that
> looks smooth to humans.
> 
> scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].
> 
> > Or do you want to keep it in 64 bits total, so the UABI can pack
> > everything into a u64 instead of needing to create a blob?
> > 
> > I don't mind as long as it's clearly documented what it is and how it
> > works, and it carries enough precision.
> > 
> > But FP16 with its 10 bits of precision might be too little for integer
> > 12-16 bpc pipelines and sinks?

The 10 bits worries me still.

If you have a pipeline that works in [0.0, 1.0] range only, then FP16
limits precision to 10 bits (in the upper half of the range?).

> > 
> > If the values can go beyond [0.0, 1.0] range, then does the blending
> > hardware and the degamma/ctm/gamma coming afterwards cope with them, or
> > do they get clamped anyway?
> >   
> 
> That probably depends on the HW and how it's configured. AMD HW can handle
> values above and below [0.0, 1.0].

Right, so how would userspace know what will happen?

Or do we need to specify that while values outside that unit range are
expressable, it is hardware-specific on how they will behave, so
generic userspace should not attempt to use values outside of the unit
range?

I guess this caveat should be documented for everything, not just for
background color? LUT inputs and outputs, CTM input and output ranges,
FB formats...


Thanks,
pq

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Harry Wentland]

On 2021-07-14 3:35 a.m., Pekka Paalanen wrote:
> On Tue, 13 Jul 2021 09:54:35 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:
>>> On Mon, 12 Jul 2021 12:15:59 -0400
>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>   
>>>> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:  
>>>>> On Fri, 9 Jul 2021 18:23:26 +0200
>>>>> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
>>>>>     
>>>>>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:    
>>>>>>> On Wed, 7 Jul 2021 08:48:47 +0000
>>>>>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
>>>>>>>      
>>>>>>>> Some display controllers can be programmed to present non-black colors
>>>>>>>> for pixels not covered by any plane (or pixels covered by the
>>>>>>>> transparent regions of higher planes).  Compositors that want a UI with
>>>>>>>> a solid color background can potentially save memory bandwidth by
>>>>>>>> setting the CRTC background property and using smaller planes to display
>>>>>>>> the rest of the content.
>>>>>>>>
>>>>>>>> To avoid confusion between different ways of encoding RGB data, we
>>>>>>>> define a standard 64-bit format that should be used for this property's
>>>>>>>> value.  Helper functions and macros are provided to generate and dissect
>>>>>>>> values in this standard format with varying component precision values.
>>>>>>>>
>>>>>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
>>>>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>>>>>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>>>>>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>>>>>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>>>>>>>>   include/drm/drm_blend.h                   |  1 +
>>>>>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
>>>>>>>>   include/drm/drm_mode_config.h             |  5 ++++
>>>>>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>>>>>>>>   8 files changed, 89 insertions(+), 2 deletions(-)  
>>>
>>> ...
>>>   
>>>>>>> The question about full vs. limited range seems unnecessary to me, as
>>>>>>> the background color will be used as-is in the blending stage, so
>>>>>>> userspace can just program the correct value that fits the pipeline it
>>>>>>> is setting up.
>>>>>>>
>>>>>>> One more question is, as HDR exists, could we need background colors
>>>>>>> with component values greater than 1.0?      
>>>>>>
>>>>>> AR4H color format should cover that case, isn't it ?    
>>>>>
>>>>> Yes, but with the inconvenience I mentioned.
>>>>>
>>>>> This is a genuine question though, would anyone actually need
>>>>> background color values > 1.0. I don't know of any case yet where it
>>>>> would be required. It would imply that plane blending happens in a
>>>>> color space where >1.0 values are meaningful. I'm not even sure if any
>>>>> hardware supporting that exists.
>>>>>
>>>>> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
>>>>> is useful, and mention in the commit message that if someone really
>>>>> needs values outside of that, they should create another background
>>>>> color property. Then, you can pick a simple unsigned integer pixel
>>>>> format, too. (I didn't see any 16 bit-per-channel formats like that in
>>>>> drm_fourcc.h though.)
>>>>>     
>>>>
>>>> I don't think we should artificially limit this to [0.0, 1.0]. As you
>>>> mentioned above when talking about full vs limited, the userspace
>>>> understands what's the correct value that fits the pipeline. If that
>>>> pipeline is FP16 with > 1.0 values then it would make sense that the
>>>> background color can be > 1.0.  
>>>
>>> Ok. The standard FP32 format then for ease of use and guaranteed enough
>>> range and precision for far into the future?
>>>   
>>
>> I don't have a strong preference for FP16 vs FP32. My understanding is
>> that FP16 is enough to represent linearly encoded data in a way that
>> looks smooth to humans.
>>
>> scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].
>>
>>> Or do you want to keep it in 64 bits total, so the UABI can pack
>>> everything into a u64 instead of needing to create a blob?
>>>
>>> I don't mind as long as it's clearly documented what it is and how it
>>> works, and it carries enough precision.
>>>
>>> But FP16 with its 10 bits of precision might be too little for integer
>>> 12-16 bpc pipelines and sinks?
> 
> The 10 bits worries me still.
> 
> If you have a pipeline that works in [0.0, 1.0] range only, then FP16
> limits precision to 10 bits (in the upper half of the range?).
> 
>>>
>>> If the values can go beyond [0.0, 1.0] range, then does the blending
>>> hardware and the degamma/ctm/gamma coming afterwards cope with them, or
>>> do they get clamped anyway?
>>>   
>>
>> That probably depends on the HW and how it's configured. AMD HW can handle
>> values above and below [0.0, 1.0].
> 
> Right, so how would userspace know what will happen?
> 
> Or do we need to specify that while values outside that unit range are
> expressable, it is hardware-specific on how they will behave, so
> generic userspace should not attempt to use values outside of the unit
> range?
> 
> I guess this caveat should be documented for everything, not just for
> background color? LUT inputs and outputs, CTM input and output ranges,
> FB formats...
> 

I'm not sure we should artificially limit the interface at this point, or
document hypotheticals. At this point I don't even know whether going beyond
[0.0, 1.0] would be a challenge for any HW that supports floating point
formats.

Harry

> 
> Thanks,
> pq
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Pekka Paalanen]

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

On Wed, 14 Jul 2021 12:13:58 -0400
Harry Wentland <harry.wentland@amd.com> wrote:

> On 2021-07-14 3:35 a.m., Pekka Paalanen wrote:
> > On Tue, 13 Jul 2021 09:54:35 -0400
> > Harry Wentland <harry.wentland@amd.com> wrote:
> >   
> >> On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:  
> >>> On Mon, 12 Jul 2021 12:15:59 -0400
> >>> Harry Wentland <harry.wentland@amd.com> wrote:
> >>>     
> >>>> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:    
> >>>>> On Fri, 9 Jul 2021 18:23:26 +0200
> >>>>> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
> >>>>>       
> >>>>>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:      
> >>>>>>> On Wed, 7 Jul 2021 08:48:47 +0000
> >>>>>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
> >>>>>>>        
> >>>>>>>> Some display controllers can be programmed to present non-black colors
> >>>>>>>> for pixels not covered by any plane (or pixels covered by the
> >>>>>>>> transparent regions of higher planes).  Compositors that want a UI with
> >>>>>>>> a solid color background can potentially save memory bandwidth by
> >>>>>>>> setting the CRTC background property and using smaller planes to display
> >>>>>>>> the rest of the content.
> >>>>>>>>
> >>>>>>>> To avoid confusion between different ways of encoding RGB data, we
> >>>>>>>> define a standard 64-bit format that should be used for this property's
> >>>>>>>> value.  Helper functions and macros are provided to generate and dissect
> >>>>>>>> values in this standard format with varying component precision values.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> >>>>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>>>>>>> ---
> >>>>>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> >>>>>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
> >>>>>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
> >>>>>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
> >>>>>>>>   include/drm/drm_blend.h                   |  1 +
> >>>>>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
> >>>>>>>>   include/drm/drm_mode_config.h             |  5 ++++
> >>>>>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
> >>>>>>>>   8 files changed, 89 insertions(+), 2 deletions(-)    
> >>>
> >>> ...
> >>>     
> >>>>>>> The question about full vs. limited range seems unnecessary to me, as
> >>>>>>> the background color will be used as-is in the blending stage, so
> >>>>>>> userspace can just program the correct value that fits the pipeline it
> >>>>>>> is setting up.
> >>>>>>>
> >>>>>>> One more question is, as HDR exists, could we need background colors
> >>>>>>> with component values greater than 1.0?        
> >>>>>>
> >>>>>> AR4H color format should cover that case, isn't it ?      
> >>>>>
> >>>>> Yes, but with the inconvenience I mentioned.
> >>>>>
> >>>>> This is a genuine question though, would anyone actually need
> >>>>> background color values > 1.0. I don't know of any case yet where it
> >>>>> would be required. It would imply that plane blending happens in a
> >>>>> color space where >1.0 values are meaningful. I'm not even sure if any
> >>>>> hardware supporting that exists.
> >>>>>
> >>>>> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
> >>>>> is useful, and mention in the commit message that if someone really
> >>>>> needs values outside of that, they should create another background
> >>>>> color property. Then, you can pick a simple unsigned integer pixel
> >>>>> format, too. (I didn't see any 16 bit-per-channel formats like that in
> >>>>> drm_fourcc.h though.)
> >>>>>       
> >>>>
> >>>> I don't think we should artificially limit this to [0.0, 1.0]. As you
> >>>> mentioned above when talking about full vs limited, the userspace
> >>>> understands what's the correct value that fits the pipeline. If that
> >>>> pipeline is FP16 with > 1.0 values then it would make sense that the
> >>>> background color can be > 1.0.    
> >>>
> >>> Ok. The standard FP32 format then for ease of use and guaranteed enough
> >>> range and precision for far into the future?
> >>>     
> >>
> >> I don't have a strong preference for FP16 vs FP32. My understanding is
> >> that FP16 is enough to represent linearly encoded data in a way that
> >> looks smooth to humans.
> >>
> >> scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].
> >>  
> >>> Or do you want to keep it in 64 bits total, so the UABI can pack
> >>> everything into a u64 instead of needing to create a blob?
> >>>
> >>> I don't mind as long as it's clearly documented what it is and how it
> >>> works, and it carries enough precision.
> >>>
> >>> But FP16 with its 10 bits of precision might be too little for integer
> >>> 12-16 bpc pipelines and sinks?  
> > 
> > The 10 bits worries me still.
> > 
> > If you have a pipeline that works in [0.0, 1.0] range only, then FP16
> > limits precision to 10 bits (in the upper half of the range?).
> >   
> >>>
> >>> If the values can go beyond [0.0, 1.0] range, then does the blending
> >>> hardware and the degamma/ctm/gamma coming afterwards cope with them, or
> >>> do they get clamped anyway?
> >>>     
> >>
> >> That probably depends on the HW and how it's configured. AMD HW can handle
> >> values above and below [0.0, 1.0].  
> > 
> > Right, so how would userspace know what will happen?
> > 
> > Or do we need to specify that while values outside that unit range are
> > expressable, it is hardware-specific on how they will behave, so
> > generic userspace should not attempt to use values outside of the unit
> > range?
> > 
> > I guess this caveat should be documented for everything, not just for
> > background color? LUT inputs and outputs, CTM input and output ranges,
> > FB formats...
> >   
> 
> I'm not sure we should artificially limit the interface at this point, or
> document hypotheticals. At this point I don't even know whether going beyond
> [0.0, 1.0] would be a challenge for any HW that supports floating point
> formats.

Exactly, we don't know. Yet we have to document how background color
works. If background color can express values outside of [0.0, 1.0],
the documentation must say something about it.

If there is no way to know, then documentation must say you don't know
(or that it is hardware-specific, which to generic userspace is the
same thing).

If userspace does not know what happens, then obviously it will avoid
using values it does not know what happens with.

For example, let's say that blending can produce values outside of
[0.0, 1.0]. The next step in the pipeline is DEGAMMA, which is a 1D
LUT. How do you sample a 1D LUT with input values beyond [0.0, 1.0]? Do
you clamp them to the unit range? Does the clamping still happen even
when the LUT is in pass-through mode?

And in general, how big or how negative values will actually go through
the pipeline?

Of course the background color property should not document everything
above, but it must say something, like "The handling of values outside
of [0.0, 1.0] depends on the capabilities of the hardware blending
engine." That makes the handling unknown to generic userspace, but
userspace drivers could make use of it.

The important bit is to understand that the background color values may
sometimes (when?) not reach the sink unmodified even if userspace has
configured the KMS pipeline to not modify them.

I would expect that values in [0.0, 1.0] have no problem passing
through the KMS pipeline unharmed, and there are obvious expectations
about how a LUT or a CTM processes them. But as soon as values outside
of that range are possible, a whole slew of questions arises. The
documentation must not be silent, it must set expectations like "it's
hardware specific" if that's what it is.


Thanks,
pq

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/2] drm: add crtc background color property

[Harry Wentland]

On 2021-07-15 5:34 a.m., Pekka Paalanen wrote:
> On Wed, 14 Jul 2021 12:13:58 -0400
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> On 2021-07-14 3:35 a.m., Pekka Paalanen wrote:
>>> On Tue, 13 Jul 2021 09:54:35 -0400
>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>   
>>>> On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:  
>>>>> On Mon, 12 Jul 2021 12:15:59 -0400
>>>>> Harry Wentland <harry.wentland@amd.com> wrote:
>>>>>     
>>>>>> On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:    
>>>>>>> On Fri, 9 Jul 2021 18:23:26 +0200
>>>>>>> Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
>>>>>>>       
>>>>>>>> On 7/9/21 10:04 AM, Pekka Paalanen wrote:      
>>>>>>>>> On Wed, 7 Jul 2021 08:48:47 +0000
>>>>>>>>> Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:
>>>>>>>>>        
>>>>>>>>>> Some display controllers can be programmed to present non-black colors
>>>>>>>>>> for pixels not covered by any plane (or pixels covered by the
>>>>>>>>>> transparent regions of higher planes).  Compositors that want a UI with
>>>>>>>>>> a solid color background can potentially save memory bandwidth by
>>>>>>>>>> setting the CRTC background property and using smaller planes to display
>>>>>>>>>> the rest of the content.
>>>>>>>>>>
>>>>>>>>>> To avoid confusion between different ways of encoding RGB data, we
>>>>>>>>>> define a standard 64-bit format that should be used for this property's
>>>>>>>>>> value.  Helper functions and macros are provided to generate and dissect
>>>>>>>>>> values in this standard format with varying component precision values.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
>>>>>>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>>>>>>>>>>   drivers/gpu/drm/drm_atomic_uapi.c         |  4 +++
>>>>>>>>>>   drivers/gpu/drm/drm_blend.c               | 34 +++++++++++++++++++++--
>>>>>>>>>>   drivers/gpu/drm/drm_mode_config.c         |  6 ++++
>>>>>>>>>>   include/drm/drm_blend.h                   |  1 +
>>>>>>>>>>   include/drm/drm_crtc.h                    | 12 ++++++++
>>>>>>>>>>   include/drm/drm_mode_config.h             |  5 ++++
>>>>>>>>>>   include/uapi/drm/drm_mode.h               | 28 +++++++++++++++++++
>>>>>>>>>>   8 files changed, 89 insertions(+), 2 deletions(-)    
>>>>>
>>>>> ...
>>>>>     
>>>>>>>>> The question about full vs. limited range seems unnecessary to me, as
>>>>>>>>> the background color will be used as-is in the blending stage, so
>>>>>>>>> userspace can just program the correct value that fits the pipeline it
>>>>>>>>> is setting up.
>>>>>>>>>
>>>>>>>>> One more question is, as HDR exists, could we need background colors
>>>>>>>>> with component values greater than 1.0?        
>>>>>>>>
>>>>>>>> AR4H color format should cover that case, isn't it ?      
>>>>>>>
>>>>>>> Yes, but with the inconvenience I mentioned.
>>>>>>>
>>>>>>> This is a genuine question though, would anyone actually need
>>>>>>> background color values > 1.0. I don't know of any case yet where it
>>>>>>> would be required. It would imply that plane blending happens in a
>>>>>>> color space where >1.0 values are meaningful. I'm not even sure if any
>>>>>>> hardware supporting that exists.
>>>>>>>
>>>>>>> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
>>>>>>> is useful, and mention in the commit message that if someone really
>>>>>>> needs values outside of that, they should create another background
>>>>>>> color property. Then, you can pick a simple unsigned integer pixel
>>>>>>> format, too. (I didn't see any 16 bit-per-channel formats like that in
>>>>>>> drm_fourcc.h though.)
>>>>>>>       
>>>>>>
>>>>>> I don't think we should artificially limit this to [0.0, 1.0]. As you
>>>>>> mentioned above when talking about full vs limited, the userspace
>>>>>> understands what's the correct value that fits the pipeline. If that
>>>>>> pipeline is FP16 with > 1.0 values then it would make sense that the
>>>>>> background color can be > 1.0.    
>>>>>
>>>>> Ok. The standard FP32 format then for ease of use and guaranteed enough
>>>>> range and precision for far into the future?
>>>>>     
>>>>
>>>> I don't have a strong preference for FP16 vs FP32. My understanding is
>>>> that FP16 is enough to represent linearly encoded data in a way that
>>>> looks smooth to humans.
>>>>
>>>> scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].
>>>>  
>>>>> Or do you want to keep it in 64 bits total, so the UABI can pack
>>>>> everything into a u64 instead of needing to create a blob?
>>>>>
>>>>> I don't mind as long as it's clearly documented what it is and how it
>>>>> works, and it carries enough precision.
>>>>>
>>>>> But FP16 with its 10 bits of precision might be too little for integer
>>>>> 12-16 bpc pipelines and sinks?  
>>>
>>> The 10 bits worries me still.
>>>
>>> If you have a pipeline that works in [0.0, 1.0] range only, then FP16
>>> limits precision to 10 bits (in the upper half of the range?).
>>>   
>>>>>
>>>>> If the values can go beyond [0.0, 1.0] range, then does the blending
>>>>> hardware and the degamma/ctm/gamma coming afterwards cope with them, or
>>>>> do they get clamped anyway?
>>>>>     
>>>>
>>>> That probably depends on the HW and how it's configured. AMD HW can handle
>>>> values above and below [0.0, 1.0].  
>>>
>>> Right, so how would userspace know what will happen?
>>>
>>> Or do we need to specify that while values outside that unit range are
>>> expressable, it is hardware-specific on how they will behave, so
>>> generic userspace should not attempt to use values outside of the unit
>>> range?
>>>
>>> I guess this caveat should be documented for everything, not just for
>>> background color? LUT inputs and outputs, CTM input and output ranges,
>>> FB formats...
>>>   
>>
>> I'm not sure we should artificially limit the interface at this point, or
>> document hypotheticals. At this point I don't even know whether going beyond
>> [0.0, 1.0] would be a challenge for any HW that supports floating point
>> formats.
> 
> Exactly, we don't know. Yet we have to document how background color
> works. If background color can express values outside of [0.0, 1.0],
> the documentation must say something about it.
> 
> If there is no way to know, then documentation must say you don't know
> (or that it is hardware-specific, which to generic userspace is the
> same thing).
> 
> If userspace does not know what happens, then obviously it will avoid
> using values it does not know what happens with.
> 
> For example, let's say that blending can produce values outside of
> [0.0, 1.0]. The next step in the pipeline is DEGAMMA, which is a 1D
> LUT. How do you sample a 1D LUT with input values beyond [0.0, 1.0]? Do
> you clamp them to the unit range? Does the clamping still happen even
> when the LUT is in pass-through mode?
> 
> And in general, how big or how negative values will actually go through
> the pipeline?
> 
> Of course the background color property should not document everything
> above, but it must say something, like "The handling of values outside
> of [0.0, 1.0] depends on the capabilities of the hardware blending
> engine." That makes the handling unknown to generic userspace, but
> userspace drivers could make use of it.
> 
> The important bit is to understand that the background color values may
> sometimes (when?) not reach the sink unmodified even if userspace has
> configured the KMS pipeline to not modify them.
> 
> I would expect that values in [0.0, 1.0] have no problem passing
> through the KMS pipeline unharmed, and there are obvious expectations
> about how a LUT or a CTM processes them. But as soon as values outside
> of that range are possible, a whole slew of questions arises. The
> documentation must not be silent, it must set expectations like "it's
> hardware specific" if that's what it is.
> 

Agreed. I think ultimately we don't know because we haven't gotten to
use-cases like that.

I'm fine with documentation stating "The handling of values outside
of [0.0, 1.0] depends on the capabilities of the hardware blending
engine." or "Handling of values outside [0.0, 1.0] is currently
undefined."

Harry

> 
> Thanks,
> pq
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] drm/stm: ltdc: add crtc background color property support

[Raphael GALLAIS-POU - foss]

This patch comes from the need to display small resolution pictures with
very few DDR usage. In practice, using a background color, produced by the
drm CRTC, around this picture allows to fetch less data in memory than
setting a full frame picture. And therefore the picture in DDR is smaller
than the size of the screen.

It uses the DRM framework background color property and modifies the
color to any value between 0x000000 and 0xFFFFFF from userland with a
RGB24 value (0x00RRGGBB).

Using this feature is observable only if layers are not full screen
or if layers use color formats with alpha and are "transparent" at
least on some pixels.

Depending on the hardware version, the background color can not be
properly displayed with non-alpha color formats derived from native
alpha color formats (such as XR24 or XR15) since the use of this
pixel format generates a non transparent layer. As a workaround,
the stage background color of the layer and the general background
color need to be synced.

Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
---
 drivers/gpu/drm/stm/ltdc.c | 48 ++++++++++++++++++++++++++++++++++----
 1 file changed, 43 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 1f9392fb58e1..0aca245288cc 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -196,6 +196,11 @@
 
 #define NB_PF		8		/* Max nb of HW pixel format */
 
+#define DRM_ARGB_TO_LTDC_RGB24(bgcolor) \
+	((u32)(DRM_ARGB_RED(bgcolor, 8) << 16	\
+	| DRM_ARGB_GREEN(bgcolor, 8) << 8	\
+	| DRM_ARGB_BLUE(bgcolor, 8)))
+
 enum ltdc_pix_fmt {
 	PF_NONE,
 	/* RGB formats */
@@ -364,6 +369,15 @@ static inline u32 get_pixelformat_without_alpha(u32 drm)
 	}
 }
 
+/*
+ * All non-alpha color formats derived from native alpha color formats are
+ * either characterized by a FourCC format code (such as XR24, RX24, BX24...)
+ */
+static inline u32 is_xrgb(u32 drm)
+{
+	return ((drm & 'X') == 'X' || (drm & ('X' << 8)) == ('X' << 8));
+}
+
 static irqreturn_t ltdc_irq_thread(int irq, void *arg)
 {
 	struct drm_device *ddev = arg;
@@ -431,7 +445,8 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc,
 	pm_runtime_get_sync(ddev->dev);
 
 	/* Sets the background color value */
-	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
+	reg_write(ldev->regs, LTDC_BCCR,
+		  DRM_ARGB_TO_LTDC_RGB24(crtc->state->bgcolor));
 
 	/* Enable IRQ */
 	reg_set(ldev->regs, LTDC_IER, IER_RRIE | IER_FUIE | IER_TERRIE);
@@ -452,6 +467,9 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc *crtc,
 
 	drm_crtc_vblank_off(crtc);
 
+	/* Reset background color */
+	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
+
 	/* disable IRQ */
 	reg_clear(ldev->regs, LTDC_IER, IER_RRIE | IER_FUIE | IER_TERRIE);
 
@@ -790,6 +808,7 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
 	u32 y1 = newstate->crtc_y + newstate->crtc_h - 1;
 	u32 src_x, src_y, src_w, src_h;
 	u32 val, pitch_in_bytes, line_length, paddr, ahbp, avbp, bpcr;
+	u32 bgcolor = DRM_ARGB_TO_LTDC_RGB24(newstate->crtc->state->bgcolor);
 	enum ltdc_pix_fmt pf;
 
 	if (!newstate->crtc || !fb) {
@@ -853,10 +872,28 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
 	if (!fb->format->has_alpha)
 		val = BF1_CA | BF2_1CA;
 
-	/* Manage hw-specific capabilities */
-	if (ldev->caps.non_alpha_only_l1 &&
-	    plane->type != DRM_PLANE_TYPE_PRIMARY)
-		val = BF1_PAXCA | BF2_1PAXCA;
+	/*
+	 * Manage hw-specific capabilities
+	 *
+	 * Depending on the hardware version, the background color can not be
+	 * properly displayed with non-alpha color formats derived from native
+	 * alpha color formats (such as XR24 or XR15) since the use of this
+	 * pixel format generates a non transparent layer. As a workaround,
+	 * the stage background color of the layer and the general background
+	 * color need to be synced.
+	 *
+	 * This is done by activating for all XRGB color format the default
+	 * color as the background color and then setting blending factor
+	 * accordingly.
+	 */
+	if (ldev->caps.non_alpha_only_l1) {
+		if (is_xrgb(fb->format->format)) {
+			val = BF1_CA | BF2_1CA;
+			reg_write(ldev->regs, LTDC_L1DCCR + lofs, bgcolor);
+		} else {
+			val = BF1_PAXCA | BF2_1PAXCA;
+		}
+	}
 
 	reg_update_bits(ldev->regs, LTDC_L1BFCR + lofs,
 			LXBFCR_BF2 | LXBFCR_BF1, val);
@@ -1033,6 +1070,7 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
 
 	drm_crtc_helper_add(crtc, &ltdc_crtc_helper_funcs);
 
+	drm_crtc_add_bgcolor_property(crtc);
 	drm_mode_crtc_set_gamma_size(crtc, CLUT_SIZE);
 	drm_crtc_enable_color_mgmt(crtc, 0, false, CLUT_SIZE);
 
-- 
2.17.1

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/2] drm/stm: ltdc: add crtc background color property support

[Pekka Paalanen]

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

On Wed, 7 Jul 2021 08:48:55 +0000
Raphael GALLAIS-POU - foss <raphael.gallais-pou@foss.st.com> wrote:

> This patch comes from the need to display small resolution pictures with
> very few DDR usage. In practice, using a background color, produced by the
> drm CRTC, around this picture allows to fetch less data in memory than
> setting a full frame picture. And therefore the picture in DDR is smaller
> than the size of the screen.
> 
> It uses the DRM framework background color property and modifies the
> color to any value between 0x000000 and 0xFFFFFF from userland with a
> RGB24 value (0x00RRGGBB).
> 
> Using this feature is observable only if layers are not full screen
> or if layers use color formats with alpha and are "transparent" at
> least on some pixels.
> 
> Depending on the hardware version, the background color can not be
> properly displayed with non-alpha color formats derived from native
> alpha color formats (such as XR24 or XR15) since the use of this
> pixel format generates a non transparent layer. As a workaround,
> the stage background color of the layer and the general background
> color need to be synced.
> 
> Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com>
> ---
>  drivers/gpu/drm/stm/ltdc.c | 48 ++++++++++++++++++++++++++++++++++----
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
> index 1f9392fb58e1..0aca245288cc 100644
> --- a/drivers/gpu/drm/stm/ltdc.c
> +++ b/drivers/gpu/drm/stm/ltdc.c
> @@ -196,6 +196,11 @@
>  
>  #define NB_PF		8		/* Max nb of HW pixel format */
>  
> +#define DRM_ARGB_TO_LTDC_RGB24(bgcolor) \
> +	((u32)(DRM_ARGB_RED(bgcolor, 8) << 16	\
> +	| DRM_ARGB_GREEN(bgcolor, 8) << 8	\
> +	| DRM_ARGB_BLUE(bgcolor, 8)))
> +
>  enum ltdc_pix_fmt {
>  	PF_NONE,
>  	/* RGB formats */
> @@ -364,6 +369,15 @@ static inline u32 get_pixelformat_without_alpha(u32 drm)
>  	}
>  }
>  
> +/*
> + * All non-alpha color formats derived from native alpha color formats are
> + * either characterized by a FourCC format code (such as XR24, RX24, BX24...)
> + */
> +static inline u32 is_xrgb(u32 drm)
> +{
> +	return ((drm & 'X') == 'X' || (drm & ('X' << 8)) == ('X' << 8));

Hi,

if you're trying to test whether the last or second last byte in the
format code is exactly 'X', this doesn't do that. What this does is
ignores all the bits that are zero in 'X' and ensures that all the bits
that are one in 'X' are also one in the tested value.


Thanks,
pq

> +}
> +
>  static irqreturn_t ltdc_irq_thread(int irq, void *arg)
>  {
>  	struct drm_device *ddev = arg;
> @@ -431,7 +445,8 @@ static void ltdc_crtc_atomic_enable(struct drm_crtc *crtc,
>  	pm_runtime_get_sync(ddev->dev);
>  
>  	/* Sets the background color value */
> -	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
> +	reg_write(ldev->regs, LTDC_BCCR,
> +		  DRM_ARGB_TO_LTDC_RGB24(crtc->state->bgcolor));
>  
>  	/* Enable IRQ */
>  	reg_set(ldev->regs, LTDC_IER, IER_RRIE | IER_FUIE | IER_TERRIE);
> @@ -452,6 +467,9 @@ static void ltdc_crtc_atomic_disable(struct drm_crtc *crtc,
>  
>  	drm_crtc_vblank_off(crtc);
>  
> +	/* Reset background color */
> +	reg_write(ldev->regs, LTDC_BCCR, BCCR_BCBLACK);
> +
>  	/* disable IRQ */
>  	reg_clear(ldev->regs, LTDC_IER, IER_RRIE | IER_FUIE | IER_TERRIE);
>  
> @@ -790,6 +808,7 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
>  	u32 y1 = newstate->crtc_y + newstate->crtc_h - 1;
>  	u32 src_x, src_y, src_w, src_h;
>  	u32 val, pitch_in_bytes, line_length, paddr, ahbp, avbp, bpcr;
> +	u32 bgcolor = DRM_ARGB_TO_LTDC_RGB24(newstate->crtc->state->bgcolor);
>  	enum ltdc_pix_fmt pf;
>  
>  	if (!newstate->crtc || !fb) {
> @@ -853,10 +872,28 @@ static void ltdc_plane_atomic_update(struct drm_plane *plane,
>  	if (!fb->format->has_alpha)
>  		val = BF1_CA | BF2_1CA;
>  
> -	/* Manage hw-specific capabilities */
> -	if (ldev->caps.non_alpha_only_l1 &&
> -	    plane->type != DRM_PLANE_TYPE_PRIMARY)
> -		val = BF1_PAXCA | BF2_1PAXCA;
> +	/*
> +	 * Manage hw-specific capabilities
> +	 *
> +	 * Depending on the hardware version, the background color can not be
> +	 * properly displayed with non-alpha color formats derived from native
> +	 * alpha color formats (such as XR24 or XR15) since the use of this
> +	 * pixel format generates a non transparent layer. As a workaround,
> +	 * the stage background color of the layer and the general background
> +	 * color need to be synced.
> +	 *
> +	 * This is done by activating for all XRGB color format the default
> +	 * color as the background color and then setting blending factor
> +	 * accordingly.
> +	 */
> +	if (ldev->caps.non_alpha_only_l1) {
> +		if (is_xrgb(fb->format->format)) {
> +			val = BF1_CA | BF2_1CA;
> +			reg_write(ldev->regs, LTDC_L1DCCR + lofs, bgcolor);
> +		} else {
> +			val = BF1_PAXCA | BF2_1PAXCA;
> +		}
> +	}
>  
>  	reg_update_bits(ldev->regs, LTDC_L1BFCR + lofs,
>  			LXBFCR_BF2 | LXBFCR_BF1, val);
> @@ -1033,6 +1070,7 @@ static int ltdc_crtc_init(struct drm_device *ddev, struct drm_crtc *crtc)
>  
>  	drm_crtc_helper_add(crtc, &ltdc_crtc_helper_funcs);
>  
> +	drm_crtc_add_bgcolor_property(crtc);
>  	drm_mode_crtc_set_gamma_size(crtc, CLUT_SIZE);
>  	drm_crtc_enable_color_mgmt(crtc, 0, false, CLUT_SIZE);
>  


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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Simon Ser]

Hi,

Thanks for working on this. Do you have plans for user-space
implementations and IGT?

Thanks,

Simon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Daniel Vetter]

On Wed, Jul 07, 2021 at 09:03:03AM +0000, Simon Ser wrote:
> Hi,
> 
> Thanks for working on this. Do you have plans for user-space
> implementations and IGT?

Note that these parts are mandatory, and there's a patch floating around
further clarifying what's all expected for new properties:

https://lore.kernel.org/dri-devel/20210706161244.1038592-1-maxime@cerno.tech/

Cheers, Daniel

> 
> Thanks,
> 
> Simon

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Raphael Gallais-Pou]

On 7/7/21 1:42 PM, Daniel Vetter wrote:
> On Wed, Jul 07, 2021 at 09:03:03AM +0000, Simon Ser wrote:
>> Hi,
>>
>> Thanks for working on this. Do you have plans for user-space
>> implementations and IGT?
> Note that these parts are mandatory, and there's a patch floating around
> further clarifying what's all expected for new properties:
>
> https://lore.kernel.org/dri-devel/20210706161244.1038592-1-maxime@cerno.tech/


Hi,


We don't usually test with piglit and igt-gpu-tools. Instead, modetest 
utility of the libdrm is used quite often (as is it the case in order to 
test this property).


We plan to port those tools on our platform before implementing this 
kind of tests, but it will require a bit more time.

An analysis is currently ongoing.


Furthermore, do you have any advice on top of documentation for 
implementing such tests ?


I was also thinking about implementing an option into modetest to ease 
the use of this drm property (support of hexadecimal values for properties).


Regards,

Raphaël

>
> Cheers, Daniel
>
>> Thanks,
>>
>> Simon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Simon Ser]

On Friday, July 9th, 2021 at 11:09, Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:

> We don't usually test with piglit and igt-gpu-tools. Instead, modetest
> utility of the libdrm is used quite often (as is it the case in order to
> test this property).

Just to make it extra clear: regardless of how you bring up your driver
implementation, without an IGT test and real-world open-source user-space
patches that make use of the new prop, your patches *cannot* be merged.

If you're planning to add support for the new prop to an open-source KMS
client, please add a link to the patches in your kernel submission. If
you don't have plans to use the new prop in an open-source KMS client,
let us know and we can discuss what the best candidate would be.

> I was also thinking about implementing an option into modetest to ease
> the use of this drm property (support of hexadecimal values for properties).

(For the record, modeset doesn't count as a real-world user-space usage: it's
just a toy implementation, just a test tool.)

Simon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] Add "BACKGROUND_COLOR" drm property

[Raphael Gallais-Pou]

On 7/9/21 11:23 AM, Simon Ser wrote:
> On Friday, July 9th, 2021 at 11:09, Raphael Gallais-Pou <raphael.gallais-pou@foss.st.com> wrote:
>
>> We don't usually test with piglit and igt-gpu-tools. Instead, modetest
>> utility of the libdrm is used quite often (as is it the case in order to
>> test this property).
> Just to make it extra clear: regardless of how you bring up your driver
> implementation, without an IGT test and real-world open-source user-space
> patches that make use of the new prop, your patches *cannot* be merged.
>
> If you're planning to add support for the new prop to an open-source KMS
> client, please add a link to the patches in your kernel submission. If
> you don't have plans to use the new prop in an open-source KMS client,
> let us know and we can discuss what the best candidate would be.

Hi,

Thanks for your feedback :)


We do not have any KMS client currently using this property apart from proprietary solutions made by consumers.

This property was originally made to display a logo picture when the screen is in idle (low power/low DDR usage).


Regards,

Raphaël


>> I was also thinking about implementing an option into modetest to ease
>> the use of this drm property (support of hexadecimal values for properties).
> (For the record, modeset doesn't count as a real-world user-space usage: it's
> just a toy implementation, just a test tool.)
>
> Simon

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel