[Intel-gfx] [PATCH v28 3/6] drm/i915: Make active_pipes check skl specific

[Ville Syrjälä]

On Tue, May 12, 2020 at 03:44:06PM +0300, Lisovskiy, Stanislav wrote:
> On Tue, May 12, 2020 at 02:39:25PM +0300, Ville Syrjälä wrote:
> > On Thu, May 07, 2020 at 05:45:00PM +0300, Stanislav Lisovskiy wrote:
> > > Seems that only skl needs to have SAGV turned off
> > > for multipipe scenarios, so lets do it this way.
> > 
> > It doesn't afaics. It's just someone added the check for some random
> > reason. So this should be reworded a bit. Also this isn't just about
> > skl/derivatives but all pre-icl so the <subject> is a bit misleading too.
> 
> This is in BSpec anyway. And it was in the code before, so I really 
> don't get what do you mean here.

That's not what it says. It just suggests that if you guarantee that
you always have enough ddb for sagv in single pipe configuration then
you can just toggle sagv when transitioning between single vs. multi
pipe configurations. The implication being that you don't guarantee
enough ddb for sagv in multi pipe configurations, hence you simply
assume sagv can't be used with multiple pipes.

We don't even guarantee that out single pipe configuration has enough
ddb for sagv, hence we have to actually check the block time constraint
even in single pipe configurations. That makes the whole bspec paragraph
moot and we might as well just do the obvious thing and check the sagv
block time for all pipes (which we're already doing anyway).

> 
> > 
> > > 
> > > If anything blows up - we can always revert this patch.
> > > 
> > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy at intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_pm.c | 15 +++++++++------
> > >  drivers/gpu/drm/i915/intel_pm.h |  3 ++-
> > >  2 files changed, 11 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 3dc1ad66beb3..db188efee21e 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3777,7 +3777,7 @@ void intel_sagv_pre_plane_update(struct intel_atomic_state *state)
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	if (!intel_can_enable_sagv(new_bw_state))
> > > +	if (!intel_can_enable_sagv(dev_priv, new_bw_state))
> > >  		intel_disable_sagv(dev_priv);
> > >  }
> > >  
> > > @@ -3800,7 +3800,7 @@ void intel_sagv_post_plane_update(struct intel_atomic_state *state)
> > >  	if (!new_bw_state)
> > >  		return;
> > >  
> > > -	if (intel_can_enable_sagv(new_bw_state))
> > > +	if (intel_can_enable_sagv(dev_priv, new_bw_state))
> > >  		intel_enable_sagv(dev_priv);
> > >  }
> > >  
> > > @@ -3853,16 +3853,19 @@ static bool skl_crtc_can_enable_sagv(const struct intel_crtc_state *crtc_state)
> > >  	return true;
> > >  }
> > >  
> > > -bool intel_can_enable_sagv(const struct intel_bw_state *bw_state)
> > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv,
> > > +			   const struct intel_bw_state *bw_state)
> > >  {
> > > -	if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes))
> > > -		return false;
> > > +	if (INTEL_GEN(dev_priv) < 11)
> > > +		if (bw_state->active_pipes && !is_power_of_2(bw_state->active_pipes))
> > 
> > If (a && b && c)
> > 	return false;
> 
> Then the line would get too long, and it does exactly same thing.
> I really don't understand such comments.
> 
> Stan
> 
> > 
> > 
> > > +			return false;
> > >  
> > >  	return bw_state->pipe_sagv_reject == 0;
> > >  }
> > >  
> > >  static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > >  {
> > > +	struct drm_i915_private *dev_priv = to_i915(state->base.dev);
> > >  	int ret;
> > >  	struct intel_crtc *crtc;
> > >  	const struct intel_crtc_state *new_crtc_state;
> > > @@ -3896,7 +3899,7 @@ static int intel_compute_sagv_mask(struct intel_atomic_state *state)
> > >  			return ret;
> > >  	}
> > >  
> > > -	if (intel_can_enable_sagv(new_bw_state) != intel_can_enable_sagv(old_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;
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.h b/drivers/gpu/drm/i915/intel_pm.h
> > > index fd1dc422e6c5..614ac7f8d4cc 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.h
> > > +++ b/drivers/gpu/drm/i915/intel_pm.h
> > > @@ -42,7 +42,8 @@ void skl_pipe_wm_get_hw_state(struct intel_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(const struct intel_bw_state *bw_state);
> > > +bool intel_can_enable_sagv(struct drm_i915_private *dev_priv,
> > > +			   const struct intel_bw_state *bw_state);
> > >  int intel_enable_sagv(struct drm_i915_private *dev_priv);
> > >  int intel_disable_sagv(struct drm_i915_private *dev_priv);
> > >  void intel_sagv_pre_plane_update(struct intel_atomic_state *state);
> > > -- 
> > > 2.24.1.485.gad05a3d8e5
> > 
> > -- 
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel