Re: [PATCH 2/4] drm/i915/psr: Perform vblank evasion on async flip as well for PSR

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Thu, 2025-11-27 at 12:57 +0200, Hogander, Jouni wrote:
> On Tue, 2025-11-25 at 23:19 +0200, Ville Syrjälä wrote:
> > On Tue, Nov 25, 2025 at 08:32:51AM +0200, Jouni Högander wrote:
> > > PSR2_MAN_TRK_CTL[SF Continuous full frame] is sampled on the
> > > rising
> > > edge of
> > > delayed vblank. SW must ensure this bit is not changing around
> > > that. Due to
> > > this PSR2 Selective Fetch needs vblank evasion.
> > > 
> > > Currently vblank evasion is not done on async flip. Perform it in
> > > case
> > > required by PSR.
> > > 
> > > Bspec: 50424
> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 153ff4b4b52c..42c4ce07f8c0 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -433,7 +433,8 @@ static bool
> > > intel_crtc_needs_vblank_work(const
> > > struct intel_crtc_state *crtc_sta
> > >  		(intel_crtc_needs_color_update(crtc_state) &&
> > >  		 !HAS_DOUBLE_BUFFERED_LUT(display)) &&
> > >  		!intel_color_uses_dsb(crtc_state) &&
> > > -		!crtc_state->use_dsb;
> > > +		!crtc_state->use_dsb &&
> > > +		!crtc_state->do_async_flip;
> > >  }
> > >  
> > >  static void intel_crtc_vblank_work(struct kthread_work *base)
> > > @@ -539,7 +540,8 @@ void intel_pipe_update_start(struct
> > > intel_atomic_state *state,
> > >  	if (new_crtc_state->do_async_flip) {
> > >  		intel_crtc_prepare_vblank_event(new_crtc_state,
> > >  						&crtc-
> > > > flip_done_event);
> > > -		return;
> > > +		if (!intel_psr_needs_evasion(new_crtc_state))
> > > +			return;
> > 
> > I don't think we want hack this into such low level code. We
> > anyway convert the first async flip to a sync flip (see
> > intel_plane_do_async_flip()), so that's when you should disable
> > selective fetch, and keep it disabled afterwards as long as
> > async flips are being requested for the plane by userspace.
> 
> Isn't async flip always initiated by user space (uapi.async_flip ==
> 1)?
> Are you concerned on this sequence:
> 
> 1. async flip on primary plane (full frame update)
> 2. normal flip on secondary plane (selective fetch/update)
> 3. async flip on primary plane (full frame update)
> 
> Is there some problem in performing selective fetch/update on step 2?
> Please note that we are not disabling PSR2 at step 2. 
> We are just performing 1 selective fetch/update in between there.

I should have said "we are not disabling PSR2 at step 1 and 3. We are
just performing full frame updates there and one selective fetch/update
in between at step2."

BR,

Jouni Högander

> 
> BR,
> 
> Jouni Högander
> 
> > 
> > The problem is that uapi.async_flip is ephemeral, so you can't
> > just check for that. I think what we need is a way to track
> > which planes have been requested to do async flips. We almost 
> > have that with the async_flip_planes bitmask, and I think we
> > can make it do exactly what we want by just dropping the
> > need_async_flip_toggle_wa check from 
> > intel_plane_atomic_calc_changes(). That should be safe since
> > all places that currently use the bitmask also check for
> > need_async_flip_toggle_wa.
> > 
> > The alternative would be to track the uapi async flip requests
> > in a separate bitmask. That might be a bit more optimal in that
> > we wouldn't clear the bit from there when some other plane
> > or the pipe itself needs a sync update while the plane is already
> > performing async flips. But not having that just means you'll
> > end up toggling selective fetch back on and the off again when
> > a sync update intervenes a stream of async flips.
> > 
> > Oh, and needs_async_flip_vtd_wa() should probably also use
> > the bitmask rather than looking at uapi.async_flip.
> > 
> > >  	}
> > >  
> > >  	if (intel_crtc_needs_vblank_work(new_crtc_state))
> > > -- 
> > > 2.43.0
> > 
>