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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@igalia.com>
> >>>>>>
> >>>>>> LGTM
> >>>>>> Reviewed-by: Bhanuprakash Modem <bhanuprakash.modem@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.

-- 
Ville Syrjälä
Intel