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

[Jyri Sarha]

On 05/05/17 16:03, Laurent Pinchart wrote:
> Hi Jyri,
> 
> On Friday 05 May 2017 15:11:06 Jyri Sarha wrote:
>> On 05/05/17 12:07, Laurent Pinchart wrote:
>>> 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.
> 
> Sounds good to me.
> 

I am now implementing this. However, there is a logical challenge in
putting the two suggestions together, splitting the encoding and range
to separate properties, and changing the property names to generic
COLOR_ENCODING and COLOR_RANGE.

In general COLOR_RANGE enum values are only defined within the selected
COLOR_ENCODING. With the standard YCbCr encodings this is not a problem,
since they all define a "full range" and a "limted range". But if we are
preparing for some unknown color ecodings, I am not sure how likely it
is that "full range" and "limited range" options make sense there.

I can implement the properties for currently known YCbCr color encodings
either way, either as YCbCr specific properties, or as generic COLOR_*
properties for all non RGB encodings. I am just not sure if defining the
generic properties would make any sense now that we (or at least I) have
no idea what the other non RGB encodings could be. What do you think?

Cheers,
Jyri