[Intel-gfx] [PATCH 3/3] drm/i915/tgl: Remove single pipe restriction from SAGV

[James Ausmus]

On Thu, Sep 26, 2019 at 03:34:35PM +0300, Ville Syrjälä wrote:
> 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 at linux.intel.com>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: James Ausmus <james.ausmus at 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.

Hmm, I'll have to look in to this. In the meantime, I'd like to get
patches 1 & 2 of this series moving forward, as those should be what's
really necessary to be able to turn on SAGV for TGL once we're ready, so
I'll send those as a separate series, and leave relaxing the 1 pipe
restriction as it's own work.

-James

> 
> >  
> > -		/* 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