All of lore.kernel.org
 help / color / mirror / Atom feed
From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: [PATCH] drm/i915: use the gmbus irq for waits
Date: Thu,  6 Sep 2012 15:44:33 +0200	[thread overview]
Message-ID: <1346939073-5965-1-git-send-email-daniel.vetter@ffwll.ch> (raw)
In-Reply-To: <6c3329$5soj8t@orsmga002.jf.intel.com>

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.

Ben Widawsky rightfully complained the lack of measurements for the
claimed benefits (especially since the first version was actually
broken and fell back to bit-banging). Previously reading the 256 byte
hdmi EDID takes about 72 ms here. With this patch it's down to 33 ms.
Given that transfering the 256 bytes over i2c at wire speed takes
20.5ms alone, the reduction in additional overhead is rather nice.

v2: Chris Wilson wondered whether GMBUS4 might contain some set bits
when booting up an hence result in some spurious interrupts. Since we
clear GMBUS4 after every wait and we do gmbus transfer really early in
the setup sequence to detect displays the window is small, but still
be paranoid and clear it properly.

v3: Clarify the comment that gmbus irq generation can only support one
kind of event, why it bothers us and how we work around that limit.

Cc: Daniel Kurtz <djkurtz@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

fixup
---
 drivers/gpu/drm/i915/i915_drv.h  |  2 ++
 drivers/gpu/drm/i915/i915_irq.c  |  4 ++++
 drivers/gpu/drm/i915/intel_i2c.c | 45 ++++++++++++++++++++++++++++++----------
 3 files changed, 40 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5c8d5e3..72db70d 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 57decac..7413595 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -64,6 +64,7 @@ intel_i2c_reset(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS0, 0);
+	I915_WRITE(dev_priv->gpio_mmio_base + GMBUS4, 0);
 }
 
 static void intel_i2c_quirk_set(struct drm_i915_private *dev_priv, bool enable)
@@ -205,20 +206,38 @@ 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! 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 + 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 +258,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 +303,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 +389,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)
@@ -474,6 +496,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

  reply	other threads:[~2012-09-06 14:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-09-06  7:09 [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
2012-09-06  7:10 ` [PATCH 2/4] drm/i915: wire up gmbus irq handler Daniel Vetter
2012-09-06  7:10 ` [PATCH 3/4] drm/i915: use the gmbus irq for waits Daniel Vetter
2012-09-06  7:10 ` [PATCH 4/4] drm/i915: use gmbus irq to wait for gmbus idle Daniel Vetter
2012-09-06 13:06   ` Chris Wilson
2012-09-06 13:44     ` Daniel Vetter [this message]
2012-09-08 17:11 ` [PATCH 1/4] drm/i915: extract gmbus_wait_hw_status Ben Widawsky

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1346939073-5965-1-git-send-email-daniel.vetter@ffwll.ch \
    --to=daniel.vetter@ffwll.ch \
    --cc=intel-gfx@lists.freedesktop.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.