[1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

Message ID 20180214192327.3250-2-ville.syrjala@linux.intel.com
State Superseded
Headers show
Series
  • [1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane
Related show

Commit Message

Ville Syrjala Feb. 14, 2018, 7:23 p.m.
From: Jyri Sarha <jsarha@ti.com>

Add a standard optional 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.

Cc: Harry Wentland <harry.wentland@amd.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Stone <daniel@fooishbar.org>
Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
Cc: Ilia Mirkin <imirkin@alum.mit.edu>
Cc: Hans Verkuil <hverkuil@xs4all.nl>
Cc: Uma Shankar <uma.shankar@intel.com>
Cc: Shashank Sharma <shashank.sharma@intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Jyri Sarha <jsarha@ti.com>
---
 drivers/gpu/drm/drm_atomic.c     |  8 ++++
 drivers/gpu/drm/drm_color_mgmt.c | 91 ++++++++++++++++++++++++++++++++++++++++
 include/drm/drm_color_mgmt.h     | 19 +++++++++
 include/drm/drm_plane.h          |  8 ++++
 4 files changed, 126 insertions(+)

Comments

Daniel Vetter Feb. 19, 2018, 3:04 p.m. | #1
On Wed, Feb 14, 2018 at 09:23:20PM +0200, Ville Syrjala wrote:
> From: Jyri Sarha <jsarha@ti.com>
> 
> Add a standard optional 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.
> 
> Cc: Harry Wentland <harry.wentland@amd.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Daniel Stone <daniel@fooishbar.org>
> Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> Cc: Hans Verkuil <hverkuil@xs4all.nl>
> Cc: Uma Shankar <uma.shankar@intel.com>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Jyri Sarha <jsarha@ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  8 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 91 ++++++++++++++++++++++++++++++++++++++++
>  include/drm/drm_color_mgmt.h     | 19 +++++++++
>  include/drm/drm_plane.h          |  8 ++++
>  4 files changed, 126 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 46733d534587..452a0b0bafbc 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -759,6 +759,10 @@ static 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);
> @@ -818,6 +822,10 @@ drm_atomic_plane_get_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 0d002b045bd2..a84fc861e406 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -88,6 +88,19 @@
>   * 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.
> + *
> + * Support for different non RGB color encodings is controlled through
> + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties:
> + *
> + * "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.

Please also mention the function to setup/register them, like we try to do
with the other optional properties.

> + *
> + * "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.
>   */
>  
>  /**
> @@ -339,3 +352,81 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static const char * const color_encoding_name[] = {
> +	[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 const char * const color_range_name[] = {
> +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> +};
> +
> +/**
> + * drm_plane_create_color_properties - color encoding related plane properties
> + * @supported_encodings: bitfield indicating supported color encodings
> + * @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 bitmask 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,

Is 0 in the above two supported_ masks a valid value? If yes, should we
still register the prop in that case? If no, please add a WARN_ON early
exit case to catch this.

Similar, if there's only 1 possible value I guess we should make the prop
immutable?

> +				      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)];
> +	int i, len;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> +		if ((supported_encodings & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_encoding_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_encoding);
> +	if (plane->state)
> +		plane->state->color_encoding = default_encoding;
> +
> +	len = 0;
> +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> +		if ((supported_ranges & BIT(i)) == 0)
> +			continue;
> +
> +		enum_list[len].type = i;
> +		enum_list[len].name = color_range_name[i];
> +		len++;
> +	}
> +
> +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> +					enum_list, len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->color_range_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_range);
> +	if (plane->state)
> +		plane->state->color_range = default_range;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(drm_plane_create_color_properties);
> diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> index 03a59cbce621..b3b6d302ca8c 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -26,6 +26,7 @@
>  #include <linux/ctype.h>
>  
>  struct drm_crtc;
> +struct drm_plane;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
>  
> @@ -37,4 +38,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,
> +	DRM_COLOR_YCBCR_BT709,
> +	DRM_COLOR_YCBCR_BT2020,
> +	DRM_COLOR_ENCODING_MAX,
> +};
> +
> +enum drm_color_range {
> +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> +	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 8185e3468a23..c0a242dcda44 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;
Needs kerneldoc.

> +
>  	/* Clipped coordinates */
>  	struct drm_rect src, dst;
>  
> @@ -558,6 +563,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;

Dito.

>  };
>  
>  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> -- 
> 2.13.6
>
Ville Syrjala Feb. 19, 2018, 8:19 p.m. | #2
On Mon, Feb 19, 2018 at 04:04:42PM +0100, Daniel Vetter wrote:
> On Wed, Feb 14, 2018 at 09:23:20PM +0200, Ville Syrjala wrote:
> > From: Jyri Sarha <jsarha@ti.com>
> > 
> > Add a standard optional 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.
> > 
> > Cc: Harry Wentland <harry.wentland@amd.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Daniel Stone <daniel@fooishbar.org>
> > Cc: Russell King - ARM Linux <linux@armlinux.org.uk>
> > Cc: Ilia Mirkin <imirkin@alum.mit.edu>
> > Cc: Hans Verkuil <hverkuil@xs4all.nl>
> > Cc: Uma Shankar <uma.shankar@intel.com>
> > Cc: Shashank Sharma <shashank.sharma@intel.com>
> > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Jyri Sarha <jsarha@ti.com>
> > ---
> >  drivers/gpu/drm/drm_atomic.c     |  8 ++++
> >  drivers/gpu/drm/drm_color_mgmt.c | 91 ++++++++++++++++++++++++++++++++++++++++
> >  include/drm/drm_color_mgmt.h     | 19 +++++++++
> >  include/drm/drm_plane.h          |  8 ++++
> >  4 files changed, 126 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 46733d534587..452a0b0bafbc 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -759,6 +759,10 @@ static 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);
> > @@ -818,6 +822,10 @@ drm_atomic_plane_get_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 0d002b045bd2..a84fc861e406 100644
> > --- a/drivers/gpu/drm/drm_color_mgmt.c
> > +++ b/drivers/gpu/drm/drm_color_mgmt.c
> > @@ -88,6 +88,19 @@
> >   * 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.
> > + *
> > + * Support for different non RGB color encodings is controlled through
> > + * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties:
> > + *
> > + * "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.
> 
> Please also mention the function to setup/register them, like we try to do
> with the other optional properties.

ack

> > + *
> > + * "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.
> >   */
> >  
> >  /**
> > @@ -339,3 +352,81 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
> >  	drm_modeset_unlock(&crtc->mutex);
> >  	return ret;
> >  }
> > +
> > +static const char * const color_encoding_name[] = {
> > +	[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 const char * const color_range_name[] = {
> > +	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
> > +	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
> > +};
> > +
> > +/**
> > + * drm_plane_create_color_properties - color encoding related plane properties
> > + * @supported_encodings: bitfield indicating supported color encodings
> > + * @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 bitmask 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,
> 
> Is 0 in the above two supported_ masks a valid value? If yes, should we
> still register the prop in that case? If no, please add a WARN_ON early
> exit case to catch this.

I guess if we go for that we should also check that the supported
bitmasks don't contain undefined enum values, and that the default
enum values are included in the bitmasks.

Jyri said he's not looked at this in a while, so I'll just go ahead
and respin this myself.

> 
> Similar, if there's only 1 possible value I guess we should make the prop
> immutable?

I wonder if we should put that logic into
drm_property_create_enum() & co. actually?

> 
> > +				      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)];
> > +	int i, len;
> > +
> > +	len = 0;
> > +	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
> > +		if ((supported_encodings & BIT(i)) == 0)
> > +			continue;
> > +
> > +		enum_list[len].type = i;
> > +		enum_list[len].name = color_encoding_name[i];
> > +		len++;
> > +	}
> > +
> > +	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
> > +					enum_list, len);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	plane->color_encoding_property = prop;
> > +	drm_object_attach_property(&plane->base, prop, default_encoding);
> > +	if (plane->state)
> > +		plane->state->color_encoding = default_encoding;
> > +
> > +	len = 0;
> > +	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
> > +		if ((supported_ranges & BIT(i)) == 0)
> > +			continue;
> > +
> > +		enum_list[len].type = i;
> > +		enum_list[len].name = color_range_name[i];
> > +		len++;
> > +	}
> > +
> > +	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
> > +					enum_list, len);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +	plane->color_range_property = prop;
> > +	drm_object_attach_property(&plane->base, prop, default_range);
> > +	if (plane->state)
> > +		plane->state->color_range = default_range;
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL(drm_plane_create_color_properties);
> > diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
> > index 03a59cbce621..b3b6d302ca8c 100644
> > --- a/include/drm/drm_color_mgmt.h
> > +++ b/include/drm/drm_color_mgmt.h
> > @@ -26,6 +26,7 @@
> >  #include <linux/ctype.h>
> >  
> >  struct drm_crtc;
> > +struct drm_plane;
> >  
> >  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
> >  
> > @@ -37,4 +38,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,
> > +	DRM_COLOR_YCBCR_BT709,
> > +	DRM_COLOR_YCBCR_BT2020,
> > +	DRM_COLOR_ENCODING_MAX,
> > +};
> > +
> > +enum drm_color_range {
> > +	DRM_COLOR_YCBCR_LIMITED_RANGE,
> > +	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 8185e3468a23..c0a242dcda44 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;
> Needs kerneldoc.

ack

> 
> > +
> >  	/* Clipped coordinates */
> >  	struct drm_rect src, dst;
> >  
> > @@ -558,6 +563,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;
> 
> Dito.

ack

> 
> >  };
> >  
> >  #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > -- 
> > 2.13.6
> > 
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Jyri Sarha Feb. 19, 2018, 8:38 p.m. | #3
On 19/02/18 22:19, Ville Syrjälä wrote:
>>> +int drm_plane_create_color_properties(struct drm_plane *plane,
>>> +				      u32 supported_encodings,
>>> +				      u32 supported_ranges,
>> Is 0 in the above two supported_ masks a valid value? If yes, should we
>> still register the prop in that case? If no, please add a WARN_ON early
>> exit case to catch this.
> I guess if we go for that we should also check that the supported
> bitmasks don't contain undefined enum values, and that the default
> enum values are included in the bitmasks.
> 

Agreed. I wonder if there should still be check in
drm_property_create_enum() for empty enum list. I can not imagine any
good reason for having an enum property without valid options. It is
almost a contradiction in terms.

> Jyri said he's not looked at this in a while, so I'll just go ahead
> and respin this myself.
> 

Thanks, please do.

>> Similar, if there's only 1 possible value I guess we should make the prop
>> immutable?
> I wonder if we should put that logic into
> drm_property_create_enum() & co. actually?
>
Daniel Vetter Feb. 19, 2018, 10:38 p.m. | #4
On Mon, Feb 19, 2018 at 10:38:46PM +0200, Jyri Sarha wrote:
> On 19/02/18 22:19, Ville Syrjälä wrote:
> >>> +int drm_plane_create_color_properties(struct drm_plane *plane,
> >>> +				      u32 supported_encodings,
> >>> +				      u32 supported_ranges,
> >> Is 0 in the above two supported_ masks a valid value? If yes, should we
> >> still register the prop in that case? If no, please add a WARN_ON early
> >> exit case to catch this.
> > I guess if we go for that we should also check that the supported
> > bitmasks don't contain undefined enum values, and that the default
> > enum values are included in the bitmasks.
> > 
> 
> Agreed. I wonder if there should still be check in
> drm_property_create_enum() for empty enum list. I can not imagine any
> good reason for having an enum property without valid options. It is
> almost a contradiction in terms.

Yeah checking for that in the the core function sounds best.

> > Jyri said he's not looked at this in a while, so I'll just go ahead
> > and respin this myself.
> > 
> 
> Thanks, please do.
> 
> >> Similar, if there's only 1 possible value I guess we should make the prop
> >> immutable?
> > I wonder if we should put that logic into
> > drm_property_create_enum() & co. actually?

I could imagine that we end up with a property that generic userspace
wants to set, but a driver somehow only wants to expose a single value
(because there's one platform where random constraints result in that).
Not quite a sure we should check for that too in the core, but I guess we
can try and see what happens.
-Daniel

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 46733d534587..452a0b0bafbc 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -759,6 +759,10 @@  static 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);
@@ -818,6 +822,10 @@  drm_atomic_plane_get_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 0d002b045bd2..a84fc861e406 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -88,6 +88,19 @@ 
  * 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.
+ *
+ * Support for different non RGB color encodings is controlled through
+ * &drm_plane specific COLOR_ENCODING and COLOR_RANGE properties:
+ *
+ * "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.
  */
 
 /**
@@ -339,3 +352,81 @@  int drm_mode_gamma_get_ioctl(struct drm_device *dev,
 	drm_modeset_unlock(&crtc->mutex);
 	return ret;
 }
+
+static const char * const color_encoding_name[] = {
+	[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 const char * const color_range_name[] = {
+	[DRM_COLOR_YCBCR_FULL_RANGE] = "YCbCr full range",
+	[DRM_COLOR_YCBCR_LIMITED_RANGE] = "YCbCr limited range",
+};
+
+/**
+ * drm_plane_create_color_properties - color encoding related plane properties
+ * @supported_encodings: bitfield indicating supported color encodings
+ * @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 bitmask 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)];
+	int i, len;
+
+	len = 0;
+	for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
+		if ((supported_encodings & BIT(i)) == 0)
+			continue;
+
+		enum_list[len].type = i;
+		enum_list[len].name = color_encoding_name[i];
+		len++;
+	}
+
+	prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
+					enum_list, len);
+	if (!prop)
+		return -ENOMEM;
+	plane->color_encoding_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_encoding);
+	if (plane->state)
+		plane->state->color_encoding = default_encoding;
+
+	len = 0;
+	for (i = 0; i < DRM_COLOR_RANGE_MAX; i++) {
+		if ((supported_ranges & BIT(i)) == 0)
+			continue;
+
+		enum_list[len].type = i;
+		enum_list[len].name = color_range_name[i];
+		len++;
+	}
+
+	prop = drm_property_create_enum(dev, 0,	"COLOR_RANGE",
+					enum_list, len);
+	if (!prop)
+		return -ENOMEM;
+	plane->color_range_property = prop;
+	drm_object_attach_property(&plane->base, prop, default_range);
+	if (plane->state)
+		plane->state->color_range = default_range;
+
+	return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_color_properties);
diff --git a/include/drm/drm_color_mgmt.h b/include/drm/drm_color_mgmt.h
index 03a59cbce621..b3b6d302ca8c 100644
--- a/include/drm/drm_color_mgmt.h
+++ b/include/drm/drm_color_mgmt.h
@@ -26,6 +26,7 @@ 
 #include <linux/ctype.h>
 
 struct drm_crtc;
+struct drm_plane;
 
 uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t bit_precision);
 
@@ -37,4 +38,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,
+	DRM_COLOR_YCBCR_BT709,
+	DRM_COLOR_YCBCR_BT2020,
+	DRM_COLOR_ENCODING_MAX,
+};
+
+enum drm_color_range {
+	DRM_COLOR_YCBCR_LIMITED_RANGE,
+	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 8185e3468a23..c0a242dcda44 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;
 
@@ -558,6 +563,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)