[PATCH RFC v2 2/2] drm: Add YCBCR_DECODE_CSC and YCBCR_CSC_PREOFFSET properties to drm_plane

[Laurent Pinchart]

Hi Jyri,

On Thursday 04 May 2017 18:23:21 Jyri Sarha wrote:
> On 05/04/17 17:51, Ville Syrjälä wrote:
> > On Thu, May 04, 2017 at 03:22:45PM +0200, Daniel Vetter wrote:
> >> On Thu, May 04, 2017 at 10:14:26AM +0300, Jyri Sarha wrote:
> >>> Add standard optinal property blobs for YCbCr to RGB conversion CSC
> >>> matrix and YCbCr preoffset vector in DRM planes. New enums are defined
> >>> to YCBCR_ENCODING property to activate the CSC and preoffset
> >>> properties. For simplicity the new properties are stored in the
> >>> drm_plane object, because the YCBCR_ENCODING is already there. The
> >>> blob contents are defined in the uapi/drm/drm_mode.h header.
> >>> 
> >>> Signed-off-by: Jyri Sarha <jsarha at ti.com>
> >> 
> >> Not sure we want to make this yuv2rgb specific, plenty of planes have
> >> generic degamma/offset, csc, gamme/offset hw. I think what we want
> >> instead is the generic csc support, plus a ycbcr2rgb mode of "bypass".
> > 
> > My idea is to not expose this. And instead just expose a normal
> > ctm, and then just refuse any combination of YCbCr->RGB/degamma/ctm
> > that can't be done by the hw.
> 
> So, pulling the suggestions together the properties would now look like
> this:
> 
> YCBCR_RANGE
>  - Full
>  - Limited
> 
> YCBCR_ENCODING
>  - BT.601
>  - BT.709
>  - BT.2020
> 
> CTM (blob)
> 
> And all these properties could be added to individual planes. I'll try
> to come up with a new patch to add simple enum properties for
> YCBCR_PREOFFSET and YCBCR_ENCODING for planes. Should we need any more
> enum values for those?
> 
> The only functional thing that is missing from the above proposal is the
> possibility to define an arbitrary YCbCr preoffset vector, but that can
> be added later if there ever is any real need.
> 
> The other thing that could be added for special purposes would be adding
> an "Identity" enum to YCBCR_ENCODING, that would allow setting the YCbCr
> CSC trough CTM without any driver level multiplication getting in way,
> but that can also be added if there is a need.

I'm a bit concerned about this. The YCBCR_ENCODING property specifies how the 
content of the framebuffer is encoded. If I understand correctly, you're 
proposing adding an enumeration value that tells the driver not to try to be 
clever and multiply the CTM matrix by the CSC matrix corresponding to the 
encoding. That would probably work in most cases, but it would combine two 
pieces of information in a single property. The driver would then lose the 
knowledge of how the plane framebuffer is encoded. Couldn't there be cases 
where that knowledge is needed for other purposes than picking the right CSC 
matrix ? If so, it might be better to always set the YCBCR_ENCODING property 
to the actual encoding, and have another property to tell the driver to skip 
multiplication by the CSC matrix. Or could that be conveyed through the CTM 
blob property ? Some kind of flag that would essentially tell that the CTM 
matrix has been pre-multiplied already ?

Before I forget, how do you plan to handle backward compatibility with 
userspace that won't set the YCBCR_ENCODING property ? Is that done by picking 
a driver-specific default value ? Do you think there would be a need for 
drivers to know that the property has not been set ?

-- 
Regards,

Laurent Pinchart