[Intel-gfx] [PATCH v2 2/3] drm: Add CRTC background color property (v2)
Ville Syrjälä
ville.syrjala at linux.intel.com
Wed Nov 14 17:20:36 UTC 2018
On Tue, Nov 13, 2018 at 03:21:48PM -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.
>
> 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>
> ---
> 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 | 26 ++++++++++++++++++++++++++
> 8 files changed, 79 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;
Wondering a bit about the granulairty of these flags. Is bgcolor
important enough to warrant its own flag, or should it just be part of
color_mgmt_changed for example?
color_mgmt_changed is rather heavy though as it would force LUT
reprogramming, so I've been thinking that it should perhaps be
split into separate
gamma_lut_changed/degamma_lut_changed/other_color_stuff_changed
flags.
Anyways, just thinking out loud here. We can combine some
of these flags later if we start to accumulate too many.
> 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..7c73cb83874a 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 drivers that do not expose this property.
> */
>
> /**
> @@ -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_rgba(16, 0, 0, 0, 0xffff));
> +}
> +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..d78f82f954e4 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;
>
> /**
> * @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_rgba();
> + * individual color components can be extracted with desired precision
> + * via the DRM_RGBA_*() macros.
> + */
> + u64 bgcolor;
> +
> /**
> * @target_vblank:
> *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 5dbeabdbaf91..8b686d228feb 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: RGBA 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..7c4f902aa290 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -888,6 +888,32 @@ struct drm_mode_revoke_lease {
> __u32 lessee_id;
> };
>
> +/*
> + * Put RGBA 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_rgba(__u8 bpc, __u16 red, __u16 green, __u16 blue, __u16 alpha)
> +{
> + 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;
So the value is now ARGB but everything still says RGBA?
> +}
> +
> +/*
> + * Extract the specified number of bits of a specific color component from a
> + * standard 64-bit RGBA value.
> + */
> +#define DRM_RGBA_BLUE(c, numbits) (__u16)((c & 0xFFFFull) >> (16-numbits))
> +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> +#define DRM_RGBA_RED(c, numbits) (__u16)((c & 0xFFFFull<<32) >> (48-numbits))
> +#define DRM_RGBA_ALPHA(c, numbits) (__u16)((c & 0xFFFFull<<48) >> (64-numbits))
> +
> #if defined(__cplusplus)
> }
> #endif
> --
> 2.14.4
--
Ville Syrjälä
Intel
More information about the Intel-gfx
mailing list