[PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN

[PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN

[Chris Wilson]

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

[PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Daniel Vetter]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Chris Wilson]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Eugeni Dodonov]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Eugeni Dodonov]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Jesse Barnes]

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

Re: [PATCH 2/2] drm/i915: Enable the PCH PLL for all generations after link training

[Chris Wilson]

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

Re: [PATCH 1/2] drm/i915: Convert BUG_ON(!pll->active) to a WARN

[Daniel Vetter]

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

[PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Chris Wilson]

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

Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Chris Wilson]

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

Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Daniel Vetter]

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

Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Eugeni Dodonov]

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

Re: [PATCH] drm/i915: Convert BUG_ON(!pll->active) and friends to a WARN

[Daniel Vetter]

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