* [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test @ 2019-03-28 21:59 Uma Shankar 2019-03-28 22:08 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) Patchwork 2019-03-29 8:39 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 0 siblings, 2 replies; 16+ messages in thread From: Uma Shankar @ 2019-03-28 21:59 UTC (permalink / raw) To: igt-dev; +Cc: ville.syrjala, maarten.lankhorst Due to Gamma/Degamma limitation with precision (lack of exact 1.0 representation) due to ABI restriction, applying 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. v5: Disabled gamma/degamma for all ctm tests as suggested by Ville. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=108147 Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- tests/kms_color.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/kms_color.c b/tests/kms_color.c index decf3c2..2a6f4b7 100644 --- a/tests/kms_color.c +++ b/tests/kms_color.c @@ -696,9 +696,10 @@ 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); + disable_degamma(primary->pipe); + disable_gamma(primary->pipe); disable_ctm(primary->pipe); + igt_display_commit(&data->display); paint_rectangles(data, mode, after, &fb); -- 1.9.1 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) 2019-03-28 21:59 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar @ 2019-03-28 22:08 ` Patchwork 2019-03-29 8:39 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork 1 sibling, 0 replies; 16+ messages in thread From: Patchwork @ 2019-03-28 22:08 UTC (permalink / raw) To: Shankar, Uma; +Cc: igt-dev == Series Details == Series: tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) URL : https://patchwork.freedesktop.org/series/58412/ State : success == Summary == CI Bug Log - changes from CI_DRM_5832 -> IGTPW_2732 ==================================================== Summary ------- **SUCCESS** No regressions found. External URL: https://patchwork.freedesktop.org/api/1.0/series/58412/revisions/8/mbox/ Known issues ------------ Here are the changes found in IGTPW_2732 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_exec_suspend@basic-s3: - fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718] * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-a: - fi-byt-clapper: PASS -> FAIL [fdo#103191] / [fdo#107362] +2 #### Possible fixes #### * igt@kms_busy@basic-flip-a: - fi-gdg-551: FAIL [fdo#103182] -> PASS #### Warnings #### * igt@i915_selftest@live_contexts: - fi-icl-u3: DMESG-FAIL [fdo#108569] -> INCOMPLETE [fdo#108569] [fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182 [fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191 [fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362 [fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718 [fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569 Participating hosts (43 -> 38) ------------------------------ Missing (5): fi-kbl-soraka fi-ilk-m540 fi-hsw-4200u fi-bsw-n3050 fi-bsw-cyan Build changes ------------- * IGT: IGT_4911 -> IGTPW_2732 CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_2732: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2732/ IGT_4911: d9fe699ea45406e279b78d1afdb4d57a205a3c99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2732/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* [igt-dev] ✗ Fi.CI.IGT: failure for tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) 2019-03-28 21:59 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar 2019-03-28 22:08 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) Patchwork @ 2019-03-29 8:39 ` Patchwork 1 sibling, 0 replies; 16+ messages in thread From: Patchwork @ 2019-03-29 8:39 UTC (permalink / raw) To: Shankar, Uma; +Cc: igt-dev == Series Details == Series: tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) URL : https://patchwork.freedesktop.org/series/58412/ State : failure == Summary == CI Bug Log - changes from CI_DRM_5832_full -> IGTPW_2732_full ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with IGTPW_2732_full absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_2732_full, please notify your bug team to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://patchwork.freedesktop.org/api/1.0/series/58412/revisions/8/mbox/ Possible new issues ------------------- Here are the unknown changes that may have been introduced in IGTPW_2732_full: ### IGT changes ### #### Possible regressions #### * igt@kms_color@pipe-a-ctm-0-25: - shard-kbl: PASS -> FAIL +8 * igt@kms_color@pipe-c-ctm-0-25: - shard-apl: PASS -> FAIL +8 * igt@kms_color@pipe-c-ctm-0-75: - shard-glk: PASS -> FAIL +8 Known issues ------------ Here are the changes found in IGTPW_2732_full that come from known issues: ### IGT changes ### #### Issues hit #### * igt@kms_atomic_transition@3x-modeset-transitions: - shard-snb: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] +12 * igt@kms_atomic_transition@6x-modeset-transitions-fencing: - shard-glk: NOTRUN -> SKIP [fdo#109271] / [fdo#109278] * igt@kms_atomic_transition@plane-all-modeset-transition: - shard-kbl: PASS -> INCOMPLETE [fdo#103665] - shard-apl: PASS -> INCOMPLETE [fdo#103927] * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a: - shard-glk: PASS -> DMESG-WARN [fdo#110222] - shard-hsw: PASS -> DMESG-WARN [fdo#110222] - shard-kbl: PASS -> DMESG-WARN [fdo#110222] * igt@kms_color@pipe-a-ctm-green-to-red: - shard-kbl: PASS -> FAIL [fdo#107201] - shard-apl: PASS -> FAIL [fdo#107201] * igt@kms_cursor_crc@cursor-128x128-suspend: - shard-snb: PASS -> INCOMPLETE [fdo#105411] * igt@kms_cursor_legacy@2x-long-cursor-vs-flip-legacy: - shard-hsw: PASS -> FAIL [fdo#105767] * igt@kms_cursor_legacy@2x-long-nonblocking-modeset-vs-cursor-atomic: - shard-glk: PASS -> FAIL [fdo#106509] / [fdo#107409] * igt@kms_fbcon_fbt@fbc: - shard-glk: NOTRUN -> SKIP [fdo#109271] +14 * igt@kms_flip@2x-flip-vs-expired-vblank-interruptible: - shard-glk: PASS -> FAIL [fdo#105363] * igt@kms_flip@2x-modeset-vs-vblank-race: - shard-glk: PASS -> FAIL [fdo#103060] * igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt: - shard-snb: NOTRUN -> SKIP [fdo#109271] +91 * igt@kms_setmode@basic: - shard-hsw: PASS -> FAIL [fdo#99912] * igt@kms_vblank@pipe-b-query-forked-busy: - shard-snb: PASS -> SKIP [fdo#109271] * igt@perf_pmu@busy-hang-vcs1: - shard-apl: NOTRUN -> SKIP [fdo#109271] #### Possible fixes #### * igt@gem_create@create-clear: - shard-snb: INCOMPLETE [fdo#105411] -> PASS * igt@kms_busy@extended-modeset-hang-newfb-render-c: - shard-hsw: DMESG-WARN [fdo#110222] -> PASS +1 * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a: - shard-snb: DMESG-WARN [fdo#110222] -> PASS * igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-b: - shard-kbl: DMESG-WARN [fdo#110222] -> PASS +2 * igt@kms_color@pipe-c-ctm-max: - shard-glk: FAIL [fdo#108147] -> PASS +2 * igt@kms_cursor_crc@cursor-128x128-dpms: - shard-kbl: FAIL [fdo#103232] -> PASS - shard-apl: FAIL [fdo#103232] -> PASS * igt@kms_draw_crc@draw-method-xrgb8888-render-xtiled: - shard-glk: FAIL [fdo#107791] -> PASS * igt@kms_frontbuffer_tracking@fbc-1p-primscrn-spr-indfb-draw-mmap-cpu: - shard-apl: FAIL [fdo#103167] -> PASS - shard-kbl: FAIL [fdo#103167] -> PASS * igt@kms_frontbuffer_tracking@fbc-2p-primscrn-shrfb-plflip-blt: - shard-glk: FAIL [fdo#103167] -> PASS * igt@kms_vblank@pipe-a-ts-continuation-modeset: - shard-apl: FAIL [fdo#104894] -> PASS - shard-kbl: FAIL [fdo#104894] -> PASS [fdo#103060]: https://bugs.freedesktop.org/show_bug.cgi?id=103060 [fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167 [fdo#103232]: https://bugs.freedesktop.org/show_bug.cgi?id=103232 [fdo#103665]: https://bugs.freedesktop.org/show_bug.cgi?id=103665 [fdo#103927]: https://bugs.freedesktop.org/show_bug.cgi?id=103927 [fdo#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894 [fdo#105363]: https://bugs.freedesktop.org/show_bug.cgi?id=105363 [fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411 [fdo#105767]: https://bugs.freedesktop.org/show_bug.cgi?id=105767 [fdo#106509]: https://bugs.freedesktop.org/show_bug.cgi?id=106509 [fdo#107201]: https://bugs.freedesktop.org/show_bug.cgi?id=107201 [fdo#107409]: https://bugs.freedesktop.org/show_bug.cgi?id=107409 [fdo#107791]: https://bugs.freedesktop.org/show_bug.cgi?id=107791 [fdo#108147]: https://bugs.freedesktop.org/show_bug.cgi?id=108147 [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271 [fdo#109278]: https://bugs.freedesktop.org/show_bug.cgi?id=109278 [fdo#110222]: https://bugs.freedesktop.org/show_bug.cgi?id=110222 [fdo#99912]: https://bugs.freedesktop.org/show_bug.cgi?id=99912 Participating hosts (10 -> 5) ------------------------------ Missing (5): shard-skl pig-hsw-4770r pig-glk-j5005 shard-iclb pig-skl-6260u Build changes ------------- * IGT: IGT_4911 -> IGTPW_2732 * Piglit: piglit_4509 -> None CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_2732: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2732/ IGT_4911: d9fe699ea45406e279b78d1afdb4d57a205a3c99 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2732/ _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test @ 2019-03-28 21:19 Uma Shankar 2019-03-28 20:56 ` Ville Syrjälä 2019-03-29 9:00 ` Daniel Vetter 0 siblings, 2 replies; 16+ messages in thread From: Uma Shankar @ 2019-03-28 21:19 UTC (permalink / raw) To: igt-dev; +Cc: ville.syrjala, maarten.lankhorst Due to Gamma/Degamma limitation with precision (lack of exact 1.0 representation) due to ABI restriction, applying 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-28 21:19 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar @ 2019-03-28 20:56 ` Ville Syrjälä 2019-03-28 21:31 ` Shankar, Uma 2019-03-28 21:41 ` Shankar, Uma 2019-03-29 9:00 ` Daniel Vetter 1 sibling, 2 replies; 16+ messages in thread From: Ville Syrjälä @ 2019-03-28 20:56 UTC (permalink / raw) To: Uma Shankar; +Cc: igt-dev, ville.syrjala, maarten.lankhorst 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 > 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); Why don't we just disable these always? > + } > + > 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 -- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-28 20:56 ` Ville Syrjälä @ 2019-03-28 21:31 ` Shankar, Uma 2019-03-28 21:41 ` Shankar, Uma 1 sibling, 0 replies; 16+ messages in thread From: Shankar, Uma @ 2019-03-28 21:31 UTC (permalink / raw) To: Ville Syrjälä Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville, Lankhorst, Maarten >-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Friday, March 29, 2019 2:27 AM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: igt-dev@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, >Maarten <maarten.lankhorst@intel.com> >Subject: Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test > >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 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); > >Why don't we just disable these always? Yeah fair point. Will update the patch. > >> + } >> + >> 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 > >-- >Ville Syrjälä >Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-28 20:56 ` Ville Syrjälä 2019-03-28 21:31 ` Shankar, Uma @ 2019-03-28 21:41 ` Shankar, Uma 1 sibling, 0 replies; 16+ messages in thread From: Shankar, Uma @ 2019-03-28 21:41 UTC (permalink / raw) To: Ville Syrjälä Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville, Lankhorst, Maarten > >>-----Original Message----- >>From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >>Sent: Friday, March 29, 2019 2:27 AM >>To: Shankar, Uma <uma.shankar@intel.com> >>Cc: igt-dev@lists.freedesktop.org; Syrjala, Ville >><ville.syrjala@intel.com>; Lankhorst, Maarten >><maarten.lankhorst@intel.com> >>Subject: Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues >>with ctm test >> >>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 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); >> >>Why don't we just disable these always? > >Yeah fair point. Will update the patch. There is an issue it seems if we disable for all ctm tests. There are failures for pipe-A-ctm-0-25 , pipe-A-ctm-0-5 and pipe-A-ctm-0-75 tests. Not sure why they depend on gamma to be enabled. > >> >>> + } >>> + >>> 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 >> >>-- >>Ville Syrjälä >>Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-28 21:19 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar 2019-03-28 20:56 ` Ville Syrjälä @ 2019-03-29 9:00 ` Daniel Vetter 2019-03-29 9:05 ` Daniel Vetter 1 sibling, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-03-29 9:00 UTC (permalink / raw) To: Uma Shankar; +Cc: igt-dev, ville.syrjala, maarten.lankhorst 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 :-) -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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-29 9:00 ` Daniel Vetter @ 2019-03-29 9:05 ` Daniel Vetter 2019-03-29 10:38 ` Ville Syrjälä 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-03-29 9:05 UTC (permalink / raw) To: Uma Shankar; +Cc: igt-dev, ville.syrjala, maarten.lankhorst 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-29 9:05 ` Daniel Vetter @ 2019-03-29 10:38 ` Ville Syrjälä 2019-04-01 7:26 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Ville Syrjälä @ 2019-03-29 10:38 UTC (permalink / raw) To: Daniel Vetter; +Cc: igt-dev, ville.syrjala, maarten.lankhorst 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. 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. 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? -- Ville Syrjälä Intel _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-03-29 10:38 ` Ville Syrjälä @ 2019-04-01 7:26 ` Daniel Vetter 2019-04-01 17:51 ` Ville Syrjälä 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-04-01 7:26 UTC (permalink / raw) To: Ville Syrjälä Cc: igt-dev, ville.syrjala, maarten.lankhorst, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-04-01 7:26 ` Daniel Vetter @ 2019-04-01 17:51 ` Ville Syrjälä 2019-04-02 8:52 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Ville Syrjälä @ 2019-04-01 17:51 UTC (permalink / raw) To: Daniel Vetter; +Cc: igt-dev, ville.syrjala, maarten.lankhorst 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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-04-01 17:51 ` Ville Syrjälä @ 2019-04-02 8:52 ` Daniel Vetter 2019-04-02 12:54 ` Shankar, Uma 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-04-02 8:52 UTC (permalink / raw) To: Ville Syrjälä Cc: igt-dev, ville.syrjala, maarten.lankhorst, Daniel Vetter 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. -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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-04-02 8:52 ` Daniel Vetter @ 2019-04-02 12:54 ` Shankar, Uma 2019-04-03 8:32 ` Daniel Vetter 0 siblings, 1 reply; 16+ messages in thread From: Shankar, Uma @ 2019-04-02 12:54 UTC (permalink / raw) To: Daniel Vetter, Ville Syrjälä Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville, Lankhorst, Maarten >-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Tuesday, April 2, 2019 2:23 PM >To: Ville Syrjälä <ville.syrjala@linux.intel.com> >Cc: Daniel Vetter <daniel@ffwll.ch>; Shankar, Uma <uma.shankar@intel.com>; igt- >dev@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, >Maarten <maarten.lankhorst@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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-04-02 12:54 ` Shankar, Uma @ 2019-04-03 8:32 ` Daniel Vetter 2019-04-03 9:29 ` Shankar, Uma 0 siblings, 1 reply; 16+ messages in thread From: Daniel Vetter @ 2019-04-03 8:32 UTC (permalink / raw) To: Shankar, Uma Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville, Daniel Vetter, Lankhorst, Maarten On Tue, Apr 02, 2019 at 12:54:26PM +0000, Shankar, Uma wrote: > > > >-----Original Message----- > >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter > >Sent: Tuesday, April 2, 2019 2:23 PM > >To: Ville Syrjälä <ville.syrjala@linux.intel.com> > >Cc: Daniel Vetter <daniel@ffwll.ch>; Shankar, Uma <uma.shankar@intel.com>; igt- > >dev@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, > >Maarten <maarten.lankhorst@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? > 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? > 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. -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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 2019-04-03 8:32 ` Daniel Vetter @ 2019-04-03 9:29 ` Shankar, Uma 0 siblings, 0 replies; 16+ messages in thread From: Shankar, Uma @ 2019-04-03 9:29 UTC (permalink / raw) To: Daniel Vetter Cc: igt-dev@lists.freedesktop.org, Syrjala, Ville, Lankhorst, Maarten >-----Original Message----- >From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter >Sent: Wednesday, April 3, 2019 2:03 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: Daniel Vetter <daniel@ffwll.ch>; Ville Syrjälä <ville.syrjala@linux.intel.com>; igt- >dev@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; Lankhorst, >Maarten <maarten.lankhorst@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@ffwll.ch] On Behalf Of >> >Daniel Vetter >> >Sent: Tuesday, April 2, 2019 2:23 PM >> >To: Ville Syrjälä <ville.syrjala@linux.intel.com> >> >Cc: Daniel Vetter <daniel@ffwll.ch>; Shankar, Uma >> ><uma.shankar@intel.com>; igt- dev@lists.freedesktop.org; Syrjala, >> >Ville <ville.syrjala@intel.com>; Lankhorst, Maarten >> ><maarten.lankhorst@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 _______________________________________________ igt-dev mailing list igt-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/igt-dev ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2019-04-03 9:29 UTC | newest] Thread overview: 16+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2019-03-28 21:59 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar 2019-03-28 22:08 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev8) Patchwork 2019-03-29 8:39 ` [igt-dev] ✗ Fi.CI.IGT: failure " Patchwork -- strict thread matches above, loose matches on Subject: below -- 2019-03-28 21:19 [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Uma Shankar 2019-03-28 20:56 ` Ville Syrjälä 2019-03-28 21:31 ` Shankar, Uma 2019-03-28 21:41 ` Shankar, Uma 2019-03-29 9:00 ` Daniel Vetter 2019-03-29 9:05 ` Daniel Vetter 2019-03-29 10:38 ` Ville Syrjälä 2019-04-01 7:26 ` Daniel Vetter 2019-04-01 17:51 ` Ville Syrjälä 2019-04-02 8:52 ` Daniel Vetter 2019-04-02 12:54 ` Shankar, Uma 2019-04-03 8:32 ` Daniel Vetter 2019-04-03 9:29 ` Shankar, Uma
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox