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

BR,
Jouni Högander

> 
> On a related note, someone should add a new igt that does async flips
> while eg. the cursor is enabled and overlapping the plane doing the
> async flips. That's basically how I noticed the problem in the first
> place (with Xorg), so would be good to have an igt to make sure we
> don't break this in the future.
> 
> >  		return false;
> >  
> >  	return true;
> > @@ -2808,24 +2816,25 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  				struct intel_crtc *crtc)
> >  {
> >  	struct intel_display *display = to_intel_display(state);
> > -	struct intel_crtc_state *crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc_state *new_crtc_state =
> > intel_atomic_get_new_crtc_state(state, crtc);
> > +	struct intel_crtc_state *old_crtc_state =
> > intel_atomic_get_old_crtc_state(state, crtc);
> >  	struct intel_plane_state *new_plane_state,
> > *old_plane_state;
> >  	struct intel_plane *plane;
> >  	bool full_update = false, cursor_in_su_area = false;
> >  	int i, ret;
> >  
> > -	if (!crtc_state->enable_psr2_sel_fetch)
> > +	if (!new_crtc_state->enable_psr2_sel_fetch)
> >  		return 0;
> >  
> > -	if (!psr2_sel_fetch_pipe_state_supported(crtc_state)) {
> > +	if (!psr2_sel_fetch_pipe_state_supported(new_crtc_state))
> > {
> >  		full_update = true;
> >  		goto skip_sel_fetch_set_loop;
> >  	}
> >  
> > -	crtc_state->psr2_su_area.x1 = 0;
> > -	crtc_state->psr2_su_area.y1 = -1;
> > -	crtc_state->psr2_su_area.x2 = drm_rect_width(&crtc_state-
> > >pipe_src);
> > -	crtc_state->psr2_su_area.y2 = -1;
> > +	new_crtc_state->psr2_su_area.x1 = 0;
> > +	new_crtc_state->psr2_su_area.y1 = -1;
> > +	new_crtc_state->psr2_su_area.x2 =
> > drm_rect_width(&new_crtc_state->pipe_src);
> > +	new_crtc_state->psr2_su_area.y2 = -1;
> >  
> >  	/*
> >  	 * Calculate minimal selective fetch area of each plane
> > and calculate
> > @@ -2838,14 +2847,14 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		struct drm_rect src, damaged_area = { .x1 = 0, .y1
> > = -1,
> >  						      .x2 =
> > INT_MAX };
> >  
> > -		if (new_plane_state->hw.crtc != crtc_state-
> > >uapi.crtc)
> > +		if (new_plane_state->hw.crtc != new_crtc_state-
> > >uapi.crtc)
> >  			continue;
> >  
> >  		if (!new_plane_state->uapi.visible &&
> >  		    !old_plane_state->uapi.visible)
> >  			continue;
> >  
> > -		if
> > (!psr2_sel_fetch_plane_state_supported(new_plane_state)) {
> > +		if
> > (!psr2_sel_fetch_plane_state_supported(old_crtc_state,
> > new_plane_state)) {
> >  			full_update = true;
> >  			break;
> >  		}
> > @@ -2861,23 +2870,23 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  			if (old_plane_state->uapi.visible) {
> >  				damaged_area.y1 = old_plane_state-
> > >uapi.dst.y1;
> >  				damaged_area.y2 = old_plane_state-
> > >uapi.dst.y2;
> > -				clip_area_update(&crtc_state-
> > >psr2_su_area, &damaged_area,
> > -						 &crtc_state-
> > >pipe_src);
> > +				clip_area_update(&new_crtc_state-
> > >psr2_su_area, &damaged_area,
> > +						 &new_crtc_state-
> > >pipe_src);
> >  			}
> >  
> >  			if (new_plane_state->uapi.visible) {
> >  				damaged_area.y1 = new_plane_state-
> > >uapi.dst.y1;
> >  				damaged_area.y2 = new_plane_state-
> > >uapi.dst.y2;
> > -				clip_area_update(&crtc_state-
> > >psr2_su_area, &damaged_area,
> > -						 &crtc_state-
> > >pipe_src);
> > +				clip_area_update(&new_crtc_state-
> > >psr2_su_area, &damaged_area,
> > +						 &new_crtc_state-
> > >pipe_src);
> >  			}
> >  			continue;
> >  		} else if (new_plane_state->uapi.alpha !=
> > old_plane_state->uapi.alpha) {
> >  			/* If alpha changed mark the whole plane
> > area as damaged */
> >  			damaged_area.y1 = new_plane_state-
> > >uapi.dst.y1;
> >  			damaged_area.y2 = new_plane_state-
> > >uapi.dst.y2;
> > -			clip_area_update(&crtc_state-
> > >psr2_su_area, &damaged_area,
> > -					 &crtc_state->pipe_src);
> > +			clip_area_update(&new_crtc_state-
> > >psr2_su_area, &damaged_area,
> > +					 &new_crtc_state-
> > >pipe_src);
> >  			continue;
> >  		}
> >  
> > @@ -2893,7 +2902,8 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		damaged_area.x1 += new_plane_state->uapi.dst.x1 -
> > src.x1;
> >  		damaged_area.x2 += new_plane_state->uapi.dst.x1 -
> > src.x1;
> >  
> > -		clip_area_update(&crtc_state->psr2_su_area,
> > &damaged_area, &crtc_state->pipe_src);
> > +		clip_area_update(&new_crtc_state->psr2_su_area,
> > &damaged_area,
> > +				 &new_crtc_state->pipe_src);
> >  	}
> >  
> >  	/*
> > @@ -2902,7 +2912,7 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	 * should identify cases where this happens and fix the
> > area
> >  	 * calculation for those.
> >  	 */
> > -	if (crtc_state->psr2_su_area.y1 == -1) {
> > +	if (new_crtc_state->psr2_su_area.y1 == -1) {
> >  		drm_info_once(display->drm,
> >  			      "Selective fetch area calculation
> > failed in pipe %c\n",
> >  			      pipe_name(crtc->pipe));
> > @@ -2912,7 +2922,7 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	if (full_update)
> >  		goto skip_sel_fetch_set_loop;
> >  
> > -	intel_psr_apply_su_area_workarounds(crtc_state);
> > +	intel_psr_apply_su_area_workarounds(new_crtc_state);
> >  
> >  	ret = drm_atomic_add_affected_planes(&state->base, &crtc-
> > >base);
> >  	if (ret)
> > @@ -2926,7 +2936,7 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  	 */
> >  	intel_psr2_sel_fetch_et_alignment(state, crtc,
> > &cursor_in_su_area);
> >  
> > -	intel_psr2_sel_fetch_pipe_alignment(crtc_state);
> > +	intel_psr2_sel_fetch_pipe_alignment(new_crtc_state);
> >  
> >  	/*
> >  	 * Now that we have the pipe damaged area check if it
> > intersect with
> > @@ -2937,11 +2947,11 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		struct drm_rect *sel_fetch_area, inter;
> >  		struct intel_plane *linked = new_plane_state-
> > >planar_linked_plane;
> >  
> > -		if (new_plane_state->hw.crtc != crtc_state-
> > >uapi.crtc ||
> > +		if (new_plane_state->hw.crtc != new_crtc_state-
> > >uapi.crtc ||
> >  		    !new_plane_state->uapi.visible)
> >  			continue;
> >  
> > -		inter = crtc_state->psr2_su_area;
> > +		inter = new_crtc_state->psr2_su_area;
> >  		sel_fetch_area = &new_plane_state-
> > >psr2_sel_fetch_area;
> >  		if (!drm_rect_intersect(&inter, &new_plane_state-
> > >uapi.dst)) {
> >  			sel_fetch_area->y1 = -1;
> > @@ -2951,12 +2961,12 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  			 * disable it
> >  			 */
> >  			if (drm_rect_height(&old_plane_state-
> > >psr2_sel_fetch_area) > 0)
> > -				crtc_state->update_planes |=
> > BIT(plane->id);
> > +				new_crtc_state->update_planes |=
> > BIT(plane->id);
> >  
> >  			continue;
> >  		}
> >  
> > -		if
> > (!psr2_sel_fetch_plane_state_supported(new_plane_state)) {
> > +		if
> > (!psr2_sel_fetch_plane_state_supported(old_crtc_state,
> > new_plane_state)) {
> >  			full_update = true;
> >  			break;
> >  		}
> > @@ -2964,7 +2974,7 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  		sel_fetch_area = &new_plane_state-
> > >psr2_sel_fetch_area;
> >  		sel_fetch_area->y1 = inter.y1 - new_plane_state-
> > >uapi.dst.y1;
> >  		sel_fetch_area->y2 = inter.y2 - new_plane_state-
> > >uapi.dst.y1;
> > -		crtc_state->update_planes |= BIT(plane->id);
> > +		new_crtc_state->update_planes |= BIT(plane->id);
> >  
> >  		/*
> >  		 * Sel_fetch_area is calculated for UV plane. Use
> > @@ -2981,14 +2991,14 @@ int intel_psr2_sel_fetch_update(struct
> > intel_atomic_state *state,
> >  			linked_sel_fetch_area =
> > &linked_new_plane_state->psr2_sel_fetch_area;
> >  			linked_sel_fetch_area->y1 =
> > sel_fetch_area->y1;
> >  			linked_sel_fetch_area->y2 =
> > sel_fetch_area->y2;
> > -			crtc_state->update_planes |= BIT(linked-
> > >id);
> > +			new_crtc_state->update_planes |=
> > BIT(linked->id);
> >  		}
> >  	}
> >  
> >  skip_sel_fetch_set_loop:
> > -	psr2_man_trk_ctl_calc(crtc_state, full_update);
> > -	crtc_state->pipe_srcsz_early_tpt =
> > -		psr2_pipe_srcsz_early_tpt_calc(crtc_state,
> > full_update);
> > +	psr2_man_trk_ctl_calc(new_crtc_state, full_update);
> > +	new_crtc_state->pipe_srcsz_early_tpt =
> > +		psr2_pipe_srcsz_early_tpt_calc(new_crtc_state,
> > full_update);
> >  	return 0;
> >  }
> >  
> > -- 
> > 2.43.0
>