[igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test

Wed Apr 3 08:32:50 UTC 2019

On Tue, Apr 02, 2019 at 12:54:26PM +0000, Shankar, Uma wrote:
> 
> 
> >-----Original Message-----
> >From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
> >Sent: Tuesday, April 2, 2019 2:23 PM
> >To: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >Cc: Daniel Vetter <daniel at ffwll.ch>; Shankar, Uma <uma.shankar at intel.com>; igt-
> >dev at lists.freedesktop.org; Syrjala, Ville <ville.syrjala at intel.com>; Lankhorst,
> >Maarten <maarten.lankhorst at intel.com>
> >Subject: Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test
> >
> >On Mon, Apr 01, 2019 at 08:51:02PM +0300, Ville Syrjälä wrote:
> >> On Mon, Apr 01, 2019 at 09:26:33AM +0200, Daniel Vetter wrote:
> >> > On Fri, Mar 29, 2019 at 12:38:55PM +0200, Ville Syrjälä wrote:
> >> > > On Fri, Mar 29, 2019 at 10:05:14AM +0100, Daniel Vetter wrote:
> >> > > > On Fri, Mar 29, 2019 at 10:00:59AM +0100, Daniel Vetter wrote:
> >> > > > > On Fri, Mar 29, 2019 at 02:49:15AM +0530, Uma Shankar wrote:
> >> > > > > > Due to Gamma/Degamma limitation with precision (lack of
> >> > > > > > exact 1.0 representation) due to ABI restriction, applying
> >> > > > >
> >> > > > > Huh, why? That sounds like a conversion bug in our gamma table handler.
> >> > > > > 0xffff == 1.0 if we don't treat it like that that's a driver
> >> > > > > bug. The gamma table is _not_ fixed point, but linear range
> >> > > > > from 0-0xffff. Which is unlike the ctm (which due to an uapi
> >> > > > > accident has a really hilarious fixed point with sign bit format).
> >> > > > >
> >> > > > > Please don't paper over driver bugs :-)
> >> > > >
> >> > > > Can you pls also review existing gamma igt coverage to make sure
> >> > > > we're catching this? Or maybe it's just the testcase that fills
> >> > > > the gamma table the wrong way.
> >> > >
> >> > > I've been pondering if we should just do value+1 in the kernel for
> >> > > the last LUT entry when using the interpolated modes.
> >> >
> >> > See my doc patch, imo we should do that. We even should have done
> >> > that with the old documentation, since 0.16 ff rounded to any LUT
> >> > with less precision measn 0.0xffff should round up to 1.0. If we do
> >> > correct rounding.
> >> >
> >> > That's always been the intention really, the docs just clarify that
> >> > yes you should round correctly even if you happen to have a 16bit LUT.
> >> >
> >> > > For userspace we could probably use the odd LUT size as a hint to
> >> > > indicate that the hardware will interpolate. So userspace could
> >> > > just do something like "if (size & 1) max = 1<<16; else max = (1<<16)-1;"
> >> > > when generating the curve (+ clamp to 0xffff). Looks like there's
> >> > > some kind of kludge for CHV in kms_color atm, but maybe we can
> >> > > just replace that with the generic logic above.
> >> >
> >> > Hm I didn't look at details, but clamp to 0xffff sounds still wrong,
> >> > we should correctly convert from 0.0-1.0 to 0-0xffff. Not that
> >> > there's going to be a huge difference except for 1.0 (if we haven't
> >> > rounded correctly thus far).
> >>
> >> Not sure what is correct rounding anyway. Userspace not knowing the
> >> precision of the LUT entries does lead to some issues.
> >>
> >> Eg. if we have a 4 entry non-interpolated LUT with 8bit precision and
> >> we want a linear ramp the correct values would be 0x00,0x55,0xaa,0xff,
> >> but with userspace filling in the full 16bits values, rounding will
> >> get us 0x00,0x55,0xab,0x100. So not quite right. OTOH if we had 16bit
> >> precision I think currently we wouldn't round at all and we'd get the
> >> correct answer.
> >>
> >> For the interpolated 5 entry LUT it would actually work out if
> >> userspace fills in 0x0000,0x3ffff.7xfff,0xbfff,0xffff so we get
> >> 0x00,0x40,0x80,0xc0,0x100 after rounding. Which is correct. But if we
> >> used the full 16bit precision then with no rounding we'd get the wrong
> >> answer.
> >
> >Hm, awkward. I guess if we need a linear map, then we need to disable the LUT. And
> >if there's a LUT the only things we can guarantee is that 0.0 and
> >1.0 get through unscated, and everything else might have tiny rounding issues. I guess
> >that means the patch is still good, but needs an improved commit message.
> 
> For legacy this will be the problem as we can't avoid this rounding and fit all values accurately.
> Our hardware has 17 bits to represent 1.0, and 0xffff and 0x10000 should be represented as
> distinct entities.

There is no 0x10000 in the current uapi. And 0xffff == 1.0, as per the
uapi clarification patch I've just merged.

Wrt 17 bits ... how does that work? I've never even seen a real world
display with more than 12 bits of depth. What do we need the additional 5
bits on top? Is that just because we operate in linear space for the CTM?

> I feel if we get the ABI extended to use u32.32, we may well be able to solve for upcoming
> new platforms and HDR type usescases were Lut precision is 24 bits. Attempted in this patch:
> https://patchwork.freedesktop.org/patch/294732/?series=30875&rev=7

Or is HDR using massive non-linear representation of bit depth?

> So for now, we may disable linear gamma programming to avoid these crc errors. I hope this is ok.

Yeah, that part makes sense. But we need to update the commit message,
because atm it's just wrong: 1.0 is the one value we can (and should,
under all conditions) represent perfectly accurately. It's the
intermediate values which cause rounding issues and inaccurancies.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch