Re: [PATCH v3 2/3] drm/i915/psr: Perform full frame update on async flip

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

On Wed, 2025-12-03 at 17:08 +0200, Ville Syrjälä wrote:
> On Wed, Dec 03, 2025 at 01:58:23PM +0000, Hogander, Jouni wrote:
> > On Wed, 2025-12-03 at 15:22 +0200, Ville Syrjälä wrote:
> > > On Mon, Dec 01, 2025 at 03:24:56PM +0200, Jouni Högander wrote:
> > > > According to bspec selective fetch is not supported with async
> > > > flips and
> > > > instructing full frame update on async flip.
> > > > 
> > > > v3:
> > > >   - rebase
> > > >   - fix old_crtc_state->pipe_srcsz_early_tpt
> > > >   - fix using intel_atomic_get_new_crtc_state
> > > > v2:
> > > >   - check also crtc_state->async_flip_planes in
> > > >     psr2_sel_fetch_plane_state_supported
> > > > 
> > > > Bspec: 55229
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/display/intel_psr.c | 72 ++++++++++++++--
> > > > ----
> > > > ----
> > > >  1 file changed, 41 insertions(+), 31 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > index 15ef3b6caad6..53cf292247d7 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> > > > @@ -2728,13 +2728,20 @@
> > > > intel_psr2_sel_fetch_et_alignment(struct
> > > > intel_atomic_state *state,
> > > >   * Plane scaling and rotation is not supported by selective
> > > > fetch
> > > > and both
> > > >   * properties can change without a modeset, so need to be
> > > > check at
> > > > every
> > > >   * atomic commit.
> > > > + *
> > > > + * If plane was having async flip previously we can't use
> > > > selective
> > > > + * fetch as we don't know if the flip is completed.
> > > >   */
> > > > -static bool psr2_sel_fetch_plane_state_supported(const struct
> > > > intel_plane_state *plane_state)
> > > > +static bool psr2_sel_fetch_plane_state_supported(const struct
> > > > intel_crtc_state *old_crtc_state,
> > > > +						 const struct
> > > > intel_plane_state *plane_state)
> > > >  {
> > > > +	struct intel_plane *plane =
> > > > to_intel_plane(plane_state-
> > > > > uapi.plane);
> > > > +
> > > >  	if (plane_state->uapi.dst.y1 < 0 ||
> > > >  	    plane_state->uapi.dst.x1 < 0 ||
> > > >  	    plane_state->scaler_id >= 0 ||
> > > > -	    plane_state->hw.rotation != DRM_MODE_ROTATE_0)
> > > > +	    plane_state->hw.rotation != DRM_MODE_ROTATE_0 ||
> > > > +	    old_crtc_state->async_flip_planes & plane->id)
> > > 
> > > Why are you looking at the old crtc state? There should be
> > > nothing of
> > > interest to us there.
> > 
> > To continue keeping CFF bit set if previous update was async flip.
> > This
> > is how I understood it (please correct):
> > 
> > 0. syncronous update
> > 
> > 1. async_flip:
> >    new_crtc_state->async_flip_planes != 0
> >    old_crtc_state->async_flip_planes == 0
> >    crtc_state->uapi.async_flip == true)
> >    -> full frame update
> > 
> > 2. async_flip:
> >    new_crtc_state->async_flip_planes != 0
> >    old_crtc_state->async_flip_planes != 0
> >    crtc_state->uapi.async_flip == true
> >    -> full frame update
> > 
> > 3. syncronous update
> >    new_crtc_state->async_flip_planes == 0
> >    old_crtc_state->async_flip_planes != 0
> >    crtc_state->uapi.async_flip == false
> >    -> full frame update
> > 
> > 4. syncronous update
> >    new_crtc_state->async_flip_planes == 0
> >    old_crtc_state->async_flip_planes == 0
> >    crtc_state->uapi.async_flip == false
> >    -> selective update
> > 
> > > 
> > > >  		return false;
> > > >  
> > > >  	return true;
> > > > @@ -2749,7 +2756,8 @@ static bool
> > > > psr2_sel_fetch_plane_state_supported(const struct
> > > > intel_plane_state
> > > >   */
> > > >  static bool psr2_sel_fetch_pipe_state_supported(const struct
> > > > intel_crtc_state *crtc_state)
> > > >  {
> > > > -	if (crtc_state->scaler_state.scaler_id >= 0)
> > > > +	if (crtc_state->scaler_state.scaler_id >= 0 ||
> > > > +	    crtc_state->uapi.async_flip)
> > > 
> > > I think just checking crtc_state->async_flip_planes!=0 here
> > > should be
> > > sufficient.
> > 
> > I'm doing this to handle step 1. above. Alternatively I could check
> > both new_crtc_state->async_flip_planes and old_crtc_state-
> > > async_flip_planes. When using crtc_state->uapi.async_flip I can
> > > decide
> > earlier.
> > 
> > > The rest of the patch seems unnecessary.
> > 
> > No need to handle selective update where planes having pending
> > async
> > flip are not involved?
> 
> There won't be pending flips of any kind. We whole CRTC commit queue
> is handled in a strict FIFO order (apart from the legacy cursor
> special case).
> 

Ok, so it's not possible to take in new CRTC commit before async flip
is completed?

BR,

Jouni Högander