diff mbox

Find bugs in i915 driver

Message ID D908B1B9E708C047A32444772B58AE8821565A@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Xu, Anhua Aug. 13, 2012, 8:07 a.m. UTC 
Hi, Paul

Thanks for your advice. I update my patch. Please review, for your question, please see my reply below. 

From d11080eda81c0503b5035ea40667b06fe2ee0fb5 Mon Sep 17 00:00:00 2001
From: Anhua Xu <anhua.xu@intel.com>
Date: Tue, 31 Jul 2012 17:16:50 +0800
Subject: [PATCH v3] drm/i915: fix wrong order of parameters in port checking functions

Wrong order of parameters passed-in when calling hdmi/adpa
/lvds_pipe_enabled(), 2nd and 3rd parameters are reversed.
This bug was indroduced by below commit:

commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
Author: Keith Packard <keithp@keithp.com>
Date:   Sat Aug 6 10:35:34 2011 -0700

    drm/i915: Fix PCH port pipe select in CPT disable paths

The reachable tag for this commit is v3.1-rc1-3-g1519b99

Signed-off-by: Anhua Xu <anhua.xu@intel.com>
Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
---
 drivers/gpu/drm/i915/intel_display.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

Comments

Daniel Vetter Aug. 21, 2012, 7:20 a.m. UTC | #1 
On Mon, Aug 13, 2012 at 08:07:43AM +0000, Xu, Anhua wrote:
> Hi, Paul
> 
> Thanks for your advice. I update my patch. Please review, for your question, please see my reply below. 
> 
> From d11080eda81c0503b5035ea40667b06fe2ee0fb5 Mon Sep 17 00:00:00 2001
> From: Anhua Xu <anhua.xu@intel.com>
> Date: Tue, 31 Jul 2012 17:16:50 +0800
> Subject: [PATCH v3] drm/i915: fix wrong order of parameters in port checking functions
> 
> Wrong order of parameters passed-in when calling hdmi/adpa
> /lvds_pipe_enabled(), 2nd and 3rd parameters are reversed.
> This bug was indroduced by below commit:
> 
> commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> Author: Keith Packard <keithp@keithp.com>
> Date:   Sat Aug 6 10:35:34 2011 -0700
> 
>     drm/i915: Fix PCH port pipe select in CPT disable paths
> 
> The reachable tag for this commit is v3.1-rc1-3-g1519b99
> 
> Signed-off-by: Anhua Xu <anhua.xu@intel.com>
> Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>

I've just remembered that we have a bug report that bisects to the above
commit:

https://bugs.freedesktop.org/show_bug.cgi?id=44876

On a quick check, the symptoms seem to match your fix here ...
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   12 ++++++------
>  1 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index f615976..5fc8c8d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1383,7 +1383,7 @@ static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
>  				     enum pipe pipe, int reg)
>  {
>  	u32 val = I915_READ(reg);
> -	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
> +	WARN(hdmi_pipe_enabled(dev_priv, pipe, val),
>  	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
>  	     reg, pipe_name(pipe));
>  
> @@ -1403,13 +1403,13 @@ static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
>  
>  	reg = PCH_ADPA;
>  	val = I915_READ(reg);
> -	WARN(adpa_pipe_enabled(dev_priv, val, pipe),
> +	WARN(adpa_pipe_enabled(dev_priv, pipe, val),
>  	     "PCH VGA enabled on transcoder %c, should be disabled\n",
>  	     pipe_name(pipe));
>  
>  	reg = PCH_LVDS;
>  	val = I915_READ(reg);
> -	WARN(lvds_pipe_enabled(dev_priv, val, pipe),
> +	WARN(lvds_pipe_enabled(dev_priv, pipe, val),
>  	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
>  	     pipe_name(pipe));
>  
> @@ -1871,7 +1871,7 @@ static void disable_pch_hdmi(struct drm_i915_private *dev_priv,
>  			     enum pipe pipe, int reg)
>  {
>  	u32 val = I915_READ(reg);
> -	if (hdmi_pipe_enabled(dev_priv, val, pipe)) {
> +	if (hdmi_pipe_enabled(dev_priv, pipe, val)) {
>  		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
>  			      reg, pipe);
>  		I915_WRITE(reg, val & ~PORT_ENABLE);
> @@ -1893,12 +1893,12 @@ static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
>  
>  	reg = PCH_ADPA;
>  	val = I915_READ(reg);
> -	if (adpa_pipe_enabled(dev_priv, val, pipe))
> +	if (adpa_pipe_enabled(dev_priv, pipe, val))
>  		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
>  
>  	reg = PCH_LVDS;
>  	val = I915_READ(reg);
> -	if (lvds_pipe_enabled(dev_priv, val, pipe)) {
> +	if (lvds_pipe_enabled(dev_priv, pipe, val)) {
>  		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val);
>  		I915_WRITE(reg, val & ~LVDS_PORT_EN);
>  		POSTING_READ(reg);
> -- 
> 1.7.1
> 
> 
> > -----Original Message-----
> > From: Paul Menzel [mailto:paulepanter@users.sourceforge.net]
> > Sent: Monday, August 13, 2012 3:11 PM
> > To: Xu, Anhua
> > Cc: Daniel Vetter; Greg KH; intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] Find bugs in i915 driver
> > 
> > Am Montag, den 13.08.2012, 03:08 +0000 schrieb Xu, Anhua:
> > > Sorry, Deniel/Greg, late response for your email because of a busy work last
> > work.
> > > I will response to you guys ASAP :), below is the updated patch:
> > >
> > >
> > > From 33eb95a3a711b2354985361ff208ea150a5ba235 Mon Sep 17 00:00:00
> > 2001
> > > From: Xu Anhua <anhua.xu@intel.com>
> > 
> > If Anhua is your first name your name is still switched here.
> > 
> > Please do the following.
> > 
> >     git commit --amend --author="Anhua Xu <anhua.xu@intel.com>"
> > 
> > > Date: Tue, 31 Jul 2012 17:16:50 +0800
> > > Subject: [PATCH] drm/i915: fix wrong order of parameters in port
> > > checking functions
> > >
> > > Wrong order of parameters passed-in when calling hdmi/adpa
> > > /lvds_pipe_enabled(), 2nd and 3rd parameters are reversed.
> > > This bug was indroduced by commit
> > > 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> > 
> > Since it is hard to remember commit hashes, you should add the following
> > summary
> > 
> >         commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> >         Author: Keith Packard <keithp@keithp.com>
> >         Date:   Sat Aug 6 10:35:34 2011 -0700
> > 
> >             drm/i915: Fix PCH port pipe select in CPT disable paths
> > 
> > or just the following.
> > 
> >         1519b995 drm/i915: Fix PCH port pipe select in CPT disable paths
> > 
> > > The reachable tag for this commit is v3.1-rc1-3-g1519b99
> > 
> > Then this should be sent to stable [1] too?
> > 
> >     Cc: <stable@vger.kernel.org>
> > 
> > Does this actually fix a bug you are seeing or did you just spot this reading the
> > code?
> 
> No, I did not met a bug before fixing these issue. This bug was found when I referenced i915 driver code.
> But I think these issue can cause potential bugs someday anyway.  
> 
> 
> 
> 
> > > Signed-off-by: Anhua Xu <anhua.xu@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   12 ++++++------
> > >  1 files changed, 6 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index f615976..5fc8c8d 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -1383,7 +1383,7 @@ static void assert_pch_hdmi_disabled(struct
> > drm_i915_private *dev_priv,
> > >  				     enum pipe pipe, int reg)
> > >  {
> > >  	u32 val = I915_READ(reg);
> > > -	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
> > > +	WARN(hdmi_pipe_enabled(dev_priv, pipe, val),
> > >  	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be
> > disabled\n",
> > >  	     reg, pipe_name(pipe));
> > >
> > > @@ -1403,13 +1403,13 @@ static void assert_pch_ports_disabled(struct
> > > drm_i915_private *dev_priv,
> > >
> > >  	reg = PCH_ADPA;
> > >  	val = I915_READ(reg);
> > > -	WARN(adpa_pipe_enabled(dev_priv, val, pipe),
> > > +	WARN(adpa_pipe_enabled(dev_priv, pipe, val),
> > >  	     "PCH VGA enabled on transcoder %c, should be disabled\n",
> > >  	     pipe_name(pipe));
> > >
> > >  	reg = PCH_LVDS;
> > >  	val = I915_READ(reg);
> > > -	WARN(lvds_pipe_enabled(dev_priv, val, pipe),
> > > +	WARN(lvds_pipe_enabled(dev_priv, pipe, val),
> > >  	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
> > >  	     pipe_name(pipe));
> > >
> > > @@ -1871,7 +1871,7 @@ static void disable_pch_hdmi(struct
> > drm_i915_private *dev_priv,
> > >  			     enum pipe pipe, int reg)
> > >  {
> > >  	u32 val = I915_READ(reg);
> > > -	if (hdmi_pipe_enabled(dev_priv, val, pipe)) {
> > > +	if (hdmi_pipe_enabled(dev_priv, pipe, val)) {
> > >  		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
> > >  			      reg, pipe);
> > >  		I915_WRITE(reg, val & ~PORT_ENABLE); @@ -1893,12 +1893,12
> > @@ static
> > > void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
> > >
> > >  	reg = PCH_ADPA;
> > >  	val = I915_READ(reg);
> > > -	if (adpa_pipe_enabled(dev_priv, val, pipe))
> > > +	if (adpa_pipe_enabled(dev_priv, pipe, val))
> > >  		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
> > >
> > >  	reg = PCH_LVDS;
> > >  	val = I915_READ(reg);
> > > -	if (lvds_pipe_enabled(dev_priv, val, pipe)) {
> > > +	if (lvds_pipe_enabled(dev_priv, pipe, val)) {
> > >  		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe,
> > val);
> > >  		I915_WRITE(reg, val & ~LVDS_PORT_EN);
> > >  		POSTING_READ(reg);
> > 
> > With the changes addressed above, please add
> > 
> >     Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
> > 
> > when sending [PATCH v3] as documented in [2].
> > 
> > 
> > Thanks,
> > 
> > Paul
> > 
> > 
> > [1]
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=Document
> > ation/stable_kernel_rules.txt;h=b0714d8f678ac51d0c280a4f5f2980196052421
> > f;hb=HEAD
> > [2]
> > http://wireless.kernel.org/en/developers/Documentation/git-guide#Annotatin
> > g_new_revision
Daniel Vetter Aug. 22, 2012, 8:38 a.m. UTC | #2 
On Tue, Aug 21, 2012 at 09:20:37AM +0200, Daniel Vetter wrote:
> On Mon, Aug 13, 2012 at 08:07:43AM +0000, Xu, Anhua wrote:
> > Hi, Paul
> > 
> > Thanks for your advice. I update my patch. Please review, for your question, please see my reply below. 
> > 
> > From d11080eda81c0503b5035ea40667b06fe2ee0fb5 Mon Sep 17 00:00:00 2001
> > From: Anhua Xu <anhua.xu@intel.com>
> > Date: Tue, 31 Jul 2012 17:16:50 +0800
> > Subject: [PATCH v3] drm/i915: fix wrong order of parameters in port checking functions
> > 
> > Wrong order of parameters passed-in when calling hdmi/adpa
> > /lvds_pipe_enabled(), 2nd and 3rd parameters are reversed.
> > This bug was indroduced by below commit:
> > 
> > commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> > Author: Keith Packard <keithp@keithp.com>
> > Date:   Sat Aug 6 10:35:34 2011 -0700
> > 
> >     drm/i915: Fix PCH port pipe select in CPT disable paths
> > 
> > The reachable tag for this commit is v3.1-rc1-3-g1519b99
> > 
> > Signed-off-by: Anhua Xu <anhua.xu@intel.com>
> > Acked-by: Paul Menzel <paulepanter@users.sourceforge.net>
> 
> I've just remembered that we have a bug report that bisects to the above
> commit:
> 
> https://bugs.freedesktop.org/show_bug.cgi?id=44876

And this does indeeed fix the bug! I've moved the patch to -fixes, with cc
stable added.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f615976..5fc8c8d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1383,7 +1383,7 @@  static void assert_pch_hdmi_disabled(struct drm_i915_private *dev_priv,
 				     enum pipe pipe, int reg)
 {
 	u32 val = I915_READ(reg);
-	WARN(hdmi_pipe_enabled(dev_priv, val, pipe),
+	WARN(hdmi_pipe_enabled(dev_priv, pipe, val),
 	     "PCH HDMI (0x%08x) enabled on transcoder %c, should be disabled\n",
 	     reg, pipe_name(pipe));
 
@@ -1403,13 +1403,13 @@  static void assert_pch_ports_disabled(struct drm_i915_private *dev_priv,
 
 	reg = PCH_ADPA;
 	val = I915_READ(reg);
-	WARN(adpa_pipe_enabled(dev_priv, val, pipe),
+	WARN(adpa_pipe_enabled(dev_priv, pipe, val),
 	     "PCH VGA enabled on transcoder %c, should be disabled\n",
 	     pipe_name(pipe));
 
 	reg = PCH_LVDS;
 	val = I915_READ(reg);
-	WARN(lvds_pipe_enabled(dev_priv, val, pipe),
+	WARN(lvds_pipe_enabled(dev_priv, pipe, val),
 	     "PCH LVDS enabled on transcoder %c, should be disabled\n",
 	     pipe_name(pipe));
 
@@ -1871,7 +1871,7 @@  static void disable_pch_hdmi(struct drm_i915_private *dev_priv,
 			     enum pipe pipe, int reg)
 {
 	u32 val = I915_READ(reg);
-	if (hdmi_pipe_enabled(dev_priv, val, pipe)) {
+	if (hdmi_pipe_enabled(dev_priv, pipe, val)) {
 		DRM_DEBUG_KMS("Disabling pch HDMI %x on pipe %d\n",
 			      reg, pipe);
 		I915_WRITE(reg, val & ~PORT_ENABLE);
@@ -1893,12 +1893,12 @@  static void intel_disable_pch_ports(struct drm_i915_private *dev_priv,
 
 	reg = PCH_ADPA;
 	val = I915_READ(reg);
-	if (adpa_pipe_enabled(dev_priv, val, pipe))
+	if (adpa_pipe_enabled(dev_priv, pipe, val))
 		I915_WRITE(reg, val & ~ADPA_DAC_ENABLE);
 
 	reg = PCH_LVDS;
 	val = I915_READ(reg);
-	if (lvds_pipe_enabled(dev_priv, val, pipe)) {
+	if (lvds_pipe_enabled(dev_priv, pipe, val)) {
 		DRM_DEBUG_KMS("disable lvds on pipe %d val 0x%08x\n", pipe, val);
 		I915_WRITE(reg, val & ~LVDS_PORT_EN);
 		POSTING_READ(reg);