Re: [PATCH v2 4/4] drm/i915/psr: Add poll for checking PSR is idle before starting update

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

On Wed, Aug 27, 2025 at 01:22:28PM +0000, Hogander, Jouni wrote:
> On Wed, 2025-08-27 at 16:06 +0300, Ville Syrjälä wrote:
> > On Tue, Aug 26, 2025 at 02:30:50PM +0000, Hogander, Jouni wrote:
> > > On Tue, 2025-08-26 at 15:36 +0300, Ville Syrjälä wrote:
> > > > On Wed, Aug 06, 2025 at 08:22:13AM +0300, Jouni Högander wrote:
> > > > > We are currently observing crc failures after we started using
> > > > > dsb
> > > > > for PSR
> > > > > updates as well. This seems to happen because PSR HW is still
> > > > > sending
> > > > > couple of updates using old framebuffers on wake-up.
> > > > > 
> > > > > Fix this by adding poll ensuring PSR is idle before starting
> > > > > update.
> > > > > 
> > > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > > ---
> > > > >  drivers/gpu/drm/i915/display/intel_display.c | 2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > index c1a3a95c65f0..411c74c73eae 100644
> > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > > > @@ -7271,6 +7271,8 @@ static void
> > > > > intel_atomic_dsb_finish(struct
> > > > > intel_atomic_state *state,
> > > > >  		intel_psr_trigger_frame_change_event(new_crtc_
> > > > > stat
> > > > > e->dsb_commit,
> > > > >  						     state,
> > > > > crtc);
> > > > >  
> > > > > +		intel_psr_wait_for_idle_dsb(new_crtc_state);
> > > > > +
> > > > >  		if (new_crtc_state->use_dsb)
> > > > >  			intel_dsb_vblank_evade(state,
> > > > > new_crtc_state->dsb_commit);
> > > > 
> > > > How come the 'evade scanline 0 + normal evasion' in
> > > > intel_dsb_vblank_evade()
> > > > is not enough here?
> > > 
> > > the problem doesn't happen when PSR is in SRD_ENT/DEEP_SLEEP as
> > > committing new changes is started. It seems to happen when PSR
> > > transitioning into SRD_ENT/DEEP_SLEEP is ongoing when new changes
> > > are
> > > being committed. In this case evasion passes and PSR is continuing
> > > transitioning into SRD_ENT/DEEP_SLEEP. Then wake-up starts
> > > immediately
> > > due to pending "Frame Change" event and in this case HW is sending
> > > a
> > > frame using old plane configuration.
> > 
> > That's a bit weird. I think we are configuring things so that there
> > should be that extra vblank already for the first frame after PSR
> > exit. So I would expect things to latch properly even if we write
> > the registers during the PSR entry sequence.
> > 
> > Hmm, though the DSB itself never waits for the delayed vblank
> > directly. Maybe the delayed vblank does get suppressed for
> > one frame during the sequence somewhere, but the undelayed
> > vblank used by the DSB does not.
> > 
> > One could perhaps try to sample a timestamp from the DSB after
> > it thinks the vblank has happened, and sample another one from
> > the CPU delayed vblank interrupt (which I would expect to match
> > when the hardware really latches stuff), and compare how they
> > look. Though that does require one to enable the CPU interrupt
> > which itself will trigger the PSR exit (at least on some hw),
> > so not sure how easy it is to hit the exact conditions required.
> > I suppose one might try to wait for the PSR state machine to
> > be in the right state just before triggering the exit.
> 
> Enabling the interrupt will trigger exit. Are you thinking that state
> machine wait as a solution or as a experiment? :

Just as an experiment to make sure we understand what's going on.

I suppose if my theory about the undelayed vblank holds there should
also be no way to hit this when not using the DSB since the mmio path
always completes the flips based on the delayed vblank. But I suppose
that's harder to use as any kind of proof since it'll be much harder
to hit the exact time window with the CPU anyway.

-- 
Ville Syrjälä
Intel