Re: [PATCH 1/2] drm: Add CRTC background color property

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 10, 2018 at 04:50:50PM -0700, 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.
> 
> Cc: dri-devel@lists.freedesktop.org
> Cc: wei.c.li@intel.com
> Cc: harish.krupo.kps@intel.com
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_state_helper.c |  1 +
>  drivers/gpu/drm/drm_atomic_uapi.c         |  5 +++++
>  drivers/gpu/drm/drm_mode_config.c         |  6 ++++++
>  include/drm/drm_crtc.h                    | 17 +++++++++++++++++
>  include/drm/drm_mode_config.h             |  5 +++++
>  include/uapi/drm/drm_mode.h               | 26 ++++++++++++++++++++++++++
>  6 files changed, 60 insertions(+)
> 
> 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 d5b7f315098c..399f0ead5370 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->background_color = 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->background_color;
>  	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_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_crtc.h b/include/drm/drm_crtc.h
> index b21437bc95bf..ddfdad9ccecb 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;
>  
> +	/**
> +	 * @background_color:
> +	 *
> +	 * 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 background_color;
> +
>  	/**
>  	 * @target_vblank:
>  	 *
> diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h
> index 928e4172a0bb..c3f57a9e5b31 100644
> --- a/include/drm/drm_mode_config.h
> +++ b/include/drm/drm_mode_config.h
> @@ -807,6 +807,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;
>  	bool quirk_addfb_prefer_xbgr_30bpp;
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index d3e0fe31efc5..66e2e2c2630e 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)blue  << msb_shift << 32 |
> +	       (__u64)green << msb_shift << 16 |
> +	       (__u64)red   << msb_shift;

I would still call that abgr in drm terminology (eg. when compared
to the fourccs). And I'd still prefer the argb order as it's what
our hardware expects anyway :) Would make me less confused when
looking at a 64bit hex color value at least.


> +}
> +
> +/*
> + * Extract the specified number of bits of a specific color component from a
> + * standard 64-bit RGBA value.
> + */
> +#define DRM_RGBA_RED(c, numbits)   (__u16)((c & 0xFFFFull)     >> (16-numbits))
> +#define DRM_RGBA_GREEN(c, numbits) (__u16)((c & 0xFFFFull<<16) >> (32-numbits))
> +#define DRM_RGBA_BLUE(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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx