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

[Daniel Vetter <daniel@ffwll.ch>]

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 also noticed that the documentation is kinda wrong in the kernel, since
it talks about 0.16 fixed point. 0.ffff = 1.0 definitely needs to hold by
convention at least, and ideally we'd round this stuff correctly for
intermediate values too. But as long as we have fewer than 16bits of gamma
precision it will only matter for 1.0/0xffff.
-Daniel

> -Daniel
> 
> > linear gamma affects crc. This patch fixes the same by making
> > ctm tests independant of gamma/degamma.
> > 
> > v2: Disable degamma/gamma programming for ctm max test as it
> > leads to crc mimsmatch. Limiting it to this test case alone as
> > other tests need it to be enabled, hence not touching those
> > scenarios.
> > 
> > v3: Fixed a fumble with compilation.
> > 
> > v4: Disabling degamma and gamma for ctm max tests, after the logic
> > in kernel has been updated by Ville's series.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108147
> > Signed-off-by: Uma Shankar <uma.shankar@intel.com>
> > ---
> >  tests/kms_color.c | 15 +++++++++++++--
> >  1 file changed, 13 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/kms_color.c b/tests/kms_color.c
> > index decf3c2..0951ffc 100644
> > --- a/tests/kms_color.c
> > +++ b/tests/kms_color.c
> > @@ -696,8 +696,19 @@ static bool test_pipe_ctm(data_t *data,
> >  		igt_assert(fb_modeset_id);
> >  		igt_plane_set_fb(primary, &fb_modeset);
> >  
> > -		set_degamma(data, primary->pipe, degamma_linear);
> > -		set_gamma(data, primary->pipe, gamma_linear);
> > +		/*
> > +		 * Don't program LUT's for max CTM cases as limitation of
> > +		 * representing 1.0 due to ABI limits causes crc mismatch
> > +		 */
> > +		if (memcmp(before, after, sizeof(color_t))) {
> > +			set_degamma(data, primary->pipe, degamma_linear);
> > +			set_gamma(data, primary->pipe, gamma_linear);
> > +		} else {
> > +			/* Disable Degamma and Gamma for ctm max test */
> > +			disable_degamma(primary->pipe);
> > +			disable_gamma(primary->pipe);
> > +		}
> > +
> >  		disable_ctm(primary->pipe);
> >  		igt_display_commit(&data->display);
> >  
> > -- 
> > 1.9.1
> > 
> > _______________________________________________
> > igt-dev mailing list
> > igt-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/igt-dev
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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