Re: [PATCH RFC v3] drm: Add optinal COLOR_ENCODING and COLOR_RANGE properties to drm_plane

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

 



On Mon, May 15, 2017 at 05:25:02PM +0300, Jyri Sarha wrote:
> On 05/15/17 16:34, Ville Syrjälä wrote:
> > On Mon, May 15, 2017 at 02:19:09PM +0300, Jyri Sarha wrote:
> >> Add a standard optinal properties to support different non RGB color
> >> encodings in DRM planes. COLOR_ENCODING select the supported non RGB
> >> color encoding, for instance ITU-R BT.709 YCbCr. COLOR_RANGE selects
> >> the value ranges within the selected color encoding. The properties
> >> are stored to drm_plane object to allow different set of supported
> >> encoding for different planes on the device.
> >>
> >> Signed-off-by: Jyri Sarha <jsarha@xxxxxx>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
> >>  drivers/gpu/drm/drm_color_mgmt.c | 97 ++++++++++++++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/drm_plane.c      |  6 +++
> >>  include/drm/drm_color_mgmt.h     | 20 +++++++++
> >>  include/drm/drm_plane.h          |  8 ++++
> >>  5 files changed, 139 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 4033384..f341dda 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -774,6 +774,10 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >>  		state->rotation = val;
> >>  	} else if (property == plane->zpos_property) {
> >>  		state->zpos = val;
> >> +	} else if (property == plane->color_encoding_property) {
> >> +		state->color_encoding = val;
> >> +	} else if (property == plane->color_range_property) {
> >> +		state->color_range = val;
> >>  	} else if (plane->funcs->atomic_set_property) {
> >>  		return plane->funcs->atomic_set_property(plane, state,
> >>  				property, val);
> >> @@ -834,6 +838,10 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
> >>  		*val = state->rotation;
> >>  	} else if (property == plane->zpos_property) {
> >>  		*val = state->zpos;
> >> +	} else if (property == plane->color_encoding_property) {
> >> +		*val = state->color_encoding;
> >> +	} else if (property == plane->color_range_property) {
> >> +		*val = state->color_range;
> >>  	} else if (plane->funcs->atomic_get_property) {
> >>  		return plane->funcs->atomic_get_property(plane, state, property, val);
> >>  	} else {
> >> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> >> index 533f3a3..2e04ebb 100644
> >> --- a/drivers/gpu/drm/drm_color_mgmt.c
> >> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> >> @@ -33,6 +33,11 @@
> >>   * properties on the &drm_crtc object. They are set up by calling
> >>   * drm_crtc_enable_color_mgmt().
> >>   *
> >> + * Support for different non RGB color encodings is controlled trough
> >> + * plane specific COLOR_ENCODING and COLOR_RANGE properties.
> > 
> > Putting this comment in middle of the crtc stuff seems a bit strange
> > when the rest of the plane stuff is at the end. 
> > 
> 
> Ok. I remove them. Of course in the future some one may want to attach
> those proerties to crtcs too, but that is a different story.

True. But at that point I guess we'll have to reword the comment anyway.

> 
> > This should specify somewhere that these plane properties control
> > the plane input and not the output.
> > 
> 
> Ok. I'll make the documentation more verbose.
> 
> >> + *
> >> + * The &drm_crtc object's properties are:
> >> + *
> >>   * "DEGAMMA_LUT”:
> >>   *	Blob property to set the degamma lookup table (LUT) mapping pixel data
> >>   *	from the framebuffer before it is given to the transformation matrix.
> >> @@ -85,6 +90,18 @@
> >>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
> >>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with the
> >>   * "GAMMA_LUT" property above.
> >> + *
> >> + * The &drm_plane object's properties are:
> >> + *
> >> + * "COLOR_ENCODING"
> >> + * 	Optional plane enum property to support different non RGB
> >> + * 	color encodings. The driver can provide a subset of standard
> >> + * 	enum values supported by the DRM plane.
> >> + *
> >> + * "COLOR_RANGE"
> >> + * 	Optional plane enum property to support different non RGB
> >> + * 	color parameter ranges. The driver can provide a subset of
> >> + * 	standard enum values supported by the DRM plane.
> >>   */
> >>  
> >>  /**
> >> @@ -333,3 +350,83 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >>  	drm_modeset_unlock(&crtc->mutex);
> >>  	return ret;
> >>  }
> >> +
> >> +static char *color_encoding_name[] = {
> > 
> > Missing a few consts.
> > 
> >> +	[DRM_COLOR_YCBCR_BT601] = "ITU-R BT.601 YCbCr",
> >> +	[DRM_COLOR_YCBCR_BT709] = "ITU-R BT.709 YCbCr",
> >> +	[DRM_COLOR_YCBCR_BT2020] = "ITU-R BT.2020 YCbCr",
> >> +};
> >> +
> >> +static char *color_range_name[] = {
> > 
> > ditto.
> > 
> 
> Will fix.
> 
> >> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr FULL RANGE",
> >> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr LIMITED RANGE",
> > 
> > Lowercase for the strings maybe? I guess we don't have any consistent
> > style in this regard.
> > 
> 
> Lower case of everything but YCbCr, right?

That would work for me. But as stated, we already totally lack a
consistent style so I guess it doesn't matter too much in the end.
Same goes for the prop names, basic atomic props are all caps, 
zpos/rotation are all lowercase, and many other props are somewhere
in between.

> 
> >> +};
> >> +
> >> +/**
> >> + * drm_plane_create_color_properties - color encoding related plane properties
> >> + * @supported_encodings: bitfield indicating supported color encdings
> >> + * @supported_ranges: bitfileld indicating supported color ranges
> >> + * @default_encoding: default color encoding
> >> + * @default_range: default color range
> >> + *
> >> + * Create and attach plane specific COLOR_ENCODING and COLOR_RANGE
> >> + * properties to to the drm_plane object. The supported encodings and
> >> + * ranges should be provided in supported_encodings and
> >> + * supported_ranges bitmasks. Each bit set in the bitmast indicates
> >> + * the its number as enum value being supported.
> >> + */
> >> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >> +				      u32 supported_encodings,
> >> +				      u32 supported_ranges,
> >> +				      enum drm_color_encoding default_encoding,
> >> +				      enum drm_color_range default_range)
> >> +{
> >> +	struct drm_device *dev = plane->dev;
> >> +	struct drm_property *prop;
> >> +	struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
> >> +						DRM_COLOR_RANGE_MAX)];
> >> +	unsigned int i, len;
> > 
> > 'unsigned int i' might surprise someone if they ever try to change the
> > loop to go backwards. Better make it signed IMO.
> > 
> 
> Ok, will fix.
> 
> >> +
> >> +
> >> +	if (WARN_ON(plane->color_encoding_property != NULL) ||
> >> +	    WARN_ON(plane->color_range_property != NULL))
> >> +		return -EEXIST;
> > 
> > I think I already stated that we don't have these sorts of checks
> > elsewhere, so IMO no point in adding them here either unless you
> > go and change all the other places too.
> > 
> > Just feels like accumulating random differences that will confuse
> > people in the future.
> > 
> >> +
> >> +	len = 0;
> >> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> >> +		if ((BIT(i) & supported_encodings) == 0)
> > 
> > 'foo & BIT(i)' would be the more conventional order.
> > 
> 
> Ok, I'll swap the order.
> 
> >> +			continue;
> >> +
> >> +		enum_list[i].type = i;
> >> +		enum_list[i].name = color_encoding_name[i];
> > 
> > enum_list[len] ?
> > 
> 
> Argh. That's a real bug. Thanks, I'll fix that.
> 
> >> +		len++;
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> +					"COLOR_ENCODING",
> >> +					enum_list, len);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	plane->color_encoding_property = prop;
> >> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> > 
> > Missing the plane->state->whatever setup here.
> 
> What do you mean? Initialize the plane->state->color_encoding with the
> default value?
> 
> There is no state allocated for a plane at initialization phase, where I
> attach the properties, at least in omapdrm. I somehow expected this to
> be taken care by the default value in the attach call, but now I see it
> does not do that.

