Re: [PATCH 11/12] drm/i915: Keep track of the cdclk as if all crtc's were active.

[Ander Conselvan De Oliveira <conselvan2@gmail.com>]

On Thu, 2015-11-26 at 15:31 +0200, Ander Conselvan De Oliveira wrote:
> On Thu, 2015-11-19 at 16:07 +0100, Maarten Lankhorst wrote:
> > On skylake when calculating plane visibility with the crtc in
> > dpms off mode the real cdclk may be different from what it would be
> > if the crtc was active. This may result in a WARN_ON(cdclk < crtc_clock)
> > from skl_max_scale. The fix is to keep a atomic_cdclk that would be true
> > if all crtc's were active.
> > 
> > This is required to get the same calculations done correctly regardless
> > of dpms mode.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h      |  2 +-
> >  drivers/gpu/drm/i915/intel_display.c | 55 ++++++++++++++++++++++++---------
> > --
> > -
> >  drivers/gpu/drm/i915/intel_drv.h     |  6 ++++
> >  3 files changed, 44 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 3caecf896f17..f3ad72abeea7 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1776,7 +1776,7 @@ struct drm_i915_private {
> >  
> >  	unsigned int fsb_freq, mem_freq, is_ddr3;
> >  	unsigned int skl_boot_cdclk;
> > -	unsigned int cdclk_freq, max_cdclk_freq;
> > +	unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq;
> >  	unsigned int max_dotclk_freq;
> >  	unsigned int hpll_freq;
> >  	unsigned int czclk_freq;
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 7f69f98d8b23..b2bf92a3b701 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5312,6 +5312,7 @@ static void modeset_put_power_domains(struct
> > drm_i915_private *dev_priv,
> >  
> >  static void modeset_update_crtc_power_domains(struct drm_atomic_state
> > *state)
> >  {
> > +	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	struct drm_device *dev = state->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	unsigned long put_domains[I915_MAX_PIPES] = {};
> > @@ -5325,13 +5326,9 @@ static void modeset_update_crtc_power_domains(struct
> > drm_atomic_state *state)
> >  				modeset_get_crtc_power_domains(crtc);
> >  	}
> >  
> > -	if (dev_priv->display.modeset_commit_cdclk) {
> > -		unsigned int cdclk = to_intel_atomic_state(state)->cdclk;
> > -
> > -		if (cdclk != dev_priv->cdclk_freq &&
> > -		    !WARN_ON(!state->allow_modeset))
> > -			dev_priv->display.modeset_commit_cdclk(state);
> > -	}
> > +	if (dev_priv->display.modeset_commit_cdclk &&
> > +	    intel_state->dev_cdclk != dev_priv->cdclk_freq)
> > +		dev_priv->display.modeset_commit_cdclk(state);
> >  
> >  	for (i = 0; i < I915_MAX_PIPES; i++)
> >  		if (put_domains[i])
> > @@ -6039,13 +6036,18 @@ static int valleyview_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	struct drm_device *dev = state->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int max_pixclk = intel_mode_max_pixclk(dev, state);
> > +	struct intel_atomic_state *intel_state =
> > +		to_intel_atomic_state(state);
> >  
> >  	if (max_pixclk < 0)
> >  		return max_pixclk;
> >  
> > -	to_intel_atomic_state(state)->cdclk =
> > +	intel_state->cdclk = intel_state->dev_cdclk =
> >  		valleyview_calc_cdclk(dev_priv, max_pixclk);
> >  
> > +	if (!intel_state->active_crtcs)
> > +		intel_state->dev_cdclk = valleyview_calc_cdclk(dev_priv,
> > 0);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -6054,13 +6056,18 @@ static int broxton_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  	struct drm_device *dev = state->dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int max_pixclk = intel_mode_max_pixclk(dev, state);
> > +	struct intel_atomic_state *intel_state =
> > +		to_intel_atomic_state(state);
> >  
> >  	if (max_pixclk < 0)
> >  		return max_pixclk;
> >  
> > -	to_intel_atomic_state(state)->cdclk =
> > +	intel_state->cdclk = intel_state->dev_cdclk =
> >  		broxton_calc_cdclk(dev_priv, max_pixclk);
> >  
> > +	if (!intel_state->active_crtcs)
> > +		intel_state->dev_cdclk = broxton_calc_cdclk(dev_priv, 0);
> > +
> >  	return 0;
> >  }
> >  
> > @@ -6103,8 +6110,10 @@ static void vlv_program_pfi_credits(struct
> > drm_i915_private *dev_priv)
> >  static void valleyview_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> >  {
> >  	struct drm_device *dev = old_state->dev;
> > -	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct intel_atomic_state *old_intel_state =
> > +		to_intel_atomic_state(old_state);
> > +	unsigned req_cdclk = old_intel_state->dev_cdclk;
> >  
> >  	/*
> >  	 * FIXME: We can end up here with all power domains off, yet
> > @@ -9574,7 +9583,9 @@ void hsw_disable_pc8(struct drm_i915_private
> > *dev_priv)
> >  static void broxton_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> >  {
> >  	struct drm_device *dev = old_state->dev;
> > -	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > +	struct intel_atomic_state *old_intel_state =
> > +		to_intel_atomic_state(old_state);
> > +	unsigned int req_cdclk = old_intel_state->dev_cdclk;
> >  
> >  	broxton_set_cdclk(dev, req_cdclk);
> >  }
> > @@ -9700,6 +9711,7 @@ static void broadwell_set_cdclk(struct drm_device
> > *dev,
> > int cdclk)
> >  static int broadwell_modeset_calc_cdclk(struct drm_atomic_state *state)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(state->dev);
> > +	struct intel_atomic_state *intel_state =
> > to_intel_atomic_state(state);
> >  	int max_pixclk = ilk_max_pixel_rate(state);
> >  	int cdclk;
> >  
> > @@ -9722,7 +9734,9 @@ static int broadwell_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  		return -EINVAL;
> >  	}
> >  
> > -	to_intel_atomic_state(state)->cdclk = cdclk;
> > +	intel_state->cdclk = intel_state->dev_cdclk = cdclk;
> > +	if (!intel_state->active_crtcs)
> > +		intel_state->dev_cdclk = 337500;
> >  
> >  	return 0;
> >  }
> > @@ -9730,7 +9744,9 @@ static int broadwell_modeset_calc_cdclk(struct
> > drm_atomic_state *state)
> >  static void broadwell_modeset_commit_cdclk(struct drm_atomic_state
> > *old_state)
> >  {
> >  	struct drm_device *dev = old_state->dev;
> > -	unsigned int req_cdclk = to_intel_atomic_state(old_state)->cdclk;
> > +	struct intel_atomic_state *old_intel_state =
> > +		to_intel_atomic_state(old_state);
> > +	unsigned req_cdclk = old_intel_state->dev_cdclk;
> >  
> >  	broadwell_set_cdclk(dev, req_cdclk);
> >  }
> > @@ -13066,18 +13082,15 @@ static int intel_modeset_checks(struct
> > drm_atomic_state *state)
> >  	 * adjusted_mode bits in the crtc directly.
> >  	 */
> >  	if (dev_priv->display.modeset_calc_cdclk) {
> > -		unsigned int cdclk;
> > -
> >  		ret = dev_priv->display.modeset_calc_cdclk(state);
> >  
> > -		cdclk = to_intel_atomic_state(state)->cdclk;
> > -		if (!ret && cdclk != dev_priv->cdclk_freq)
> > +		if (!ret && intel_state->dev_cdclk != dev_priv->cdclk_freq)
> >  			ret = intel_modeset_all_pipes(state);
> >  
> >  		if (ret < 0)
> >  			return ret;
> >  	} else
> > -		to_intel_atomic_state(state)->cdclk = dev_priv->cdclk_freq;
> > +		to_intel_atomic_state(state)->cdclk = dev_priv
> > ->atomic_cdclk_freq;
> >  
> >  	intel_modeset_clear_plls(state);
> >  
> > @@ -13358,6 +13371,7 @@ static int intel_atomic_commit(struct drm_device
> > *dev,
> >  		memcpy(dev_priv->min_pixclk, intel_state->min_pixclk,
> >  		       sizeof(intel_state->min_pixclk));
> >  		dev_priv->active_crtcs = intel_state->active_crtcs;
> > +		dev_priv->atomic_cdclk_freq = intel_state->cdclk;
> >  	}
> >  
> >  	for_each_crtc_in_state(state, crtc, crtc_state, i) {
> > @@ -15026,7 +15040,12 @@ static void i915_disable_vga(struct drm_device
> > *dev)
> >  
> >  void intel_modeset_init_hw(struct drm_device *dev)
> >  {
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> >  	intel_update_cdclk(dev);
> > +
> > +	dev_priv->atomic_cdclk_freq = dev_priv->cdclk_freq;
> > +
> >  	intel_prepare_ddi(dev);
> >  	intel_init_clock_gating(dev);
> >  	intel_enable_gt_powersave(dev);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index d2e1dc698fca..7fd025f64f4c 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -247,6 +247,12 @@ struct intel_atomic_state {
> >  
> >  	unsigned int cdclk;
> >  
> > +	/*
> > +	 * Calculated device cdclk, can be different from cdclk
> > +	 * only when all crtc's are DPMS off.
> > +	 */
> > +	unsigned int dev_cdclk;
> > +
> >  	bool dpll_set, modeset;
> >  
> >  	unsigned int active_crtcs;
> 
> So state->cdclk and dev_priv->atomic_cdclk_freq hold the value for when all
> CRTCs are enabled and state->dev_cdclkand dev_priv->cdclk_freq hold the actual

s/enabled/active/

> cdclk value (which changes for DPMS off). In intel_atomic_check(), there is
> the
> following assignment:
> 
> if (any_ms) {
> 	...
> } else
> 	intel_state->cdclk = to_i915(state->dev)->cdclk_freq;
> 
> Can an update during DPMS off cause the value of cdclk for when all CRTCs are
> enabled to be overwritten with the DPMS off value?
> 
> Patch looks fine other than that, but maybe as a followup we could rename
> those
> fields so the mapping between them is a bit clearer. Maybe current_cdclk for
> both state and dev_priv for the value currently programmed to the hardware.
> Not
> sure about the atomic_cdclk. How about dpms_on_cdclk or active_cdclk?
> 
> Ander
> 
> 
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx