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

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

 



On Wed, Jan 30, 2019 at 06:11:16PM -0800, Matt Roper wrote:
> On Wed, Jan 30, 2019 at 11:01:25PM +0200, Ville Syrjälä wrote:
> > On Wed, Jan 30, 2019 at 10:51:21AM -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)
> > > 
> > > v4:
> > >  - Drop the bgcolor_changed flag.  (Ville, Brian Starkey)
> > >  - Clarify in kerneldoc that background color is expected to undergo the
> > >    same pipe-level degamma/csc/gamma transformations that planes do.
> > >    (Brian Starkey)
> > >  - Update kerneldoc to indicate non-opaque colors are allowed, but are
> > >    generally only useful in special cases such as when writeback
> > >    connectors are used (Brian Starkey / Eric Anholt)
> > > 
> > > 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>
> > > Cc: Brian Starkey <Brian.Starkey@xxxxxxx>
> > > Cc: Eric Anholt <eric@xxxxxxxxxx>
> > > Cc: Stéphane Marchesin <marcheu@xxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Reviewed-by(v2): Sean Paul <sean@xxxxxxxxxx>
> > > Reviewed-by: Brian Starkey <brian.starkey@xxxxxxx>
> > > ---
> > >  drivers/gpu/drm/drm_atomic_uapi.c |  4 ++++
> > >  drivers/gpu/drm/drm_blend.c       | 27 ++++++++++++++++++++++++---
> > >  drivers/gpu/drm/drm_mode_config.c |  6 ++++++
> > >  include/drm/drm_blend.h           |  1 +
> > >  include/drm/drm_crtc.h            | 12 ++++++++++++
> > >  include/drm/drm_mode_config.h     |  5 +++++
> > >  include/uapi/drm/drm_mode.h       | 28 ++++++++++++++++++++++++++++
> > >  7 files changed, 80 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
> > > index 9a1f41adfc67..d569e20e34e3 100644
> > > --- a/drivers/gpu/drm/drm_atomic_uapi.c
> > > +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> > > @@ -469,6 +469,8 @@ 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;
> > >  	} else if (crtc->funcs->atomic_set_property) {
> > >  		return crtc->funcs->atomic_set_property(crtc, state, property, val);
> > >  	} else {
> > > @@ -503,6 +505,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..d451ac9e1d6d 100644
> > > --- a/drivers/gpu/drm/drm_blend.c
> > > +++ b/drivers/gpu/drm/drm_blend.c
> > > @@ -175,9 +175,22 @@
> > >   *		 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:
> > > + *
> > > + * BACKGROUND_COLOR:
> > > + *	Background color is setup with drm_crtc_add_bgcolor_property().  It
> > > + *	controls the ARGB color of a full-screen 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.  Although
> > > + *	background color isn't a plane, it is assumed that the color provided
> > > + *	here undergoes the same pipe-level degamma/CSC/gamma transformations
> > > + *	that planes undergo.  Note that the color value provided here includes
> > > + *	an alpha channel...non-opaque background color values are allowed,
> > > + *	but are generally only honored in special cases (e.g., when a memory
> > > + *	writeback connector is in use).
> > 
> > What would the alpha<1.0 do in that case? Blend the writeback output
> > with what's already there?
> 
> I think it's more for cases where your hardware's pipe writes back
> actual ARGB data to memory, including an alpha component, rather than
> just RGB or XRGB.  You could then take that ARGB writeback buffer and
> pass it through some other form of useful compositing if the bgcolor
> pixels have a non-opaque alpha.

I guess someone should specify the blend equation for the alpha
channel as well. I presume it should just be 'da = sa + (1-sa)*da'
regardless of the plane pre-multiply setting?

> 
> 
> Matt
> 
> > 
> > >   */
> > >  
> > >  /**
> > > @@ -593,3 +606,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));
> > 
> > if (crtc->state)
> >  crtc->state->bg = default value; ?
> > 
> > > +}
> > > +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 4a1c2023ccf0..8a7c346b3191 100644
> > > --- a/drivers/gpu/drm/drm_mode_config.c
> > > +++ b/drivers/gpu/drm/drm_mode_config.c
> > > @@ -364,6 +364,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 85abd3fe9e83..dbe0b45d5da6 100644
> > > --- a/include/drm/drm_crtc.h
> > > +++ b/include/drm/drm_crtc.h
> > > @@ -274,6 +274,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 1e6cb885994d..0463d3f4ae59 100644
> > > --- a/include/drm/drm_mode_config.h
> > > +++ b/include/drm/drm_mode_config.h
> > > @@ -836,6 +836,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 a439c2e67896..5f31e6a05bd9 100644
> > > --- a/include/uapi/drm/drm_mode.h
> > > +++ b/include/uapi/drm/drm_mode.h
> > > @@ -907,6 +907,34 @@ struct drm_mode_rect {
> > >  	__s32 y2;
> > >  };
> > >  
> > > +/*
> > > + * 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.
> > > + */
> > > +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.5
> > 
> > -- 
> > Ville Syrjälä
> > Intel
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> IoTG Platform Enabling & Development
> Intel Corporation
> (916) 356-2795

-- 
Ville Syrjälä
Intel
_______________________________________________
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