* [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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.