[Intel-gfx] [PATCH v3 2/3] drm: Add CRTC background color property (v3)
Brian Starkey
Brian.Starkey at arm.com
Mon Nov 19 16:52:23 UTC 2018
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 at lists.freedesktop.org
>Cc: wei.c.li at intel.com
>Cc: harish.krupo.kps at intel.com
>Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>Cc: Sean Paul <sean at poorly.run>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>Reviewed-by(v2): Sean Paul <sean at poorly.run>
>---
> 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 at lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list