Re: [Intel-gfx] [PATCH 1/6] drm/i915: Correctly populate use_sagv_wm for all pipes

["Lisovskiy, Stanislav" <stanislav.lisovskiy@intel.com>]

On Mon, Feb 14, 2022 at 11:18:06AM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When changing between SAGV vs. no SAGV on tgl+ we have to
> update the use_sagv_wm flag for all the crtcs or else
> an active pipe not already in the state will end up using
> the wrong watermarks. That is especially bad when we end up
> with the tighter non-SAGV watermarks with SAGV enabled.
> Usually ends up in underruns.

Probably valid point. Just noticed that we have this constant 
confusion, between cases when we have to update only crtc
which are added to the state(i.e only those which changed)
versus cases when everything has to be updated, regardless 
if its in the state or not.

I think it didn't ever caused underruns however, which is
strange - currently we mostly hit underruns once due to 
some PSR magic. Might be we just are lucky enough to get
all crtcs added to the state for some other reasons.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

Stan

> 
> Cc: stable@vger.kernel.org
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Fixes: 7241c57d3140 ("drm/i915: Add TGL+ SAGV support")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 1179bf31f743..d8eb553ffad3 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4009,6 +4009,17 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			return ret;
>  	}
>  
> +	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> +	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> +		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> +		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	for_each_new_intel_crtc_in_state(state, crtc,
>  					 new_crtc_state, i) {
>  		struct skl_pipe_wm *pipe_wm = &new_crtc_state->wm.skl.optimal;
> @@ -4024,17 +4035,6 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
>  			intel_can_enable_sagv(dev_priv, new_bw_state);
>  	}
>  
> -	if (intel_can_enable_sagv(dev_priv, new_bw_state) !=
> -	    intel_can_enable_sagv(dev_priv, old_bw_state)) {
> -		ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	} else if (new_bw_state->pipe_sagv_reject != old_bw_state->pipe_sagv_reject) {
> -		ret = intel_atomic_lock_global_state(&new_bw_state->base);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	return 0;
>  }
>  
> -- 
> 2.34.1
>