All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status
@ 2012-09-05 19:24 Daniel Vetter
  2012-09-05 19:24 ` [PATCH 2/3] drm/i915: wire up gmbus irq handler Daniel Vetter
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-09-05 19:24 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

The gmbus interrupt generation is rather fiddly: We can only ever
enable one interrupt source (but we always want to check for NAK
in addition to the real bit). And the bits in the gmbus status
register don't map at all to the bis in the irq register.

To prepare for this mess, start by extracting the hw status wait
loop into it's own function, consolidate the NAK error handling a
bit. To keep things flexible, pass in the status bit we care about
(in addition to any NAK signalling).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_i2c.c | 52 +++++++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b9755f6..3a90b87 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,6 +204,24 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 }
 
 static int
+gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
+		     u32 gmbus2_status)
+{
+	int ret;
+	int reg_offset = dev_priv->gpio_mmio_base;
+	u32 gmbus2;
+
+	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
+		       (GMBUS_SATOER | gmbus2_status),
+		       50);
+
+	if (gmbus2 & GMBUS_SATOER)
+		return -ENXIO;
+
+	return ret;
+}
+
+static int
 gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		u32 gmbus1_index)
 {
@@ -220,15 +238,10 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 	while (len) {
 		int ret;
 		u32 val, loop = 0;
-		u32 gmbus2;
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_RDY),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
 		if (ret)
-			return -ETIMEDOUT;
-		if (gmbus2 & GMBUS_SATOER)
-			return -ENXIO;
+			return ret;
 
 		val = I915_READ(GMBUS3 + reg_offset);
 		do {
@@ -262,7 +275,6 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
-		u32 gmbus2;
 
 		val = loop = 0;
 		do {
@@ -271,13 +283,9 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_RDY),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
 		if (ret)
-			return -ETIMEDOUT;
-		if (gmbus2 & GMBUS_SATOER)
-			return -ENXIO;
+			return ret;
 	}
 	return 0;
 }
@@ -346,8 +354,6 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	I915_WRITE(GMBUS0 + reg_offset, bus->reg0);
 
 	for (i = 0; i < num; i++) {
-		u32 gmbus2;
-
 		if (gmbus_is_index_read(msgs, i, num)) {
 			ret = gmbus_xfer_index_read(dev_priv, &msgs[i]);
 			i += 1;  /* set i to the index of the read xfer */
@@ -362,13 +368,11 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-			       (GMBUS_SATOER | GMBUS_HW_WAIT_PHASE),
-			       50);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
+		if (ret == -ENXIO)
+			goto clear_err;
 		if (ret)
 			goto timeout;
-		if (gmbus2 & GMBUS_SATOER)
-			goto clear_err;
 	}
 
 	/* Generate a STOP condition on the bus. Note that gmbus can't generata
@@ -381,8 +385,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
 		ret = -ETIMEDOUT;
@@ -406,8 +409,7 @@ clear_err:
 	 * it's slow responding and only answers on the 2nd retry.
 	 */
 	ret = -ENXIO;
-	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
-		     10)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
 		ret = -ETIMEDOUT;
-- 
1.7.11.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/3] drm/i915: wire up gmbus irq handler
  2012-09-05 19:24 [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
@ 2012-09-05 19:24 ` Daniel Vetter
  2012-09-05 21:29   ` Chris Wilson
  2012-09-05 19:24 ` [PATCH 3/3] drm/i915: use the gmbus irq for waits Daniel Vetter
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-09-05 19:24 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Only enables the interrupt and puts a irq handler into place, doesn't
do anything yet.

Unfortunately there's no gmbus interrupt support for gen2/3 (safe for
pnv, but there the irq is marked as "Test mode").

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index a61b41a..8415fa6 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -596,6 +596,11 @@ out:
 	return ret;
 }
 
+static void gmbus_irq_handler(struct drm_device *dev)
+{
+	DRM_DEBUG_DRIVER("GMBUS interrupt\n");
+}
+
 static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 {
 	drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
@@ -607,7 +612,7 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
 				 SDE_AUDIO_POWER_SHIFT);
 
 	if (pch_iir & SDE_GMBUS)
-		DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n");
+		gmbus_irq_handler(dev);
 
 	if (pch_iir & SDE_AUDIO_HDCP_MASK)
 		DRM_DEBUG_DRIVER("PCH HDCP audio interrupt\n");
@@ -650,7 +655,7 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
 		DRM_DEBUG_DRIVER("AUX channel interrupt\n");
 
 	if (pch_iir & SDE_GMBUS_CPT)
-		DRM_DEBUG_DRIVER("PCH GMBUS interrupt\n");
+		gmbus_irq_handler(dev);
 
 	if (pch_iir & SDE_AUDIO_CP_REQ_CPT)
 		DRM_DEBUG_DRIVER("Audio CP request interrupt\n");
@@ -1841,12 +1846,14 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
 		hotplug_mask = (SDE_CRT_HOTPLUG_CPT |
 				SDE_PORTB_HOTPLUG_CPT |
 				SDE_PORTC_HOTPLUG_CPT |
-				SDE_PORTD_HOTPLUG_CPT);
+				SDE_PORTD_HOTPLUG_CPT |
+				SDE_GMBUS_CPT);
 	} else {
 		hotplug_mask = (SDE_CRT_HOTPLUG |
 				SDE_PORTB_HOTPLUG |
 				SDE_PORTC_HOTPLUG |
 				SDE_PORTD_HOTPLUG |
+				SDE_GMBUS |
 				SDE_AUX_MASK);
 	}
 
@@ -1906,7 +1913,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
 	hotplug_mask = (SDE_CRT_HOTPLUG_CPT |
 			SDE_PORTB_HOTPLUG_CPT |
 			SDE_PORTC_HOTPLUG_CPT |
-			SDE_PORTD_HOTPLUG_CPT);
+			SDE_PORTD_HOTPLUG_CPT |
+			SDE_GMBUS_CPT);
 	dev_priv->pch_irq_mask = ~hotplug_mask;
 
 	I915_WRITE(SDEIIR, I915_READ(SDEIIR));
@@ -1959,6 +1967,7 @@ static int valleyview_irq_postinstall(struct drm_device *dev)
 	POSTING_READ(VLV_IER);
 
 	i915_enable_pipestat(dev_priv, 0, pipestat_enable);
+	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS);
 	i915_enable_pipestat(dev_priv, 1, pipestat_enable);
 
 	I915_WRITE(VLV_IIR, 0xffffffff);
@@ -2454,6 +2463,7 @@ static int i965_irq_postinstall(struct drm_device *dev)
 
 	dev_priv->pipestat[0] = 0;
 	dev_priv->pipestat[1] = 0;
+	i915_enable_pipestat(dev_priv, 0, PIPE_GMBUS_INTERRUPT_STATUS);
 
 	/*
 	 * Enable some error detection, note the instruction error mask
-- 
1.7.11.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 3/3] drm/i915: use the gmbus irq for waits
  2012-09-05 19:24 [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
  2012-09-05 19:24 ` [PATCH 2/3] drm/i915: wire up gmbus irq handler Daniel Vetter
@ 2012-09-05 19:24 ` Daniel Vetter
  2012-09-05 21:34   ` Chris Wilson
  2012-09-05 21:25 ` [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Chris Wilson
  2012-09-05 22:12 ` Chris Wilson
  3 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-09-05 19:24 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

We need two special things to properly wire this up:
- Add another argument to gmbus_wait_hw_status to pass in the
  correct interrupt bit in gmbus4.
- Since we can only get an irq for one of the two events we want,
  hand-roll the wait_event_timeout code so that we wake up every
  jiffie and can check for NAKs. This way we also subsume gmbus
  support for platforms without interrupts (or where those are not
  yet enabled).

The important bit really is to only enable one gmbus interrupt source
at the same time - with that piece of lore figured out, this seems to
work flawlessly.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c  |  4 ++++
 drivers/gpu/drm/i915/intel_i2c.c | 46 ++++++++++++++++++++++++++++------------
 3 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9fce782..d93ce14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -418,6 +418,8 @@ typedef struct drm_i915_private {
 	 */
 	uint32_t gpio_mmio_base;
 
