diff mbox

Find bugs in i915 driver

Message ID D908B1B9E708C047A32444772B58AE8821306D@SHSMSX101.ccr.corp.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Xu, Anhua July 31, 2012, 9:17 a.m. UTC 
Thanks Chris. I add this in the the commit description. The updated patch is below: 

commit 71c3ff04834a01c81a5843996b87397273eb538d
Author: Xu Anhua <anhua.xu@intel.com>
Date:   Tue Jul 31 17:16:50 2012 +0800

    i915: make the parameters passed-in coherent with functions'
    	definition when calling hdmi/adpa/lvds_pipe_enabled()
    
    This bug is indroduced by commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37


> -----Original Message-----
> From: Chris Wilson [mailto:chris@chris-wilson.co.uk]
> Sent: Tuesday, July 31, 2012 3:58 PM
> To: Xu, Anhua; intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] Find bugs in i915 driver
> 
> On Tue, 31 Jul 2012 02:29:01 +0000, "Xu, Anhua" <anhua.xu@intel.com>
> wrote:
> > Hi, I found some bugs in i915 driver when reviewing intel_display.c The
> parameter passed-in for function hdmi/adpa/lvds_pipe_enabled() are not
> coherent with functions' definition.
> > This is the patch.
> 
> Patch looks good, you might want to mention that the regression was
> introduced with commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37.
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

Comments

Greg KH July 31, 2012, 2:23 p.m. UTC | #1 
On Tue, Jul 31, 2012 at 09:17:15AM +0000, Xu, Anhua wrote:
> Thanks Chris. I add this in the the commit description. The updated patch is below: 
> 
> commit 71c3ff04834a01c81a5843996b87397273eb538d
> Author: Xu Anhua <anhua.xu@intel.com>
> Date:   Tue Jul 31 17:16:50 2012 +0800
> 
>     i915: make the parameters passed-in coherent with functions'
>     	definition when calling hdmi/adpa/lvds_pipe_enabled()
>     
>     This bug is indroduced by commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c

No signed-off-by?  No tag for inclusion in the stable tree?

This patch isn't going very far :(
Paul Menzel Aug. 1, 2012, 9:31 a.m. UTC | #2 
Dear Anhua,


five additional comments to Greg’s ones.


Am Dienstag, den 31.07.2012, 07:23 -0700 schrieb Greg KH:
> On Tue, Jul 31, 2012 at 09:17:15AM +0000, Xu, Anhua wrote:
> > Thanks Chris. I add this in the the commit description. The updated patch is below: 
> > 
> > commit 71c3ff04834a01c81a5843996b87397273eb538d
> > Author: Xu Anhua <anhua.xu@intel.com>

1. Looking at your address in the From field of this message there is
"Xu, Anhua", so I am not sure which is your first name.

If it is Anhua, please do the following.

    git config --global author.name "Anhua Xu"

> > Date:   Tue Jul 31 17:16:50 2012 +0800
> > 
> >     i915: make the parameters passed-in coherent with functions'
> >     	definition when calling hdmi/adpa/lvds_pipe_enabled()
> >     
> >     This bug is indroduced by commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37

1. There is a typo: in*t*roduced. The spell checker of your email
program should mention that. Or you hook up Git to check that [1][2][3].

> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> 
> No signed-off-by?  No tag for inclusion in the stable tree?
> 
> This patch isn't going very far :(

2. Please do not just mention the hash of the commit but also the
summary. Only a few people have memorized all hashes. ;-)

        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

You can use `git commit --amend --author="Anhua Xu" to edit the last
commit.

3. To include the patch you seem to copy the output of `git log` or `git
show`. The recommended way is to use `git format-patch -s -1`. This way
the commit message is indented correctly.

4. Your first message of this thread included a HTML part. Luckily your
reply did not which suggests that your mail program supports writing
just plain text messages. In the future it would be great if you could
write just plain text messages which is the recommended way for mailing
lists [4].


Thanks,

Paul


[1] http://blog.mpdaugherty.com/2010/04/06/how-to-include-git-hooks-in-a-repository-and-still-personalize-your-machine/
[2] http://stackoverflow.com/questions/1691060/vim-set-spell-in-file-git-commit-editmsg
[3] http://petereisentraut.blogspot.de/2011/01/git-commit-mode.html
[4] http://en.opensuse.org/openSUSE:Mailing_list_netiquette
Daniel Vetter Aug. 10, 2012, 11:41 a.m. UTC | #3 
On Tue, Jul 31, 2012 at 07:23:18AM -0700, Greg KH wrote:
> On Tue, Jul 31, 2012 at 09:17:15AM +0000, Xu, Anhua wrote:
> > Thanks Chris. I add this in the the commit description. The updated patch is below: 
> > 
> > commit 71c3ff04834a01c81a5843996b87397273eb538d
> > Author: Xu Anhua <anhua.xu@intel.com>
> > Date:   Tue Jul 31 17:16:50 2012 +0800
> > 
> >     i915: make the parameters passed-in coherent with functions'
> >     	definition when calling hdmi/adpa/lvds_pipe_enabled()
> >     
> >     This bug is indroduced by commit 1519b9956eb4b4180fa3f47c73341463cdcfaa37
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> 
> No signed-off-by?  No tag for inclusion in the stable tree?
> 
> This patch isn't going very far :(

Xu Anhua, can you please update your patch with signed-off-by and cc:
stable? Also, your commit headline is a bit long, it should fit on one
line of at most 70 chars (or thereabouts).
-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);