Re: [Intel-gfx] [PATCH 1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Jyri Sarha]

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?
> 

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Ville Syrjälä]

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 
> > 
> > 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 
> > Cc: Daniel Vetter 
> > Cc: Daniel Stone 
> > Cc: Russell King - ARM Linux 
> > Cc: Ilia Mirkin 
> > Cc: Hans Verkuil 
> > Cc: Uma Shankar 
> > Cc: Shashank Sharma 
> > Reviewed-by: Ville Syrjälä 
> > Signed-off-by: Jyri Sarha 
> > ---
> >  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
> > + * _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(>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
> > + * 

Re: [Intel-gfx] [PATCH 1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Daniel Vetter]

On Wed, Feb 14, 2018 at 09:23:20PM +0200, Ville Syrjala wrote:
> From: Jyri Sarha 
> 
> 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 
> Cc: Daniel Vetter 
> Cc: Daniel Stone 
> Cc: Russell King - ARM Linux 
> Cc: Ilia Mirkin 
> Cc: Hans Verkuil 
> Cc: Uma Shankar 
> Cc: Shashank Sharma 
> Reviewed-by: Ville Syrjälä 
> Signed-off-by: Jyri Sarha 
> ---
>  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
> + * _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(>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 

[Intel-gfx] [PATCH 1/8] drm: Add optional COLOR_ENCODING and COLOR_RANGE properties to drm_plane

[Ville Syrjala]

From: Jyri Sarha 

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 
Cc: Daniel Vetter 
Cc: Daniel Stone 
Cc: Russell King - ARM Linux 
Cc: Ilia Mirkin 
Cc: Hans Verkuil 
Cc: Uma Shankar 
Cc: Shashank Sharma 
Reviewed-by: Ville Syrjälä 
Signed-off-by: Jyri Sarha 
---
 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
+ * _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(>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)
+{
+