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

[Mika Kahola <mika.kahola@intel.com>]

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.
> 
Reviewed-by: Mika Kahola <mika.kahola@intel.com>
> 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;


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx