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

[Shankar, Uma]

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

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

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

Thanks Ville and Daniel for your inputs and suggestions. 

Regards,
Uma Shankar

>-Daniel


>Daniel Vetter
>Software Engineer, Intel Corporation
>http://blog.ffwll.ch