+	wait_queue_head_t gmbus_wait_queue;
+
 	struct pci_dev *bridge_dev;
 	struct intel_ring_buffer ring[I915_NUM_RINGS];
 	uint32_t next_seqno;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 8415fa6..dadc86b 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -598,7 +598,11 @@ out:
 
 static void gmbus_irq_handler(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
 	DRM_DEBUG_DRIVER("GMBUS interrupt\n");
+
+	wake_up_all(&dev_priv->gmbus_wait_queue);
 }
 
 static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 3a90b87..4d9df5e 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -205,20 +205,36 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
 
 static int
 gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
-		     u32 gmbus2_status)
+		     u32 gmbus2_status,
+		     u32 gmbus4_irq_en)
 {
-	int ret;
+	int i;
 	int reg_offset = dev_priv->gpio_mmio_base;
-	u32 gmbus2;
+	u32 gmbus2 = 0;
+	DEFINE_WAIT(wait);
+
+	/* Important: The hw handles only the first bit, so set only one! */
+	I915_WRITE(GMBUS4 + reg_offset, gmbus4_irq_en);
 
-	ret = wait_for((gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
-		       (GMBUS_SATOER | gmbus2_status),
-		       50);
+	for (i = 0; i < msecs_to_jiffies(50) + 1; i++) {
+		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
+				TASK_UNINTERRUPTIBLE);
+
+		gmbus2 = I915_READ(GMBUS2 + reg_offset);
+		if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
+			break;
+
+		schedule_timeout(1);
+	}
+	finish_wait(&dev_priv->gmbus_wait_queue, &wait);
+
+	I915_WRITE(GMBUS4 + reg_offset, 0);
 
 	if (gmbus2 & GMBUS_SATOER)
 		return -ENXIO;
-
-	return ret;
+	if (gmbus2 & gmbus2_status)
+		return 0;
+	return -ETIMEDOUT;
 }
 
 static int
@@ -239,7 +255,8 @@ gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
 		int ret;
 		u32 val, loop = 0;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 
@@ -283,7 +300,8 @@ gmbus_xfer_write(struct drm_i915_private *dev_priv, struct i2c_msg *msg)
 
 		I915_WRITE(GMBUS3 + reg_offset, val);
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
+					   GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 	}
@@ -368,7 +386,8 @@ gmbus_xfer(struct i2c_adapter *adapter,
 		if (ret == -ENXIO)
 			goto clear_err;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE);
+		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
+					   GMBUS_HW_WAIT_EN);
 		if (ret == -ENXIO)
 			goto clear_err;
 		if (ret)
@@ -385,7 +404,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
 	 * We will re-enable it at the start of the next xfer,
 	 * till then let it sleep.
 	 */
-	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE, GMBUS_IDLE_EN)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
 			 adapter->name);
 		ret = -ETIMEDOUT;
@@ -409,7 +428,7 @@ clear_err:
 	 * it's slow responding and only answers on the 2nd retry.
 	 */
 	ret = -ENXIO;
-	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {
+	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE, GMBUS_IDLE_EN)) {
 		DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
 			      adapter->name);
 		ret = -ETIMEDOUT;
@@ -472,6 +491,7 @@ int intel_setup_gmbus(struct drm_device *dev)
 		dev_priv->gpio_mmio_base = 0;
 
 	mutex_init(&dev_priv->gmbus_mutex);
