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

[Jyri Sarha]

On 05/05/17 12:07, Laurent Pinchart wrote:
> 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 :-)
> 

COLOR_ENCODING? Why not, the YCbCr could then be in the enum names.

>>  /**
>> @@ -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).
> 

I used a bitmask property first, but abandoned it because the encodings
do not behave like bitmasks. You can not have BT.601 | BT.709 at the
same time.

A bitmask in the function API would certainly work and be probably
better, but I've tried to keep the implementation simple, while we are
still discussing what we should actually do.

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

I was thinking of that, but AFAIK there is really not that many planes
on the know HW that it would justify the complexity.

> 
>> +{
>> +	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().
> 

Absolutely.

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