Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751117AbdCQKdZ (ORCPT <rfc822;w@1wt.eu>);
        Fri, 17 Mar 2017 06:33:25 -0400
Received: from foss.arm.com ([217.140.101.70]:44182 "EHLO foss.arm.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1750926AbdCQKdX (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Fri, 17 Mar 2017 06:33:23 -0400
Date: Fri, 17 Mar 2017 10:33:15 +0000
From: Brian Starkey <brian.starkey@arm.com>
To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= <ville.syrjala@linux.intel.com>
Cc: "Sharma, Shashank" <shashank.sharma@intel.com>,
        Local user for Liviu Dudau <liviu.dudau@arm.com>,
        dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org,
        linux-kernel@vger.kernel.org, mihail.atanassov@arm.com,
        "Cyr, Aric" <Aric.Cyr@amd.com>,
        "Wentland, Harry" <Harry.Wentland@amd.com>,
        Alex Deucher <alexdeucher@gmail.com>
Subject: Re: DRM Atomic property for color-space conversion
Message-ID: <20170317103313.GA2090@e106950-lin.cambridge.arm.com>
References: <20170131151546.GT31595@intel.com>
 <20170131155541.GF11506@e106950-lin.cambridge.arm.com>
 <20170316140725.GF31595@intel.com>
 <0cff6bab-7593-d3d2-f3b5-71dc21669dab@intel.com>
 <20170316143059.GG31595@intel.com>
 <20170316143721.GN6268@e110455-lin.cambridge.arm.com>
 <f806a38d-f52c-0cf2-bee6-582f1a35d312@intel.com>
 <20170316155501.GA25006@e106950-lin.cambridge.arm.com>
 <fc590903-a88e-d2c2-968f-26d59963caba@intel.com>
 <20170316173656.GI31595@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20170316173656.GI31595@intel.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 8821
Lines: 220

Hi Ville,

On Thu, Mar 16, 2017 at 07:36:56PM +0200, Ville Syrj?l? wrote:
>On Thu, Mar 16, 2017 at 07:05:12PM +0200, Sharma, Shashank wrote:
>> On 3/16/2017 5:55 PM, Brian Starkey wrote:
>> > On Thu, Mar 16, 2017 at 05:14:07PM +0200, Sharma Shashank wrote:
>> >> 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:
>> >>>>> On 3/16/2017 4:07 PM, Ville Syrj?l? wrote:

[snip]

>> >>>>>>
>> >>>>>> 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 ?
>
>Looks pretty sane to me.
>
>Though how we'll do the degamma/gamma is rather unclear still.
>
>I think we might be looking at two variants of hardware:
>- A fully programmable one with separate stages:
>  csc -> degamma -> gamut -> gamma
>- A totally fixed one with just a few different variants
>  of the pipeline baked into the hardware
>
>If we want to expose the gamma/degamma to the user, how exactly are we
>going to do that with the latter form or hardware. I guess we could
>specify that if the degamma property is not exposed, there will be an
>implicit degamma stage between the two csc and gamut. And if it is
>exposed the output from the first csc is non-linear and thus needs
>the degamma programmed to make it so before the gamut mapping.
>
>And perhaps a similar rule could work for the gamma; If it's present
>the output from the gamut mapping is expected to be linear, and
>otherwise non-linear. Not quite sure about this. In fact I don't yet
>know what our hardware would output from the end of the fixed pipeline.

I don't really like the implicit nature of that, and I also don't
think it fits with practical use-cases - for instance on current
Android, blending is assumed to be done with non-linear data, so an
implicit linearisation doesn't fit with that.
I think it's much better to make sure each element in the pipeline is
discoverable and configurable from userspace.

There's also the fact that we already have GAMMA/DEGAMMA properties
with a defined interface and semantics.

Mali-DP falls in the "fully programmable" camp - we can use a
programmable de-gamma curve in the plane pipeline, and for this, the
existing DEGAMMA property is a good fit.

For not-programmable hardware, would a second "DEGAMMA_FIXED" property
make sense, which is an enum type exposing what curves are supported?
(with analogous GAMMA_FIXED as well)

Drivers should expose one or the other (but not both) for each
gamma/degamma conversion the hardware implements.

-Brian

>
>>
>> - 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
>> >>
>
>-- 
>Ville Syrj?l?
>Intel OTC