+	init_waitqueue_head(&dev_priv->gmbus_wait_queue);
 
 	for (i = 0; i < GMBUS_NUM_PORTS; i++) {
 		struct intel_gmbus *bus = &dev_priv->gmbus[i];
-- 
1.7.11.2

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status
  2012-09-05 19:24 [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
  2012-09-05 19:24 ` [PATCH 2/3] drm/i915: wire up gmbus irq handler Daniel Vetter
  2012-09-05 19:24 ` [PATCH 3/3] drm/i915: use the gmbus irq for waits Daniel Vetter
@ 2012-09-05 21:25 ` Chris Wilson
  2012-09-05 21:30   ` Daniel Vetter
  2012-09-05 22:12 ` Chris Wilson
  3 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-09-05 21:25 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> The gmbus interrupt generation is rather fiddly: We can only ever
> enable one interrupt source (but we always want to check for NAK
> in addition to the real bit). And the bits in the gmbus status
> register don't map at all to the bis in the irq register.
> 
> To prepare for this mess, start by extracting the hw status wait
> loop into it's own function, consolidate the NAK error handling a
> bit. To keep things flexible, pass in the status bit we care about
> (in addition to any NAK signalling).

There are some subtle changes in that we introduce new error detection
which is promptly ignored with a different wait period, but the changes
look good.
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] drm/i915: wire up gmbus irq handler
  2012-09-05 19:24 ` [PATCH 2/3] drm/i915: wire up gmbus irq handler Daniel Vetter
@ 2012-09-05 21:29   ` Chris Wilson
  2012-09-05 21:36     ` Daniel Vetter
  0 siblings, 1 reply; 10+ messages in thread
From: Chris Wilson @ 2012-09-05 21:29 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

On Wed,  5 Sep 2012 21:24:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> Only enables the interrupt and puts a irq handler into place, doesn't
> do anything yet.
> 
> Unfortunately there's no gmbus interrupt support for gen2/3 (safe for
> pnv, but there the irq is marked as "Test mode").

The basics look good, but the paranoia says I'd like for the interrupt
to only be enabled as required.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status
  2012-09-05 21:25 ` [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Chris Wilson
@ 2012-09-05 21:30   ` Daniel Vetter
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2012-09-05 21:30 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel Graphics Development

On Wed, Sep 5, 2012 at 11:25 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> The gmbus interrupt generation is rather fiddly: We can only ever
>> enable one interrupt source (but we always want to check for NAK
>> in addition to the real bit). And the bits in the gmbus status
>> register don't map at all to the bis in the irq register.
>>
>> To prepare for this mess, start by extracting the hw status wait
>> loop into it's own function, consolidate the NAK error handling a
>> bit. To keep things flexible, pass in the status bit we care about
>> (in addition to any NAK signalling).
>
> There are some subtle changes in that we introduce new error detection
> which is promptly ignored with a different wait period, but the changes
> look good.

Hm, I've tried to match the old code (at least where there are no
interrupts, i.e. gen2/3) as closely as possible. Hence the
schedule_timeout(1) instead of a simple wait_event_timeout. And the
error return values /should/ match what has been there before.

The only thing that changes is that I use 50ms for the timeout
everywhere - I don't see any reason why we should use something else.
So can you please elaborate for I don't really see the change in error
handling?
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 3/3] drm/i915: use the gmbus irq for waits
  2012-09-05 19:24 ` [PATCH 3/3] drm/i915: use the gmbus irq for waits Daniel Vetter
@ 2012-09-05 21:34   ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2012-09-05 21:34 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

On Wed,  5 Sep 2012 21:24:41 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> We need two special things to properly wire this up:
> - Add another argument to gmbus_wait_hw_status to pass in the
>   correct interrupt bit in gmbus4.
> - Since we can only get an irq for one of the two events we want,
>   hand-roll the wait_event_timeout code so that we wake up every
>   jiffie and can check for NAKs. This way we also subsume gmbus
>   support for platforms without interrupts (or where those are not
>   yet enabled).
> 
> The important bit really is to only enable one gmbus interrupt source
> at the same time - with that piece of lore figured out, this seems to
> work flawlessly.

Outline looks good. Did have a worry over the conversion to schedule(1),
but then realised that is equivalent to the current msleep. So there is
a potential that we need to do better for gen2/3.

Need to dig a little more, so just a-b for now.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] drm/i915: wire up gmbus irq handler
  2012-09-05 21:29   ` Chris Wilson
