[PATCH RFC v2 1/2] drm: Add optinal YCBCR_ENCODING property to drm_plane

[Laurent Pinchart]

Hi Jyri,

Thank you for the patch.

On Thursday 04 May 2017 10:14:25 Jyri Sarha wrote:
> Add a standard optinal property to control YCbCr conversion in DRM
> planes. The property is stored to drm_plane object to allow different
> set of supported conversion modes for different planes on the device.
> 
> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> ---
>  drivers/gpu/drm/drm_atomic.c     |  5 ++++
>  drivers/gpu/drm/drm_color_mgmt.c | 59 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/drm_plane.c      |  3 ++
>  include/drm/drm_color_mgmt.h     | 14 ++++++++++
>  include/drm/drm_plane.h          |  6 ++++
>  5 files changed, 87 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c index 533f3a3..245b14a 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c

[snip]

> @@ -85,6 +90,13 @@
>   * drm_mode_crtc_set_gamma_size(). Drivers which support both should use
>   * drm_atomic_helper_legacy_gamma_set() to alias the legacy gamma ramp with
> the
>   * "GAMMA_LUT" property above.
> + *
> + * The &drm_plane object's properties are:
> + *
> + * "YCBCR_ENCODING"
> + * 	Optional plane enum property to control YCbCr to RGB
> + * 	conversion. The driver provides a subset of standard
> + *	enum values supported by the DRM plane.
>   */

As already mentioned by Hans Verkuil, I also recommend not mixing the encoding 
and quantization in a single property. If you split them, I would then drop 
the YCBCR_ prefix (or replace it by something more generic) at least for the 
quantization property, as it would apply to RGB as well. For the encoding 
property, we have support in V4L2 for a two HSV encodings, so it could make 
sense to drop or replace the YCBCR_ prefix, but on the other hand I doubt 
we'll see any display hardware with native support for HSV :-)

>  /**
> @@ -333,3 +345,50 @@ int drm_mode_gamma_get_ioctl(struct drm_device *dev,
>  	drm_modeset_unlock(&crtc->mutex);
>  	return ret;
>  }
> +
> +static char *ycbcr_encoding_name[] = {
> +	[DRM_PLANE_YCBCR_BT601_FULL_RANGE] = "BT.601 full range",
> +	[DRM_PLANE_YCBCR_BT601_LIMITED_RANGE] = "BT.601 limited range",
> +	[DRM_PLANE_YCBCR_BT709_LIMITED_RANGE] = "BT.709 limited range",
> +	[DRM_PLANE_YCBCR_BT2020_LIMITED_RANGE] = "BT.2020 limited range",
> +};
> +
> +/**
> + * drm_plane_create_ycbcr_properties - ycbcr related plane properties
> + * @enum_list: drm_prop_enum_list array of supported modes without names
> + * @enum_list_len: length of enum_list array
> + * @default_mode: default YCbCr encoding
> + *
> + * Create and attach plane specific YCBCR_ENCODING property to to the
> + * drm_plane object. The supported encodings should be provided in the
> + * enum_list parameter. The enum_list parameter should not contain the
> + * enum names, because the standard names are added by this function.
> + */
> +int drm_plane_create_ycbcr_properties(struct drm_plane *plane,
> +			struct drm_prop_enum_list *enum_list,
> +			unsigned int enum_list_len,
> +			enum drm_plane_ycbcr_encoding default_mode)

I wonder whether we shouldn't simplify the API by passing a bitmask of 
supported encodings. Sure, you would have to allocate the array of 
drm_prop_enum_list internally in the function, but driver code would be 
simpler. Even if you don't like bitmasks, I think we should pass a const 
pointer and duplicate the array internally to fill the names. Drivers will in 
many cases pass the same array for all planes, modifying it in the function is 
asking for trouble (even if it should be OK with the current implementation).

By the way, for drivers that support the same encodings for all planes, would 
it be worth it to support allocation of a single property instead of 
allocating one per plane ?

> +{
> +	struct drm_device *dev = plane->dev;
> +	struct drm_property *prop;
> +	unsigned int i;
> +
> +	if (WARN_ON(plane->ycbcr_encoding_property != NULL))
> +		return -EEXIST;
> +
> +	for (i = 0; i < enum_list_len; i++) {
> +		enum drm_plane_ycbcr_encoding encoding = enum_list[i].type;
> +
> +		enum_list[i].name = ycbcr_encoding_name[encoding];
> +	}
> +
> +	prop = drm_property_create_enum(dev, DRM_MODE_PROP_ATOMIC,
> +					"YCBCR_ENCODING",
> +					enum_list, enum_list_len);
> +	if (!prop)
> +		return -ENOMEM;
> +	plane->ycbcr_encoding_property = prop;
> +	drm_object_attach_property(&plane->base, prop, default_mode);
> +
> +	return 0;
> +}
> diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c
> index fedd4d6..007c4d7 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -244,6 +244,9 @@ void drm_plane_cleanup(struct drm_plane *plane)
> 
>  	kfree(plane->name);
> 
> +	if (plane->ycbcr_encoding_property)
> +		drm_property_destroy(dev, plane->ycbcr_encoding_property);

There's lots of similar code all over the place, I wonder whether we shouldn't 
move the NULL check to drm_property_destroy().

> +
>  	memset(plane, 0, sizeof(*plane));
>  }
>  EXPORT_SYMBOL(drm_plane_cleanup);

[snip]

-- 
Regards,

Laurent Pinchart