[PATCH 1/2] drm/i915: Take forcewake once for the entire GMBUS transaction

[PATCH 1/2] drm/i915: Take forcewake once for the entire GMBUS transaction

[Chris Wilson]

As we do many register reads within a very short period of time, hold
the GMBUS powerwell from start to finish.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 131 +++++++++++++++++++--------------------
 1 file changed, 63 insertions(+), 68 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 1f266d7df2ec..6841c41281a3 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -255,67 +255,59 @@ intel_gpio_setup(struct intel_gmbus *bus, unsigned int pin)
 	algo->data = bus;
 }
 
-static int
-gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
-		     u32 gmbus2_status,
-		     u32 gmbus4_irq_en)
+static int gmbus_wait(struct drm_i915_private *dev_priv, u32 status, u32 irq_en)
 {
-	int i;
-	u32 gmbus2 = 0;
 	DEFINE_WAIT(wait);
-
-	if (!HAS_GMBUS_IRQ(dev_priv))
-		gmbus4_irq_en = 0;
+	u32 gmbus2;
+	int ret;
 
 	/* Important: The hw handles only the first bit, so set only one! Since
 	 * we also need to check for NAKs besides the hw ready/idle signal, we
-	 * need to wake up periodically and check that ourselves. */
-	I915_WRITE(GMBUS4, gmbus4_irq_en);
-
-	for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
-		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
-				TASK_UNINTERRUPTIBLE);
+	 * need to wake up periodically and check that ourselves.
+	 */
+	if (!HAS_GMBUS_IRQ(dev_priv))
+		irq_en = 0;
 
-		gmbus2 = I915_READ_NOTRACE(GMBUS2);
-		if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
-			break;
+	add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
+	I915_WRITE_FW(GMBUS4, irq_en);
 
-		schedule_timeout(1);
-	}
-	finish_wait(&dev_priv->gmbus_wait_queue, &wait);
+	status |= GMBUS_SATOER;
+	ret = wait_for_us((gmbus2 = I915_READ_FW(GMBUS2)) & status, 2);
+	if (ret)
+		ret = wait_for((gmbus2 = I915_READ_FW(GMBUS2)) & status, 50);
 
-	I915_WRITE(GMBUS4, 0);
+	I915_WRITE_FW(GMBUS4, 0);
+	remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
 
 	if (gmbus2 & GMBUS_SATOER)
 		return -ENXIO;
-	if (gmbus2 & gmbus2_status)
-		return 0;
-	return -ETIMEDOUT;
+
+	return ret;
 }
 
 static int
 gmbus_wait_idle(struct drm_i915_private *dev_priv)
 {
+	DEFINE_WAIT(wait);
+	u32 irq_enable;
 	int ret;
 
-	if (!HAS_GMBUS_IRQ(dev_priv))
-		return intel_wait_for_register(dev_priv,
-					       GMBUS2, GMBUS_ACTIVE, 0,
-					       10);
-
 	/* Important: The hw handles only the first bit, so set only one! */
-	I915_WRITE(GMBUS4, GMBUS_IDLE_EN);
+	irq_enable = 0;
+	if (HAS_GMBUS_IRQ(dev_priv))
+		irq_enable = GMBUS_IDLE_EN;
 
-	ret = wait_event_timeout(dev_priv->gmbus_wait_queue,
-				 (I915_READ_NOTRACE(GMBUS2) & GMBUS_ACTIVE) == 0,
-				 msecs_to_jiffies_timeout(10));
+	add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
+	I915_WRITE_FW(GMBUS4, irq_enable);
 
-	I915_WRITE(GMBUS4, 0);
+	ret = intel_wait_for_register_fw(dev_priv,
+					 GMBUS2, GMBUS_ACTIVE, 0,
+					 10);
 
-	if (ret)
-		return 0;
-	else
-		return -ETIMEDOUT;
+	I915_WRITE_FW(GMBUS4, 0);
+	remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
+
+	return ret;
 }
 
 static int
@@ -323,22 +315,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
 		      unsigned short addr, u8 *buf, unsigned int len,
 		      u32 gmbus1_index)
 {
-	I915_WRITE(GMBUS1,
-		   gmbus1_index |
-		   GMBUS_CYCLE_WAIT |
-		   (len << GMBUS_BYTE_COUNT_SHIFT) |
-		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
-		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
+	I915_WRITE_FW(GMBUS1,
+		      gmbus1_index |
+		      GMBUS_CYCLE_WAIT |
+		      (len << GMBUS_BYTE_COUNT_SHIFT) |
+		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
 		u32 val, loop = 0;
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
-					   GMBUS_HW_RDY_EN);
+		ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 
-		val = I915_READ(GMBUS3);
+		val = I915_READ_FW(GMBUS3);
 		do {
 			*buf++ = val & 0xff;
 			val >>= 8;
@@ -385,12 +376,12 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
 		len -= 1;
 	}
 
-	I915_WRITE(GMBUS3, val);
-	I915_WRITE(GMBUS1,
-		   GMBUS_CYCLE_WAIT |
-		   (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
-		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
-		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
+	I915_WRITE_FW(GMBUS3, val);
+	I915_WRITE_FW(GMBUS1,
+		      GMBUS_CYCLE_WAIT |
+		      (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
+		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
+		      GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
 	while (len) {
 		int ret;
 
@@ -399,10 +390,9 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
 			val |= *buf++ << (8 * loop);
 		} while (--len && ++loop < 4);
 
-		I915_WRITE(GMBUS3, val);
+		I915_WRITE_FW(GMBUS3, val);
 
-		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
-					   GMBUS_HW_RDY_EN);
+		ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
 		if (ret)
 			return ret;
 	}
@@ -460,13 +450,13 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
 
 	/* GMBUS5 holds 16-bit index */
 	if (gmbus5)
-		I915_WRITE(GMBUS5, gmbus5);
+		I915_WRITE_FW(GMBUS5, gmbus5);
 
 	ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
 
 	/* Clear GMBUS5 after each index transfer */
 	if (gmbus5)
-		I915_WRITE(GMBUS5, 0);
+		I915_WRITE_FW(GMBUS5, 0);
 
 	return ret;
 }
@@ -478,11 +468,15 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
 					       struct intel_gmbus,
 					       adapter);
 	struct drm_i915_private *dev_priv = bus->dev_priv;
+	const unsigned int fw =
+		intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
+					       FW_REG_READ | FW_REG_WRITE);
 	int i = 0, inc, try = 0;
 	int ret = 0;
 
+	intel_uncore_forcewake_get(dev_priv, fw);
 retry:
-	I915_WRITE(GMBUS0, bus->reg0);
+	I915_WRITE_FW(GMBUS0, bus->reg0);
 
 	for (; i < num; i += inc) {
 		inc = 1;
@@ -496,8 +490,8 @@ retry:
 		}
 
 		if (!ret)
-			ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
-						   GMBUS_HW_WAIT_EN);
+			ret = gmbus_wait(dev_priv,
+					 GMBUS_HW_WAIT_PHASE, GMBUS_HW_WAIT_EN);
 		if (ret == -ETIMEDOUT)
 			goto timeout;
 		else if (ret)
@@ -508,7 +502,7 @@ retry:
 	 * a STOP on the very first cycle. To simplify the code we
 	 * unconditionally generate the STOP condition with an additional gmbus
 	 * cycle. */
-	I915_WRITE(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
+	I915_WRITE_FW(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
 
 	/* Mark the GMBUS interface as disabled after waiting for idle.
 	 * We will re-enable it at the start of the next xfer,
@@ -519,7 +513,7 @@ retry:
 			 adapter->name);
 		ret = -ETIMEDOUT;
 	}
-	I915_WRITE(GMBUS0, 0);
+	I915_WRITE_FW(GMBUS0, 0);
 	ret = ret ?: i;
 	goto out;
 
@@ -548,9 +542,9 @@ clear_err:
 	 * of resetting the GMBUS controller and so clearing the
 	 * BUS_ERROR raised by the slave's NAK.
 	 */
-	I915_WRITE(GMBUS1, GMBUS_SW_CLR_INT);
-	I915_WRITE(GMBUS1, 0);
-	I915_WRITE(GMBUS0, 0);
+	I915_WRITE_FW(GMBUS1, GMBUS_SW_CLR_INT);
+	I915_WRITE_FW(GMBUS1, 0);
+	I915_WRITE_FW(GMBUS0, 0);
 
 	DRM_DEBUG_KMS("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
 			 adapter->name, msgs[i].addr,
@@ -573,7 +567,7 @@ clear_err:
 timeout:
 	DRM_DEBUG_KMS("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
 		      bus->adapter.name, bus->reg0 & 0xff);
-	I915_WRITE(GMBUS0, 0);
+	I915_WRITE_FW(GMBUS0, 0);
 
 	/*
 	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
@@ -582,6 +576,7 @@ timeout:
 	ret = -EAGAIN;
 
 out:
+	intel_uncore_forcewake_put(dev_priv, fw);
 	return ret;
 }
 
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: Try GPIO NAK discovery before GMBUS

[Chris Wilson]

Some GMBUS devices fail to report NAKs (even recent Skylakes), resulting
in us hitting the 50ms timeout every time we try to read an EDID on a
disconnected device. Try a quick GPIO discovery first by setting the
clock line and seeing if the device responds.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: David Weinehall <david.weinehall@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_i2c.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 6841c41281a3..72945a7c5547 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -461,6 +461,22 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
 	return ret;
 }
 
+static bool gpio_is_alive(struct intel_gmbus *bus)
+{
+	bool result = true;
+
+	intel_i2c_quirk_set(bus->dev_priv, true);
+	set_clock(bus, 1);
+
+	if (wait_for_us(get_clock(bus), 10) && wait_for(get_clock(bus), 2))
+		result = false;
+
+	set_clock(bus, 0);
+	intel_i2c_quirk_set(bus->dev_priv, false);
+
+	return result;
+}
+
 static int
 do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
 {
@@ -474,6 +490,13 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
 	int i = 0, inc, try = 0;
 	int ret = 0;
 
+	/* Some GMBUS devices fail to report NAKs and so we hit the 50ms
+	 * timeout, every time. Try a quick GPIO discovery first by seeing
+	 * if the device responds to setting the clock line high.
+	 */
+	if (!gpio_is_alive(bus))
+		return -ENXIO;
+
 	intel_uncore_forcewake_get(dev_priv, fw);
 retry:
 	I915_WRITE_FW(GMBUS0, bus->reg0);
-- 
2.9.3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for series starting with [1/2] drm/i915: Take forcewake once for the entire GMBUS transaction

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Take forcewake once for the entire GMBUS transaction
URL   : https://patchwork.freedesktop.org/series/11332/
State : failure

== Summary ==

Series 11332v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/11332/revisions/1/mbox

Test gem_exec_gttfill:
        Subgroup basic:
                pass       -> SKIP       (fi-snb-i7-2600)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-legacy:
                dmesg-warn -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-cursor-vs-flip-varying-size:
                dmesg-warn -> PASS       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-skl3-i5-6260u)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                dmesg-warn -> SKIP       (ro-bdw-i5-5250u)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (fi-hsw-i7-4770k)
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:185  dwarn:30  dfail:0   fail:2   skip:27 
fi-snb-i7-2600   total:244  pass:201  dwarn:0   dfail:0   fail:0   skip:43 
ro-bdw-i5-5250u  total:240  pass:220  dwarn:1   dfail:0   fail:0   skip:19 
ro-bdw-i7-5600u  total:240  pass:205  dwarn:1   dfail:0   fail:2   skip:32 
ro-bsw-n3050     total:240  pass:195  dwarn:0   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:196  dwarn:0   dfail:0   fail:4   skip:40 
ro-hsw-i3-4010u  total:240  pass:213  dwarn:0   dfail:0   fail:1   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:2   skip:59 
ro-ivb-i7-3770   total:240  pass:204  dwarn:0   dfail:0   fail:1   skip:35 
ro-ivb2-i7-3770  total:240  pass:208  dwarn:0   dfail:0   fail:1   skip:31 
ro-skl3-i5-6260u total:240  pass:222  dwarn:0   dfail:0   fail:4   skip:14 
fi-skl-i7-6700k failed to connect after reboot
ro-bdw-i7-5557U failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1946/

f53a8d1 drm-intel-nightly: 2016y-08m-19d-16h-24m-21s UTC integration manifest
dc57725 drm/i915: Try GPIO NAK discovery before GMBUS
b9da26f drm/i915: Take forcewake once for the entire GMBUS transaction

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Take forcewake once for the entire GMBUS transaction

[David Weinehall]

On Fri, Aug 19, 2016 at 05:45:02PM +0100, Chris Wilson wrote:
> As we do many register reads within a very short period of time, hold
> the GMBUS powerwell from start to finish.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Weinehall <david.weinehall@linux.intel.com>

Looks good, works well.

Reviewed-by: David Weinehall <david.weinehall@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 131 +++++++++++++++++++--------------------
>  1 file changed, 63 insertions(+), 68 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 1f266d7df2ec..6841c41281a3 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -255,67 +255,59 @@ intel_gpio_setup(struct intel_gmbus *bus, unsigned int pin)
>  	algo->data = bus;
>  }
>  
> -static int
> -gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
> -		     u32 gmbus2_status,
> -		     u32 gmbus4_irq_en)
> +static int gmbus_wait(struct drm_i915_private *dev_priv, u32 status, u32 irq_en)
>  {
> -	int i;
> -	u32 gmbus2 = 0;
>  	DEFINE_WAIT(wait);
> -
> -	if (!HAS_GMBUS_IRQ(dev_priv))
> -		gmbus4_irq_en = 0;
> +	u32 gmbus2;
> +	int ret;
>  
>  	/* Important: The hw handles only the first bit, so set only one! Since
>  	 * we also need to check for NAKs besides the hw ready/idle signal, we
> -	 * need to wake up periodically and check that ourselves. */
> -	I915_WRITE(GMBUS4, gmbus4_irq_en);
> -
> -	for (i = 0; i < msecs_to_jiffies_timeout(50); i++) {
> -		prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
> -				TASK_UNINTERRUPTIBLE);
> +	 * need to wake up periodically and check that ourselves.
> +	 */
> +	if (!HAS_GMBUS_IRQ(dev_priv))
> +		irq_en = 0;
>  
> -		gmbus2 = I915_READ_NOTRACE(GMBUS2);
> -		if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
> -			break;
> +	add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> +	I915_WRITE_FW(GMBUS4, irq_en);
>  
> -		schedule_timeout(1);
> -	}
> -	finish_wait(&dev_priv->gmbus_wait_queue, &wait);
> +	status |= GMBUS_SATOER;
> +	ret = wait_for_us((gmbus2 = I915_READ_FW(GMBUS2)) & status, 2);
> +	if (ret)
> +		ret = wait_for((gmbus2 = I915_READ_FW(GMBUS2)) & status, 50);
>  
> -	I915_WRITE(GMBUS4, 0);
> +	I915_WRITE_FW(GMBUS4, 0);
> +	remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
>  
>  	if (gmbus2 & GMBUS_SATOER)
>  		return -ENXIO;
> -	if (gmbus2 & gmbus2_status)
> -		return 0;
> -	return -ETIMEDOUT;
> +
> +	return ret;
>  }
>  
>  static int
>  gmbus_wait_idle(struct drm_i915_private *dev_priv)
>  {
> +	DEFINE_WAIT(wait);
> +	u32 irq_enable;
>  	int ret;
>  
> -	if (!HAS_GMBUS_IRQ(dev_priv))
> -		return intel_wait_for_register(dev_priv,
> -					       GMBUS2, GMBUS_ACTIVE, 0,
> -					       10);
> -
>  	/* Important: The hw handles only the first bit, so set only one! */
> -	I915_WRITE(GMBUS4, GMBUS_IDLE_EN);
> +	irq_enable = 0;
> +	if (HAS_GMBUS_IRQ(dev_priv))
> +		irq_enable = GMBUS_IDLE_EN;
>  
> -	ret = wait_event_timeout(dev_priv->gmbus_wait_queue,
> -				 (I915_READ_NOTRACE(GMBUS2) & GMBUS_ACTIVE) == 0,
> -				 msecs_to_jiffies_timeout(10));
> +	add_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> +	I915_WRITE_FW(GMBUS4, irq_enable);
>  
> -	I915_WRITE(GMBUS4, 0);
> +	ret = intel_wait_for_register_fw(dev_priv,
> +					 GMBUS2, GMBUS_ACTIVE, 0,
> +					 10);
>  
> -	if (ret)
> -		return 0;
> -	else
> -		return -ETIMEDOUT;
> +	I915_WRITE_FW(GMBUS4, 0);
> +	remove_wait_queue(&dev_priv->gmbus_wait_queue, &wait);
> +
> +	return ret;
>  }
>  
>  static int
> @@ -323,22 +315,21 @@ gmbus_xfer_read_chunk(struct drm_i915_private *dev_priv,
>  		      unsigned short addr, u8 *buf, unsigned int len,
>  		      u32 gmbus1_index)
>  {
> -	I915_WRITE(GMBUS1,
> -		   gmbus1_index |
> -		   GMBUS_CYCLE_WAIT |
> -		   (len << GMBUS_BYTE_COUNT_SHIFT) |
> -		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> -		   GMBUS_SLAVE_READ | GMBUS_SW_RDY);
> +	I915_WRITE_FW(GMBUS1,
> +		      gmbus1_index |
> +		      GMBUS_CYCLE_WAIT |
> +		      (len << GMBUS_BYTE_COUNT_SHIFT) |
> +		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> +		      GMBUS_SLAVE_READ | GMBUS_SW_RDY);
>  	while (len) {
>  		int ret;
>  		u32 val, loop = 0;
>  
> -		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
> -					   GMBUS_HW_RDY_EN);
> +		ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
>  		if (ret)
>  			return ret;
>  
> -		val = I915_READ(GMBUS3);
> +		val = I915_READ_FW(GMBUS3);
>  		do {
>  			*buf++ = val & 0xff;
>  			val >>= 8;
> @@ -385,12 +376,12 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
>  		len -= 1;
>  	}
>  
> -	I915_WRITE(GMBUS3, val);
> -	I915_WRITE(GMBUS1,
> -		   GMBUS_CYCLE_WAIT |
> -		   (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
> -		   (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> -		   GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
> +	I915_WRITE_FW(GMBUS3, val);
> +	I915_WRITE_FW(GMBUS1,
> +		      GMBUS_CYCLE_WAIT |
> +		      (chunk_size << GMBUS_BYTE_COUNT_SHIFT) |
> +		      (addr << GMBUS_SLAVE_ADDR_SHIFT) |
> +		      GMBUS_SLAVE_WRITE | GMBUS_SW_RDY);
>  	while (len) {
>  		int ret;
>  
> @@ -399,10 +390,9 @@ gmbus_xfer_write_chunk(struct drm_i915_private *dev_priv,
>  			val |= *buf++ << (8 * loop);
>  		} while (--len && ++loop < 4);
>  
> -		I915_WRITE(GMBUS3, val);
> +		I915_WRITE_FW(GMBUS3, val);
>  
> -		ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_RDY,
> -					   GMBUS_HW_RDY_EN);
> +		ret = gmbus_wait(dev_priv, GMBUS_HW_RDY, GMBUS_HW_RDY_EN);
>  		if (ret)
>  			return ret;
>  	}
> @@ -460,13 +450,13 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>  
>  	/* GMBUS5 holds 16-bit index */
>  	if (gmbus5)
> -		I915_WRITE(GMBUS5, gmbus5);
> +		I915_WRITE_FW(GMBUS5, gmbus5);
>  
>  	ret = gmbus_xfer_read(dev_priv, &msgs[1], gmbus1_index);
>  
>  	/* Clear GMBUS5 after each index transfer */
>  	if (gmbus5)
> -		I915_WRITE(GMBUS5, 0);
> +		I915_WRITE_FW(GMBUS5, 0);
>  
>  	return ret;
>  }
> @@ -478,11 +468,15 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  					       struct intel_gmbus,
>  					       adapter);
>  	struct drm_i915_private *dev_priv = bus->dev_priv;
> +	const unsigned int fw =
> +		intel_uncore_forcewake_for_reg(dev_priv, GMBUS0,
> +					       FW_REG_READ | FW_REG_WRITE);
>  	int i = 0, inc, try = 0;
>  	int ret = 0;
>  
> +	intel_uncore_forcewake_get(dev_priv, fw);
>  retry:
> -	I915_WRITE(GMBUS0, bus->reg0);
> +	I915_WRITE_FW(GMBUS0, bus->reg0);
>  
>  	for (; i < num; i += inc) {
>  		inc = 1;
> @@ -496,8 +490,8 @@ retry:
>  		}
>  
>  		if (!ret)
> -			ret = gmbus_wait_hw_status(dev_priv, GMBUS_HW_WAIT_PHASE,
> -						   GMBUS_HW_WAIT_EN);
> +			ret = gmbus_wait(dev_priv,
> +					 GMBUS_HW_WAIT_PHASE, GMBUS_HW_WAIT_EN);
>  		if (ret == -ETIMEDOUT)
>  			goto timeout;
>  		else if (ret)
> @@ -508,7 +502,7 @@ retry:
>  	 * a STOP on the very first cycle. To simplify the code we
>  	 * unconditionally generate the STOP condition with an additional gmbus
>  	 * cycle. */
> -	I915_WRITE(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
> +	I915_WRITE_FW(GMBUS1, GMBUS_CYCLE_STOP | GMBUS_SW_RDY);
>  
>  	/* Mark the GMBUS interface as disabled after waiting for idle.
>  	 * We will re-enable it at the start of the next xfer,
> @@ -519,7 +513,7 @@ retry:
>  			 adapter->name);
>  		ret = -ETIMEDOUT;
>  	}
> -	I915_WRITE(GMBUS0, 0);
> +	I915_WRITE_FW(GMBUS0, 0);
>  	ret = ret ?: i;
>  	goto out;
>  
> @@ -548,9 +542,9 @@ clear_err:
>  	 * of resetting the GMBUS controller and so clearing the
>  	 * BUS_ERROR raised by the slave's NAK.
>  	 */
> -	I915_WRITE(GMBUS1, GMBUS_SW_CLR_INT);
> -	I915_WRITE(GMBUS1, 0);
> -	I915_WRITE(GMBUS0, 0);
> +	I915_WRITE_FW(GMBUS1, GMBUS_SW_CLR_INT);
> +	I915_WRITE_FW(GMBUS1, 0);
> +	I915_WRITE_FW(GMBUS0, 0);
>  
>  	DRM_DEBUG_KMS("GMBUS [%s] NAK for addr: %04x %c(%d)\n",
>  			 adapter->name, msgs[i].addr,
> @@ -573,7 +567,7 @@ clear_err:
>  timeout:
>  	DRM_DEBUG_KMS("GMBUS [%s] timed out, falling back to bit banging on pin %d\n",
>  		      bus->adapter.name, bus->reg0 & 0xff);
> -	I915_WRITE(GMBUS0, 0);
> +	I915_WRITE_FW(GMBUS0, 0);
>  
>  	/*
>  	 * Hardware may not support GMBUS over these pins? Try GPIO bitbanging
> @@ -582,6 +576,7 @@ timeout:
>  	ret = -EAGAIN;
>  
>  out:
> +	intel_uncore_forcewake_put(dev_priv, fw);
>  	return ret;
>  }
>  
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: Try GPIO NAK discovery before GMBUS

[David Weinehall]

On Fri, Aug 19, 2016 at 05:45:03PM +0100, Chris Wilson wrote:
> Some GMBUS devices fail to report NAKs (even recent Skylakes), resulting
> in us hitting the 50ms timeout every time we try to read an EDID on a
> disconnected device. Try a quick GPIO discovery first by setting the
> clock line and seeing if the device responds.

Sadly doesn't work.  Doesn't make a difference on the problematic port
(HDMI-A-2 on a ThinkPad 13) -- still very slow, nor on the
non-problematic port (HDMI-A-1 on the same laptop) -- still fast,
but seems to make things a lot worse on HDMI-A-1 when there *is*
a display connected -- incredibly slow.

So, ironically I'd say NAK to the NAK-patch.


Kind regards, David Weinehall

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: David Weinehall <david.weinehall@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_i2c.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
> index 6841c41281a3..72945a7c5547 100644
> --- a/drivers/gpu/drm/i915/intel_i2c.c
> +++ b/drivers/gpu/drm/i915/intel_i2c.c
> @@ -461,6 +461,22 @@ gmbus_xfer_index_read(struct drm_i915_private *dev_priv, struct i2c_msg *msgs)
>  	return ret;
>  }
>  
> +static bool gpio_is_alive(struct intel_gmbus *bus)
> +{
> +	bool result = true;
> +
> +	intel_i2c_quirk_set(bus->dev_priv, true);
> +	set_clock(bus, 1);
> +
> +	if (wait_for_us(get_clock(bus), 10) && wait_for(get_clock(bus), 2))
> +		result = false;
> +
> +	set_clock(bus, 0);
> +	intel_i2c_quirk_set(bus->dev_priv, false);
> +
> +	return result;
> +}
> +
>  static int
>  do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  {
> @@ -474,6 +490,13 @@ do_gmbus_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
>  	int i = 0, inc, try = 0;
>  	int ret = 0;
>  
> +	/* Some GMBUS devices fail to report NAKs and so we hit the 50ms
> +	 * timeout, every time. Try a quick GPIO discovery first by seeing
> +	 * if the device responds to setting the clock line high.
> +	 */
> +	if (!gpio_is_alive(bus))
> +		return -ENXIO;
> +
>  	intel_uncore_forcewake_get(dev_priv, fw);
>  retry:
>  	I915_WRITE_FW(GMBUS0, bus->reg0);
> -- 
> 2.9.3
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx