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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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-devel@xxxxxxxxxxxxxxxxxxxxx
>Cc: wei.c.li@xxxxxxxxx
>Cc: harish.krupo.kps@xxxxxxxxx
>Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>Cc: Sean Paul <sean@xxxxxxxxxx>
>Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
>Reviewed-by(v2): Sean Paul <sean@xxxxxxxxxx>
>---
> 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 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.

>  */
>
> /**
>@@ -593,3 +600,11 @@ int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> 	return 0;
> }
> EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
>+
>+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 ee80788f2c40..75e376755176 100644
>--- a/drivers/gpu/drm/drm_mode_config.c
>+++ b/drivers/gpu/drm/drm_mode_config.c
>@@ -352,6 +352,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 b21437bc95bf..8f13d485df60 100644
>--- a/include/drm/drm_crtc.h
>+++ b/include/drm/drm_crtc.h
>@@ -168,6 +168,11 @@ struct drm_crtc_state {
> 	 * drivers to steer the atomic commit control flow.
> 	 */
> 	bool color_mgmt_changed : 1;
>+	/**
>+	 * @bgcolor_changed: Background color value has changed.  Used by
>+	 * drivers to steer the atomic commit control flow.
>+	 */
>+	bool bgcolor_changed : 1;

On the back of Ville's comment on v2, I think I'd rather have these
flags being the exception rather than the norm. I mean, I think
color_mgmt_changed can be worthwhile, because an update might be
expensive - however changing the bg color I'd expect to be ~1 register
write on most hardware, so the flag hardly seems worth it. Happy to
hear different opinions though.

>
> 	/**
> 	 * @no_vblank:
>@@ -274,6 +279,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 5dbeabdbaf91..f31ce517c70d 100644
>--- a/include/drm/drm_mode_config.h
>+++ b/include/drm/drm_mode_config.h
>@@ -813,6 +813,11 @@ struct drm_mode_config {
> 	 */
> 	struct drm_property *writeback_out_fence_ptr_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 d3e0fe31efc5..47088cae69e7 100644
>--- a/include/uapi/drm/drm_mode.h
>+++ b/include/uapi/drm/drm_mode.h
>@@ -888,6 +888,34 @@ struct drm_mode_revoke_lease {
> 	__u32 lessee_id;
> };
>
>+/*
>+ * 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.
>+ */

It's a bit of a future problem - but I wonder if we shouldn't have a
bit more "future-proof" representation here. scRGB and extended-range
sRGB are things, which allow values outside the range 0-1.

I'm also thinking of Android where the layer solid color values in
HWComposer got changed from 8-bit uints to floats in 'P'. If we could
just have one color representation in the kernel API that might be
nice.

Thanks,
-Brian

>+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.14.4
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux