Re: [Intel-gfx] [PATCH v3 2/3] drm: Add CRTC background color property (v3)

[Eric Anholt]

Brian Starkey  writes:

> Hi Matt,
>
> On Thu, Nov 15, 2018 at 02:13:45PM -0800, Matt Roper 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.
>>
>>v2:
>> - Swap internal representation's blue and red bits to make it easier
>>   to read if printed out.  (Ville)
>> - Document bgcolor property in drm_blend.c.  (Sean Paul)
>> - s/background_color/bgcolor/ for consistency between property name and
>>   value storage field.  (Sean Paul)
>> - Add a convenience function to attach property to a given crtc.
>>
>>v3:
>> - Restructure ARGB component extraction macros to be easier to
>>   understand and enclose the parameters in () to avoid calculations
>>   if expressions are passed.  (Sean Paul)
>> - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>>   to not worry about the internal representation of the u64, it can
>>   still be confusing to look at code that uses 'rgba' terminology, but
>>   stores values with argb ordering.  (Ville)
>>
>>Cc: dri-de...@lists.freedesktop.org
>>Cc: wei.c...@intel.com
>>Cc: harish.krupo@intel.com
>>Cc: Ville Syrjälä 
>>Cc: Sean Paul 
>>Signed-off-by: Matt Roper 
>>Reviewed-by(v2): Sean Paul 
>>---

>
>>+ *   Background color is setup with drm_crtc_add_bgcolor_property().  It
>>+ *   controls the RGB color of a full-screen, fully-opaque 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.
>
> Might be worth explicitly mentioning that this value is used as-is,
> without any gamma/gamut conversion before blending, i.e. it's assumed
> to already be in whatever blend-space the CRTC is using (at least I
> assume that's the case?). As we start making the color pipe more
> complicated/explicit, the details matter more.

Raspberry Pi should be able to do background color as well, but the
question of where this property is in the pipeline (particularly for CTM
and gamma) also came up for me.  My background color would apply at the
same stage as plane composition, so before gamma and CTM.

FWIW, my background color on the writeback connector (the only one where
the value would be relevant) can have alpha.


signature.asc
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 2/3] drm: Add CRTC background color property (v3)

[Brian Starkey]

Hi Matt,

On Thu, Nov 15, 2018 at 02:13:45PM -0800, Matt Roper 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.
>
>v2:
> - Swap internal representation's blue and red bits to make it easier
>   to read if printed out.  (Ville)
> - Document bgcolor property in drm_blend.c.  (Sean Paul)
> - s/background_color/bgcolor/ for consistency between property name and
>   value storage field.  (Sean Paul)
> - Add a convenience function to attach property to a given crtc.
>
>v3:
> - Restructure ARGB component extraction macros to be easier to
>   understand and enclose the parameters in () to avoid calculations
>   if expressions are passed.  (Sean Paul)
> - s/rgba/argb/ in helper function/macro names.  Even though the idea is
>   to not worry about the internal representation of the u64, it can
>   still be confusing to look at code that uses 'rgba' terminology, but
>   stores values with argb ordering.  (Ville)
>
>Cc: dri-de...@lists.freedesktop.org
>Cc: wei.c...@intel.com
>Cc: harish.krupo@intel.com
>Cc: Ville Syrjälä 
>Cc: Sean Paul 
>Signed-off-by: Matt Roper 
>Reviewed-by(v2): Sean Paul 
>---
> drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
> drivers/gpu/drm/drm_atomic_uapi.c |  5 +
> drivers/gpu/drm/drm_blend.c   | 21 ++---
> drivers/gpu/drm/drm_mode_config.c |  6 ++
> include/drm/drm_blend.h   |  1 +
> include/drm/drm_crtc.h| 17 +
> include/drm/drm_mode_config.h |  5 +
> include/uapi/drm/drm_mode.h   | 28 
> 8 files changed, 81 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
>b/drivers/gpu/drm/drm_atomic_state_helper.c
>index 3ba996069d69..2f8c55668089 100644
>--- a/drivers/gpu/drm/drm_atomic_state_helper.c
>+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
>@@ -101,6 +101,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct 
>drm_crtc *crtc,
>   state->planes_changed = false;
>   state->connectors_changed = false;
>   state->color_mgmt_changed = false;
>+  state->bgcolor_changed = false;
>   state->zpos_changed = false;
>   state->commit = NULL;
>   state->event = NULL;
>diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
>b/drivers/gpu/drm/drm_atomic_uapi.c
>index 86ac33922b09..b95a55a778e2 100644
>--- a/drivers/gpu/drm/drm_atomic_uapi.c
>+++ b/drivers/gpu/drm/drm_atomic_uapi.c
>@@ -467,6 +467,9 @@ static int drm_atomic_crtc_set_property(struct drm_crtc 
>*crtc,
>   return -EFAULT;
>
>   set_out_fence_for_crtc(state->state, crtc, fence_ptr);
>+  } else if (property == config->bgcolor_property) {
>+  state->bgcolor = val;
>+  state->bgcolor_changed = true;
>   } else if (crtc->funcs->atomic_set_property) {
>   return crtc->funcs->atomic_set_property(crtc, state, property, 
> val);
>   } else {
>@@ -499,6 +502,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
>   *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
>   else if (property == config->prop_out_fence_ptr)
>   *val = 0;
>+  else if (property == config->bgcolor_property)
>+  *val = state->bgcolor;

The docs say it's always opaque - so perhaps override alpha to all
'1's here, or reject values which aren't opaque?

>   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 0c78ca386cbe..7da28c0cb74d 100644
>--- a/drivers/gpu/drm/drm_blend.c
>+++ b/drivers/gpu/drm/drm_blend.c
>@@ -175,9 +175,16 @@
>  * plane does not expose the "alpha" property, then this is
>  * 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).
>+ * The property extensions described above all apply to the plane.  Drivers
>+ * may also expose the following crtc property extension:
>+ *
>+ * bgcolor:

That should be "BACKGROUND_COLOR" shouldn't it?

>+ *Background color is setup with drm_crtc_add_bgcolor_property().  It
>+ *controls the RGB color 

[Intel-gfx] [PATCH v3 2/3] drm: Add CRTC background color property (v3)

[Matt Roper]

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.

v2:
 - Swap internal representation's blue and red bits to make it easier
   to read if printed out.  (Ville)
 - Document bgcolor property in drm_blend.c.  (Sean Paul)
 - s/background_color/bgcolor/ for consistency between property name and
   value storage field.  (Sean Paul)
 - Add a convenience function to attach property to a given crtc.

v3:
 - Restructure ARGB component extraction macros to be easier to
   understand and enclose the parameters in () to avoid calculations
   if expressions are passed.  (Sean Paul)
 - s/rgba/argb/ in helper function/macro names.  Even though the idea is
   to not worry about the internal representation of the u64, it can
   still be confusing to look at code that uses 'rgba' terminology, but
   stores values with argb ordering.  (Ville)

Cc: dri-de...@lists.freedesktop.org
Cc: wei.c...@intel.com
Cc: harish.krupo@intel.com
Cc: Ville Syrjälä 
Cc: Sean Paul 
Signed-off-by: Matt Roper 
Reviewed-by(v2): Sean Paul 
---
 drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
 drivers/gpu/drm/drm_atomic_uapi.c |  5 +
 drivers/gpu/drm/drm_blend.c   | 21 ++---
 drivers/gpu/drm/drm_mode_config.c |  6 ++
 include/drm/drm_blend.h   |  1 +
 include/drm/drm_crtc.h| 17 +
 include/drm/drm_mode_config.h |  5 +
 include/uapi/drm/drm_mode.h   | 28 
 8 files changed, 81 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c 
b/drivers/gpu/drm/drm_atomic_state_helper.c
index 3ba996069d69..2f8c55668089 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -101,6 +101,7 @@ void __drm_atomic_helper_crtc_duplicate_state(struct 
drm_crtc *crtc,
state->planes_changed = false;
state->connectors_changed = false;
state->color_mgmt_changed = false;
+   state->bgcolor_changed = false;
state->zpos_changed = false;
state->commit = NULL;
state->event = NULL;
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c 
b/drivers/gpu/drm/drm_atomic_uapi.c
index 86ac33922b09..b95a55a778e2 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -467,6 +467,9 @@ static int drm_atomic_crtc_set_property(struct drm_crtc 
*crtc,
return -EFAULT;
 
set_out_fence_for_crtc(state->state, crtc, fence_ptr);
+   } else if (property == config->bgcolor_property) {
+   state->bgcolor = val;
+   state->bgcolor_changed = true;
} else if (crtc->funcs->atomic_set_property) {
return crtc->funcs->atomic_set_property(crtc, state, property, 
val);
} else {
@@ -499,6 +502,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
else if (property == config->prop_out_fence_ptr)
*val = 0;
+   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 0c78ca386cbe..7da28c0cb74d 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -175,9 +175,16 @@
  *  plane does not expose the "alpha" property, then this is
  *  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).
+ * The property extensions described above all apply to the plane.  Drivers
+ * may also expose the following crtc property extension:
+ *
+ * bgcolor:
+ * Background color is setup with drm_crtc_add_bgcolor_property().  It
+ * controls the RGB color of a full-screen, fully-opaque 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