i915 fixes for review

i915 fixes for review

[Chris Wilson]

Hopefully the culmination of the DDC:0xa0 saga along with another bug
that was also found lurking the VGA regression reports.
-Chris

[PATCH 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Chris Wilson]

We were using uninitialised watermarks values for disabled pipes which
were combined into a single WM register and so corrupting the values for
the enabled pipe and upsetting the display hardware.

Reported-by: Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=32612
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_display.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e1c6da1..f1798f2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3800,8 +3800,10 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	int entries, tlb_miss;
 
 	crtc = intel_get_crtc_for_plane(dev, plane);
-	if (crtc->fb == NULL || !crtc->enabled)
+	if (crtc->fb == NULL || !crtc->enabled) {
+		*cursor_wm = *plane_wm = display->guard_size;
 		return false;
+	}
 
 	htotal = crtc->mode.htotal;
 	hdisplay = crtc->mode.hdisplay;
-- 
1.7.4.1

Re: [PATCH 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 584 bytes --]

On Tue,  5 Apr 2011 10:24:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

>  	crtc = intel_get_crtc_for_plane(dev, plane);
> -	if (crtc->fb == NULL || !crtc->enabled)
> +	if (crtc->fb == NULL || !crtc->enabled) {
> +		*cursor_wm = *plane_wm = display->guard_size;
>  		return false;
> +	}

Would it be clearer to have g4x_update_wm set these instead?

I'm also a bit concerned about the default value; it would be lovely to
have the docs say what the value should be for disabled pipes, but I
couldn't find any mention of them.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Chris Wilson]

On Tue, 05 Apr 2011 13:56:37 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Tue,  5 Apr 2011 10:24:14 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> >  	crtc = intel_get_crtc_for_plane(dev, plane);
> > -	if (crtc->fb == NULL || !crtc->enabled)
> > +	if (crtc->fb == NULL || !crtc->enabled) {
> > +		*cursor_wm = *plane_wm = display->guard_size;
> >  		return false;
> > +	}
> 
> Would it be clearer to have g4x_update_wm set these instead?
> 
> I'm also a bit concerned about the default value; it would be lovely to
> have the docs say what the value should be for disabled pipes, but I
> couldn't find any mention of them.

Indeed, I started by setting them to zero in the caller. Decided that
there was some precedent to use the guard_size as the minimum value for
unused planes (and so perhaps the unused planes on the unused pipes) and
so it was then natural to do it inside g4x_compute_wm. I guess it all
depends on how many FIFOs are split between the pipes. Using guard_size,
I believe, should be safest.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On Tue, 05 Apr 2011 22:12:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Indeed, I started by setting them to zero in the caller. Decided that
> there was some precedent to use the guard_size as the minimum value for
> unused planes (and so perhaps the unused planes on the unused pipes) and
> so it was then natural to do it inside g4x_compute_wm. I guess it all
> depends on how many FIFOs are split between the pipes. Using guard_size,
> I believe, should be safest.

guard_size is probably better than random stack stuff. Any opinion on
whether this should be done in g4x_update_wm instead of g4x_compute_wm0?

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Chris Wilson]

On Tue, 05 Apr 2011 18:02:51 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Tue, 05 Apr 2011 22:12:19 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Indeed, I started by setting them to zero in the caller. Decided that
> > there was some precedent to use the guard_size as the minimum value for
> > unused planes (and so perhaps the unused planes on the unused pipes) and
> > so it was then natural to do it inside g4x_compute_wm. I guess it all
> > depends on how many FIFOs are split between the pipes. Using guard_size,
> > I believe, should be safest.
> 
> guard_size is probably better than random stack stuff. Any opinion on
> whether this should be done in g4x_update_wm instead of g4x_compute_wm0?

I'd prefer to keep the mucking around with intel_watermak_params in the
one spot. How about:

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f1798f2..dbe11eb 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3799,11 +3799,14 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	int line_time_us, line_count;
 	int entries, tlb_miss;
 
+	/* The FIFO requires a minimum number of entries (the guard size)
+	 * as it switches between planes:
+	 */
+	*cursor_wm = cursor->guard_size;
+	*plane_wm = display->guard_size;
+
 	crtc = intel_get_crtc_for_plane(dev, plane);
-	if (crtc->fb == NULL || !crtc->enabled) {
-		*cursor_wm = *plane_wm = display->guard_size;
+	if (crtc->fb == NULL || !crtc->enabled)
 		return false;
-	}
 
 	htotal = crtc->mode.htotal;
 	hdisplay = crtc->mode.hdisplay;
@@ -3816,7 +3819,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	if (tlb_miss > 0)
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, display->cacheline_size);
-	*plane_wm = entries + display->guard_size;
+	*plane_wm += entries;
 	if (*plane_wm > (int)display->max_wm)
 		*plane_wm = display->max_wm;
 
@@ -3828,7 +3831,7 @@ static bool g4x_compute_wm0(struct drm_device *dev,
 	if (tlb_miss > 0)
 		entries += tlb_miss;
 	entries = DIV_ROUND_UP(entries, cursor->cacheline_size);
-	*cursor_wm = entries + cursor->guard_size;
+	*cursor_wm += entries;
 	if (*cursor_wm > (int)cursor->max_wm)
 		*cursor_wm = (int)cursor->max_wm;
 

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 640 bytes --]

On Wed, 06 Apr 2011 07:59:37 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> I'd prefer to keep the mucking around with intel_watermak_params in the
> one spot. How about:

My concern is that g4x_compute_wm0 is now different from
ironlake_compute_wm0, which seems like a potential trap for the
unwary.

> -	*plane_wm = entries + display->guard_size;
> +	*plane_wm += entries;
...
> -	*cursor_wm = entries + cursor->guard_size;
> +	*cursor_wm += entries;

Uh, no. If we find out that '0' is the right value for the off-case, I
don't want to discover that we forgot to reset these...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Chris Wilson]

On Wed, 06 Apr 2011 00:36:50 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Wed, 06 Apr 2011 07:59:37 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > I'd prefer to keep the mucking around with intel_watermak_params in the
> > one spot. How about:
> 
> My concern is that g4x_compute_wm0 is now different from
> ironlake_compute_wm0, which seems like a potential trap for the
> unwary.

A trap that I wrote for myself and fell into. The goal was to reduce the
number of copies of the watermark computation by gradual refactoring.

Looks like we can now indeed merge g4x_compute_wm0 and ironlake_compute_wm0
and ignore the off-values for gen5+.

So fix the use of uninitialised values for -fixes and remove the redundant
copy in -next?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] drm/i915: Initialise g4x watermarks for disabled pipes

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 405 bytes --]

On Wed, 06 Apr 2011 09:02:22 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Looks like we can now indeed merge g4x_compute_wm0 and ironlake_compute_wm0
> and ignore the off-values for gen5+.

They do seem surprisingly similar at this point...

> So fix the use of uninitialised values for -fixes and remove the redundant
> copy in -next?

Sounds good.

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH 2/5] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Chris Wilson]

Rather than proceed on and return false by default, return the result
from detecting a connection but to a digital panel and include some
debug output as to why.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_crt.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 8342259..49ce713 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -308,6 +308,8 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
+		 *
+		 * On the other hand, what should we do if it is a broken EDID?
 		 */
 		if (edid != NULL) {
 			is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
@@ -318,6 +320,8 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 		if (!is_digital) {
 			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
 			return true;
+		} else {
+			DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
 		}
 	}
 
-- 
1.7.4.1

Re: [PATCH 2/5] drm/i915/crt: Explicitly return false if connected to a digital monitor

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 550 bytes --]

On Tue,  5 Apr 2011 10:24:15 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Rather than proceed on and return false by default, return the result
> from detecting a connection but to a digital panel and include some
> debug output as to why.

This comment does not match the patch anymore; there is no new return
point, just an added comment (which doesn't even need to be in an else
block now).
> +		} else {
> +			DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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

[PATCH 3/5] drm/i915/crt: Remove 0xa0 probe for VGA

[Chris Wilson]

This is a moral revert of 6ec3d0c0e9c0c605696e91048eebaca7b0c36695.

Following the fix to reset the GMBUS controller after a NAK, we finally
utilize the 0xa0 probe for a CRT connection. And discover that it the
code is broken. Shock.

There are a number of issues, but following a key insight from Dave
Airlie, that 0xA0 is an invalid address on a 7-bit bus (though not if we
were to enable 10-bit addressing), and would look like the EDID port
0x50, it is possible to see where the confusion starts.

In short, a write to 0xA0 is accepted by the GMBUS controller which we
interpreted as meaning the existence of a connection (a slave on the
other end of the wire ACKing the write). That was false.

During testing with a broken GMBUS implementation, which never reset an
earlier NAK, this test always reported a NAK and so we proceeded on to
the next test.

Reported-and-tested-by: Sitsofe Wheeler <sitsofe@yahoo.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=35904
Reported-and-tested-by:  Riccardo Magliocchetti <riccardo.magliocchetti@gmail.com>
Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=32612
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |   20 --------------------
 1 files changed, 0 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 49ce713..e17bc6b 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -269,21 +269,6 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 	return ret;
 }
 
