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: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.

-- 
Ville Syrjälä
Intel