[PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked

public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed

* [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
@ 2016-05-24  9:27 Imre Deak
  2016-05-24  9:27 ` [PATCH 2/2] drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume Imre Deak
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Imre Deak @ 2016-05-24  9:27 UTC (permalink / raw)
  To: intel-gfx

If the CDCLK PLL isn't locked we can just assume that it's off resulting
in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
CDCLK PLL sanitization added in the following patch can be done on BXT
the same way as it's done on SKL.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c1e666b..b8e5995 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	dev_priv->cdclk_pll.ref = 24000;
+	dev_priv->cdclk_pll.vco = 0;
 
 	val = I915_READ(LCPLL1_CTL);
-	if ((val & LCPLL_PLL_ENABLE) == 0) {
-		dev_priv->cdclk_pll.vco = 0;
+	if ((val & LCPLL_PLL_ENABLE) == 0)
 		return;
-	}
 
-	WARN_ON((val & LCPLL_PLL_LOCK) == 0);
+	if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
+		return;
 
 	val = I915_READ(DPLL_CTRL1);
 
@@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
 	if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
 		goto sanitize;
 
+	intel_update_cdclk(dev_priv->dev);
 	/* Is PLL enabled and locked ? */
-	if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
-	    (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
+	if (!dev_priv->cdclk_pll.vco ||
+	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
 		goto sanitize;
 
 	if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |
@@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
 	    DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
 		goto sanitize;
 
-	intel_update_cdclk(dev_priv->dev);
-
 	/* DPLL okay; verify the cdclock
 	 *
 	 * Noticed in some instances that the freq selection is correct but
@@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
 	u32 val;
 
 	dev_priv->cdclk_pll.ref = 19200;
+	dev_priv->cdclk_pll.vco = 0;
 
 	val = I915_READ(BXT_DE_PLL_ENABLE);
-	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
-		dev_priv->cdclk_pll.vco = 0;
+	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
 		return;
-	}
 
-	WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
+	if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
+		return;
 
 	val = I915_READ(BXT_DE_PLL_CTL);
 	dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
-- 
2.5.0

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/2] drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume
  2016-05-24  9:27 [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Imre Deak
@ 2016-05-24  9:27 ` Imre Deak
  2016-05-24  9:52 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Patchwork
  2016-05-24 10:22 ` [PATCH 1/2] " Ville Syrjälä
  2 siblings, 0 replies; 6+ messages in thread
From: Imre Deak @ 2016-05-24  9:27 UTC (permalink / raw)
  To: intel-gfx

I noticed that during S4 resume BIOS incorrectly sets bits 18, 19 which
are reserved/MBZ and sets the decimal frequency fields to all 0xff in
the CDCLK register. The result is a hard lockup as display register
accesses are attempted later. Work around this by sanitizing the CDCLK
PLL/dividers the same way it's done on SKL.

While this is clearly a BIOS bug which should be fixed separately, it
doesn't hurt to check/sanitize this regardless.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 51 ++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b8e5995..479d2e4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5412,11 +5412,58 @@ static void broxton_set_cdclk(struct drm_i915_private *dev_priv, int cdclk)
 	intel_update_cdclk(dev_priv->dev);
 }
 
+static void bxt_sanitize_cdclk(struct drm_i915_private *dev_priv)
+{
+	u32 cdctl, expected;
+
+	intel_update_cdclk(dev_priv->dev);
+
+	if (!dev_priv->cdclk_pll.vco ||
+	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
+		goto sanitize;
+
+	/* DPLL okay; verify the cdclock
+	 *
+	 * Some BIOS versions leave an incorrect decimal frequency value and
+	 * set reserved MBZ bits in CDCLK_CTL at least during exiting from S4,
+	 * so sanitize this register.
+	 */
+	cdctl = I915_READ(CDCLK_CTL);
+	/*
+	 * Let's ignore the pipe field, since BIOS could have configured the
+	 * dividers both synching to an active pipe, or asynchronously
+	 * (PIPE_NONE).
+	 */
+	cdctl &= ~BXT_CDCLK_CD2X_PIPE_NONE;
+
+	expected = (cdctl & BXT_CDCLK_CD2X_DIV_SEL_MASK) |
+		   skl_cdclk_decimal(dev_priv->cdclk_freq);
+	/*
+	 * Disable SSA Precharge when CD clock frequency < 500 MHz,
+	 * enable otherwise.
+	 */
+	if (dev_priv->cdclk_freq >= 500000)
+		expected |= BXT_CDCLK_SSA_PRECHARGE_ENABLE;
+
+	if (cdctl == expected)
+		/* All well; nothing to sanitize */
+		return;
+
+sanitize:
+	DRM_DEBUG_KMS("Sanitizing cdclk programmed by pre-os\n");
+
+	/* force cdclk programming */
+	dev_priv->cdclk_freq = 0;
+
+	/* force full PLL disable + enable */
+	dev_priv->cdclk_pll.vco = -1;
+}
+
 void broxton_init_cdclk(struct drm_i915_private *dev_priv)
 {
-	intel_update_cdclk(dev_priv->dev);
+	bxt_sanitize_cdclk(dev_priv);
 
-	if (dev_priv->cdclk_pll.vco != 0)
+	if (dev_priv->cdclk_freq > 0 && dev_priv->cdclk_pll.vco > 0)
 		return;
 
 	/*
-- 
2.5.0

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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
  2016-05-24  9:27 [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Imre Deak
  2016-05-24  9:27 ` [PATCH 2/2] drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume Imre Deak
@ 2016-05-24  9:52 ` Patchwork
  2016-05-24 10:22 ` [PATCH 1/2] " Ville Syrjälä
  2 siblings, 0 replies; 6+ messages in thread
From: Patchwork @ 2016-05-24  9:52 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
URL   : https://patchwork.freedesktop.org/series/7621/
State : warning

== Summary ==

Series 7621v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/7621/revisions/1/mbox

Test gem_busy:
        Subgroup basic-parallel-vebox:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_mmap_gtt:
        Subgroup basic-read:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test gem_ringfill:
        Subgroup basic-default-interruptible:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test gem_storedw_loop:
        Subgroup basic-default:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_pipe_crc_basic:
        Subgroup bad-pipe:
                dmesg-warn -> PASS       (ro-skl-i7-6700hq)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                pass       -> DMESG-WARN (ro-skl-i7-6700hq)
Test kms_sink_crc_basic:
                pass       -> SKIP       (ro-skl-i7-6700hq)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                fail       -> DMESG-WARN (ro-skl-i7-6700hq)

ro-bdw-i5-5250u  total:209  pass:172  dwarn:0   dfail:0   fail:0   skip:37 
ro-bdw-i7-5557U  total:209  pass:197  dwarn:0   dfail:0   fail:0   skip:12 
ro-bdw-i7-5600u  total:209  pass:180  dwarn:0   dfail:0   fail:1   skip:28 
ro-bsw-n3050     total:209  pass:168  dwarn:0   dfail:0   fail:2   skip:39 
ro-byt-n2820     total:209  pass:170  dwarn:0   dfail:0   fail:2   skip:37 
ro-hsw-i3-4010u  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-hsw-i7-4770r  total:209  pass:186  dwarn:0   dfail:0   fail:0   skip:23 
ro-ilk-i7-620lm  total:209  pass:146  dwarn:0   dfail:0   fail:1   skip:62 
ro-ilk1-i5-650   total:204  pass:146  dwarn:0   dfail:0   fail:1   skip:57 
ro-ivb-i7-3770   total:209  pass:177  dwarn:0   dfail:0   fail:0   skip:32 
ro-ivb2-i7-3770  total:209  pass:181  dwarn:0   dfail:0   fail:0   skip:28 
ro-skl-i7-6700hq total:204  pass:176  dwarn:6   dfail:0   fail:0   skip:22 
ro-snb-i7-2620M  total:209  pass:170  dwarn:0   dfail:0   fail:1   skip:38 

Results at /archive/results/CI_IGT_test/RO_Patchwork_988/

8621fb5 drm-intel-nightly: 2016y-05m-23d-18h-18m-33s UTC integration manifest
764f0ce drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume
8a66284 drm/i915/gen9: Assume CDCLK PLL is off if it's not locked

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
  2016-05-24  9:27 [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Imre Deak
  2016-05-24  9:27 ` [PATCH 2/2] drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume Imre Deak
  2016-05-24  9:52 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Patchwork
@ 2016-05-24 10:22 ` Ville Syrjälä
  2016-05-24 11:59   ` Imre Deak
  2 siblings, 1 reply; 6+ messages in thread
From: Ville Syrjälä @ 2016-05-24 10:22 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

On Tue, May 24, 2016 at 12:27:50PM +0300, Imre Deak wrote:
> If the CDCLK PLL isn't locked we can just assume that it's off resulting
> in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
> CDCLK PLL sanitization added in the following patch can be done on BXT
> the same way as it's done on SKL.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c1e666b..b8e5995 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
>  	u32 val;
>  
>  	dev_priv->cdclk_pll.ref = 24000;
> +	dev_priv->cdclk_pll.vco = 0;
>  
>  	val = I915_READ(LCPLL1_CTL);
> -	if ((val & LCPLL_PLL_ENABLE) == 0) {
> -		dev_priv->cdclk_pll.vco = 0;
> +	if ((val & LCPLL_PLL_ENABLE) == 0)
>  		return;
> -	}
>  
> -	WARN_ON((val & LCPLL_PLL_LOCK) == 0);
> +	if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
> +		return;
>  
>  	val = I915_READ(DPLL_CTRL1);
>  
> @@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
>  		goto sanitize;
>  
> +	intel_update_cdclk(dev_priv->dev);
>  	/* Is PLL enabled and locked ? */
> -	if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
> -	    (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
> +	if (!dev_priv->cdclk_pll.vco ||

== 0 please. I find that more pleasing to the eye when we end up mixing
with == anyway on the next line.

Actually is there any extra benefit from the cdclk_freq check? As
in would vco==0 be sufficient on its own?

> +	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
>  		goto sanitize;
>  
>  	if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |

Maybe toss out this DPLL_CTRL1 check that I added as well then, and have
skl_dpll0_update() set the vco to 0 when it's crap. If we ever actually
hit this in the real world, we'll get the warn, and then we perhaps get
to rethink this stuff, but for now simpler seems better.

> @@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
>  	    DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
>  		goto sanitize;
>  
> -	intel_update_cdclk(dev_priv->dev);
> -
>  	/* DPLL okay; verify the cdclock
>  	 *
>  	 * Noticed in some instances that the freq selection is correct but
> @@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
>  	u32 val;
>  
>  	dev_priv->cdclk_pll.ref = 19200;
> +	dev_priv->cdclk_pll.vco = 0;
>  
>  	val = I915_READ(BXT_DE_PLL_ENABLE);
> -	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
> -		dev_priv->cdclk_pll.vco = 0;
> +	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
>  		return;
> -	}
>  
> -	WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
> +	if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> +		return;
>  
>  	val = I915_READ(BXT_DE_PLL_CTL);
>  	dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
> -- 
> 2.5.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
  2016-05-24 10:22 ` [PATCH 1/2] " Ville Syrjälä
@ 2016-05-24 11:59   ` Imre Deak
  2016-05-24 12:09     ` Ville Syrjälä
  0 siblings, 1 reply; 6+ messages in thread
From: Imre Deak @ 2016-05-24 11:59 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: intel-gfx

On ti, 2016-05-24 at 13:22 +0300, Ville Syrjälä wrote:
> On Tue, May 24, 2016 at 12:27:50PM +0300, Imre Deak wrote:
> > If the CDCLK PLL isn't locked we can just assume that it's off resulting
> > in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
> > CDCLK PLL sanitization added in the following patch can be done on BXT
> > the same way as it's done on SKL.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index c1e666b..b8e5995 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
> >  	u32 val;
> >  
> >  	dev_priv->cdclk_pll.ref = 24000;
> > +	dev_priv->cdclk_pll.vco = 0;
> >  
> >  	val = I915_READ(LCPLL1_CTL);
> > -	if ((val & LCPLL_PLL_ENABLE) == 0) {
> > -		dev_priv->cdclk_pll.vco = 0;
> > +	if ((val & LCPLL_PLL_ENABLE) == 0)
> >  		return;
> > -	}
> >  
> > -	WARN_ON((val & LCPLL_PLL_LOCK) == 0);
> > +	if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
> > +		return;
> >  
> >  	val = I915_READ(DPLL_CTRL1);
> >  
> > @@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  	if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
> >  		goto sanitize;
> >  
> > +	intel_update_cdclk(dev_priv->dev);
> >  	/* Is PLL enabled and locked ? */
> > -	if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
> > -	    (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
> > +	if (!dev_priv->cdclk_pll.vco ||
> 
> == 0 please. I find that more pleasing to the eye when we end up mixing
> with == anyway on the next line.

Ok.

> Actually is there any extra benefit from the cdclk_freq check? As
> in would vco==0 be sufficient on its own?

The other check is for the case of an invalid CDCLK divider setting.
Don't we care about that?

> > +	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
> >  		goto sanitize;
> >  
> >  	if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |
> 
> Maybe toss out this DPLL_CTRL1 check that I added as well then, and have
> skl_dpll0_update() set the vco to 0 when it's crap. If we ever actually
> hit this in the real world, we'll get the warn, and then we perhaps get
> to rethink this stuff, but for now simpler seems better.

Ok, makes sense.

> > @@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> >  	    DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
> >  		goto sanitize;
> >  
> > -	intel_update_cdclk(dev_priv->dev);
> > -
> >  	/* DPLL okay; verify the cdclock
> >  	 *
> >  	 * Noticed in some instances that the freq selection is correct but
> > @@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
> >  	u32 val;
> >  
> >  	dev_priv->cdclk_pll.ref = 19200;
> > +	dev_priv->cdclk_pll.vco = 0;
> >  
> >  	val = I915_READ(BXT_DE_PLL_ENABLE);
> > -	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
> > -		dev_priv->cdclk_pll.vco = 0;
> > +	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
> >  		return;
> > -	}
> >  
> > -	WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
> > +	if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> > +		return;
> >  
> >  	val = I915_READ(BXT_DE_PLL_CTL);
> >  	dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked
  2016-05-24 11:59   ` Imre Deak
@ 2016-05-24 12:09     ` Ville Syrjälä
  0 siblings, 0 replies; 6+ messages in thread
From: Ville Syrjälä @ 2016-05-24 12:09 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

On Tue, May 24, 2016 at 02:59:55PM +0300, Imre Deak wrote:
> On ti, 2016-05-24 at 13:22 +0300, Ville Syrjälä wrote:
> > On Tue, May 24, 2016 at 12:27:50PM +0300, Imre Deak wrote:
> > > If the CDCLK PLL isn't locked we can just assume that it's off resulting
> > > in fully re-initializing both CDCLK PLL and CDCLK dividers. This way the
> > > CDCLK PLL sanitization added in the following patch can be done on BXT
> > > the same way as it's done on SKL.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 23 +++++++++++------------
> > >  1 file changed, 11 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index c1e666b..b8e5995 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -5461,14 +5461,14 @@ skl_dpll0_update(struct drm_i915_private *dev_priv)
> > >  	u32 val;
> > >  
> > >  	dev_priv->cdclk_pll.ref = 24000;
> > > +	dev_priv->cdclk_pll.vco = 0;
> > >  
> > >  	val = I915_READ(LCPLL1_CTL);
> > > -	if ((val & LCPLL_PLL_ENABLE) == 0) {
> > > -		dev_priv->cdclk_pll.vco = 0;
> > > +	if ((val & LCPLL_PLL_ENABLE) == 0)
> > >  		return;
> > > -	}
> > >  
> > > -	WARN_ON((val & LCPLL_PLL_LOCK) == 0);
> > > +	if (WARN_ON((val & LCPLL_PLL_LOCK) == 0))
> > > +		return;
> > >  
> > >  	val = I915_READ(DPLL_CTRL1);
> > >  
> > > @@ -5690,9 +5690,10 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > >  	if ((I915_READ(SWF_ILK(0x18)) & 0x00FFFFFF) == 0)
> > >  		goto sanitize;
> > >  
> > > +	intel_update_cdclk(dev_priv->dev);
> > >  	/* Is PLL enabled and locked ? */
> > > -	if ((I915_READ(LCPLL1_CTL) & (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK)) !=
> > > -	    (LCPLL_PLL_ENABLE | LCPLL_PLL_LOCK))
> > > +	if (!dev_priv->cdclk_pll.vco ||
> > 
> > == 0 please. I find that more pleasing to the eye when we end up mixing
> > with == anyway on the next line.
> 
> Ok.
> 
> > Actually is there any extra benefit from the cdclk_freq check? As
> > in would vco==0 be sufficient on its own?
> 
> The other check is for the case of an invalid CDCLK divider setting.
> Don't we care about that?

Oh right. I guess there's no harm in checking for it.

> 
> > > +	    dev_priv->cdclk_freq == dev_priv->cdclk_pll.ref)
> > >  		goto sanitize;
> > >  
> > >  	if ((I915_READ(DPLL_CTRL1) & (DPLL_CTRL1_HDMI_MODE(SKL_DPLL0) |
> > 
> > Maybe toss out this DPLL_CTRL1 check that I added as well then, and have
> > skl_dpll0_update() set the vco to 0 when it's crap. If we ever actually
> > hit this in the real world, we'll get the warn, and then we perhaps get
> > to rethink this stuff, but for now simpler seems better.
> 
> Ok, makes sense.
> 
> > > @@ -5701,8 +5702,6 @@ static void skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
> > >  	    DPLL_CTRL1_OVERRIDE(SKL_DPLL0))
> > >  		goto sanitize;
> > >  
> > > -	intel_update_cdclk(dev_priv->dev);
> > > -
> > >  	/* DPLL okay; verify the cdclock
> > >  	 *
> > >  	 * Noticed in some instances that the freq selection is correct but
> > > @@ -6608,14 +6607,14 @@ static void bxt_de_pll_update(struct drm_i915_private *dev_priv)
> > >  	u32 val;
> > >  
> > >  	dev_priv->cdclk_pll.ref = 19200;
> > > +	dev_priv->cdclk_pll.vco = 0;
> > >  
> > >  	val = I915_READ(BXT_DE_PLL_ENABLE);
> > > -	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0) {
> > > -		dev_priv->cdclk_pll.vco = 0;
> > > +	if ((val & BXT_DE_PLL_PLL_ENABLE) == 0)
> > >  		return;
> > > -	}
> > >  
> > > -	WARN_ON((val & BXT_DE_PLL_LOCK) == 0);
> > > +	if (WARN_ON((val & BXT_DE_PLL_LOCK) == 0))
> > > +		return;
> > >  
> > >  	val = I915_READ(BXT_DE_PLL_CTL);
> > >  	dev_priv->cdclk_pll.vco = (val & BXT_DE_PLL_RATIO_MASK) *
> > > -- 
> > > 2.5.0
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2016-05-24 12:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-05-24  9:27 [PATCH 1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Imre Deak
2016-05-24  9:27 ` [PATCH 2/2] drm/i915/bxt: Sanitize CDCLK to fix breakage during S4 resume Imre Deak
2016-05-24  9:52 ` ✗ Ro.CI.BAT: warning for series starting with [1/2] drm/i915/gen9: Assume CDCLK PLL is off if it's not locked Patchwork
2016-05-24 10:22 ` [PATCH 1/2] " Ville Syrjälä
2016-05-24 11:59   ` Imre Deak
2016-05-24 12:09     ` Ville Syrjälä

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox