* 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-28 21:17 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev7) Patchwork
` (2 subsequent siblings)
3 siblings, 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
* [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev7)
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:17 ` Patchwork
2019-03-29 7:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2019-03-29 9:00 ` [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Daniel Vetter
3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2019-03-28 21:17 UTC (permalink / raw)
To: Uma Shankar; +Cc: igt-dev
== Series Details ==
Series: tests/kms_color: Fix CRC mismatch issues with ctm test (rev7)
URL : https://patchwork.freedesktop.org/series/58412/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5832 -> IGTPW_2731
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/58412/revisions/7/mbox/
Known issues
------------
Here are the changes found in IGTPW_2731 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_module_load@reload:
- fi-blb-e6850: PASS -> INCOMPLETE [fdo#107718]
* igt@i915_selftest@live_hangcheck:
- fi-skl-iommu: PASS -> INCOMPLETE [fdo#108602] / [fdo#108744]
* igt@kms_frontbuffer_tracking@basic:
- fi-byt-clapper: PASS -> FAIL [fdo#103167]
* igt@runner@aborted:
- fi-skl-iommu: NOTRUN -> FAIL [fdo#104108] / [fdo#108602]
#### Possible fixes ####
* igt@i915_selftest@live_uncore:
- fi-ivb-3770: DMESG-FAIL [fdo#110210] -> PASS
* igt@kms_busy@basic-flip-a:
- fi-gdg-551: FAIL [fdo#103182] -> PASS +1
* igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
- fi-byt-clapper: FAIL [fdo#103191] / [fdo#107362] -> PASS
[fdo#103167]: https://bugs.freedesktop.org/show_bug.cgi?id=103167
[fdo#103182]: https://bugs.freedesktop.org/show_bug.cgi?id=103182
[fdo#103191]: https://bugs.freedesktop.org/show_bug.cgi?id=103191
[fdo#104108]: https://bugs.freedesktop.org/show_bug.cgi?id=104108
[fdo#107362]: https://bugs.freedesktop.org/show_bug.cgi?id=107362
[fdo#107718]: https://bugs.freedesktop.org/show_bug.cgi?id=107718
[fdo#108602]: https://bugs.freedesktop.org/show_bug.cgi?id=108602
[fdo#108744]: https://bugs.freedesktop.org/show_bug.cgi?id=108744
[fdo#110210]: https://bugs.freedesktop.org/show_bug.cgi?id=110210
Participating hosts (43 -> 35)
------------------------------
Missing (8): fi-kbl-soraka fi-ilk-m540 fi-bsw-n3050 fi-hsw-4200u fi-hsw-peppy fi-bsw-cyan fi-snb-2520m fi-pnv-d510
Build changes
-------------
* IGT: IGT_4911 -> IGTPW_2731
CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2731: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2731/
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_2731/
_______________________________________________
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ä
` (3 more replies)
0 siblings, 4 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 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, 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, 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
* [igt-dev] ✓ Fi.CI.IGT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev7)
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:17 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev7) Patchwork
@ 2019-03-29 7:46 ` Patchwork
2019-03-29 9:00 ` [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test Daniel Vetter
3 siblings, 0 replies; 16+ messages in thread
From: Patchwork @ 2019-03-29 7:46 UTC (permalink / raw)
To: Shankar, Uma; +Cc: igt-dev
== Series Details ==
Series: tests/kms_color: Fix CRC mismatch issues with ctm test (rev7)
URL : https://patchwork.freedesktop.org/series/58412/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_5832_full -> IGTPW_2731_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://patchwork.freedesktop.org/api/1.0/series/58412/revisions/7/mbox/
Known issues
------------
Here are the changes found in IGTPW_2731_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_ctx_isolation@vcs1-s3:
- shard-kbl: PASS -> INCOMPLETE [fdo#103665] +1
* 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_busy@extended-modeset-hang-newfb-with-reset-render-a:
- shard-glk: PASS -> DMESG-WARN [fdo#110222]
* igt@kms_fbcon_fbt@fbc:
- shard-glk: NOTRUN -> SKIP [fdo#109271] +14
* igt@kms_frontbuffer_tracking@psr-1p-primscrn-shrfb-pgflip-blt:
- shard-snb: NOTRUN -> SKIP [fdo#109271] +87
* igt@kms_setmode@basic:
- shard-hsw: PASS -> FAIL [fdo#99912]
* igt@kms_vblank@pipe-a-ts-continuation-dpms-rpm:
- shard-kbl: PASS -> FAIL [fdo#104894]
* igt@kms_vblank@pipe-b-ts-continuation-dpms-rpm:
- shard-apl: PASS -> FAIL [fdo#104894] +1
* 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-b:
- shard-hsw: DMESG-WARN [fdo#110222] -> PASS +1
- shard-kbl: DMESG-WARN [fdo#110222] -> PASS +1
* igt@kms_busy@extended-modeset-hang-newfb-with-reset-render-a:
- shard-snb: 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_plane_scaling@pipe-a-scaler-with-clipping-clamping:
- shard-glk: SKIP [fdo#109271] / [fdo#109278] -> PASS
* igt@kms_vblank@pipe-a-ts-continuation-modeset:
- shard-apl: FAIL [fdo#104894] -> PASS
- shard-kbl: FAIL [fdo#104894] -> PASS
[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#104894]: https://bugs.freedesktop.org/show_bug.cgi?id=104894
[fdo#105411]: https://bugs.freedesktop.org/show_bug.cgi?id=105411
[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_2731
* Piglit: piglit_4509 -> None
CI_DRM_5832: f1fc30ad3723a8b6265c2edf50a7f637ecd75a23 @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_2731: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_2731/
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_2731/
_______________________________________________
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
` (2 preceding siblings ...)
2019-03-29 7:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
@ 2019-03-29 9:00 ` Daniel Vetter
2019-03-29 9:05 ` Daniel Vetter
3 siblings, 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 ` [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 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, 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, 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, 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
* [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test
@ 2019-03-28 21:59 Uma Shankar
0 siblings, 0 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
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: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-28 21:17 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/kms_color: Fix CRC mismatch issues with ctm test (rev7) Patchwork
2019-03-29 7:46 ` [igt-dev] ✓ Fi.CI.IGT: " Patchwork
2019-03-29 9:00 ` [igt-dev] [v4] tests/kms_color: Fix CRC mismatch issues with ctm test 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
2019-03-28 21:59 Uma Shankar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.