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

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

 



On Wed, Nov 18, 2015 at 01:35:54PM -0800, Bob Paauwe wrote:
> On Thu, 22 Oct 2015 17:25:34 -0700
> Matt Roper <matthew.d.roper@xxxxxxxxx> wrote:
> 
> > To support CRTC background color, we need a way of communicating RGB
> > color values to the DRM.  However there is often a mismatch between how
> > userspace wants to represent the color value vs how it must be
> > programmed into the hardware; this mismatch can easily lead to
> > non-obvious bugs.  Let's create a property type that standardizes the
> > user<->kernel format and add some macros that allow drivers to extract
> > the bits they care about without having to worry about the internal
> > representation.
> > 
> > These properties are still exposed to userspace as range properties, so
> > the only userspace change we need are some helpers to build RGBA values
> > appropriately.
> > 
> > Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> > Cc: Chandra Konduru <chandra.konduru@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> >  drivers/gpu/drm/drm_atomic.c |  4 ++++
> >  drivers/gpu/drm/drm_crtc.c   | 33 +++++++++++++++++++++++++++++
> >  include/drm/drm_crtc.h       | 49 ++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 86 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 7bb3845..688ca75 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -415,6 +415,8 @@ int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
> >  
> >  	if (property == config->prop_active)
> >  		state->active = val;
> > +	else if (property == config->prop_background_color)
> > +		state->background_color.v = val;
> >  	else if (property == config->prop_mode_id) {
> >  		struct drm_property_blob *mode =
> >  			drm_property_lookup_blob(dev, val);
> > @@ -450,6 +452,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> >  		*val = state->active;
> >  	else if (property == config->prop_mode_id)
> >  		*val = (state->mode_blob) ? state->mode_blob->base.id : 0;
> > +	else if (property == config->prop_background_color)
> > +		*val = state->background_color.v;
> >  	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_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index e54660a..1e0dd09 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -3807,6 +3807,30 @@ struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
> >  EXPORT_SYMBOL(drm_property_create_bool);
> >  
> >  /**
> > + * drm_property_create_rgba - create a new RGBA property type
> > + * @dev: drm device
> > + * @flags: flags specifying the property type
> > + * @name: name of the property
> > + *
> > + * This creates a new generic drm property which can then be attached to a drm
> > + * object with drm_object_attach_property. The returned property object must be
> > + * freed with drm_property_destroy.
> > + *
> > + * Userspace should use the DRM_RGBA() macro to build values with the proper
> > + * bit layout.
> > + *
> > + * Returns:
> > + * A pointer to the newly created property on success, NULL on failure.
> > + */
> > +struct drm_property *drm_property_create_rgba(struct drm_device *dev, int flags,
> > +					      const char *name)
> > +{
> > +	return drm_property_create_range(dev, flags, name,
> > +					 0, GENMASK_ULL(63, 0));
> > +}
> > +EXPORT_SYMBOL(drm_property_create_rgba);
> 
> I'm not sure I understand why drm_property_create_rgba was added.  It's
> not being used.  Maybe if the
> drm_mode_create_background_color_property() below called this instead
> of create_range directly, we could then change the underlying property
> type used for rgba without having to locate all properties that are
> supposed to be of that type.  Was that the intention?

Yeah, I meant to use this for the background_color_property function
below, but I guess I forgot to go back and actually make that change.
This is more just a helper so that drivers that make driver-specific
rgba properties won't have to think about what the internal
representation really is (the same way boolean properties are really
just a helper for range [0,1] properties).

> 
> > +
> > +/**
> >   * drm_property_add_enum - add a possible value to an enumeration property
> >   * @property: enumeration property to change
> >   * @index: index of the new enumeration
> > @@ -5778,6 +5802,15 @@ struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
> >  }
> >  EXPORT_SYMBOL(drm_mode_create_rotation_property);
> >  
> > +struct drm_property
> > +*drm_mode_create_background_color_property(struct drm_device *dev)
> > +{
> > +	return drm_property_create_range(dev, DRM_MODE_PROP_ATOMIC,
> > +					 "background_color",
> > +					 0, GENMASK_ULL(63, 0));
> > +}
> > +EXPORT_SYMBOL(drm_mode_create_background_color_property);
> > +
> >  /**
> >   * DOC: Tile group
> >   *
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 3f0c690..64f3e62 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -251,6 +251,45 @@ struct drm_bridge;
> >  struct drm_atomic_state;
> >  
> >  /**
> > + * drm_rgba_t - RGBA property value type
> > + *
> > + * Structure to abstract away the representation of RGBA values with precision
> > + * up to 16 bits per color component.  This is primarily intended for use with
> > + * DRM properties that need to take a color value since we don't want userspace
> > + * to have to worry about the bit layout expected by the underlying hardware.
> > + *
> > + * We wrap the value in a structure here so that the compiler will flag any
> > + * accidental attempts by driver code to directly attempt bitwise operations
> > + * that could potentially misinterpret the value.  Drivers should instead use
> > + * the DRM_RGBA_{RED,GREEN,BLUE,ALPHA}BITS() macros to obtain the component
> > + * bits and then build values in the format their hardware expects.
> > + */
> > +typedef struct {
> > +	uint64_t v;
> > +} drm_rgba_t;
> > +
> > +static inline uint16_t
> > +drm_rgba_bits(drm_rgba_t c, unsigned compshift, unsigned bits) {
> > +	uint64_t val;
> > +
> > +	if (WARN_ON(bits > 16))
> > +		bits = 16;
> > +
> > +	val = c.v & GENMASK_ULL(compshift + 15, compshift);
> > +	return val >> (compshift + 16 - bits);
> > +}
> > +
> > +/*
> > + * Macros to access the individual color components of an RGBA property value.
> > + * If the requested number of bits is less than 16, only the most significant
> > + * bits of the color component will be returned.
> > + */
> > +#define DRM_RGBA_REDBITS(c, bits)   drm_rgba_bits(c, 48, bits)
> > +#define DRM_RGBA_GREENBITS(c, bits) drm_rgba_bits(c, 32, bits)
> > +#define DRM_RGBA_BLUEBITS(c, bits)  drm_rgba_bits(c, 16, bits)
> > +#define DRM_RGBA_ALPHABITS(c, bits) drm_rgba_bits(c, 0, bits)
> 
> Do we also need macros that drivers can use to create a RGBA value?
> I.E. maybe when doing the attach?
> 

Yeah, makes sense; the macro I have in libdrm should also be available
in the kernel.  I'll add that in the next version.


Matt


> > +
> > +/**
> >   * struct drm_crtc_state - mutable CRTC state
> >   * @crtc: backpointer to the CRTC
> >   * @enable: whether the CRTC should be enabled, gates all other state
> > @@ -264,6 +303,7 @@ struct drm_atomic_state;
> >   * 	update to ensure framebuffer cleanup isn't done too early
> >   * @adjusted_mode: for use by helpers and drivers to compute adjusted mode timings
> >   * @mode: current mode timings
> > + * @background_color: background color of regions not covered by planes
> >   * @event: optional pointer to a DRM event to signal upon completion of the
> >   * 	state update
> >   * @state: backpointer to global drm_atomic_state
> > @@ -304,6 +344,9 @@ struct drm_crtc_state {
> >  	/* blob property to expose current mode to atomic userspace */
> >  	struct drm_property_blob *mode_blob;
> >  
> > +	/* CRTC background color */
> > +	drm_rgba_t background_color;
> > +
> >  	struct drm_pending_vblank_event *event;
> >  
> >  	struct drm_atomic_state *state;
> > @@ -1115,6 +1158,9 @@ struct drm_mode_config {
> >  	struct drm_property *prop_active;
> >  	struct drm_property *prop_mode_id;
> >  
> > +	/* crtc properties */
> > +	struct drm_property *prop_background_color;
> > +
> >  	/* DVI-I properties */
> >  	struct drm_property *dvi_i_subconnector_property;
> >  	struct drm_property *dvi_i_select_subconnector_property;
> > @@ -1365,6 +1411,8 @@ struct drm_property *drm_property_create_object(struct drm_device *dev,
> >  					 int flags, const char *name, uint32_t type);
> >  struct drm_property *drm_property_create_bool(struct drm_device *dev, int flags,
> >  					 const char *name);
> > +struct drm_property *drm_property_create_rgba(struct drm_device *dev,
> > +					      int flags, const char *name);
> >  struct drm_property_blob *drm_property_create_blob(struct drm_device *dev,
> >                                                     size_t length,
> >                                                     const void *data);
> > @@ -1495,6 +1543,7 @@ extern int drm_format_vert_chroma_subsampling(uint32_t format);
> >  extern const char *drm_get_format_name(uint32_t format);
> >  extern struct drm_property *drm_mode_create_rotation_property(struct drm_device *dev,
> >  							      unsigned int supported_rotations);
> > +extern struct drm_property *drm_mode_create_background_color_property(struct drm_device *dev);
> >  extern unsigned int drm_rotation_simplify(unsigned int rotation,
> >  					  unsigned int supported_rotations);
> >  
> 
> 
> 
> -- 
> --
> Bob Paauwe                  
> Bob.J.Paauwe@xxxxxxxxx
> IOTG / PED Software Organization
> Intel Corp.  Folsom, CA
> (916) 356-6193    
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux