Re: [PATCH i-g-t] tests/kms_rotation_crc: Add MTK device support

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, May 27, 2026 at 05:04:36PM +0000, Jason-JH Lin (林睿祥) wrote:
> On Wed, 2026-05-27 at 15:52 +0300, Jani Nikula wrote:
> > On Tue, 26 May 2026, Manasi Navare <navaremanasi@google.com> wrote:
> > > Hi @Ville Syrjälä <ville.syrjala@linux.intel.com> @Karthik B S
> > > <karthik.b.s@intel.com>  @Kurmi, Suresh Kumar
> > > <suresh.kumar.kurmi@intel.com>
> > >  ,
> > > 
> > > Could we get some feedback on this upstream discussion, Ville has
> > > suggested
> > > to use the frame counter instead of adding an extra Vblank wait.
> > > However since MTK hardware does not have a support for the frame
> > > counter,
> > > this patch adds an extra vblank wait before grabbing CRC only for
> > > MTK hw.
> > > We need this change to get the test passing on MTK HW and would
> > > appreciate your feedback and Intel's help
> > > in getting this landed.
> > > 
> > > @Jason-JH Lin <jason-jh.lin@mediatek.com>  has provided the
> > > justification
> > > on why Ville's approach will not work for MTK HW.
> > 
> > Not sure why the driver couldn't do the vblank waits itself before
> > exposing the CRCs to the userspace. Maybe I'm missing something.
> > 
> > And that really makes it a non-upstream discussion, because the
> > upstream
> > Mediatek driver doesn't have a CRC implementation.
> > 
> > 
> > BR,
> > Jani.
> > 
> 
> Hi Jani,
> 
> Thank you for taking a look at this discussion.
> 
> You raise a valid point — having the driver handle things internally
> is indeed the cleaner approach. However, we would like to clarify
> why this is not straightforward for MTK hardware.
> 
> Unlike Intel, MTK hardware does not have a HW frame counter
> register, which means the driver cannot reliably associate each CRC
> entry with the correct frame number when calling
> drm_crtc_add_crc_entry(). A software frame counter would not help
> here either, because the MTK driver calls drm_crtc_add_crc_entry()
> on every Vblank, but the latency between atomic_commit() and the
> correct CRC becoming available is variable (1~2 vblanks) depending
> on asynchronous CMDQ execution timing.

Why is the CRC even being added from the vblank if the CMDQ
thing is the one that actually generates it? Sounds like you
may need to handle more stuff from the CMDQ so that both the
flips and CRCs know which order they happen in.

> 
> It is also worth noting that in MTK's DRM driver, the page flip
> completion event is fired in the CMDQ callback (i.e.
> drm_crtc_send_vblank_event()), which is only called when the HW
> configuration actually takes effect. However, this IGT test does not
> wait for the page flip event before capturing the CRC — it captures
> the CRC directly on every Vblank.

AFAICS the test either does a blocking commit or explicitly waits
for the page flip event before calling igt_pipe_crc_get_current().
If the CRC returned by that doesn't match the commit then the CRCs
clearly have bogus frame numbers.

I guess if you can't fix the CRC frame numbers to be accurate then
maybe just stop telling drm_crtc_add_crc_entry() that you even have
any frame numbers. That should make igt_pipe_crc_get_current() throw
out all captured CRCs and wait for the next one to come in. Perhaps
that is enough to get you through this.

> 
> As illustrated below, even if WaitPageFlip() were used, the correct 
> CRC for MTK would still only be available one additional Vblank after
> the page flip event is received:
> 
> atomic_commit()
>     |
>     v
>   EOF N -> CMDQ callback
>         -> HW config takes effect
>         -> drm_crtc_send_vblank_event() -> page flip event fired
> 
>   Vblank N -> drm_crtc_handle_vblank() -> vblank counter++
>            -> CRC(old) reported via drm_crtc_add_crc_entry()
>              (CRC captured before EOF N, reflects old config)
> 
>   EOF N+1 -> CRC(new) captured by HW
> 
>   Vblank N+1 -> drm_crtc_handle_vblank() -> vblank counter++
>              -> CRC(new) reported via drm_crtc_add_crc_entry() ✅
> 
> Note that if atomic_commit() is issued after EOF N has already
> passed, the CMDQ callback will not take effect until EOF N+2,
> meaning the correct CRC would not be available until VBlank N+2.
> This is why the latency to the correct CRC is variable (1~2
> vblanks). However, by waiting one extra vblank at the userspace
> level after atomic_commit(), this is effectively equivalent to
> waiting for the page flip event (WaitPageFlip()) before capturing
> the CRC, which ensures the correct CRC is always captured regardless
> of when atomic_commit() was issued.
> 
> 
> Regarding the upstream CRC implementation, we did submit a patch
> previously:
> https://patchwork.kernel.org/project/linux-mediatek/patch/20240614024620.19011-20-shawn.sung@mediatek.com/
> 
> 
> However, it has not been rebased and resubmitted due to internal
> scheduling constraints. Additionally, the MTK DRM driver is currently
> undergoing significant expansion and refactoring to support the
> latest generation hardware, which has further delayed the
> prioritization of the CRC upstream patch.
> 
> In the short term, we are relying on the downstream CRC
> implementation for IGT validation. We do intend to upstream the
> complete CRC implementation once the ongoing driver refactoring
> stabilizes.
> 
> In the meantime, we would appreciate any guidance on the preferred
> approach for handling the CRC reporting delay within the driver, so
> we can make sure the eventual upstream submission aligns with the
> expected design.
> 
> Thanks again for your input.
> 
> Best regards,
> Jason-JH Lin

-- 
Ville Syrjälä
Intel