Re: [PATCH i-g-t] tests/kms_plane: Relax CRC frame sequence validation

Hi Kamil,

Sorry to bother you.

Since this patch has been pending since June 8th.
I think it was accidentally lost. Could you please help review it or
find someone else to help?

I would really appreciate any suggestion or feedback you may have.

Regards,
Jason-JH Lin

On Thu, 2026-06-11 at 23:07 -0700, Manasi Navare wrote:
> Thanks Jason for the patch, this makes sense to me.
> @Karthik B S  @Swati Sharma  could we get some reviews on this since
> this is extremely critical for us.
> 
> Regards
> Manasi
> 
> On Sun, Jun 7, 2026 at 7:28 PM Jason-JH Lin
> <[email protected]> wrote:
> > 
> > The capture_crc() function validates that the CRC frame sequence
> > returned by igt_pipe_crc_get_for_frame() matches the expected
> > value.
> > The current check uses exact equality (!=), however the library
> > function igt_pipe_crc_get_for_frame() uses >= comparison
> > internally:
> > 
> >     do {
> >         read_one_crc(pipe_crc, crc);
> >         ...
> >     } while (igt_vblank_before(crc->frame, vblank));
> > 
> > The function loops while crc->frame < vblank, returning the first
> > CRC where crc->frame >= expected, not necessarily an exact match.
> > 
> > This strict validation can cause test failures on drivers that
> > report CRC entries with frame sequences larger than expected. This
> > happens when drivers use internal queuing mechanisms to ensure CRC
> > values are correctly correlated with their corresponding frames.
> > 
> > Example without internal queue (continuous CRC reporting):
> >   - Driver generates CRC entries continuously every frame
> >   - Frame sequence increments sequentially
> >   - Test finds exact matches for all expected frames → PASS
> > 
> > Example with internal queue (per-commit CRC reporting):
> >   - Driver queues CRC and reports with current frame sequence
> >   - Due to queue processing delay, reported sequence > expected
> >   - For the last CRC, test adds +1 to expected (per IGT logic)
> >   - Library's >= comparison returns valid CRC
> >   - capture_crc's != check fails on last frame → FAIL
> > 
> > Relax the validation to use < comparison instead of !=, which:
> > 1. Aligns with igt_pipe_crc_get_for_frame() internal behavior
> > 2. Accepts CRC entries with frame >= expected (valid cases)
> > 3. Still catches real errors where frame < expected, indicating
> >    CRC buffer overflow or stale data
> > 
> > Signed-off-by: Jason-JH Lin <[email protected]>
> > ---
> >  tests/kms_plane.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/tests/kms_plane.c b/tests/kms_plane.c
> > index f21006784bfc..548506290c9b 100644
> > --- a/tests/kms_plane.c
> > +++ b/tests/kms_plane.c
> > @@ -766,9 +766,13 @@ static void capture_crc(data_t *data, unsigned
> > int vblank, igt_crc_t *crc)
> >  {
> >         igt_pipe_crc_get_for_frame(data->drm_fd, data->pipe_crc,
> > vblank, crc);
> > 
> > +       /*
> > +        * igt_pipe_crc_get_for_frame uses >= comparison, so we
> > check the same.
> > +        * Getting a frame < expected indicates CRC buffer overflow
> > or timing issue.
> > +        */
> >         igt_fail_on_f(!igt_skip_crc_compare &&
> > !igt_run_in_simulation() &&
> > -                     crc->has_valid_frame && crc->frame != vblank,
> > -                     "Got CRC for the wrong frame (got %u,
> > expected %u). CRC buffer overflow?\n",
> > +                     crc->has_valid_frame && crc->frame < vblank,
> > +                     "Got CRC for the wrong frame (got %u,
> > expected >= %u). CRC buffer overflow?\n",
> >                       crc->frame, vblank);
> >  }
> > 
> > --
> > 2.43.0
> >