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