-static bool intel_crt_ddc_probe(struct drm_i915_private *dev_priv, int ddc_bus)
-{
-	u8 buf;
-	struct i2c_msg msgs[] = {
-		{
-			.addr = 0xA0,
-			.flags = 0,
-			.len = 1,
-			.buf = &buf,
-		},
-	};
-	/* DDC monitor detect: Does it ACK a write to 0xA0? */
-	return i2c_transfer(&dev_priv->gmbus[ddc_bus].adapter, msgs, 1) == 1;
-}
-
 static bool intel_crt_detect_ddc(struct drm_connector *connector)
 {
 	struct intel_crt *crt = intel_attached_crt(connector);
@@ -293,11 +278,6 @@ static bool intel_crt_detect_ddc(struct drm_connector *connector)
 	if (crt->base.type != INTEL_OUTPUT_ANALOG)
 		return false;
 
-	if (intel_crt_ddc_probe(dev_priv, dev_priv->crt_ddc_pin)) {
-		DRM_DEBUG_KMS("CRT detected via DDC:0xa0\n");
-		return true;
-	}
-
 	if (intel_ddc_probe(&crt->base, dev_priv->crt_ddc_pin)) {
 		struct edid *edid;
 		bool is_digital = false;
-- 
1.7.4.1

[PATCH 4/5] drm/i915/lvds: Remove 0xA0 DDC probe for LVDS

[Chris Wilson]

This is a revert of 428d2e828c0a68206e5158a42451487601dc9194.

This is broken in the same manner as for VGA: trying to write to an
invalid address on the (currently 7-bit) i2c bus.

One notable failure appears to be for MacBooks. The scary part was that
it gave the appearance of working (i.e. reporting the absence of the
panel) on various all-in-one machines with ghost LVDS panels and not
failing for laptops.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lvds.c |   24 ------------------------
 1 files changed, 0 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 86cd30b..a562bd2 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -829,25 +829,6 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
 	return false;
 }
 
-static bool intel_lvds_ddc_probe(struct drm_device *dev, u8 pin)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u8 buf = 0;
-	struct i2c_msg msgs[] = {
-		{
-			.addr = 0xA0,
-			.flags = 0,
-			.len = 1,
-			.buf = &buf,
-		},
-	};
-	struct i2c_adapter *i2c = &dev_priv->gmbus[pin].adapter;
-	/* XXX this only appears to work when using GMBUS */
-	if (intel_gmbus_is_forced_bit(i2c))
-		return true;
-	return i2c_transfer(i2c, msgs, 1) == 1;
-}
-
 /**
  * intel_lvds_init - setup LVDS connectors on this device
  * @dev: drm device
@@ -888,11 +869,6 @@ bool intel_lvds_init(struct drm_device *dev)
 		}
 	}
 
-	if (!intel_lvds_ddc_probe(dev, pin)) {
-		DRM_DEBUG_KMS("LVDS did not respond to DDC probe\n");
-		return false;
-	}
-
 	intel_lvds = kzalloc(sizeof(struct intel_lvds), GFP_KERNEL);
 	if (!intel_lvds) {
 		return false;
-- 
1.7.4.1

[PATCH 5/5] drm/i915/gmbus: Reset the controller on initialisation

[Chris Wilson]

Toggle the Software Clear Interrupt bit which resets the controller to
clear any prior BUS_ERROR condition before we begin to use the
controller in earnest.

Suggested-by: Ben Widawsky <ben@bwidawsk.net>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_i2c.c |   14 +++++++++++---
 1 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index d3b903b..f9f0c42 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -55,10 +55,18 @@ void
 intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int reg_offset;
+
+	reg_offset = 0;
 	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(PCH_GMBUS0, 0);
-	else
-		I915_WRITE(GMBUS0, 0);
+		reg_offset = PCH_GMBUS0 - GMBUS0;
+
+	/* First reset the controller by toggling the Sw Clear Interrupt. */
+	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
+	I915_WRITE(GMBUS1 + reg_offset, 0);
+
+	/* Then mark the controller as disabled. */
+	I915_WRITE(GMBUS0 + reg_offset, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
-- 
1.7.4.1

Re: [PATCH 5/5] drm/i915/gmbus: Reset the controller on initialisation

[Ben Widawsky]

On Tue, Apr 05, 2011 at 10:24:18AM +0100, Chris Wilson wrote:
> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.
> 
> Suggested-by: Ben Widawsky <ben@bwidawsk.net>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c |   14 +++++++++++---
>  1 files changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index d3b903b..f9f0c42 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -55,10 +55,18 @@ void
>  intel_i2c_reset(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int reg_offset;
> +
> +	reg_offset = 0;
>  	if (HAS_PCH_SPLIT(dev))
> -		I915_WRITE(PCH_GMBUS0, 0);
> -	else
> -		I915_WRITE(GMBUS0, 0);
> +		reg_offset = PCH_GMBUS0 - GMBUS0;
> +
> +	/* First reset the controller by toggling the Sw Clear Interrupt. */
> +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> +	I915_WRITE(GMBUS1 + reg_offset, 0);
> +
> +	/* Then mark the controller as disabled. */
> +	I915_WRITE(GMBUS0 + reg_offset, 0);
>  }
>  

I think in addition to this we should try to send a STOP condition.
That way any devices in an unknown state from a failed reset (or
something similar), should go back to their default state after seeing a
stop. As long as the host is always the master, and the devices are
fairly compliant, it should work...  Alas, I'm just looking at the GMBUS
interface for the first time really, and it appears there is no straight
forward way to do what I want outside of bitbanging (you want to avoid
toggling SCL until you've issues the STOP).

Outside of being able to do that directly, I would add a call to
intel_i2c_reset() from intel_teardown_gmbus().

Ben

Re: [PATCH 5/5] drm/i915/gmbus: Reset the controller on initialisation

[Ben Widawsky]

On Tue, Apr 05, 2011 at 09:18:37AM -0700, Ben Widawsky wrote:

> Outside of being able to do that directly, I would add a call to
> intel_i2c_reset() from intel_teardown_gmbus().

Hmm, forget that, I think this is silly. How about just a comment in the
code stating that intel_i2c_reset() should always be the first thing
that happens when you bring up the gmbus.

Ben

Re: [PATCH 5/5] drm/i915/gmbus: Reset the controller on initialisation

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 899 bytes --]

On Tue,  5 Apr 2011 10:24:18 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Toggle the Software Clear Interrupt bit which resets the controller to
> clear any prior BUS_ERROR condition before we begin to use the
> controller in earnest.

We do this in two places now, do we want to share the code?

> +	int reg_offset;
> +
> +	reg_offset = 0;
>  	if (HAS_PCH_SPLIT(dev))
> -		I915_WRITE(PCH_GMBUS0, 0);
> -	else
> -		I915_WRITE(GMBUS0, 0);
> +		reg_offset = PCH_GMBUS0 - GMBUS0;
> +
> +	/* First reset the controller by toggling the Sw Clear Interrupt. */
> +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> +	I915_WRITE(GMBUS1 + reg_offset, 0);
> +
> +	/* Then mark the controller as disabled. */
> +	I915_WRITE(GMBUS0 + reg_offset, 0);

That's really ugly register addressing, but it looks like a common idiom
in this file...

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 189 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 5/5] drm/i915/gmbus: Reset the controller on initialisation

