[PATCH RFC 3/6] drm: Plane YCbCr to RGB conversion related properties

[Brian Starkey]

On Fri, Apr 21, 2017 at 07:49:04PM +0300, Ville Syrjälä wrote:
>On Fri, Apr 21, 2017 at 04:34:00PM +0100, Brian Starkey wrote:
>> On Fri, Apr 21, 2017 at 06:21:48PM +0300, Ville Syrjälä wrote:
>> >On Fri, Apr 21, 2017 at 03:53:42PM +0100, Brian Starkey wrote:
>> >> Hi,
>> >>
>> >> Thanks for picking this up Jyri.
>> >>
>> >> On Fri, Apr 21, 2017 at 04:52:03PM +0300, Ville Syrjälä wrote:
>> >> >On Fri, Apr 21, 2017 at 04:39:16PM +0300, Jyri Sarha wrote:
>> >> >> On 04/21/17 14:17, Ville Syrjälä wrote:
>> >> >> >> +static char *ycbcr_to_rgb_mode_name[] = {
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT601_FULL] =
>> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.601 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_FULL_TO_RGB_BT601_FULL] =
>> >> >> >> +		"YCbCr BT.601 full range TO RGB BT.601 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT709_LIM_TO_RGB_BT709_FULL] =
>> >> >> >> +		"YCbCr BT.709 limited range TO RGB BT.709 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT2020_LIM_TO_RGB_BT2020_FULL] =
>> >> >> >> +		"YCbCr BT.2020 limited range TO RGB BT.2020 full range",
>> >> >> >> +	[DRM_PLANE_YCBCR_BT601_LIM_TO_RGB_BT709_FULL] =
>> >> >> >> +		"YCbCr BT.601 limited range TO RGB BT.709 full range",
>> >> >> > We probably don't want to conflate the YCbCr->RGB part with the colorspace
>> >> >> > conversion because the YCbCr->RGB part should be performed on gamma encoded
>> >> >> > data and the colorspace conversion on linear data. So we need a degamma
>> >> >> > stage in between. At least that seemed to be the general concencus after
>> >> >> > the last round of mails on this topic.
>> >> >> >
>> >>
>> >> I thought the conclusion of the last round was that on some hardware
>> >> this would be conflated as a conscious choice. If the HW doesn't have
>> >> the required degamma->csc->gamma stages, and the implementor/user
>> >> doesn't care about being a little incorrect, then it can all be
>> >> described in this single property.
>> >
>> >I was proposing the single prop approach initially, but now I think it
>> >might just lead to more confusion. So a dedicated property for each stage
>> >is the clearer design I think. We do lose potentially a bit of
>> >discoverability when not all combinations are supported, but we have
>> >that problem in many other places as well, so not a big deal I think.
>> >
>>
>> Yeah you lose discoverability, but do you also lose the ability to do
>> the non-perfect, single-stage conversions?
>
>Not sure why one vs. multiple props should matter here. Either you
>accept the less than perfect pipeline or you don't. Whether you ask it
>via one of multiple props doesn't seem all that different to me.
>
>>
>> For HW that only has a matrix, is the driver expected to combine all
>> of the separated stages down into a single matrix? Or it wouldn't
>> expose the other properties, only a matrix, and userspace has to come
>> up with a blob that does the (approximate) right thing?
>
>If you're not happy with exposing just one or the other, then I guess
>you would expose both but indicate that there's no degamma in between
>and userspace can then choose whether it's happy with that solution or
>or not.
>

I might understand/explain better if we take a concrete example.

Let's assume I have a hardware pipeline with the following bits:

Input -> 3x3 + 3 matrix -> degamma LUT -> CRTC

My input is non-linear YCbCr BT.601 full-range, and I want linear RGB
BT.709, full-range to reach the CRTC.


Which properties would my driver expose, and which values should be
set on them?

My assumption /was/ that:

3x3 + 3 matrix:
    Exposed as
    YCBCR_TO_RGB_MODE = "YCbCr BT.601 full-range to RGB BT.709 full-range"

degamma LUT:
    Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
    curve


Are you suggesting instead that:

3x3 + 3 matrix:
    Exposed as
    YCBCR_TO_RGB_MODE = "YCbCr TO RGB CSC full range preoffset"
    with YCBCR_TO_RGB_CSC = <userspace-derived-matrix>

degamma LUT:
    Exposed as DEGAMMA, set by userspace to an inverse BT.601 gamma
    curve

Thanks,
Brian

>>
>> >>
>> >> If there is more capable hardware with the additional stages, then
>> >> they should expose additional properties (in pipeline order):
>> >>
>> >> YCBCR_TO_RGB_*:
>> >>    Does YCbCr->RGB conversion on non-linear YCbCr data,
>> >>    only the enum values which don't have a CSC are exposed.
>> >
>> >Not sure what you mean but that last part.
>> >
>>
>> Just that the enum list should only contain things that are:
>> YCbCr BT.601  -> RGB BT.601
>> YCbCr BT.709  -> RGB BT.709
>> YcbCr BT.2020 -> RGB BT.2020
>>
>> and not a those including a color space change, e.g.:
>> YCbCr BT.601  -> RGB BT.709
>
>Right. We probably shouldn't even have the "BT.whatever" on both sides
>since it's not really a colorspace, just the encoding, and once you
>decoded the YCbCr into RGB it's just RGB. We could actually just define
>the enum values as "BT.601","BT.709" etc.
>
>>
>> because an RGB BT.601 -> RGB BT.709 conversion can be performed
>> later with the RGB_TO_RGB/CTM/whatever property.
>>
>> >>
>> >> DEGAMMA:
>> >>    Does the non-linear to linear conversion on the RGB data output from
>> >>    the YCBCR_TO_RGB stage.
>> >>
>> >> RGB_TO_RGB_*:
>> >>    Similar set of properties to YCBCR_TO_RGB_*, allowing RGB->RGB CSC
>> >>    on the linear data.
>> >
>> >We may want to call this just CTM to match the matching crtc prop.
>> >
>>
>> Good call.
>>
>> -Brian
>>
>> >>
>> >> GAMMA:
>> >>    Convert the CSC'd RGB data back in to non-linear for blending (if
>> >>    blending is to be done with non-linear data).
>> >>
>> >> Drivers can expose as many or as few of the above properties as their
>> >> hardware supports.
>> >>
>> >> >>
>> >> >> I do not really have the expertise to argue with that. I merely copied
>> >> >> the idea from the mail thread I referred to in the cover letter.
>> >> >> However, there are several display HWs out there that do not have all
>> >> >> bolts and knobs to make the color-space conversion in exactly the ideal
>> >> >> order, omap DSS being one of them.
>> >> >
>> >> >Yeah. Intel hardware is in the same boat for the time being. On current
>> >> >hw I think we can only really expose the YCbCr->RGB and degamma stages.
>> >> >
>> >> >On some limited set of platforms we could expose a blob as well, and I
>> >> >suppose it would then be possible to use it for color space conversion
>> >> >if you ignore gamma and/or only deal with linear RGB data. But it's such
>> >> >a limited subset of hardware for us that I don't think I'm interested
>> >> >in exposing it.
>> >> >
>> >>
>> >> I'm not sure the custom blob is worth having either. It can easily be
>> >> added later if we decide we want it after all.
>> >>
>> >> Thanks,
>> >> -Brian
>> >>
>> >> >In the future we should be getting a more fully fleged pipeline.
>> >> >
>> >> >>
>> >> >> > After staring at the v4l docs on this stuff I kinda like their
>> >> >> > "encoding" terminology to describe the YCbCr->RGB part, so I'm now a
>> >> >> > little partial to calling the prop something like YCBCR_ENCODING. OTOH
>> >> >>
>> >> >> I guess this property should be called YCBCR_DECODING.
>> >> >
>> >> >Only if you think of it as a verb.
>> >> >
>> >> >>
>> >> >> > if we want to expose the raw matrix as a blob then maybe calling it a
>> >> >> > CSC might be better. Not sure there's much point in exposing it though.
>> >> >>
>> >> >> In my first version it was called just CSC, but then I wanted to make it
>> >> >> explicit what this CSC was used for to avoid mixing the YCbCr decoding
>> >> >> matrix with YCbCr encoding matrix. At least on OMAP DSS we have pieces
>> >> >> of HW that can do only one or the other, e.g. the offset calculations
>> >> >> are supported only to one direction.
>> >> >
>> >> >Are you planning to do RGB->YCbCr conversion in the plane as well? I
>> >> >think we'll be only doing that at crtc/connector level.
>> >> >
>> >> >>
>> >> >> > I don't think most people are in the habit if cooking up new ways to
>> >> >> > encode their pixel data.
>> >> >> >
>> >> >>
>> >> >> In the embedded side I can imagine there could be some custom appliances
>> >> >> where one may want to do some custom thing with the CSC and not needing
>> >> >> a custom kernel for that could make a life easier... but then again I am
>> >> >> not really an expert in this area.
>> >> >
>> >> >I would assume most customy things you'd do in the crtc (eg. color
>> >> >correction and whatnot). But could be that I just lack imagination.
>> >> >
>> >> >--
>> >> >Ville Syrjälä
>> >> >Intel OTC
>> >
>> >--
>> >Ville Syrjälä
>> >Intel OTC
>
>-- 
>Ville Syrjälä
>Intel OTC