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

Hi Jason-JH,

On 14/08/2026 05.46, Jason-JH Lin (林睿祥) wrote:
> On Thu, 2026-08-13 at 14:07 +0300, Juha-Pekka Heikkilä wrote:
>> 
>> External email : Please do not click links or open attachments until
>> you have verified the sender or the content.
>> 
>> 
>> 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.
> 
> Hi JP,
> 
> Thanks for the review. Let me explain the background of this issue.
> 
> MediaTek CRC Internal Queue Mechanism:
> MediaTek's DRM driver uses a per-commit CRC queue rather than
> continuous frame-by-frame reporting. CRC entries are reported only
> after the hardware pipeline has stabilized (3 vblanks after commit),
> and the frame number reflects the vblank count at the time of CRC
> readout, not the original commit latch time.
> 
> For example, consider 4 consecutive flips:
> 
> Timeline:
>    Frame 100: Flip 0 latches → ev.sequence=100
>    Frame 101: Flip 1 latches → ev.sequence=101
>    Frame 102: Flip 2 latches → ev.sequence=102
>    Frame 103: Flip 3 latches → ev.sequence=103 (last flip)
> 
> Queue reports CRCs with 3-vblank delay:
>    Frame 103: Reports flip 0 CRC → frame=103
>    Frame 104: Reports flip 1 CRC → frame=104
>    Frame 105: Reports flip 2 CRC → frame=105
>    Frame 106: Reports flip 3 CRC → frame=106
> 
> PlaneTest expected vblanks:
>    vblank[0] = 101  (flip 1's ev.sequence, for flip 0's CRC)
>    vblank[1] = 102  (flip 2's ev.sequence, for flip 1's CRC)
>    vblank[2] = 103  (flip 3's ev.sequence, for flip 2's CRC)
>    vblank[3] = 103 + 1 = 104  (last frame)
> 
> capture_crc checks (== exact match):
>    flip 0: expected=101, got=103 → 103 != 101 → FAIL
>    flip 1: expected=102, got=104 → 104 != 102 → FAIL
>    flip 2: expected=103, got=105 → 105 != 103 → FAIL
>    flip 3: expected=104, got=106 → 106 != 104 → FAIL
> 
> Library behavior:
>    igt_pipe_crc_get_for_frame(expected=104) → returns CRC with frame=106
>    Since 106 >= 104, the library considers this valid and returns it.
> 
> The CRC value is correct (it corresponds to the right framebuffer
> content), but the frame number is larger than expected due to the
> queue processing delay. This is an inherent characteristic of hardware
> that batches CRC reporting, and does not indicate data corruption or
> buffer overflow.
> 

that's all MediaTek specific behavior. It doesn't change the fact that 
this check you are trying to remove is what catches wrong-frame crcs for 
every other driver too.

> 
> Consistency with other IGT tests and library contract
> 
> Looking at how other tests handle igt_pipe_crc_get_for_frame():
> 1. kms_rotation_crc also calls igt_pipe_crc_get_for_frame() but does
> NOT validate that crc.frame == expected. It only compares CRC values
> between software and hardware rotated frames (igt_assert_crc_equal).
> 2. kms_pipe_crc_basic (read-crc-frame-sequence subtest) validates that
> consecutive CRCs have frame + 1 == next_frame (relative increment),
> but does NOT validate absolute match against an expected vblank count.
> 3. The library API igt_pipe_crc_get_for_frame() is explicitly designed
> with >= semantics:
> do {
>      read_one_crc(pipe_crc, crc);
> } while (igt_vblank_before(crc->frame, vblank));
> Its contract is "return the first CRC at or after the requested frame."
> No other caller in IGT adds a stricter == check on top of this.
> 
> The capture_crc() exact-match check in kms_plane is the only place in
> the entire IGT codebase that imposes a stricter requirement than the
> library's own API contract. This makes it incompatible with drivers
> that use queued CRC reporting, while the actual CRC validation
> (comparing pixel content) remains correct.
> 
> Given that the library's API contract is >=, and no other caller
> enforces ==, what would be the preferred approach here?

The "no other test does ==" argument doesn't apply here either. Other 
tests don't reconstruct per-frame crc/vblank mapping the way this test 
does, so they don't need it. This one does.

Disable the check for MediaTek specifically if you don't want to care 
about this check (e.g. is_mtk_device() like other mtk workarounds 
already in this file), don't remove a correctness check that other 
drivers rely on.

/Juha-Pekka