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

Wed Apr 3 09:29:27 UTC 2019

>-----Original Message-----
>From: Daniel Vetter [mailto:daniel.vetter at ffwll.ch] On Behalf Of Daniel Vetter
>Sent: Wednesday, April 3, 2019 2:03 PM
>To: Shankar, Uma <uma.shankar at intel.com>
>Cc: Daniel Vetter <daniel at ffwll.ch>; Ville Syrjälä <ville.syrjala at linux.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 Tue, Apr 02, 2019 at 12:54:26PM +0000, Shankar, Uma wrote:
>>
>>
>> >-----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.
>
>There is no 0x10000 in the current uapi. And 0xffff == 1.0, as per the uapi clarification
>patch I've just merged.
>
>Wrt 17 bits ... how does that work? I've never even seen a real world display with
>more than 12 bits of depth. What do we need the additional 5 bits on top? Is that just
>because we operate in linear space for the CTM?

Yeah its mainly for linear/non-linear conversion before and after CTM, more
precision curves with too much non-linearity (like HDR EOTF) can be represented
accurately with less loss of details due to these conversions.

>> 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
>
>Or is HDR using massive non-linear representation of bit depth?

Yes exactly, HDR EOTF curve is extremely non-linear compared to sRGB 2.2 gamma etc.,
therefore we require high precision to avoid loss of details, if suppose we want to convert
HDR content to linear using degamma or applying non-linearity at output using gamma
blocks.

>> So for now, we may disable linear gamma programming to avoid these crc errors. I
>hope this is ok.
>
>Yeah, that part makes sense. But we need to update the commit message, because
>atm it's just wrong: 1.0 is the one value we can (and should, under all conditions)
>represent perfectly accurately. It's the intermediate values which cause rounding
>issues and inaccurancies.

Sure, I will update the commit message to reflect this properly. Thanks Daniel.

Regards,
Uma Shankar

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