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

[Matt Roper]

On Wed, Nov 14, 2018 at 11:17:25AM -0500, Sean Paul wrote:
> 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(-)
> 
> /snip
> 
> > 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;
> > +}
> > +
> > +/*
> > + * 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))
> 
> 
> #define  DRM_RGBA_COMP(c, shift, numbits) \
>         (__u16)(((c) & 0xFFFFull << (shift)) >> (32 - ((shift) + 16 - (numbits)))

I think this should just be

    #define  DRM_RGBA_COMP(c, shift, numbits) \
          (__u16)(((c) & 0xFFFFull << (shift)) >> ((shift) + 16 - (numbits)))

right?  I.e., right shift the same amount as the left shift, plus an
additional (16-numbits) to account for the lower precision requested.


Matt

> 
> #define DRM_RGBA_BLUE(c, numbits)  DRM_RGBA_COMP(c, 0, numbits)
> #define DRM_RGBA_GREEN(c, numbits) DRM_RGBA_COMP(c, 16, numbits)
> #define DRM_RGBA_RED(c, numbits)   DRM_RGBA_COMP(c, 32, numbits)
> #define DRM_RGBA_ALPHA(c, numbits) DRM_RGBA_COMP(c, 48, numbits)
> 
> 
> With that,
> 
> Reviewed-by: Sean Paul <sean at poorly.run>
> 
> 
> 
> > +
> >  #if defined(__cplusplus)
> >  }
> >  #endif
> > -- 
> > 2.14.4
> > 
> 
> -- 
> Sean Paul, Software Engineer, Google / Chromium OS

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795