Re: [PATCH v2 2/3] drm: Add CRTC background color property (v2)

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

 



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




[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux