Re: [Intel-gfx] [PATCH v2 2/2] drm/i915/psr: Add proper handling for disabling sel fetch for planes

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

On Fri, Nov 17, 2023 at 12:02:27PM +0200, Jouni Högander wrote:
> Currently we are enabling selective fetch for all planes that are visible.
> This is suboptimal as we might be fetching for memory for planes that are
> not part of selective update.
> 
> Fix this by adding proper handling for disabling plane selective fetch:
> If plane previously part of selective update is now not part of update:
> Add it into updated planes and let the plane configuration to disable
> selective fetch for it.
> 
> v2:
>   - Add setting sel_fetch_area->y1/y2 to -1
>   - Remove setting again local sel_fetch_area variable
> 
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_cursor.c   | 22 +++++++++++--------
>  drivers/gpu/drm/i915/display/intel_psr.c      | 13 ++++++++++-
>  .../drm/i915/display/skl_universal_plane.c    |  8 +++++--
>  3 files changed, 31 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index c089dd6f9781..299d22708fa4 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -485,22 +485,22 @@ static int i9xx_check_cursor(struct intel_crtc_state *crtc_state,
>  	return 0;
>  }
>  
> -static void i9xx_cursor_update_sel_fetch_arm(struct intel_plane *plane,
> -					     const struct intel_crtc_state *crtc_state,
> -					     const struct intel_plane_state *plane_state)
> +static void i9xx_cursor_disable_sel_fetch_arm(struct intel_plane *plane,
> +					    const struct intel_crtc_state *crtc_state)
>  {
> -	struct drm_i915_private *i915 = to_i915(plane->base.dev);
> +	struct drm_i915_private *dev_priv = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
>  
>  	if (!crtc_state->enable_psr2_sel_fetch)
>  		return;
>  
> -	intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> -			  plane_state->ctl);
> +
> +	intel_de_write_fw(dev_priv, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
>  }
>  
> -static void i9xx_cursor_disable_sel_fetch_arm(struct intel_plane *plane,
> -					    const struct intel_crtc_state *crtc_state)
> +static void i9xx_cursor_update_sel_fetch_arm(struct intel_plane *plane,
> +					     const struct intel_crtc_state *crtc_state,
> +					     const struct intel_plane_state *plane_state)
>  {
>  	struct drm_i915_private *i915 = to_i915(plane->base.dev);
>  	enum pipe pipe = plane->pipe;
> @@ -508,7 +508,11 @@ static void i9xx_cursor_disable_sel_fetch_arm(struct intel_plane *plane,
>  	if (!crtc_state->enable_psr2_sel_fetch)
>  		return;
>  
> -	intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id), 0);
> +	if (drm_rect_height(&plane_state->psr2_sel_fetch_area) > 0)

drm_rect_visible() is less magic.

> +		intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +				  plane_state->ctl);

Does this even have anything besides the enable bit?

> +	else
> +		i9xx_cursor_disable_sel_fetch_arm(plane, crtc_state);
>  }
>  
>  /* TODO: split into noarm+arm pair */
> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 87eb1535ba98..239365c666e2 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -2173,8 +2173,19 @@ int intel_psr2_sel_fetch_update(struct intel_atomic_state *state,
>  			continue;
>  
>  		inter = pipe_clip;
> -		if (!drm_rect_intersect(&inter, &new_plane_state->uapi.dst))
> +		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;
> +			sel_fetch_area->y2 = -1;
> +			/*
> +			 * if plane sel fetch was previously enabled ->
> +			 * disable it
> +			 */
> +			if (drm_rect_height(&old_plane_state->psr2_sel_fetch_area) > 0)
> +				crtc_state->update_planes |= BIT(plane->id);
> +
>  			continue;
> +		}

I tried to look at this code, but it just looks entirely confused
about things.

I had a quick stab at rewriting it all:
 https://github.com/vsyrjala/linux.git sel_fetch_redo_2
but I don't have a machine to test it, so can't guarantee that it's 100%
correct.

>  
>  		if (!psr2_sel_fetch_plane_state_supported(new_plane_state)) {
>  			full_update = true;
> diff --git a/drivers/gpu/drm/i915/display/skl_universal_plane.c b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> index 99d33ac5ceee..a969bb835baf 100644
> --- a/drivers/gpu/drm/i915/display/skl_universal_plane.c
> +++ b/drivers/gpu/drm/i915/display/skl_universal_plane.c
> @@ -1336,8 +1336,12 @@ static void icl_plane_update_sel_fetch_arm(struct intel_plane *plane,
>  	if (!crtc_state->enable_psr2_sel_fetch)
>  		return;
>  
> -	intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> -			  PLANE_SEL_FETCH_CTL_ENABLE);
> +
> +	if (drm_rect_height(&plane_state->psr2_sel_fetch_area) > 0)
> +		intel_de_write_fw(i915, PLANE_SEL_FETCH_CTL(pipe, plane->id),
> +				  PLANE_SEL_FETCH_CTL_ENABLE);
> +	else
> +		icl_plane_disable_sel_fetch_arm(plane, crtc_state);
>  }
>  
>  static void
> -- 
> 2.34.1

-- 
Ville Syrjälä
Intel