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

[Karthik B S <karthik.b.s@intel.com>]

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 <jason-jh.lin@mediatek.com>
> ---
>   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);
}

Regards,
Karthik.B.S
> +		      "Got CRC for the wrong frame (got %u, expected >= %u). CRC buffer overflow?\n",
>   		      crc->frame, vblank);
>   }
>