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

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

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.

>  		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. The rest of the patch seems unnecessary.

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

-- 
Ville Syrjälä
Intel