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

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.


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?

Thanks,
Jason-JH Lin