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

[Daniel Vetter]

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch