* [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN
@ 2012-05-13 8:54 Chris Wilson
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Chris Wilson @ 2012-05-13 8:54 UTC (permalink / raw)
To: intel-gfx
Turn a fatal lockup into a merely blank display with lots of shouty
messages.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12dba60..a679a9a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1524,7 +1524,11 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc)
pll->pll_reg, pll->active, pll->on,
intel_crtc->base.base.id);
- BUG_ON(pll->active == 0);
+ if (WARN_ON(pll->active == 0)) {
+ assert_pch_pll_disabled(dev_priv, intel_crtc);
+ return;
+ }
+
if (--pll->active) {
assert_pch_pll_enabled(dev_priv, intel_crtc);
return;
--
1.7.10
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 8:54 [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Chris Wilson
@ 2012-05-13 8:54 ` Chris Wilson
2012-05-13 14:08 ` Daniel Vetter
` (2 more replies)
2012-05-13 13:42 ` [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Daniel Vetter
2012-05-13 19:16 ` [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends " Chris Wilson
2 siblings, 3 replies; 15+ messages in thread
From: Chris Wilson @ 2012-05-13 8:54 UTC (permalink / raw)
To: intel-gfx
Hidden away within one chipset specific path was the necessary logic to
turn on the PLL. This needs to be done everywhere in order for us to
drive any display! As such as soon as we tested on a non-CougarPoint
chipset, we failed to bring up any DisplayPorts and generated a nice set
of assertion failures in the process. At least one part of our logic is
working, the part that assumes that we have no idea what we are doing.
Reported-by: guang.a.yang@intel.com
References: https://bugs.freedesktop.org/show_bug.cgi?id=49712
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
---
drivers/gpu/drm/i915/intel_display.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a679a9a..d0112ec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2887,14 +2887,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
/* For PCH output, training FDI link */
dev_priv->display.fdi_link_train(crtc);
+ intel_enable_pch_pll(intel_crtc);
+
if (HAS_PCH_LPT(dev)) {
DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
lpt_program_iclkip(crtc);
} else if (HAS_PCH_CPT(dev)) {
u32 sel;
- intel_enable_pch_pll(intel_crtc);
-
temp = I915_READ(PCH_DPLL_SEL);
switch (pipe) {
default:
--
1.7.10
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN
2012-05-13 8:54 [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Chris Wilson
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
@ 2012-05-13 13:42 ` Daniel Vetter
2012-05-13 19:16 ` [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends " Chris Wilson
2 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2012-05-13 13:42 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, May 13, 2012 at 09:54:08AM +0100, Chris Wilson wrote:
> Turn a fatal lockup into a merely blank display with lots of shouty
> messages.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Queued for -next, thanks for the patch. I guess we could give
pll->refcount a similar treatment, but we could also wait with that until
it claims it's first victim ;-)
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
@ 2012-05-13 14:08 ` Daniel Vetter
2012-05-13 14:25 ` Chris Wilson
2012-05-13 23:35 ` Eugeni Dodonov
2012-05-14 15:12 ` Jesse Barnes
2 siblings, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2012-05-13 14:08 UTC (permalink / raw)
To: Chris Wilson, Eugeni Dodonov, Jesse Barnes; +Cc: intel-gfx
On Sun, May 13, 2012 at 09:54:09AM +0100, Chris Wilson wrote:
> Hidden away within one chipset specific path was the necessary logic to
> turn on the PLL. This needs to be done everywhere in order for us to
> drive any display! As such as soon as we tested on a non-CougarPoint
> chipset, we failed to bring up any DisplayPorts and generated a nice set
> of assertion failures in the process. At least one part of our logic is
> working, the part that assumes that we have no idea what we are doing.
>
> Reported-by: guang.a.yang@intel.com
> References: https://bugs.freedesktop.org/show_bug.cgi?id=49712
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
I guess as-is this patch will blow up on hsw. I think we need to change
the BUG_ON(!pll) in there into a return, like in the disable code. Eugeni?
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a679a9a..d0112ec 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2887,14 +2887,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> /* For PCH output, training FDI link */
> dev_priv->display.fdi_link_train(crtc);
>
> + intel_enable_pch_pll(intel_crtc);
> +
> if (HAS_PCH_LPT(dev)) {
> DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
> lpt_program_iclkip(crtc);
> } else if (HAS_PCH_CPT(dev)) {
> u32 sel;
>
> - intel_enable_pch_pll(intel_crtc);
> -
> temp = I915_READ(PCH_DPLL_SEL);
> switch (pipe) {
> default:
> --
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 14:08 ` Daniel Vetter
@ 2012-05-13 14:25 ` Chris Wilson
0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2012-05-13 14:25 UTC (permalink / raw)
To: Daniel Vetter, Eugeni Dodonov, Jesse Barnes; +Cc: intel-gfx
On Sun, 13 May 2012 16:08:32 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, May 13, 2012 at 09:54:09AM +0100, Chris Wilson wrote:
> > Hidden away within one chipset specific path was the necessary logic to
> > turn on the PLL. This needs to be done everywhere in order for us to
> > drive any display! As such as soon as we tested on a non-CougarPoint
> > chipset, we failed to bring up any DisplayPorts and generated a nice set
> > of assertion failures in the process. At least one part of our logic is
> > working, the part that assumes that we have no idea what we are doing.
> >
> > Reported-by: guang.a.yang@intel.com
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=49712
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> I guess as-is this patch will blow up on hsw. I think we need to change
> the BUG_ON(!pll) in there into a return, like in the disable code. Eugeni?
Whoops, should have looked harder. My thinking was that on hsw, the pll
would be NULL and so the routine would have no effect. Tricksy BUG_ON.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 8:54 [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Chris Wilson
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
2012-05-13 13:42 ` [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Daniel Vetter
@ 2012-05-13 19:16 ` Chris Wilson
2012-05-13 20:08 ` Daniel Vetter
2012-05-13 23:51 ` Eugeni Dodonov
2 siblings, 2 replies; 15+ messages in thread
From: Chris Wilson @ 2012-05-13 19:16 UTC (permalink / raw)
To: intel-gfx
Turn a fatal lockup into a merely blank display with lots of shouty
messages.
v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
In particular, note that we may be called on a PCH platform not using
PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------
1 file changed, 15 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 12dba60..f9e1e1c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1473,14 +1473,18 @@ out_unlock:
static void intel_enable_pch_pll(struct intel_crtc *intel_crtc)
{
struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
- struct intel_pch_pll *pll = intel_crtc->pch_pll;
+ struct intel_pch_pll *pll;
int reg;
u32 val;
- /* PCH only available on ILK+ */
+ /* PCH PLLs only available on ILK, SNB and IVB */
BUG_ON(dev_priv->info->gen < 5);
- BUG_ON(pll == NULL);
- BUG_ON(pll->refcount == 0);
+ pll = intel_crtc->pch_pll;
+ if (pll == NULL)
+ return;
+
+ if (WARN_ON(pll->refcount == 0))
+ return;
DRM_DEBUG_KMS("enable PCH PLL %x (active %d, on? %d)for crtc %d\n",
pll->pll_reg, pll->active, pll->on,
@@ -1518,13 +1522,18 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc)
if (pll == NULL)
return;
- BUG_ON(pll->refcount == 0);
+ if (WARN_ON(pll->refcount == 0))
+ return;
DRM_DEBUG_KMS("disable PCH PLL %x (active %d, on? %d) for crtc %d\n",
pll->pll_reg, pll->active, pll->on,
intel_crtc->base.base.id);
- BUG_ON(pll->active == 0);
+ if (WARN_ON(pll->active == 0)) {
+ assert_pch_pll_disabled(dev_priv, intel_crtc);
+ return;
+ }
+
if (--pll->active) {
assert_pch_pll_enabled(dev_priv, intel_crtc);
return;
--
1.7.10
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 19:16 ` [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends " Chris Wilson
@ 2012-05-13 20:08 ` Daniel Vetter
2012-05-13 20:15 ` Chris Wilson
2012-05-13 23:51 ` Eugeni Dodonov
1 sibling, 1 reply; 15+ messages in thread
From: Daniel Vetter @ 2012-05-13 20:08 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, May 13, 2012 at 08:16:12PM +0100, Chris Wilson wrote:
> Turn a fatal lockup into a merely blank display with lots of shouty
> messages.
>
> v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
> In particular, note that we may be called on a PCH platform not using
> PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Ok, I guess you want me to drop v1 of this again. I'll wait for a bit of
feedback on these two then (and the tested-by from qa on the 2nd one).
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 21 +++++++++++++++------
> 1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 12dba60..f9e1e1c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1473,14 +1473,18 @@ out_unlock:
> static void intel_enable_pch_pll(struct intel_crtc *intel_crtc)
> {
> struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
> - struct intel_pch_pll *pll = intel_crtc->pch_pll;
> + struct intel_pch_pll *pll;
> int reg;
> u32 val;
>
> - /* PCH only available on ILK+ */
> + /* PCH PLLs only available on ILK, SNB and IVB */
> BUG_ON(dev_priv->info->gen < 5);
> - BUG_ON(pll == NULL);
> - BUG_ON(pll->refcount == 0);
> + pll = intel_crtc->pch_pll;
> + if (pll == NULL)
> + return;
> +
> + if (WARN_ON(pll->refcount == 0))
> + return;
>
> DRM_DEBUG_KMS("enable PCH PLL %x (active %d, on? %d)for crtc %d\n",
> pll->pll_reg, pll->active, pll->on,
> @@ -1518,13 +1522,18 @@ static void intel_disable_pch_pll(struct intel_crtc *intel_crtc)
> if (pll == NULL)
> return;
>
> - BUG_ON(pll->refcount == 0);
> + if (WARN_ON(pll->refcount == 0))
> + return;
>
> DRM_DEBUG_KMS("disable PCH PLL %x (active %d, on? %d) for crtc %d\n",
> pll->pll_reg, pll->active, pll->on,
> intel_crtc->base.base.id);
>
> - BUG_ON(pll->active == 0);
> + if (WARN_ON(pll->active == 0)) {
> + assert_pch_pll_disabled(dev_priv, intel_crtc);
> + return;
> + }
> +
> if (--pll->active) {
> assert_pch_pll_enabled(dev_priv, intel_crtc);
> return;
> --
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 20:08 ` Daniel Vetter
@ 2012-05-13 20:15 ` Chris Wilson
2012-05-19 21:11 ` Daniel Vetter
0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2012-05-13 20:15 UTC (permalink / raw)
To: Daniel Vetter; +Cc: intel-gfx
On Sun, 13 May 2012 22:08:10 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Sun, May 13, 2012 at 08:16:12PM +0100, Chris Wilson wrote:
> > Turn a fatal lockup into a merely blank display with lots of shouty
> > messages.
> >
> > v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
> > In particular, note that we may be called on a PCH platform not using
> > PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> Ok, I guess you want me to drop v1 of this again. I'll wait for a bit of
> feedback on these two then (and the tested-by from qa on the 2nd one).
Up to you, I thought that the original patch was overshadowed by the
your change request and little reason for it to be split. It would be
good to get QA's tested-by on the second patch at any rate.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
2012-05-13 14:08 ` Daniel Vetter
@ 2012-05-13 23:35 ` Eugeni Dodonov
2012-05-13 23:39 ` Eugeni Dodonov
2012-05-14 15:12 ` Jesse Barnes
2 siblings, 1 reply; 15+ messages in thread
From: Eugeni Dodonov @ 2012-05-13 23:35 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 526 bytes --]
On Sun, May 13, 2012 at 5:54 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:
> + intel_enable_pch_pll(intel_crtc);
> +
> if (HAS_PCH_LPT(dev)) {
> DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
> lpt_program_iclkip(crtc);
> } else if (HAS_PCH_CPT(dev)) {
>
Could we just replace this 'else if()' with 'else' to achieve the same
functionality? This way, everything will work and we won't touch wrong
registers on LPT.
--
Eugeni Dodonov
<http://eugeni.dodonov.net/>
[-- Attachment #1.2: Type: text/html, Size: 900 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 23:35 ` Eugeni Dodonov
@ 2012-05-13 23:39 ` Eugeni Dodonov
0 siblings, 0 replies; 15+ messages in thread
From: Eugeni Dodonov @ 2012-05-13 23:39 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
[-- Attachment #1.1: Type: text/plain, Size: 1007 bytes --]
On Sun, May 13, 2012 at 8:35 PM, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> On Sun, May 13, 2012 at 5:54 AM, Chris Wilson <chris@chris-wilson.co.uk>wrote:
>
>> + intel_enable_pch_pll(intel_crtc);
>> +
>> if (HAS_PCH_LPT(dev)) {
>> DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
>> lpt_program_iclkip(crtc);
>> } else if (HAS_PCH_CPT(dev)) {
>>
>
> Could we just replace this 'else if()' with 'else' to achieve the same
> functionality? This way, everything will work and we won't touch wrong
> registers on LPT.
>
Err, English fail here from my side. What I thought was:
if (HAS_PCH_LPT(dev)) {
DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
lpt_program_iclkip(crtc);
} else
intel_enable_pch_pll(intel_crtc);
if (HAS_PCH_CPT(dev)) {
u32 sel;
temp = I915_READ(PCH_DPLL_SEL);
switch (pipe) {
default:
...
--
Eugeni Dodonov
<http://eugeni.dodonov.net/>
[-- Attachment #1.2: Type: text/html, Size: 1917 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 19:16 ` [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends " Chris Wilson
2012-05-13 20:08 ` Daniel Vetter
@ 2012-05-13 23:51 ` Eugeni Dodonov
2012-05-19 21:09 ` Daniel Vetter
1 sibling, 1 reply; 15+ messages in thread
From: Eugeni Dodonov @ 2012-05-13 23:51 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, May 13, 2012 at 4:16 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Turn a fatal lockup into a merely blank display with lots of shouty
> messages.
>
> v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
> In particular, note that we may be called on a PCH platform not using
> PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
I think that we need to keep in mind that we can have Haswell+PPT
combination, but I don't know what will happen in this case yet.
So I'd suggest adding a:
WARN_ON(pll == NULL && !HAS_PCH_LPT(dev));
as well somewhere in the checks. What do you think?
--
Eugeni Dodonov
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
2012-05-13 14:08 ` Daniel Vetter
2012-05-13 23:35 ` Eugeni Dodonov
@ 2012-05-14 15:12 ` Jesse Barnes
2012-05-14 15:23 ` Chris Wilson
2 siblings, 1 reply; 15+ messages in thread
From: Jesse Barnes @ 2012-05-14 15:12 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, 13 May 2012 09:54:09 +0100
Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Hidden away within one chipset specific path was the necessary logic to
> turn on the PLL. This needs to be done everywhere in order for us to
> drive any display! As such as soon as we tested on a non-CougarPoint
> chipset, we failed to bring up any DisplayPorts and generated a nice set
> of assertion failures in the process. At least one part of our logic is
> working, the part that assumes that we have no idea what we are doing.
>
> Reported-by: guang.a.yang@intel.com
> References: https://bugs.freedesktop.org/show_bug.cgi?id=49712
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
> drivers/gpu/drm/i915/intel_display.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a679a9a..d0112ec 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2887,14 +2887,14 @@ static void ironlake_pch_enable(struct drm_crtc *crtc)
> /* For PCH output, training FDI link */
> dev_priv->display.fdi_link_train(crtc);
>
> + intel_enable_pch_pll(intel_crtc);
> +
> if (HAS_PCH_LPT(dev)) {
> DRM_DEBUG_KMS("LPT detected: programming iCLKIP\n");
> lpt_program_iclkip(crtc);
> } else if (HAS_PCH_CPT(dev)) {
> u32 sel;
>
> - intel_enable_pch_pll(intel_crtc);
> -
> temp = I915_READ(PCH_DPLL_SEL);
> switch (pipe) {
> default:
Thanks Chris. Only thing I'm not sure about is the LPT bit; does this
function do what we want there?
Other than that:
Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training
2012-05-14 15:12 ` Jesse Barnes
@ 2012-05-14 15:23 ` Chris Wilson
0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2012-05-14 15:23 UTC (permalink / raw)
To: Jesse Barnes; +Cc: intel-gfx
On Mon, 14 May 2012 08:12:13 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Thanks Chris. Only thing I'm not sure about is the LPT bit; does this
> function do what we want there?
The other patch transforms intel_pch_enable_pll (hmm, that's actually
a better name ;-) into a no-op if the crtc does not require a pll. Which
I think is the easier way of tracking PLLs - allocate on up front if the
chipset/output requires a PLL and then we do no need any more checks
other than has-pll? in the main body of the code.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 23:51 ` Eugeni Dodonov
@ 2012-05-19 21:09 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2012-05-19 21:09 UTC (permalink / raw)
To: Eugeni Dodonov; +Cc: intel-gfx
On Sun, May 13, 2012 at 08:51:41PM -0300, Eugeni Dodonov wrote:
> On Sun, May 13, 2012 at 4:16 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Turn a fatal lockup into a merely blank display with lots of shouty
> > messages.
> >
> > v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
> > In particular, note that we may be called on a PCH platform not using
> > PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
>
> I think that we need to keep in mind that we can have Haswell+PPT
> combination, but I don't know what will happen in this case yet.
>
> So I'd suggest adding a:
>
> WARN_ON(pll == NULL && !HAS_PCH_LPT(dev));
>
> as well somewhere in the checks. What do you think?
Sounds sensible. Care to stitch together that patch?
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN
2012-05-13 20:15 ` Chris Wilson
@ 2012-05-19 21:11 ` Daniel Vetter
0 siblings, 0 replies; 15+ messages in thread
From: Daniel Vetter @ 2012-05-19 21:11 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Sun, May 13, 2012 at 09:15:51PM +0100, Chris Wilson wrote:
> On Sun, 13 May 2012 22:08:10 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Sun, May 13, 2012 at 08:16:12PM +0100, Chris Wilson wrote:
> > > Turn a fatal lockup into a merely blank display with lots of shouty
> > > messages.
> > >
> > > v2: Whilst in the area, convert the other BUG_ON into less fatal errors.
> > > In particular, note that we may be called on a PCH platform not using
> > > PLLs, such as Haswell, and so we do not always want to BUG_ON(!pll)
> > >
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> > Ok, I guess you want me to drop v1 of this again. I'll wait for a bit of
> > feedback on these two then (and the tested-by from qa on the 2nd one).
>
> Up to you, I thought that the original patch was overshadowed by the
> your change request and little reason for it to be split. It would be
> good to get QA's tested-by on the second patch at any rate.
Ok, I've picked up v2 of patch 1 and the 2nd patch. Looks like we still
have an issue somewhere in that code that makes ilk unhappy :(
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2012-05-19 21:10 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-13 8:54 [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Chris Wilson
2012-05-13 8:54 ` [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training Chris Wilson
2012-05-13 14:08 ` Daniel Vetter
2012-05-13 14:25 ` Chris Wilson
2012-05-13 23:35 ` Eugeni Dodonov
2012-05-13 23:39 ` Eugeni Dodonov
2012-05-14 15:12 ` Jesse Barnes
2012-05-14 15:23 ` Chris Wilson
2012-05-13 13:42 ` [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN Daniel Vetter
2012-05-13 19:16 ` [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends " Chris Wilson
2012-05-13 20:08 ` Daniel Vetter
2012-05-13 20:15 ` Chris Wilson
2012-05-19 21:11 ` Daniel Vetter
2012-05-13 23:51 ` Eugeni Dodonov
2012-05-19 21:09 ` Daniel Vetter
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.