Message-ID: <fc590903-a88e-d2c2-968f-26d59963caba@intel.com>
Date:   Thu, 16 Mar 2017 19:05:12 +0200
From:   "Sharma, Shashank" <shashank.sharma@...el.com>
To:     Brian Starkey <brian.starkey@....com>
Cc:     Local user for Liviu Dudau <liviu.dudau@....com>,
        Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        dri-devel@...ts.freedesktop.org, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org, mihail.atanassov@....com,
        "Cyr, Aric" <Aric.Cyr@....com>,
        "Wentland, Harry" <Harry.Wentland@....com>,
        Alex Deucher <alexdeucher@...il.com>
Subject: Re: DRM Atomic property for color-space conversion

Regards

Shashank


On 3/16/2017 5:55 PM, Brian Starkey wrote:
> Hi,
>
> On Thu, Mar 16, 2017 at 05:14:07PM +0200, Sharma Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 3/16/2017 4:37 PM, Local user for Liviu Dudau wrote:
>>> On Thu, Mar 16, 2017 at 04:30:59PM +0200, Ville Syrjälä wrote:
>>>> On Thu, Mar 16, 2017 at 04:20:29PM +0200, Sharma, Shashank wrote:
>>>>> Regards
>>>>>
>>>>> Shashank
>>>>>
>>>>>
>>>>> On 3/16/2017 4:07 PM, Ville Syrjälä wrote:
>>>>>> On Tue, Jan 31, 2017 at 03:55:41PM +0000, Brian Starkey wrote:
>>>>>>> On Tue, Jan 31, 2017 at 05:15:46PM +0200, Ville Syrjälä wrote:
>>>>>>>> On Tue, Jan 31, 2017 at 12:33:29PM +0000, Brian Starkey wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On Mon, Jan 30, 2017 at 03:35:13PM +0200, Ville Syrjälä wrote:
>>>>>>>>>> On Fri, Jan 27, 2017 at 05:23:24PM +0000, Brian Starkey wrote:
>>>>>>>>>>> Hi,
>>>>>>>>>>>
>>>>>>>>>>> We're looking to enable the per-plane color management 
>>>>>>>>>>> hardware in
>>>>>>>>>>> Mali-DP with atomic properties, which has sparked some 
>>>>>>>>>>> conversation
>>>>>>>>>>> around how to handle YCbCr formats.
>>>>>>>>>>>
>>>>>>>>>>> As it stands today, it's assumed that a driver will 
>>>>>>>>>>> implicitly "do the
>>>>>>>>>>> right thing" to display a YCbCr buffer.
>>>>>>>>>>>
>>>>>>>>>>> YCbCr data often uses different gamma curves and signal 
>>>>>>>>>>> ranges (e.g.
>>>>>>>>>>> BT.609, BT.701, BT.2020, studio range, full-range), so its 
>>>>>>>>>>> desirable
>>>>>>>>>>> to be able to explicitly control the YCbCr to RGB conversion 
>>>>>>>>>>> process
>>>>>>>>>>> from userspace.
>>>>>>>>>>>
>>>>>>>>>>> We're proposing adding a "CSC" (color-space conversion) 
>>>>>>>>>>> property to
>>>>>>>>>>> control this - primarily per-plane for framebuffer->pipeline 
>>>>>>>>>>> CSC, but
>>>>>>>>>>> perhaps one per CRTC too for devices which have an RGB 
>>>>>>>>>>> pipeline and
>>>>>>>>>>> want to output in YUV to the display:
>>>>>>>>>>>
>>>>>>>>>>> Name: "CSC"
>>>>>>>>>>> Type: ENUM | ATOMIC;
>>>>>>>>>>> Enum values (representative):
>>>>>>>>>>> "default":
>>>>>>>>>>>     Same behaviour as now. "Some kind" of YCbCr->RGB conversion
>>>>>>>>>>>     for YCbCr buffers, bypass for RGB buffers
>>>>>>>>>>> "disable":
>>>>>>>>>>>     Explicitly disable all colorspace conversion (i.e. use an
>>>>>>>>>>>     identity matrix).
>>>>>>>>>>> "YCbCr to RGB: BT.709":
>>>>>>>>>>>     Only valid for YCbCr formats. CSC in accordance with BT.709
>>>>>>>>>>>     using [16..235] for (8-bit) luma values, and [16..240] for
>>>>>>>>>>>     8-bit chroma values. For 10-bit formats, the range 
>>>>>>>>>>> limits are
>>>>>>>>>>>     multiplied by 4.
>>>>>>>>>>> "YCbCr to RGB: BT.709 full-swing":
>>>>>>>>>>>     Only valid for YCbCr formats. CSC in accordance with 
>>>>>>>>>>> BT.709,
>>>>>>>>>>>     but using the full range of each channel.
>>>>>>>>>>> "YCbCr to RGB: Use CTM":*
>>>>>>>>>>>     Only valid for YCbCr formats. Use the matrix applied via 
>>>>>>>>>>> the
>>>>>>>>>>>     plane's CTM property
>>>>>>>>>>> "RGB to RGB: Use CTM":*
>>>>>>>>>>>     Only valid for RGB formats. Use the matrix applied via the
>>>>>>>>>>>     plane's CTM property
>>>>>>>>>>> "Use CTM":*
>>>>>>>>>>>     Valid for any format. Use the matrix applied via the 
>>>>>>>>>>> plane's
>>>>>>>>>>>     CTM property
>>>>>>>>>>> ... any other values for BT.601, BT.2020, RGB to YCbCr etc. 
>>>>>>>>>>> etc. as
>>>>>>>>>>> they are required.
>>>>>>>>>> Having some RGB2RGB and YCBCR2RGB things in the same property 
>>>>>>>>>> seems
>>>>>>>>>> weird. I would just go with something very simple like:
>>>>>>>>>>
>>>>>>>>>> YCBCR_TO_RGB_CSC:
>>>>>>>>>> * BT.601
>>>>>>>>>> * BT.709
>>>>>>>>>> * custom matrix
>>>>>>>>>>
>>>>>>>>> I think we've agreed in #dri-devel that this CSC property
>>>>>>>>> can't/shouldn't be mapped on-to the existing (hardware 
>>>>>>>>> implementing
>>>>>>>>> the) CTM property - even in the case of per-plane color 
>>>>>>>>> management -
>>>>>>>>> because CSC needs to be done before DEGAMMA.
>>>>>>>>>
>>>>>>>>> So, I'm in favour of going with what you suggested in the 
>>>>>>>>> first place:
>>>>>>>>>
>>>>>>>>> A new YCBCR_TO_RGB_CSC property, enum type, with a list of fixed
>>>>>>>>> conversions. I'd drop the custom matrix for now, as we'd need 
>>>>>>>>> to add
>>>>>>>>> another property to attach the custom matrix blob too.
>>>>>>>>>
>>>>>>>>> I still think we need a way to specify whether the source data 
>>>>>>>>> range
>>>>>>>>> is broadcast/full-range, so perhaps the enum list should be 
>>>>>>>>> expanded
>>>>>>>>> to all combinations of BT.601/BT.709 + broadcast/full-range.
>>>>>>>> Sounds reasonable. Not that much full range YCbCr stuff out there
>>>>>>>> perhaps. Well, apart from jpegs I suppose. But no harm in being 
>>>>>>>> able
>>>>>>>> to deal with it.
>>>>>>>>
>>>>>>>>> (I'm not sure what the canonical naming for 
>>>>>>>>> broadcast/full-range is,
>>>>>>>>> we call them narrow and wide)
>>>>>>>> We tend to call them full vs. limited range. That's how our
>>>>>>>> "Broadcast RGB" property is defined as well.
>>>>>>>>
>>>>>>> OK, using the same ones sounds sensible.
>>>>>>>
>>>>>>>>>> And trying to use the same thing for the crtc stuff is 
>>>>>>>>>> probably not
>>>>>>>>>> going to end well. Like Daniel said we already have the
>>>>>>>>>> 'Broadcast RGB' property muddying the waters there, and that 
>>>>>>>>>> stuff
>>>>>>>>>> also ties in with what colorspace we signal to the sink via
>>>>>>>>>> infoframes/whatever the DP thing was called. So my gut 
>>>>>>>>>> feeling is
>>>>>>>>>> that trying to use the same property everywhere will just end up
>>>>>>>>>> messy.
>>>>>>>>> Yeah, agreed. If/when someone wants to add CSC on the output 
>>>>>>>>> of a CRTC
>>>>>>>>> (after GAMMA), we can add a new property.
>>>>>>>>>
>>>>>>>>> That makes me wonder about calling this one 
>>>>>>>>> SOURCE_YCBCR_TO_RGB_CSC to
>>>>>>>>> be explicit that it describes the source data. Then we can 
>>>>>>>>> later add
>>>>>>>>> SINK_RGB_TO_YCBCR_CSC, and it will be reasonably obvious that its
>>>>>>>>> value describes the output data rather than the input data.
>>>>>>>> Source and sink have a slight connotation in my mind wrt. the 
>>>>>>>> box that
>>>>>>>> produces the display signal and the box that eats the signal. 
>>>>>>>> So trying
>>>>>>>> to use the same terms to describe the internals of the pipeline 
>>>>>>>> inside
>>>>>>>> the "source box" migth lead to some confusion. But we do 
>>>>>>>> probably need
>>>>>>>> some decent names for these to make the layout of the pipeline 
>>>>>>>> clear.
>>>>>>>> Input/output are the other names that popped to my mind but 
>>>>>>>> those aren't
>>>>>>>> necessarily any better. But in the end I think I could live 
>>>>>>>> with whatever
>>>>>>>> names we happen to pick, as long as we document the pipeline 
>>>>>>>> clearly.
>>>>>>>>
>>>>>>>> Long ago I did wonder if we should just start indexing these 
>>>>>>>> things
>>>>>>>> somehow, and then just looking at the index should tell you the 
>>>>>>>> order
>>>>>>>> of the operations. But we already have the ctm/gamma w/o any 
>>>>>>>> indexes so
>>>>>>>> that idea probably isn't so great anymore.
>>>>>>>>
>>>>>>>>> I want to avoid confusion caused by ending up with two
>>>>>>>>> {CS}_TO_{CS}_CSC properties, where one is describing the data 
>>>>>>>>> to the
>>>>>>>>> left of it, and the other describing the data to the right of 
>>>>>>>>> it, with
>>>>>>>>> no real way of telling which way around it is.
>>>>>>>> Not really sure what you mean. It should always be
>>>>>>>> <left>_to_<right>_csc.
>>>>>>> Agreed, left-to-right. But for instance on a CSC property 
>>>>>>> representing
>>>>>>> a CRTC output CSC (just before hitting the connector), which 
>>>>>>> happens
>>>>>>> to be converting RGB to YCbCr:
>>>>>>>
>>>>>>> CRTC -> GAMMA -> RGB_TO_YCBCR_CSC
>>>>>>>
>>>>>>> ...the enum value "BT.601 Limited" means that the data on the 
>>>>>>> *right*
>>>>>>> of RGB_TO_YCBCR_CSC is "BT.601 Limited"
>>>>>>>
>>>>>>> On the other hand for a CSC on the input of a plane, which 
>>>>>>> happens to
>>>>>>> be converting YCbCr to RGB:
>>>>>>>
>>>>>>> RAM -> YCBCR_TO_RGB_CSC -> DEGAMMA
>>>>>>>
>>>>>>> ...the enum value "BT.601 Limited" means that the data on the 
>>>>>>> *left*
>>>>>>> of YCBCR_TO_RGB_CSC is "BT.601 Limited".
>>>>>>>
>>>>>>> Indicating in the property name whether its value is describing the
>>>>>>> data on the left or the right is needed (and I don't think 
>>>>>>> inferring
>>>>>>> that "it's always the YCBCR one" is the correct approach).
>>>>>>>
>>>>>>> In my example above, "SOURCE_xxx" would mean the enum value is
>>>>>>> describing the "source" data (i.e. the data on the left) and
>>>>>>> "SINK_xxx" would mean the enum value is describing the "sink" data
>>>>>>> (i.e. the data on the right). This doesn't necessarily need to 
>>>>>>> infer a
>>>>>>> particular point in the pipeline.
>>>>>> Right, so I guess you want the values to be named "<a> to <b>" as 
>>>>>> well?
>>>>>> Yes, I think we'll be wanting that as well.
>>>>>>
>>>>>> So what we might need is something like:
>>>>>> enum YCBCR_TO_RGB_CSC
>>>>>>   * YCbCr BT.601 limited to RGB BT.709 full
>>>>>>   * YCbCr BT.709 limited to RGB BT.709 full <this would be the 
>>>>>> likely default value IMO>
>>>>>>   * YCbCr BT.601 limited to RGB BT.2020 full
>>>>>>   * YCbCr BT.709 limited to RGB BT.2020 full
>>>>>>   * YCbCr BT.2020 limited to RGB BT.2020 full
>>>>>>
>>>>>> And thanks to BT.2020 we'll need a RGB->RGB CSC property as well. 
>>>>>> Eg:
>>>>>> enum RGB_TO_RGB_CSC
>>>>>>   * bypass (or separate 709->709, 2020->2020?) <this would be the 
>>>>>> default>
>>>>>>   * RGB BT.709 full to RGB BT.2020 full
>
> I like this approach, from a point of view of being explicit and
> discoverable by userspace. It also happens to map quite nicely to our
> hardware... we have a matrix before degamma, so we could do a
> CSC + Gamut conversion there in one go, which is apparently not 100%
> mathematically correct, but in general is good enough.
>
> ... however having talked this over a bit with someone who understands
> the detail a lot better than me, it sounds like the "correct" thing to
> do as per the spec is:
>
> CSC -> DEGAMMA -> GAMUT
>
> e.g.
>
> YCbCr bt.601 limited to RGB bt.601 full -> degamma ->
>     RGB bt.601 full to RGB bt.709 full
>
> So that sounds like what we need to support in the API, and also
> sounds more like the "separate properties" approach.
I agree.
>
>>>>>>
>>>>>> Alternatives would involve two properties to define the input and 
>>>>>> output
>>>>>> from the CSC separately, but then you lose the capability to see 
>>>>>> which
>>>>>> combinations are actually supoorted.
>>>>> I was thinking about this too, or would it make more sense to 
>>>>> create two
>>>>> properties:
>>>>> - one for gamut mapping (cases like RGB709->RGB2020)
>>>>> - other one for Color space conversion (cases lile YUV 709 -> RGB 
>>>>> 709)
>>>>>
>>>>> Gamut mapping can represent any of the fix function mapping, 
>>>>> wereas CSC
>>>>> can bring up any programmable matrix
>>>>>
>>>>> Internally these properties can use the same HW unit or even same 
>>>>> function.
>>>>> Does it sound any good ?
>
> It seems to me that actually the two approaches can be combined into
> the same thing:
>  * We definitely need a YCbCr-to-RGB conversion before degamma
>    (for converting YUV data to RGB, in some flavour)
>  * We definitely need an RGB-to-RGB conversion after gamma to handle
>    709 layers blended with Rec.2020.
> The exact conversion each of those properties represents (CSC + gamut,
> CSC only, gamut only) can be implicit in the enum name.
>
> For hardware which has a fixed-function CSC before DEGAMMA with a
> matrix after DEGAMMA, I'd expect to see something like below. None of
> the YCBCR_TO_RGB_CSC values include a gamut conversion, because that
> is instead exposed with the RGB_TO_RGB_CSC property (which represents
> the hardware matrix)
>
> YCBCR_TO_RGB_CSC (before DEGAMMA):
>     YCbCr BT.601 limited to RGB BT.601 full
>     YCbCr BT.709 limited to RGB BT.709 full
>     YCbCr BT.2020 limited to RGB BT.2020 full
>
> RGB_TO_RGB_CSC (after DEGAMMA):
>     RGB BT.601 full to RGB BT.709 full
>     RGB BT.709 full to RGB BT.2020 full
>
>
> On the other hand, on hardware which does a CSC + Gamut conversion in
> one go, before DEGAMMA (like ours), you might have:
>
> YCBCR_TO_RGB_CSC (before DEGAMMA):
>     YCbCr BT.601 limited to RGB BT.601 full
>     YCbCr BT.601 limited to RGB BT.709 full
>     YCbCr BT.709 limited to RGB BT.709 full
>     YCbCr BT.2020 limited to RGB BT.2020 full
>
> RGB_TO_RGB_CSC (after DEGAMMA):
>     Not supported
>
> Userspace can parse the two properties to figure out its options to
> get from desired input -> desired output. It is perhaps a little
> verbose, but it's descriptive and flexible.
>
Seems to be a good idea, Ville ?

- Shashank
>>>> It's certainly possible. One problem is that we can't inform userspace
>>>> upfront which combinations are supported. Whether that's a real 
>>>> problem
>>>> I'm not sure. With atomic userspace can of course check upfront if
>>>> something can be done or not, but the main problem is then coming up
>>>> with a fallback strategy that doesn't suck too badly.
>>>>
>
> The approach above helps limit the set exposed to userspace to be only
> those which are supported - because devices which don't have separate
> hardware for the two stages won't expose values for both.
>
>>>> Anyways, I don't think I have any strong favorites here. Would be nice
>>>> to hear what everyone else thinks.
>>> I confess to a lack of experience in the subject here, but what is 
>>> the more common
>>> request coming from userspace: converting YUV <-> RGB but keeping 
>>> the gammut mapping
>>> separate, or YUV (gammut x) <-> RGB (gammut y) ? In other words: I 
>>> can see the usefulness
>>> of having an explicit way of decomposing the color mapping process 
>>> and control the
>>> parameters, but how often do apps or compositors go through the 
>>> whole chain?
>> Right now, more or less the interest is on the RGB->YUV conversion 
>> side, coz till now BT 2020 gamut was not in
>> picture. REC 601 and 709 have very close gamuts, so it was ok to 
>> blend frames mostly without bothering about
>> gamut, but going fwd, ones REC 2020 comes into picture, we need to 
>> bother about mapping gamuts too, else
>> blending Rec709 buffers and Rec2020 buffers together would cause very 
>> visible gamut mismatch.
>>
>> So considering futuristic developments, it might be ok to consider 
>> both. Still, as Ville mentioned, it would be good
>> to hear from other too.
>>
>
> Yeah I agree that we definitely need to consider both for anything we
> come up with now.
>
> Cheers,
> Brian
>
>> - Shashank
>>>
>>> Best regards,
>>> Liviu
>>>
>>>> -- 
>>>> Ville Syrjälä
>>>> Intel OTC
>>

Powered by blists - more mailing lists