Re: [PATCH 3/3] drm/i915/tgl: Remove single pipe restriction from SAGV

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

On Wed, Sep 25, 2019 at 01:33:52PM -0700, James Ausmus wrote:
> For Gen12, BSpec no longer tells us to disable SAGV when > 1 pipe is
> active. Update intel_can_enable_sagv to allow this, and loop through all
> active planes on all active crtcs to check against the interlaced and
> latency restrictions.
> 
> BSpec: 49325
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: James Ausmus <james.ausmus@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 63 +++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index ca2bec09edb5..cb50c697a6b8 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3775,7 +3775,6 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  	struct intel_crtc *crtc;
>  	struct intel_plane *plane;
>  	struct intel_crtc_state *crtc_state;
> -	enum pipe pipe;
>  	int level, latency;
>  	int sagv_block_time_us;
>  
> @@ -3791,47 +3790,49 @@ bool intel_can_enable_sagv(struct intel_atomic_state *state)
>  		return true;
>  
>  	/*
> -	 * SKL+ workaround: bspec recommends we disable SAGV when we have
> +	 * SKL-ICL workaround: bspec recommends we disable SAGV when we have
>  	 * more then one pipe enabled
>  	 */
> -	if (hweight8(state->active_pipes) > 1)
> +	if (INTEL_GEN(dev_priv) < 12 && hweight8(state->active_pipes) > 1)
>  		return false;
>  
> -	/* Since we're now guaranteed to only have one active CRTC... */
> -	pipe = ffs(state->active_pipes) - 1;
> -	crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> -	crtc_state = to_intel_crtc_state(crtc->base.state);
> +	for_each_intel_crtc(&dev_priv->drm, crtc) {
> +		crtc_state = to_intel_crtc_state(crtc->base.state);
> +		if (!crtc_state->base.active)
> +			continue;
>  
> -	if (crtc->base.state->adjusted_mode.flags & DRM_MODE_FLAG_INTERLACE)
> -		return false;
> +		if (crtc->base.state->adjusted_mode.flags &
> +		    DRM_MODE_FLAG_INTERLACE)
> +			return false;
>  
> -	for_each_intel_plane_on_crtc(dev, crtc, plane) {
> -		struct skl_plane_wm *wm =
> -			&crtc_state->wm.skl.optimal.planes[plane->id];
> +		for_each_intel_plane_on_crtc(dev, crtc, plane) {
> +			struct skl_plane_wm *wm =
> +				&crtc_state->wm.skl.optimal.planes[plane->id];

This whole loop is bothering me. I'd much rather we move to a scheme
where each plane computes it's SAGV friendlyness when computing the
watermarks. We'll anyway need that since we need to caclulate the
watermarks differently for the SAGV on vs. off cases.

>  
> -		/* Skip this plane if it's not enabled */
> -		if (!wm->wm[0].plane_en)
> -			continue;
> +			/* Skip this plane if it's not enabled */
> +			if (!wm->wm[0].plane_en)
> +				continue;
>  
> -		/* Find the highest enabled wm level for this plane */
> -		for (level = ilk_wm_max_level(dev_priv);
> -		     !wm->wm[level].plane_en; --level)
> -		     { }
> +			/* Find the highest enabled wm level for this plane */
> +			for (level = ilk_wm_max_level(dev_priv);
> +			     !wm->wm[level].plane_en; --level)
> +			     { }
>  
> -		latency = dev_priv->wm.skl_latency[level];
> +			latency = dev_priv->wm.skl_latency[level];
>  
> -		if (skl_needs_memory_bw_wa(dev_priv) &&
> -		    plane->base.state->fb->modifier ==
> -		    I915_FORMAT_MOD_X_TILED)
> -			latency += 15;
> +			if (skl_needs_memory_bw_wa(dev_priv) &&
> +			    plane->base.state->fb->modifier ==
> +			    I915_FORMAT_MOD_X_TILED)
> +				latency += 15;
>  
> -		/*
> -		 * 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.
> -		 */
> -		if (latency < sagv_block_time_us)
> -			return false;
> +			/*
> +			 * 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.
> +			 */
> +			if (latency < sagv_block_time_us)
> +				return false;
> +		}
>  	}
>  
>  	return true;
> -- 
> 2.22.1

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx