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

[Ville Syrjälä]

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.

-- 
Ville Syrjälä
Intel