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

[Laurent Pinchart]

Hi Jyri,

On Thursday 11 May 2017 17:30:01 Jyri Sarha wrote:
> On 05/05/17 16:03, Laurent Pinchart wrote:
> > 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?

I won't claim to know what quantization methods will make sense for non-YCbCr 
encodings in the future (or, for that matter, even for YCbCr encodings). We 
might even not need any new quantization method. However I don't think this is 
an argument to not standardize a quantization method property. We can start 
with an enumerated property with two values, clearly defined in the API 
documentation as applying to YCbCr only. If and when we'll need quantization 
methods for other encodings, we can just add values to that enumeration. I 
would have proposed to even reuse the enumerated values for a different 
purpose depending on the colour encoding, but as the DRM API exposes 
enumerated values as strings, we unfortunately can't do that.

-- 
Regards,

Laurent Pinchart