[igt-dev] [PATCH i-g-t] tests/kms_color: fix crc assert condition

[Modem, Bhanuprakash]

On Wed-21-09-2022 04:20 pm, Ville Syrjälä wrote:
> On Wed, Sep 21, 2022 at 09:36:13AM -0100, Melissa Wen wrote:
>> On 09/21, Ville Syrjälä wrote:
>>> On Tue, Sep 20, 2022 at 03:32:39PM +0530, Modem, Bhanuprakash wrote:
>>>> On Mon-19-09-2022 09:24 pm, Melissa Wen wrote:
>>>>> On 09/01, Modem, Bhanuprakash wrote:
>>>>>> On Wed-31-08-2022 03:41 pm, Melissa Wen wrote:
>>>>>>> In test_pipe_degamma/gamma/ctm, igt_assert_crc_equal() was replaced by
>>>>>>>
>>>>>>> ret = !igt_skip_crc_compare || igt_check_crc_equal()
>>>>>>> and then igt_assert(ret)
>>>>>>>
>>>>>>> where igt_check_crc_equal returns !mismatch, and therefore we can
>>>>>>> translate as:
>>>>>>>
>>>>>>> ret = !igt_skip_crc_compare || !mismatch
>>>>>>>
>>>>>>> However, the original igt_assert_crc_equal() assertion does:
>>>>>>>
>>>>>>> igt_assert(!mismatch || igt_skip_crc_compare)
>>>>>>>
>>>>>>> That means, the replacement changes the original assertion. Moreover,
>>>>>>> negating `igt_skip_crc_compare` makes the test assertion to be always
>>>>>>> true (sucessful) by default and reverses the logic of
>>>>>>> --skip-crc-compare.
>>>>>>>
>>>>>>> Fixes: d61e4598142 ("tests/kms_color: Convert tests to dynamic")
>>>>>>> Signed-off-by: Melissa Wen <mwen at igalia.com>
>>>>>>
>>>>>> LGTM
>>>>>> Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem at intel.com>
>>>>>>
>>>>>> - Bhanu
>>>>>
>>>>> Thanks for reviewing, Bhanu.
>>>>>
>>>>> Siqueira,
>>>>>
>>>>> Is it okay from AMD side? If so, can you apply it?
>>>>
>>>> Applied.
>>>
>>> It did _not_ pass CI and so should not have been pushed!
>>
>> Hi Ville,
>>
>> So, this change "not passed" because the test was not actually assessing
>> the CRC results (i.e. !igt_skip_crc_compare makes the assertion always
>> successful - a false PASS). In this context, I think CI testlist should be
>> updated, instead of not applying this change, right?
> 
> No. Either the test never worked correctly or there was an earlier
> regression hidden by the thing this is now fixing. Either way it
> needs to be fixed. Just reverting the whole sordid mess and starting
> from scratch may be the easiest option.

I have floated a patch to revert this.
https://patchwork.freedesktop.org/series/108835/

Ville/Melissa, Can I have an Ack to merge this?

- Bhanu

>