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
