Re: [igt-dev] [PATCH i-g-t] tests/kms_available_modes_crc: Fix handling of unknown single plane modes

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 05.11.2018 14:42, Maarten Lankhorst wrote:
> Op 05-11-18 om 12:24 schreef Juha-Pekka Heikkila:
>> On 05.11.2018 11:54, Maarten Lankhorst wrote:
>>> Op 03-11-18 om 12:28 schreef Juha-Pekka Heikkila:
>>>> Fix creation of gem buffer so that used bpp will always have value other
>>>> than 0.
>>>>
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106701
>>>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>
>>>> ---
>>>>    tests/kms_available_modes_crc.c | 10 +++++-----
>>>>    1 file changed, 5 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/tests/kms_available_modes_crc.c b/tests/kms_available_modes_crc.c
>>>> index 45a4775..006c2db 100644
>>>> --- a/tests/kms_available_modes_crc.c
>>>> +++ b/tests/kms_available_modes_crc.c
>>>> @@ -164,7 +164,7 @@ static const struct {
>>>>    #ifdef DRM_FORMAT_P016
>>>>        { DRM_FORMAT_P016, 0, P010, 0x8000eb00},
>>>>    #endif
>>>> -    { 0, 0, 0, 0 }
>>>> +    { 0, 0, SKIP4, 0 }
>>> Remove this entry, and make the loop
>>> for (i = 0; i < ARRAY_SIZE(fillers); i++) { }
>>>
>>> if (i == ARRAY_SIZE(fillers))
>>> return false;
>>>
>>> ?
>>>
>>> This should prevent it from ever being a problem in the first place.
>>>
>>
>> I was thinking this at first too but that 'skip4' flag is used to skip CRC checking at later time. This test runs in two levels, at first try to enable plane format and then do CRC. This last element is for the case where plane format was not hardcoded here.
>>
>> I did have another look at this test and made another patch which also relax CRC checking so this test will pass on our modern HW, I got idea from Ville's usage of gamma on kms_plane. I'll put it on the list in a minute.
> We shouldn't test unknown modes at least. SKIP4 was specifically added for Alpha RGB formats on gen9-10, because in some cases fully transparant and fully opaque framebuffers would have rounding errors resulting in a small mismatch picked up by the CRC tests.

While I agree for the most part this test is from the time when IGT 
supported much fever fb formats than now. It really wanted to test all 
formats which kernel advertised by trying to set up plane using those 
formats. This is why this test doesn't try to do anything difficult.

Those alpha bugs btw are not small rounding errors. If you try setting 
up sprite plane w/ rgb-alpha on top of primary plane with semicomplex 
image you don't need CRC to see those errors. :/

> 
> This is exactly why it might not be a good idea to randomly apply gamma tables hiding those rounding errors. In the future we want to prevent future occurences. If it's just the YUV formats that need a small correction to be fully white, we need to document it explicitly and not paper over it.
> 

For those tests there are more complex tests which will try to apply 
more features and see what happen. This test was just to see what fb 
formats kernel advertised and what will happen if try to use those.

In those ways I'd be ok just ditching this test as I think it probably 
doesn't anymore serve its original purpose but until that is done this 
test can be made to serve its original purpose with just using gamma table.

I just checked kms_plane also seems to apply gamma for all tests.

/Juha-Pekka
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev