[Intel-gfx] [PATCH] drm/i915: Disable SAGV on pre plane update.

Rodrigo Vivi rodrigo.vivi at intel.com
Thu Feb 22 20:12:00 UTC 2018


On Thu, Feb 22, 2018 at 10:00:47PM +0200, Ville Syrjälä wrote:
> On Thu, Feb 22, 2018 at 11:28:58AM -0800, Rodrigo Vivi wrote:
> > According to Spec "Requirement before plane enabling or
> > configuration change: Disable SAGV if any enabled plane will not
> > be able to enable watermarks for memory latency >= SAGV block
> > time, or any transcoder is interlaced. Else, enable SAGV."
> > 
> > Currently we are only enabling and disabling SAGV on full
> > modeset. If we end up changing plane configurations and
> > sagv remains enabled when latency is higher than sagv block
> > time the machine can hang.
> > 
> > Also we are computing the latency values in different places
> > and following different conditions/rules.
> > 
> > So let's move the can_enable_sagv check to be inside
> > skl_compute_wm and disable and enable on {pre,post} plane
> > updates.
> > 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104975
> > Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Cc: Azhar Shaikh <azhar.shaikh at intel.com>
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 18 +++++-----
> >  drivers/gpu/drm/i915/intel_drv.h     |  3 +-
> >  drivers/gpu/drm/i915/intel_pm.c      | 64 +++++++++++++-----------------------
> >  3 files changed, 32 insertions(+), 53 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 65c8487be7c7..008254579be1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5090,6 +5090,8 @@ static bool hsw_post_update_enable_ips(const struct intel_crtc_state *old_crtc_s
> >  static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >  {
> >  	struct intel_crtc *crtc = to_intel_crtc(old_crtc_state->base.crtc);
> > +	struct drm_device *dev = crtc->base.dev;
> > +	struct drm_i915_private *dev_priv = to_i915(dev);
> >  	struct drm_atomic_state *old_state = old_crtc_state->base.state;
> >  	struct intel_crtc_state *pipe_config =
> >  		intel_atomic_get_new_crtc_state(to_intel_atomic_state(old_state),
> > @@ -5120,6 +5122,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state)
> >  		     !old_primary_state->base.visible))
> >  			intel_post_enable_primary(&crtc->base, pipe_config);
> >  	}
> > +
> > +	if (pipe_config->sagv)
> > +		intel_enable_sagv(dev_priv);
> 
> Unfortunately a single pipe can't make a unilateral decision to
> enable sagv. The other pipes must be disabled first.

I wondered that...

> And we can't use
> state->active_crtcs for this in non-modeset commits since that's only
> valid during a modeset.

