diff mbox

drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

Message ID c897e37cfd059a86b9cbc77fd7699e014455fd60.1497214705.git.jsarha@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

Jyri Sarha June 11, 2017, 9:10 p.m. UTC
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.

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

kernel test robot June 11, 2017, 10 p.m. UTC | #1
Hi Jyri,

[auto build test WARNING on drm/drm-next]
[also build test WARNING on next-20170609]
[cannot apply to v4.12-rc4]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Jyri-Sarha/drm-Add-optional-COLOR_ENCODING-and-COLOR_RANGE-properties-to-drm_plane/20170612-051658
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: x86_64-randconfig-x010-201724 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All warnings (new ones prefixed by >>):

   drivers/gpu/drm/drm_color_mgmt.c: In function 'drm_plane_create_color_properties':
>> drivers/gpu/drm/drm_color_mgmt.c:395:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      enum_list[len].name = color_encoding_name[i];
                          ^
   drivers/gpu/drm/drm_color_mgmt.c:414:23: warning: assignment discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
      enum_list[len].name = color_range_name[i];
                          ^

vim +/const +395 drivers/gpu/drm/drm_color_mgmt.c

   379					      u32 supported_ranges,
   380					      enum drm_color_encoding default_encoding,
   381					      enum drm_color_range default_range)
   382	{
   383		struct drm_device *dev = plane->dev;
   384		struct drm_property *prop;
   385		struct drm_prop_enum_list enum_list[max(DRM_COLOR_ENCODING_MAX,
   386							DRM_COLOR_RANGE_MAX)];
   387		int i, len;
   388	
   389		len = 0;
   390		for (i = 0; i < DRM_COLOR_ENCODING_MAX; i++) {
   391			if ((supported_encodings & BIT(i)) == 0)
   392				continue;
   393	
   394			enum_list[len].type = i;
 > 395			enum_list[len].name = color_encoding_name[i];
   396			len++;
   397		}
   398	
   399		prop = drm_property_create_enum(dev, 0, "COLOR_ENCODING",
   400						enum_list, len);
   401		if (!prop)
   402			return -ENOMEM;
   403		plane->color_encoding_property = prop;

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
Ville Syrjälä Feb. 6, 2018, 5:58 p.m. UTC | #2
On Mon, Jun 12, 2017 at 12:10:04AM +0300, Jyri Sarha wrote:
> 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.
> 
> 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 77dcef0..8067681 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -786,6 +786,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);
> @@ -846,6 +850,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 3eda500..9077bf6 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.
>   */
>  
>  /**
> @@ -336,3 +349,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",

I was just looking at this again (in the hopes of landing it finally),
and then I realized we probably need to split this BT.2020 into
constant and non-constant luminance variants.

..._BT2020_CL and ..._BT2020_NCL maybe?

> +};
> +
> +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 03a59cb..b3b6d30 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 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
Hans Verkuil Feb. 6, 2018, 6:08 p.m. UTC | #3
On 02/06/2018 06:58 PM, Ville Syrjälä wrote:
> On Mon, Jun 12, 2017 at 12:10:04AM +0300, Jyri Sarha wrote:
>> 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.
>>
>> 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 77dcef0..8067681 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -786,6 +786,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);
>> @@ -846,6 +850,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 3eda500..9077bf6 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.
>>   */
>>  
>>  /**
>> @@ -336,3 +349,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",
> 
> I was just looking at this again (in the hopes of landing it finally),
> and then I realized we probably need to split this BT.2020 into
> constant and non-constant luminance variants.
> 
> ..._BT2020_CL and ..._BT2020_NCL maybe?

This is what V4L2 uses:

https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html

V4L2_YCBCR_ENC_BT2020 and V4L2_YCBCR_ENC_BT2020_CONST_LUM.

I've never seen V4L2_YCBCR_ENC_BT2020_CONST_LUM in the wild. Non-constant
Bt.2020 is what is normally used, so I'd keep the define just as-is:
DRM_COLOR_YCBCR_BT2020. And add a DRM_COLOR_YCBCR_BT2020_CONST_LUM if
you think you need it. I've never seen anyone use this.

And in case you wonder why: this is the BT.2020 OpenGL Y'CbCr to R'G'B'
conversion:

	float r = y + 1.4719 * v;
        float g = y - 0.1646 * u - 0.5703 * v;
        float b = y + 1.8814 * u;

And this for BT.2020 constant luminance:

	float b = u <= 0.0 ? y + 1.9404 * u : y + 1.5816 * u;
	float r = v <= 0.0 ? y + 1.7184 * v : y + 0.9936 * v;
	float lin_r = (r < 0.081) ? r / 4.5 : pow((r + 0.099) / 1.099, 1.0 / 0.45);
	float lin_b = (b < 0.081) ? b / 4.5 : pow((b + 0.099) / 1.099, 1.0 / 0.45);
	float lin_y = (y < 0.081) ? y / 4.5 : pow((y + 0.099) / 1.099, 1.0 / 0.45);
	float lin_g = lin_y / 0.6780 - lin_r * 0.2627 / 0.6780 - lin_b * 0.0593 / 0.6780;
	float g = (lin_g < 0.018) ? lin_g * 4.5 : 1.099 * pow(lin_g, 0.45) - 0.099;

Brr :-)

Regards,

	Hans

> 
>> +};
>> +
>> +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 03a59cb..b3b6d30 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 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
>
Jyri Sarha Feb. 6, 2018, 6:16 p.m. UTC | #4
I have not been following the subject for half a year now. Just let me
know if I can do something to help land this patch, e.g. rebase and
resend it.


On 02/06/18 20:08, Hans Verkuil wrote:
> On 02/06/2018 06:58 PM, Ville Syrjälä wrote:
>> On Mon, Jun 12, 2017 at 12:10:04AM +0300, Jyri Sarha wrote:
>>> 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.
>>>
>>> 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 77dcef0..8067681 100644
>>> --- a/drivers/gpu/drm/drm_atomic.c
>>> +++ b/drivers/gpu/drm/drm_atomic.c
>>> @@ -786,6 +786,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);
>>> @@ -846,6 +850,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 3eda500..9077bf6 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.
>>>   */
>>>  
>>>  /**
>>> @@ -336,3 +349,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",
>>
>> I was just looking at this again (in the hopes of landing it finally),
>> and then I realized we probably need to split this BT.2020 into
>> constant and non-constant luminance variants.
>>
>> ..._BT2020_CL and ..._BT2020_NCL maybe?
> 
> This is what V4L2 uses:
> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
> 
> V4L2_YCBCR_ENC_BT2020 and V4L2_YCBCR_ENC_BT2020_CONST_LUM.
> 
> I've never seen V4L2_YCBCR_ENC_BT2020_CONST_LUM in the wild. Non-constant
> Bt.2020 is what is normally used, so I'd keep the define just as-is:
> DRM_COLOR_YCBCR_BT2020. And add a DRM_COLOR_YCBCR_BT2020_CONST_LUM if
> you think you need it. I've never seen anyone use this.
> 
> And in case you wonder why: this is the BT.2020 OpenGL Y'CbCr to R'G'B'
> conversion:
> 
> 	float r = y + 1.4719 * v;
>         float g = y - 0.1646 * u - 0.5703 * v;
>         float b = y + 1.8814 * u;
> 
> And this for BT.2020 constant luminance:
> 
> 	float b = u <= 0.0 ? y + 1.9404 * u : y + 1.5816 * u;
> 	float r = v <= 0.0 ? y + 1.7184 * v : y + 0.9936 * v;
> 	float lin_r = (r < 0.081) ? r / 4.5 : pow((r + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_b = (b < 0.081) ? b / 4.5 : pow((b + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_y = (y < 0.081) ? y / 4.5 : pow((y + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_g = lin_y / 0.6780 - lin_r * 0.2627 / 0.6780 - lin_b * 0.0593 / 0.6780;
> 	float g = (lin_g < 0.018) ? lin_g * 4.5 : 1.099 * pow(lin_g, 0.45) - 0.099;
> 
> Brr :-)
> 
> Regards,
> 
> 	Hans
> 
>>
>>> +};
>>> +
>>> +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 03a59cb..b3b6d30 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 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ä Feb. 6, 2018, 6:35 p.m. UTC | #5
On Tue, Feb 06, 2018 at 08:16:56PM +0200, Jyri Sarha wrote:
> 
> I have not been following the subject for half a year now. Just let me
> know if I can do something to help land this patch, e.g. rebase and
> resend it.

I guess I can just pick it up and repost along with my i915 stuff. I can
just toss in the const luminance thing on top I suppose.

> 
> 
> On 02/06/18 20:08, Hans Verkuil wrote:
> > On 02/06/2018 06:58 PM, Ville Syrjälä wrote:
> >> On Mon, Jun 12, 2017 at 12:10:04AM +0300, Jyri Sarha wrote:
> >>> 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.
> >>>
> >>> 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 77dcef0..8067681 100644
> >>> --- a/drivers/gpu/drm/drm_atomic.c
> >>> +++ b/drivers/gpu/drm/drm_atomic.c
> >>> @@ -786,6 +786,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);
> >>> @@ -846,6 +850,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 3eda500..9077bf6 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.
> >>>   */
> >>>  
> >>>  /**
> >>> @@ -336,3 +349,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",
> >>
> >> I was just looking at this again (in the hopes of landing it finally),
> >> and then I realized we probably need to split this BT.2020 into
> >> constant and non-constant luminance variants.
> >>
> >> ..._BT2020_CL and ..._BT2020_NCL maybe?
> > 
> > This is what V4L2 uses:
> > 
> > https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
> > 
> > V4L2_YCBCR_ENC_BT2020 and V4L2_YCBCR_ENC_BT2020_CONST_LUM.
> > 
> > I've never seen V4L2_YCBCR_ENC_BT2020_CONST_LUM in the wild. Non-constant
> > Bt.2020 is what is normally used, so I'd keep the define just as-is:
> > DRM_COLOR_YCBCR_BT2020. And add a DRM_COLOR_YCBCR_BT2020_CONST_LUM if
> > you think you need it. I've never seen anyone use this.
> > 
> > And in case you wonder why: this is the BT.2020 OpenGL Y'CbCr to R'G'B'
> > conversion:
> > 
> > 	float r = y + 1.4719 * v;
> >         float g = y - 0.1646 * u - 0.5703 * v;
> >         float b = y + 1.8814 * u;
> > 
> > And this for BT.2020 constant luminance:
> > 
> > 	float b = u <= 0.0 ? y + 1.9404 * u : y + 1.5816 * u;
> > 	float r = v <= 0.0 ? y + 1.7184 * v : y + 0.9936 * v;
> > 	float lin_r = (r < 0.081) ? r / 4.5 : pow((r + 0.099) / 1.099, 1.0 / 0.45);
> > 	float lin_b = (b < 0.081) ? b / 4.5 : pow((b + 0.099) / 1.099, 1.0 / 0.45);
> > 	float lin_y = (y < 0.081) ? y / 4.5 : pow((y + 0.099) / 1.099, 1.0 / 0.45);
> > 	float lin_g = lin_y / 0.6780 - lin_r * 0.2627 / 0.6780 - lin_b * 0.0593 / 0.6780;
> > 	float g = (lin_g < 0.018) ? lin_g * 4.5 : 1.099 * pow(lin_g, 0.45) - 0.099;
> > 
> > Brr :-)
> > 
> > Regards,
> > 
> > 	Hans
> > 
> >>
> >>> +};
> >>> +
> >>> +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 03a59cb..b3b6d30 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 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
> >>
> > 
> 
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Ville Syrjälä Feb. 6, 2018, 6:45 p.m. UTC | #6
On Tue, Feb 06, 2018 at 07:08:34PM +0100, Hans Verkuil wrote:
> On 02/06/2018 06:58 PM, Ville Syrjälä wrote:
> > On Mon, Jun 12, 2017 at 12:10:04AM +0300, Jyri Sarha wrote:
> >> 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.
> >>
> >> 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 77dcef0..8067681 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -786,6 +786,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);
> >> @@ -846,6 +850,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 3eda500..9077bf6 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.
> >>   */
> >>  
> >>  /**
> >> @@ -336,3 +349,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",
> > 
> > I was just looking at this again (in the hopes of landing it finally),
> > and then I realized we probably need to split this BT.2020 into
> > constant and non-constant luminance variants.
> > 
> > ..._BT2020_CL and ..._BT2020_NCL maybe?
> 
> This is what V4L2 uses:
> 
> https://hverkuil.home.xs4all.nl/spec/uapi/v4l/colorspaces-defs.html
> 
> V4L2_YCBCR_ENC_BT2020 and V4L2_YCBCR_ENC_BT2020_CONST_LUM.
> 
> I've never seen V4L2_YCBCR_ENC_BT2020_CONST_LUM in the wild. Non-constant
> Bt.2020 is what is normally used, so I'd keep the define just as-is:
> DRM_COLOR_YCBCR_BT2020. And add a DRM_COLOR_YCBCR_BT2020_CONST_LUM if
> you think you need it. I've never seen anyone use this.

Hmm. I suppose that's a reasonable apporach. As long as we document that
the the plain BT2020 enum value is in fact the non-const luminance
variant.

> 
> And in case you wonder why: this is the BT.2020 OpenGL Y'CbCr to R'G'B'
> conversion:
> 
> 	float r = y + 1.4719 * v;
>         float g = y - 0.1646 * u - 0.5703 * v;
>         float b = y + 1.8814 * u;
> 
> And this for BT.2020 constant luminance:
> 
> 	float b = u <= 0.0 ? y + 1.9404 * u : y + 1.5816 * u;
> 	float r = v <= 0.0 ? y + 1.7184 * v : y + 0.9936 * v;
> 	float lin_r = (r < 0.081) ? r / 4.5 : pow((r + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_b = (b < 0.081) ? b / 4.5 : pow((b + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_y = (y < 0.081) ? y / 4.5 : pow((y + 0.099) / 1.099, 1.0 / 0.45);
> 	float lin_g = lin_y / 0.6780 - lin_r * 0.2627 / 0.6780 - lin_b * 0.0593 / 0.6780;
> 	float g = (lin_g < 0.018) ? lin_g * 4.5 : 1.099 * pow(lin_g, 0.45) - 0.099;
> 
> Brr :-)

Yeah. In my HDR Weston fork I was doing the non-const luminace with
a generic 4x4 matrix (could be 4x3 but that would have required
some extension I didn't want to look up).

Then I looked at adding the non-const luminance variant, and in
the end decided that I didn't want to uglify the shader code with
that :) If it wasn't for those darned different scaling factors
for the <0.0 vs. >0.0 cases...

> 
> Regards,
> 
> 	Hans
> 
> > 
> >> +};
> >> +
> >> +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 03a59cb..b3b6d30 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 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
> >
diff mbox

Patch

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 77dcef0..8067681 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -786,6 +786,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);
@@ -846,6 +850,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 3eda500..9077bf6 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.
  */
 
 /**
@@ -336,3 +349,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 03a59cb..b3b6d30 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 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)