Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755286AbdCPRFY (ORCPT ); Thu, 16 Mar 2017 13:05:24 -0400 Received: from mga02.intel.com ([134.134.136.20]:53558 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753439AbdCPRFW (ORCPT ); Thu, 16 Mar 2017 13:05:22 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,173,1486454400"; d="scan'208";a="77281047" Subject: Re: DRM Atomic property for color-space conversion To: Brian Starkey References: <20170127172324.GB12018@e106950-lin.cambridge.arm.com> <20170130133513.GO31595@intel.com> <20170131123329.GB24500@e106950-lin.cambridge.arm.com> <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> <20170316155501.GA25006@e106950-lin.cambridge.arm.com> Cc: Local user for Liviu Dudau , =?UTF-8?B?VmlsbGUgU3lyasOkbMOk?= , dri-devel@lists.freedesktop.org, linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, mihail.atanassov@arm.com, "Cyr, Aric" , "Wentland, Harry" , Alex Deucher From: "Sharma, Shashank" Message-ID: Date: Thu, 16 Mar 2017 19:05:12 +0200 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20170316155501.GA25006@e106950-lin.cambridge.arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 14851 Lines: 378 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 >>>>>>>> _to__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 " to " 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 >>>>> 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?) >>>>> 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 >>