but I thought this was enough :(

> 
> cxsr has the same problem pretty much. Currently that's handled somewhat
> hackily from foo_merge_wm() to disable cxsr when multiple pipes are
> active. But I think a better idea might be to maintain a bitmask of
> pipes allowing cxsr in dev_priv. And then have enable/disable hooks
> that get called for each pipe from the commit path to manipulate the
> bitmask and based on what the bitmask ends up looking either enable or
> disable cxsr.

I considered some kind of bitmask for this case this morning, but
I thought that state->active_crtcs would take care of that so I ignored
and move forward.

> That idea could be used for sagv as well I suppose.

could it be the other way around?

> 
> >  }
> >  
> >  static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> > @@ -5205,6 +5210,9 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state,
> >  						     pipe_config);
> >  	else if (pipe_config->update_wm_pre)
> >  		intel_update_watermarks(crtc);
> > +
> > +	if (!pipe_config->sagv)
> > +		intel_disable_sagv(dev_priv);
> >  }
> >  
> >  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned plane_mask)
> > @@ -12366,13 +12374,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  
> >  		intel_set_cdclk(dev_priv, &dev_priv->cdclk.actual);
> >  
> > -		/*
> > -		 * SKL workaround: bspec recommends we disable the SAGV when we
> > -		 * have more then one pipe enabled
> > -		 */
> > -		if (!intel_can_enable_sagv(state))
> > -			intel_disable_sagv(dev_priv);
> > -
> >  		intel_modeset_verify_disabled(dev, state);
> >  	}
> >  
> > @@ -12431,9 +12432,6 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state)
> >  	if (intel_state->modeset)
> >  		intel_verify_planes(intel_state);
> >  
> > -	if (intel_state->modeset && intel_can_enable_sagv(state))
> > -		intel_enable_sagv(dev_priv);
> > -
> >  	drm_atomic_helper_commit_hw_done(state);
> >  
> >  	if (intel_state->modeset) {
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 1535bfb7ea40..4c10a8a94d73 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -712,6 +712,7 @@ struct intel_crtc_state {
> >  	bool update_pipe; /* can a fast modeset be performed? */
> >  	bool disable_cxsr;
> >  	bool update_wm_pre, update_wm_post; /* watermarks are updated */
> > +	bool sagv; /* Disable SAGV on any latency higher than its block time */
> >  	bool fb_changed; /* fb on any of the planes is changed */
> >  	bool fifo_changed; /* FIFO split is changed */
> >  
> > @@ -2001,7 +2002,7 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc,
> >  			      struct skl_pipe_wm *out);
> >  void g4x_wm_sanitize(struct drm_i915_private *dev_priv);
> >  void vlv_wm_sanitize(struct drm_i915_private *dev_priv);
> > -bool intel_can_enable_sagv(struct drm_atomic_state *state);
> > +bool intel_can_enable_sagv(struct intel_atomic_state *state, u32 latency);
> >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> >  bool skl_wm_level_equals(const struct skl_wm_level *l1,
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 21dac6ebc202..731b3808a62e 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3681,16 +3681,13 @@ intel_disable_sagv(struct drm_i915_private *dev_priv)
> >  	return 0;
> >  }
> >  
> > -bool intel_can_enable_sagv(struct drm_atomic_state *state)
> > +bool intel_can_enable_sagv(struct intel_atomic_state *intel_state, u32 latency)
> >  {
> > -	struct drm_device *dev = state->dev;
> > +	struct drm_device *dev = intel_state->base.dev;
> >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > -	struct intel_atomic_state *intel_state = to_intel_atomic_state(state);
> >  	struct intel_crtc *crtc;
> > -	struct intel_plane *plane;
> >  	struct intel_crtc_state *cstate;
> >  	enum pipe pipe;
> > -	int level, latency;
> >  	int sagv_block_time_us;
> >  
> >  	if (!intel_has_sagv(dev_priv))
> > @@ -3703,6 +3700,9 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> >  	else
> >  		sagv_block_time_us = 10;
> >  
> > +	if (latency < sagv_block_time_us)
> > +		return false;
> > +
> >  	/*
> >  	 * SKL+ workaround: bspec recommends we disable the SAGV when we have
> >  	 * more then one pipe enabled
> > @@ -3722,35 +3722,6 @@ bool intel_can_enable_sagv(struct drm_atomic_state *state)
> >  	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 =
> > -			&cstate->wm.skl.optimal.planes[plane->id];
> > -
> > -		/* 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)
> > -		     { }
> > -
> > -		latency = dev_priv->wm.skl_latency[level];
> > -
> > -		if (skl_needs_memory_bw_wa(intel_state) &&
> > -		    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 the SAGV.
> > -		 */
> > -		if (latency < sagv_block_time_us)
> > -			return false;
> > -	}
> > -
> >  	return true;
> >  }
> >  
> > @@ -4501,7 +4472,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  				const struct skl_wm_params *wp,
> >  				uint16_t *out_blocks, /* out */
> >  				uint8_t *out_lines, /* out */
> > -				bool *enabled /* out */)
> > +				bool *enabled, /* out */
> > +				bool *sagv /* out */)
> >  {
> >  	const struct drm_plane_state *pstate = &intel_pstate->base;
> >  	uint32_t latency = dev_priv->wm.skl_latency[level];
> > @@ -4528,6 +4500,8 @@ static int skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
> >  	if (apply_memory_bw_wa && wp->x_tiled)
> >  		latency += 15;
> >  
> > +	*sagv = intel_can_enable_sagv(state, latency);
> > +
> >  	method1 = skl_wm_method1(dev_priv, wp->plane_pixel_rate,
> >  				 wp->cpp, latency, wp->dbuf_block_size);
> >  	method2 = skl_wm_method2(wp->plane_pixel_rate,
> > @@ -4629,7 +4603,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> >  		      struct intel_crtc_state *cstate,
> >  		      const struct intel_plane_state *intel_pstate,
> >  		      const struct skl_wm_params *wm_params,
> > -		      struct skl_plane_wm *wm)
> > +		      struct skl_plane_wm *wm,
> > +		      bool *sagv)
> >  {
> >  	struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc);
> >  	struct drm_plane *plane = intel_pstate->base.plane;
> > @@ -4655,7 +4630,8 @@ skl_compute_wm_levels(const struct drm_i915_private *dev_priv,
> >  					   wm_params,
> >  					   &result->plane_res_b,
> >  					   &result->plane_res_l,
> > -					   &result->plane_en);
> > +					   &result->plane_en,
> > +					   sagv);
> >  		if (ret)
> >  			return ret;
> >  	}
> > @@ -4743,7 +4719,8 @@ static void skl_compute_transition_wm(struct intel_crtc_state *cstate,
> >  
> >  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> >  			     struct skl_ddb_allocation *ddb,
> > -			     struct skl_pipe_wm *pipe_wm)
> > +			     struct skl_pipe_wm *pipe_wm,
> > +			     bool *sagv)
> >  {
> >  	struct drm_device *dev = cstate->base.crtc->dev;
> >  	struct drm_crtc_state *crtc_state = &cstate->base;
> > @@ -4777,7 +4754,7 @@ static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> >  			return ret;
> >  
> >  		ret = skl_compute_wm_levels(dev_priv, ddb, cstate,
> > -					    intel_pstate, &wm_params, wm);
> > +					    intel_pstate, &wm_params, wm, sagv);
> >  		if (ret)
> >  			return ret;
> >  		skl_compute_transition_wm(cstate, &wm_params, &wm->wm[0],
> > @@ -4899,12 +4876,13 @@ static int skl_update_pipe_wm(struct drm_crtc_state *cstate,
> >  			      const struct skl_pipe_wm *old_pipe_wm,
> >  			      struct skl_pipe_wm *pipe_wm, /* out */
> >  			      struct skl_ddb_allocation *ddb, /* out */
> > -			      bool *changed /* out */)
> > +			      bool *changed /* out */,
> > +			      bool *sagv /* out */)
> >  {
> >  	struct intel_crtc_state *intel_cstate = to_intel_crtc_state(cstate);
> >  	int ret;
> >  
> > -	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm);
> > +	ret = skl_build_pipe_wm(intel_cstate, ddb, pipe_wm, sagv);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -5099,6 +5077,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> >  	struct drm_device *dev = state->dev;
> >  	struct skl_pipe_wm *pipe_wm;
> >  	bool changed = false;
> > +	bool sagv = true;
> >  	int ret, i;
> >  
> >  	/*
> > @@ -5147,7 +5126,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> >  
> >  		pipe_wm = &intel_cstate->wm.skl.optimal;
> >  		ret = skl_update_pipe_wm(cstate, old_pipe_wm, pipe_wm,
> > -					 &results->ddb, &changed);
> > +					 &results->ddb, &changed, &sagv);
> >  		if (ret)
> >  			return ret;
> >  
> > @@ -5159,6 +5138,7 @@ skl_compute_wm(struct drm_atomic_state *state)
> >  			continue;
> >  
> >  		intel_cstate->update_wm_pre = true;
> > +		intel_cstate->sagv &= sagv;
> >  	}
> >  
> >  	skl_print_wm_changes(state);
> > -- 
> > 2.13.6
> 
> -- 
> Ville Syrjälä
> Intel OTC


More information about the Intel-gfx mailing list