[Chris Wilson]

On Tue, 05 Apr 2011 13:59:58 -0700, Keith Packard <keithp@keithp.com> wrote:
> On Tue,  5 Apr 2011 10:24:18 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > Toggle the Software Clear Interrupt bit which resets the controller to
> > clear any prior BUS_ERROR condition before we begin to use the
> > controller in earnest.
> 
> We do this in two places now, do we want to share the code?
> 
> > +	int reg_offset;
> > +
> > +	reg_offset = 0;
> >  	if (HAS_PCH_SPLIT(dev))
> > -		I915_WRITE(PCH_GMBUS0, 0);
> > -	else
> > -		I915_WRITE(GMBUS0, 0);
> > +		reg_offset = PCH_GMBUS0 - GMBUS0;
> > +
> > +	/* First reset the controller by toggling the Sw Clear Interrupt. */
> > +	I915_WRITE(GMBUS1 + reg_offset, GMBUS_SW_CLR_INT);
> > +	I915_WRITE(GMBUS1 + reg_offset, 0);
> > +
> > +	/* Then mark the controller as disabled. */
> > +	I915_WRITE(GMBUS0 + reg_offset, 0);
> 
> That's really ugly register addressing, but it looks like a common idiom
> in this file...

I'd change the lot for a cleaner method, the best I thought of was a
change of names for the constants/variables.

IMO,

static void i915_gmbus_write(struct drm_device *dev, int reg, int value)
{
	struct drm_i915_private *dev_priv = dev->dev_private;
	I915_WRITE(reg + (HAS_PCH_SPLIT(dev) ? PCH_GMBUS0 : GMBUS0), value);
}

expands to something dreadful.

But with a

#define GMBUS_WRITE(reg, value) i915_gmbus_write(dev, reg, value)

we go from 

  I915_WRITE(GMBUS0 + reg_offset, 0);

to

  GMBUS_WRITE(0, 0);

I would still prefer GMBUS_WRITE(GMBUS0, 0); though.

As the patch only addresses a theoretical bug, we can punt the meat of the
patch till later and attack the stylistic points first. (Obviously through
-next.)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre