Re: [PATCH i-g-t v2] tests/kms_plane: Remove redundant CRC frame sequence check

["Juha-Pekka Heikkilä" <juhapekka.heikkila@gmail.com>]

Hi,

On 13/08/2026 06.37, Karthik B S wrote:
> Hi Jason-JH,
> 
> On 8/11/2026 9:44 PM, Jason-JH Lin wrote:
>> The capture_crc() function validated that the CRC frame sequence
>> returned by igt_pipe_crc_get_for_frame() matches the expected value.
>> However, igt_pipe_crc_get_for_frame() already guarantees
>> crc->frame >= expected via its internal loop:
>>
>>      do {
>>          read_one_crc(pipe_crc, crc);
>>      } while (igt_vblank_before(crc->frame, vblank));
> This isn't fully true IMHO. The capture CRC function actually ensured 
> the exact match of frame sequence and with this patch we're just 
> guaranteeing '>='.

as Karthik said; the claim it not true. What this change would do is 
relax the sequence check to be open ended .. while current check is 
making exact expectation. In other words, we _expect_ to see certain crc 
with correct vblank number, with the proposed change if expected crc 
never arrived in correct sequence we would be unaware of it.

Let's not do this.

> So we need more context here from: https://patchwork.freedesktop.org/ 
> series/168037/
>>
>> The additional check in capture_crc() is therefore redundant.
>> Remove it and rely on the library's existing guarantee.
>>
>> Signed-off-by: Jason-JH Lin <jason-jh.lin@mediatek.com>
>> ---
>>   tests/kms_plane.c | 5 -----
>>   1 file changed, 5 deletions(-)
>>
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>> index 12dfbfe1d82b..fe8ee2ab26ab 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -765,11 +765,6 @@ static int num_unique_crcs(const igt_crc_t crc[], 
>> int num_crc)
>>   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);
> 
> Also if this is only igt_pipe_crc_get_for_frame now, ideally we can just 
> remove this function itself and call the helper directly. But before 
> doing that, as the existing assert was added by a patch from Ville and 
> rb'ed by JP, I'll request an ack from them or if they have any inputs on 
> this.
> 
> Regards,
> Karthik.B.S
>> -
>> -    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->frame, vblank);
>>   }
>>   static void capture_format_crcs_single(data_t *data, igt_crtc_t *crtc,