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

From: Daniel Vetter <daniel@ffwll.ch>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: igt-dev@lists.freedesktop.org, ville.syrjala@intel.com,
	maarten.lankhorst@intel.com, Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test
Date: Mon, 1 Apr 2019 09:26:33 +0200	[thread overview]
Message-ID: <20190401072633.GT2665@phenom.ffwll.local> (raw)
In-Reply-To: <20190329103855.GS3888@intel.com>

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).

> I'm also not sure the gamma tests actually are testing things
> sufficiently. IIRC we have the 0 vs. max value type of tests
> but is there anything to make sure eg. a LUT value of 0.5 does
> what it's supposed to?

I think we decided we can't do that, because rounding. And only require
that at least 0.0 and 1.0 go through correctly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev