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

[Melissa Wen]

On 09/21, Ville Syrjälä wrote:
> On Wed, Sep 21, 2022 at 09:24:25PM +0530, Modem, Bhanuprakash wrote:
> > 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?
> 
> I mean we have to revert all the way back to before the
> dunamic subtest conversion. AIUI that is when we started
> to ignore real test failures, and so we don't even know
> whether it was that commit itself or something later that
> really broke the test.

It's clear to me that the regression was introduced by:
d61e4598142 ("tests/kms_color: Convert tests to dynamic")

As I explained in the commit message, we can see it in the patchwork
diff [1] (or gitlab commit diff, but the patchwork just displays it
clear enough), as here:

---
@@ -285,7 +286,7 @@  static void test_pipe_legacy_gamma(data_t *data,
 	/* Verify that the CRC of the software computed output is
 	 * equal to the CRC of the gamma LUT transformation output.
 	 */
-	igt_assert_crc_equal(&crc_fullgamma, &crc_fullcolors);
+	ret = !igt_skip_crc_compare || igt_check_crc_equal(&crc_fullgamma, &crc_fullcolors);

 	/* Reset output. */
 	for (i = 1; i < legacy_lut_size; i++)
---

[1] https://patchwork.freedesktop.org/patch/491540/

btw, I'm okay with the approach of reverting both. So, I understand
Bhanuprakash will submit a new patch with a dynamic test conversion that
handles the assertions correctly, right?

BR,

Melissa

> 
> -- 
> Ville Syrjälä
> Intel
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/igt-dev/attachments/20220921/c195b740/attachment.sig>