Re: [Intel-gfx] [PATCH 10/10] drm/i915: Perform a central cdclk state sanity check

2017-10-24 Thread Rodrigo Vivi 
On Tue, Oct 24, 2017 at 09:52:16AM +, Ville Syrjala wrote:
> From: Ville Syrjälä 
> 
> WARN if the cdclk state doesn't match what we expect after programming.
> And let's remove the WARN from bdw_set_cdclk() that's trying to achieve
> the same thing in a more limite fashion.
> 
> Also take the opportunity to refactor the code to use a common function
> for dumping out a cdclk state.
> 
> Cc: Mika Kahola 
> Cc: Manasi Navare 
> Cc: Rodrigo Vivi 
> Signed-off-by: Ville Syrjälä 

better code, better logs and right checks.

Reviewed-by: Rodrigo Vivi 

> ---
>  drivers/gpu/drm/i915/intel_cdclk.c   | 30 +++---
>  drivers/gpu/drm/i915/intel_display.c |  3 +++
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  3 files changed, 24 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_cdclk.c 
> b/drivers/gpu/drm/i915/intel_cdclk.c
> index fedfe3c720b6..51cd23dd8676 100644
> --- a/drivers/gpu/drm/i915/intel_cdclk.c
> +++ b/drivers/gpu/drm/i915/intel_cdclk.c
> @@ -768,10 +768,6 @@ static void bdw_set_cdclk(struct drm_i915_private 
> *dev_priv,
>   I915_WRITE(CDCLK_FREQ, DIV_ROUND_CLOSEST(cdclk, 1000) - 1);
>  
>   intel_update_cdclk(dev_priv);
> -
> - WARN(cdclk != dev_priv->cdclk.hw.cdclk,
> -  "cdclk requested %d kHz but got %d kHz\n",
> -  cdclk, dev_priv->cdclk.hw.cdclk);
>  }
>  
>  static int skl_calc_cdclk(int min_cdclk, int vco)
> @@ -1068,6 +1064,8 @@ static void skl_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>   goto sanitize;
>  
>   intel_update_cdclk(dev_priv);
> + intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
> +
>   /* Is PLL enabled and locked ? */
>   if (dev_priv->cdclk.hw.vco == 0 ||
>   dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
> @@ -1407,6 +1405,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>   u32 cdctl, expected;
>  
>   intel_update_cdclk(dev_priv);
> + intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
>  
>   if (dev_priv->cdclk.hw.vco == 0 ||
>   dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
> @@ -1713,6 +1712,7 @@ static void cnl_sanitize_cdclk(struct drm_i915_private 
> *dev_priv)
>   u32 cdctl, expected;
>  
>   intel_update_cdclk(dev_priv);
> + intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
>  
>   if (dev_priv->cdclk.hw.vco == 0 ||
>   dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
> @@ -1826,6 +1826,14 @@ bool intel_cdclk_changed(const struct 
> intel_cdclk_state *a,
>   a->voltage_level != b->voltage_level;
>  }
>  
> +void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
> + const char *context)
> +{
> + DRM_DEBUG_DRIVER("%s %d kHz, VCO %d kHz, ref %d kHz, voltage level 
> %d\n",
> +  context, cdclk_state->cdclk, cdclk_state->vco,
> +  cdclk_state->ref, cdclk_state->voltage_level);
> +}
> +
>  /**
>   * intel_set_cdclk - Push the CDCLK state to the hardware
>   * @dev_priv: i915 device
> @@ -1843,11 +1851,15 @@ void intel_set_cdclk(struct drm_i915_private 
> *dev_priv,
>   if (WARN_ON_ONCE(!dev_priv->display.set_cdclk))
>   return;
>  
> - DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz, 
> voltage_level %d\n",
> -  cdclk_state->cdclk, cdclk_state->vco,
> -  cdclk_state->ref, cdclk_state->voltage_level);
> + intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
>  
>   dev_priv->display.set_cdclk(dev_priv, cdclk_state);
> +
> + if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
> +  "cdclk state doesn't match!\n")) {
> + intel_dump_cdclk_state(&dev_priv->cdclk.hw, "[hw state]");
> + intel_dump_cdclk_state(cdclk_state, "[sw state]");
> + }
>  }
>  
>  static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
> @@ -2280,10 +2292,6 @@ void intel_update_cdclk(struct drm_i915_private 
> *dev_priv)
>  {
>   dev_priv->display.get_cdclk(dev_priv, &dev_priv->cdclk.hw);
>  
> - DRM_DEBUG_DRIVER("Current CD clock rate: %d kHz, VCO: %d kHz, ref: %d 
> kHz\n",
> -  dev_priv->cdclk.hw.cdclk, dev_priv->cdclk.hw.vco,
> -  dev_priv->cdclk.hw.ref);
> -
>   /*
>* 9:0 CMBUS [sic] CDCLK frequency (cdfreq):
>* Programmng [sic] note: bit[9:2] should be programmed to the number
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b5fa643e1812..7d7952b78a3b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8857,7 +8857,9 @@ static void hsw_restore_lcpll(struct drm_i915_private 
> *dev_priv)
>   }
>  
>   intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +
>   intel_update_cdclk(dev_priv);
> + intel_dump_cd

[Intel-gfx] [PATCH 10/10] drm/i915: Perform a central cdclk state sanity check

2017-10-24 Thread Ville Syrjala 
From: Ville Syrjälä 

WARN if the cdclk state doesn't match what we expect after programming.
And let's remove the WARN from bdw_set_cdclk() that's trying to achieve
the same thing in a more limite fashion.

Also take the opportunity to refactor the code to use a common function
for dumping out a cdclk state.

Cc: Mika Kahola 
Cc: Manasi Navare 
Cc: Rodrigo Vivi 
Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/intel_cdclk.c   | 30 +++---
 drivers/gpu/drm/i915/intel_display.c |  3 +++
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_cdclk.c 
b/drivers/gpu/drm/i915/intel_cdclk.c
index fedfe3c720b6..51cd23dd8676 100644
--- a/drivers/gpu/drm/i915/intel_cdclk.c
+++ b/drivers/gpu/drm/i915/intel_cdclk.c
@@ -768,10 +768,6 @@ static void bdw_set_cdclk(struct drm_i915_private 
*dev_priv,
I915_WRITE(CDCLK_FREQ, DIV_ROUND_CLOSEST(cdclk, 1000) - 1);
 
intel_update_cdclk(dev_priv);
-
-   WARN(cdclk != dev_priv->cdclk.hw.cdclk,
-"cdclk requested %d kHz but got %d kHz\n",
-cdclk, dev_priv->cdclk.hw.cdclk);
 }
 
 static int skl_calc_cdclk(int min_cdclk, int vco)
@@ -1068,6 +1064,8 @@ static void skl_sanitize_cdclk(struct drm_i915_private 
*dev_priv)
goto sanitize;
 
intel_update_cdclk(dev_priv);
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
+
/* Is PLL enabled and locked ? */
if (dev_priv->cdclk.hw.vco == 0 ||
dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
@@ -1407,6 +1405,7 @@ static void bxt_sanitize_cdclk(struct drm_i915_private 
*dev_priv)
u32 cdctl, expected;
 
intel_update_cdclk(dev_priv);
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
 
if (dev_priv->cdclk.hw.vco == 0 ||
dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
@@ -1713,6 +1712,7 @@ static void cnl_sanitize_cdclk(struct drm_i915_private 
*dev_priv)
u32 cdctl, expected;
 
intel_update_cdclk(dev_priv);
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
 
if (dev_priv->cdclk.hw.vco == 0 ||
dev_priv->cdclk.hw.cdclk == dev_priv->cdclk.hw.ref)
@@ -1826,6 +1826,14 @@ bool intel_cdclk_changed(const struct intel_cdclk_state 
*a,
a->voltage_level != b->voltage_level;
 }
 
+void intel_dump_cdclk_state(const struct intel_cdclk_state *cdclk_state,
+   const char *context)
+{
+   DRM_DEBUG_DRIVER("%s %d kHz, VCO %d kHz, ref %d kHz, voltage level 
%d\n",
+context, cdclk_state->cdclk, cdclk_state->vco,
+cdclk_state->ref, cdclk_state->voltage_level);
+}
+
 /**
  * intel_set_cdclk - Push the CDCLK state to the hardware
  * @dev_priv: i915 device
@@ -1843,11 +1851,15 @@ void intel_set_cdclk(struct drm_i915_private *dev_priv,
if (WARN_ON_ONCE(!dev_priv->display.set_cdclk))
return;
 
-   DRM_DEBUG_DRIVER("Changing CDCLK to %d kHz, VCO %d kHz, ref %d kHz, 
voltage_level %d\n",
-cdclk_state->cdclk, cdclk_state->vco,
-cdclk_state->ref, cdclk_state->voltage_level);
+   intel_dump_cdclk_state(cdclk_state, "Changing CDCLK to");
 
dev_priv->display.set_cdclk(dev_priv, cdclk_state);
+
+   if (WARN(intel_cdclk_changed(&dev_priv->cdclk.hw, cdclk_state),
+"cdclk state doesn't match!\n")) {
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "[hw state]");
+   intel_dump_cdclk_state(cdclk_state, "[sw state]");
+   }
 }
 
 static int intel_pixel_rate_to_cdclk(struct drm_i915_private *dev_priv,
@@ -2280,10 +2292,6 @@ void intel_update_cdclk(struct drm_i915_private 
*dev_priv)
 {
dev_priv->display.get_cdclk(dev_priv, &dev_priv->cdclk.hw);
 
-   DRM_DEBUG_DRIVER("Current CD clock rate: %d kHz, VCO: %d kHz, ref: %d 
kHz\n",
-dev_priv->cdclk.hw.cdclk, dev_priv->cdclk.hw.vco,
-dev_priv->cdclk.hw.ref);
-
/*
 * 9:0 CMBUS [sic] CDCLK frequency (cdfreq):
 * Programmng [sic] note: bit[9:2] should be programmed to the number
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index b5fa643e1812..7d7952b78a3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8857,7 +8857,9 @@ static void hsw_restore_lcpll(struct drm_i915_private 
*dev_priv)
}
 
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+
intel_update_cdclk(dev_priv);
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDCLK");
 }
 
 /*
@@ -14358,6 +14360,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
struct drm_i915_private *dev_priv = to_i915(dev);
 
intel_update_cdclk(dev_priv);
+   intel_dump_cdclk_state(&dev_priv->cdclk.hw, "Current CDC