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

On Fri, 2026-08-14 at 13:57 +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 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.
> 

I understand your point that the strict == check is crucial for other
drivers to correctly detect wrong-frame CRCs, and removing it entirely
would break that correctness verification for them.

> > 
> > 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

I agree with your suggestion to make this a MediaTek-specific
skip. We will update the patch to use is_mtk_device()
and only skip the check for MediaTek devices. This preserves the
strict == correctness check for other drivers while allowing our
queued CRC reporting mechanism to pass on MTK hardware.

Thanks,
Jason-JH Lin