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

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);
> >   }
> >