[Intel-gfx] [PATCH 2/6] drm/i915: Tighten SAGV constraint for pre-tgl

[Lisovskiy, Stanislav]

On Fri, Mar 05, 2021 at 05:36:06PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> Say we have two planes enabled with watermarks configured
> as follows:
> plane A: wm0=enabled/can_sagv=false, wm1=enabled/can_sagv=true
> plane B: wm0=enabled/can_sagv=true,  wm1=disabled

Was thinking about this, always thought its not possible, i.e
wm1 kinda requires more resources, so if we can do wm1, should
always be able to do wm0..

> 
> This is possible since the latency we use to calculate
> can_sagv may not be the same for both planes due to
> skl_needs_memory_bw_wa().

The current code, which I see in internal at least looks like this:

/*
 * FIXME: We still don't have the proper code detect if we need to apply the WA,
 * so assume we'll always need it in order to avoid underruns.
 */
static bool skl_needs_memory_bw_wa(struct drm_i915_private *dev_priv)
{
      return IS_GEN9_BC(dev_priv) || IS_BROXTON(dev_priv);
}

i.e I think it will return same latency for all planes.

Or am I missing something?..


Stan

> 
> In this case skl_crtc_can_enable_sagv() will see that
> both planes have enabled at least one watermark level
> with can_sagv==true, and thus proceeds to allow SAGV.
> However, since plane B does not have wm1 enabled
> plane A can't actually use it either. Thus we are
> now running with SAGV enabled, but plane A can't
> actually tolerate the extra latency it imposes.
> 
> To remedy this only allow SAGV on if the highest common
> enabled watermark level for all active planes can tolerate
> the extra SAGV latency.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 854ffecd98d9..b6e34d1701a0 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3876,6 +3876,7 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
>  	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
>  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>  	enum plane_id plane_id;
> +	int max_level = INT_MAX;
>  
>  	if (!intel_has_sagv(dev_priv))
>  		return false;
> @@ -3900,12 +3901,23 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
>  		     !wm->wm[level].plane_en; --level)
>  		     { }
>  
> +		/* Highest common enabled wm level for all planes */
> +		max_level = min(level, max_level);
> +	}
> +
> +	/* No enabled planes? */
> +	if (max_level == INT_MAX)
> +		return true;
> +
> +	for_each_plane_id_on_crtc(crtc, plane_id) {
> +		const struct skl_plane_wm *wm =
> +			&crtc_state->wm.skl.optimal.planes[plane_id];
> +
>  		/*
> -		 * If any of the planes on this pipe don't enable wm levels that
> -		 * incur memory latencies higher than sagv_block_time_us we
> -		 * can't enable SAGV.
> +		 * All enabled planes must have enabled a common wm level that
> +		 * can tolerate memory latencies higher than sagv_block_time_us
>  		 */
> -		if (!wm->wm[level].can_sagv)
> +		if (wm->wm[0].plane_en && !wm->wm[max_level].can_sagv)
>  			return false;
>  	}
>  
> -- 
> 2.26.2
>