Yeah. For the props we just do 'if (state) state->foo = bar;'.

> 
> > 
> >> +
> >> +	len = 0;
> >> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> >> +		if ((BIT(i) & supported_ranges) == 0)
> >> +			continue;
> >> +
> >> +		enum_list[i].type = i;
> >> +		enum_list[i].name = color_range_name[i];
> >> +		len++;
> >> +	}
> >> +
> >> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> >> +					"COLOR_RANGE",
> >> +					enum_list, len);
> >> +	if (!prop)
> >> +		return -ENOMEM;
> >> +	plane->color_range_property = prop;
> >> +	drm_object_attach_property(&plane->base, prop, default_range);
> > 
> > Same issues here
> > 
> > I was somewhat wondering if we should even have these two props handled
> > in the same function, but I suppose it does make some sense. Not much
> > point in adding just one but not the other.
> > 
> 
> I was thinking about the same thing, and decided that if only one range
> is supported, it is better to expose that as a singleton enum as well.
> 
> >> +
> >> +	return 0;
> >> +}
> >> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> >> index fedd4d6..a6762e8 100644
> >> --- a/drivers/gpu/drm/drm_plane.c
> >> +++ b/drivers/gpu/drm/drm_plane.c
> >> @@ -244,6 +244,12 @@ void drm_plane_cleanup(struct drm_plane *plane)
> >>  
> >>  	kfree(plane->name);
> >>  
> >> +	if (plane->color_encoding_property)
> >> +		drm_property_destroy(dev, plane->color_encoding_property);
> >> +
> >> +	if (plane->color_range_property)
> >> +		drm_property_destroy(dev, plane->color_range_property);
> > 
> > Not needed.
> > 
> 
> Is that some late addition? I remember having some trouble earlier and
> added these because of that. Maybe I remember wrong...