@ 2012-09-05 21:36     ` Daniel Vetter
  2012-09-05 21:52       ` Chris Wilson
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2012-09-05 21:36 UTC (permalink / raw)
  To: Chris Wilson; +Cc: Intel Graphics Development

On Wed, Sep 5, 2012 at 11:29 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed,  5 Sep 2012 21:24:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> Only enables the interrupt and puts a irq handler into place, doesn't
>> do anything yet.
>>
>> Unfortunately there's no gmbus interrupt support for gen2/3 (safe for
>> pnv, but there the irq is marked as "Test mode").
>
> The basics look good, but the paranoia says I'd like for the interrupt
> to only be enabled as required.

I do that ;-) The real interrupt generation is also controlled by
GMBUS4 - as long as that's 0, no interrupt shows up anywhere. And
frobbing GMBUS4 is much easier than adding a bunch of of spinlocks
around frobbing SDE_IIR ...
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/3] drm/i915: wire up gmbus irq handler
  2012-09-05 21:36     ` Daniel Vetter
@ 2012-09-05 21:52       ` Chris Wilson
  0 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2012-09-05 21:52 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Intel Graphics Development

On Wed, 5 Sep 2012 23:36:46 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Wed, Sep 5, 2012 at 11:29 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Wed,  5 Sep 2012 21:24:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> >> Only enables the interrupt and puts a irq handler into place, doesn't
> >> do anything yet.
> >>
> >> Unfortunately there's no gmbus interrupt support for gen2/3 (safe for
> >> pnv, but there the irq is marked as "Test mode").
> >
> > The basics look good, but the paranoia says I'd like for the interrupt
> > to only be enabled as required.
> 
> I do that ;-) The real interrupt generation is also controlled by
> GMBUS4 - as long as that's 0, no interrupt shows up anywhere. And
> frobbing GMBUS4 is much easier than adding a bunch of of spinlocks
> around frobbing SDE_IIR ...

Indeed, killing it at source is preferrable all round. Are you sure
GMBUS4 is always initialised to zero?

So this patch looks sound,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status
  2012-09-05 19:24 [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
                   ` (2 preceding siblings ...)
  2012-09-05 21:25 ` [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Chris Wilson
@ 2012-09-05 22:12 ` Chris Wilson
  3 siblings, 0 replies; 10+ messages in thread
From: Chris Wilson @ 2012-09-05 22:12 UTC (permalink / raw)
  To: Intel Graphics Development; +Cc: Daniel Vetter

Who wrote wait_for()? It is a deadly booby trap!

On Wed,  5 Sep 2012 21:24:39 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> -	if (wait_for((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0,
> -		     10)) {
> +	if (gmbus_wait_hw_status(dev_priv, GMBUS_ACTIVE)) {

This is reversing the status being waited for. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2012-09-05 22:12 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-05 19:24 [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
2012-09-05 19:24 ` [PATCH 2/3] drm/i915: wire up gmbus irq handler Daniel Vetter
2012-09-05 21:29   ` Chris Wilson
2012-09-05 21:36     ` Daniel Vetter
2012-09-05 21:52       ` Chris Wilson
2012-09-05 19:24 ` [PATCH 3/3] drm/i915: use the gmbus irq for waits Daniel Vetter
2012-09-05 21:34   ` Chris Wilson
2012-09-05 21:25 ` [PATCH 1/3] drm/i915: extract gmbus_wait_hw_status Chris Wilson
2012-09-05 21:30   ` Daniel Vetter
2012-09-05 22:12 ` Chris Wilson

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.