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

Thanks @Karthik B S <[email protected]>  for your review , @Jason-JH Lin
<[email protected]>  please update the patch to address the review
so we can get this landed while we have traction on this from Intel

Regards
Manasi

On Mon, Aug 10, 2026 at 2:04 AM Jason-JH Lin (林睿祥) <
[email protected]> wrote:

> On Mon, 2026-08-10 at 10:20 +0530, Karthik B S wrote:
> > Hi Jason-JH,
> >
> > On 6/8/2026 7:57 AM, Jason-JH Lin 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,
> >
> > Agree on the concept, but with that then isn't this check itself
> > redundant as this has been taken care of already in the
> > 'igt_pipe_crc_get_for_frame'?
> >
> > static inline bool igt_vblank_before(uint32_t a, uint32_t b)
> > {
> >          return igt_vblank_after(b, a);
> > }
> >
>
> Hi Karthik,
>
> You're right. Since igt_pipe_crc_get_for_frame() internally loops
> with igt_vblank_before(crc->frame, vblank), it already guarantees
> crc->frame >= vblank upon return. The < vblank check in capture_crc()
> would never trigger under normal conditions, making it redundant.
>
> I've removed the entire frame sequence validation in capture_crc()
> and will rely on the library's existing guarantee. Will update the
> patch accordingly.
>
> Thanks for catching this.
>
> Regards,
> Jason-JH Lin
>
>
> > Regards,
> > Karthik.B.S
> > > +                 "Got CRC for the wrong frame (got %u,
> > > expected >= %u). CRC buffer overflow?\n",
> > >                   crc->frame, vblank);
> > >   }
> > >
>
>