I believe all props should end up on some global list that gets
cleaned up by the core.

> 
> >> +
> >>  	memset(plane, 0, sizeof(*plane));
> >>  }
> >>  EXPORT_SYMBOL(drm_plane_cleanup);
> >> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> >> index 03a59cb..7a1af63 100644
> >> --- a/include/drm/drm_color_mgmt.h
> >> +++ b/include/drm/drm_color_mgmt.h
> >> @@ -26,6 +26,8 @@
> >>  #include <linux/ctype.h>
> >>  
> >>  struct drm_crtc;
> >> +struct drm_plane;
> >> +struct drm_prop_enum_list;
> >>  
> >>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> >>  
> >> @@ -37,4 +39,22 @@ void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
> >>  int drm_mode_crtc_set_gamma_size(struct drm_crtc *crtc,
> >>  				 int gamma_size);
> >>  
> >> +enum drm_color_encoding {
> >> +	DRM_COLOR_YCBCR_BT601 = 0,
> > 
> > = 0 seems pointless.
> > 
> 
> I added them just for stating that the code breaks if the enums do not
> start from 0, but I can remove that.

I'm pretty sure enums always start at 0.

> 
> >> +	DRM_COLOR_YCBCR_BT709,
> >> +	DRM_COLOR_YCBCR_BT2020,
> >> +	DRM_COLOR_ENCODING_MAX,
> >> +};
> >> +
> >> +enum drm_color_range {
> >> +	DRM_COLOR_YCBCR_LIMITED_RANGE = 0,
> > 
> > ditto
> > 
> >> +	DRM_COLOR_YCBCR_FULL_RANGE,
> >> +	DRM_COLOR_RANGE_MAX,
> >> +};
> >> +
> >> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >> +				      u32 supported_encodings,
> >> +				      u32 supported_ranges,
> >> +				      enum drm_color_encoding default_encoding,
> >> +				      enum drm_color_range default_range);
> >>  #endif
> >> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> index 9ab3e70..f4de5f9 100644
> >> --- a/include/drm/drm_plane.h
> >> +++ b/include/drm/drm_plane.h
> >> @@ -26,6 +26,7 @@
> >>  #include <linux/list.h>
> >>  #include <linux/ctype.h>
> >>  #include <drm/drm_mode_object.h>
> >> +#include <drm/drm_color_mgmt.h>
> >>  
> >>  struct drm_crtc;
> >>  struct drm_printer;
> >> @@ -112,6 +113,10 @@ struct drm_plane_state {
> >>  	unsigned int zpos;
> >>  	unsigned int normalized_zpos;
> >>  
> >> +	/* Color encoding for non RGB formats */
> >> +	enum drm_color_encoding color_encoding;
> >> +	enum drm_color_range color_range;
> >> +
> >>  	/* Clipped coordinates */
> >>  	struct drm_rect src, dst;
> >>  
> >> @@ -523,6 +528,9 @@ struct drm_plane {
> >>  
> >>  	struct drm_property *zpos_property;
> >>  	struct drm_property *rotation_property;
> >> +
> >> +	struct drm_property *color_encoding_property;
> >> +	struct drm_property *color_range_property;
> >>  };
> >>  
> >>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> >> -- 
> >> 1.9.1
> > 

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