* [PATCH 2/8] drm/i915: extract gmbus_wait_hw_status
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:00 ` [PATCH 3/8] drm/i915: wire up gmbus irq handler Daniel Vetter
` (6 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 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).
v2: I've failed to notice that the sens of GMBUS_ACTIVE is inverted,
Chris Wilson gladly pointed that out for me. To keep things simple,
ignore that case for now (we only need to idle the gmbus controller
at the end of an entire i2c transaction, not after every message).
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_i2c.c | 46 ++++++++++++++++++++++------------------
1 file changed, 25 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index b9755f6..57decac 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
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 3/8] drm/i915: wire up gmbus irq handler
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
2012-09-09 9:00 ` [PATCH 2/8] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:00 ` [PATCH 4/8] drm/i915: use the gmbus irq for waits Daniel Vetter
` (5 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 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 d601013..86f1690 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");
@@ -1864,12 +1869,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);
}
@@ -1929,7 +1936,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));
@@ -1982,6 +1990,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);
@@ -2477,6 +2486,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] 12+ messages in thread* [PATCH 4/8] drm/i915: use the gmbus irq for waits
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
2012-09-09 9:00 ` [PATCH 2/8] drm/i915: extract gmbus_wait_hw_status Daniel Vetter
2012-09-09 9:00 ` [PATCH 3/8] drm/i915: wire up gmbus irq handler Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-10 2:53 ` Daniel Kurtz
2012-09-09 9:00 ` [PATCH 5/8] drm/i915: use gmbus irq to wait for gmbus idle Daniel Vetter
` (4 subsequent siblings)
7 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 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.
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>
---
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 26c6959..13b9e6a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -419,6 +419,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 86f1690..1741f2e 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
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH 4/8] drm/i915: use the gmbus irq for waits
2012-09-09 9:00 ` [PATCH 4/8] drm/i915: use the gmbus irq for waits Daniel Vetter
@ 2012-09-10 2:53 ` Daniel Kurtz
0 siblings, 0 replies; 12+ messages in thread
From: Daniel Kurtz @ 2012-09-10 2:53 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development
On Sun, Sep 9, 2012 at 5:00 PM, 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).
>
Hi Daniel V,
>
> 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.
Great find!
Overall, this looks and sounds great.
See some comments inline...
> 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>
> ---
> 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 26c6959..13b9e6a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -419,6 +419,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 86f1690..1741f2e 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;
Technically, initializing gmbus2 here isn't necessary, since you
always assign it first before reading.
>
> + 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. */
-- It is unfortunate that we can't enable both HW_WAIT/HW_RDY & NAK irqs.
When I tried it before, it seemed to work... but something was always
unstable, and I never figured out what.
>
> + 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++) {
Should there be an initial check of our condition before entering the wait?
>
> + prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
> + TASK_UNINTERRUPTIBLE);
Should this wait be interruptible?
> +
> + gmbus2 = I915_READ(GMBUS2 + reg_offset);
> + if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
> + break;
> +
> + schedule_timeout(1);
> + }
> + finish_wait(&dev_priv->gmbus_wait_queue, &wait);
Would it be more clear to just do 50 1-jiffy wait_event_timeout()s?
for (i = 0; i < msecs_to_jiffies(50) + 1; i++)
if (wait_event_timeout(&dev_priv->gmbus_wait_queue,
(gmbus2 = I915_READ(GMBUS2 + reg_offset)) &
(GMBUS_SATOER | gmbus2_status),
1))
break;
>
> +
> + 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
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/8] drm/i915: use gmbus irq to wait for gmbus idle
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
` (2 preceding siblings ...)
2012-09-09 9:00 ` [PATCH 4/8] drm/i915: use the gmbus irq for waits Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:00 ` [PATCH 6/8] drm/i915: only read SDE_IIR when required on ilk/snb Daniel Vetter
` (3 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
GMBUS_ACTIVE has inverted sense and so doesn't fit into the
wait_hw_status helper, hence create a new gmbus_wait_idle functions.
Also, we only care about the idle irq event and nothing else, which
allows us to use the wait_event_timeout helper directly without
jumping through hoops to catch NAKs.
Since gen2/3 don't have gmbus interrupts, handle them separately with
the old wait_for macro.
This shaves another few ms off reading EDID from a hdmi screen on my
testbox here. EDID reading with interrupt driven gmbus is now as fast
as with busy-looping gmbus at 28 ms here (with negligible cpu
overhead).
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_i2c.c | 32 ++++++++++++++++++++++++++++----
1 file changed, 28 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 7413595..4a38a30 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -204,6 +204,7 @@ intel_gpio_setup(struct intel_gmbus *bus, u32 pin)
algo->data = bus;
}
+#define HAS_GMBUS_IRQ(dev) (INTEL_INFO(dev)->gen >= 4)
static int
gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
u32 gmbus2_status,
@@ -241,6 +242,31 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
}
static int
+gmbus_wait_idle(struct drm_i915_private *dev_priv)
+{
+ int ret;
+ int reg_offset = dev_priv->gpio_mmio_base;
+
+#define C ((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0)
+
+ if (!HAS_GMBUS_IRQ(dev_priv->dev))
+ return wait_for(C, 10);
+
+ /* Important: The hw handles only the first bit, so set only one! */
+ I915_WRITE(GMBUS4 + reg_offset, GMBUS_IDLE_EN);
+
+ ret = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
+
+ I915_WRITE(GMBUS4 + reg_offset, 0);
+
+ if (ret)
+ return 0;
+ else
+ return -ETIMEDOUT;
+#undef C
+}
+
+static int
gmbus_xfer_read(struct drm_i915_private *dev_priv, struct i2c_msg *msg,
u32 gmbus1_index)
{
@@ -407,8 +433,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_idle(dev_priv)) {
DRM_DEBUG_KMS("GMBUS [%s] timed out waiting for idle\n",
adapter->name);
ret = -ETIMEDOUT;
@@ -432,8 +457,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_idle(dev_priv)) {
DRM_DEBUG_KMS("GMBUS [%s] timed out after NAK\n",
adapter->name);
ret = -ETIMEDOUT;
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 6/8] drm/i915: only read SDE_IIR when required on ilk/snb
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
` (3 preceding siblings ...)
2012-09-09 9:00 ` [PATCH 5/8] drm/i915: use gmbus irq to wait for gmbus idle Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:00 ` [PATCH 7/8] drm/i915: wire up do aux channel done interrupt Daniel Vetter
` (2 subsequent siblings)
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
The same optimization has already been applied to the ivb irq handler in
commit 0e43406bcc1868a316eea6012a0a09d992c53521
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date: Wed May 9 21:45:44 2012 +0100
drm/i915: Simplify interrupt processing for IvyBridge
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_irq.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1741f2e..f836e89 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -754,7 +754,7 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
struct drm_device *dev = (struct drm_device *) arg;
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
int ret = IRQ_NONE;
- u32 de_iir, gt_iir, de_ier, pch_iir, pm_iir;
+ u32 de_iir, gt_iir, de_ier, pm_iir;
u32 hotplug_mask;
atomic_inc(&dev_priv->irq_received);
@@ -766,11 +766,9 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
de_iir = I915_READ(DEIIR);
gt_iir = I915_READ(GTIIR);
- pch_iir = I915_READ(SDEIIR);
pm_iir = I915_READ(GEN6_PMIIR);
- if (de_iir == 0 && gt_iir == 0 && pch_iir == 0 &&
- (!IS_GEN6(dev) || pm_iir == 0))
+ if (de_iir == 0 && gt_iir == 0 && (!IS_GEN6(dev) || pm_iir == 0))
goto done;
if (HAS_PCH_CPT(dev))
@@ -806,12 +804,17 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
/* check event from PCH */
if (de_iir & DE_PCH_EVENT) {
+ u32 pch_iir = I915_READ(SDEIIR);
+
if (pch_iir & hotplug_mask)
queue_work(dev_priv->wq, &dev_priv->hotplug_work);
if (HAS_PCH_CPT(dev))
cpt_irq_handler(dev, pch_iir);
else
ibx_irq_handler(dev, pch_iir);
+
+ /* should clear PCH hotplug event before clear CPU irq */
+ I915_WRITE(SDEIIR, pch_iir);
}
if (IS_GEN5(dev) && de_iir & DE_PCU_EVENT)
@@ -820,8 +823,6 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS)
gen6_queue_rps_work(dev_priv, pm_iir);
- /* should clear PCH hotplug event before clear CPU irq */
- I915_WRITE(SDEIIR, pch_iir);
I915_WRITE(GTIIR, gt_iir);
I915_WRITE(DEIIR, de_iir);
I915_WRITE(GEN6_PMIIR, pm_iir);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 7/8] drm/i915: wire up do aux channel done interrupt
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
` (4 preceding siblings ...)
2012-09-09 9:00 ` [PATCH 6/8] drm/i915: only read SDE_IIR when required on ilk/snb Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:00 ` [PATCH 8/8] drm/i915: irq-drive the dp aux communication Daniel Vetter
2012-09-09 10:24 ` [PATCH] drm/i915: use _NOTRACE for gmbus/dp aux wait loops Daniel Vetter
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Doesn't do anything yet than call dp_aux_irq_handler.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_irq.c | 28 +++++++++++++++++++++++-----
1 file changed, 23 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index f836e89..c483531 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -605,6 +605,11 @@ static void gmbus_irq_handler(struct drm_device *dev)
wake_up_all(&dev_priv->gmbus_wait_queue);
}
+static void dp_aux_irq_handler(struct drm_device *dev)
+{
+ DRM_DEBUG_DRIVER("AUX channel 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;
@@ -615,6 +620,9 @@ static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
(pch_iir & SDE_AUDIO_POWER_MASK) >>
SDE_AUDIO_POWER_SHIFT);
+ if (pch_iir & SDE_AUX_MASK)
+ dp_aux_irq_handler(dev);
+
if (pch_iir & SDE_GMBUS)
gmbus_irq_handler(dev);
@@ -656,7 +664,7 @@ static void cpt_irq_handler(struct drm_device *dev, u32 pch_iir)
SDE_AUDIO_POWER_SHIFT_CPT);
if (pch_iir & SDE_AUX_MASK_CPT)
- DRM_DEBUG_DRIVER("AUX channel interrupt\n");
+ dp_aux_irq_handler(dev);
if (pch_iir & SDE_GMBUS_CPT)
gmbus_irq_handler(dev);
@@ -697,6 +705,9 @@ static irqreturn_t ivybridge_irq_handler(DRM_IRQ_ARGS)
de_iir = I915_READ(DEIIR);
if (de_iir) {
+ if (de_iir & DE_AUX_CHANNEL_A_IVB)
+ dp_aux_irq_handler(dev);
+
if (de_iir & DE_GSE_IVB)
intel_opregion_gse_intr(dev);
@@ -783,6 +794,9 @@ static irqreturn_t ironlake_irq_handler(DRM_IRQ_ARGS)
else
snb_gt_irq_handler(dev, dev_priv, gt_iir);
+ if (de_iir & DE_AUX_CHANNEL_A)
+ dp_aux_irq_handler(dev);
+
if (de_iir & DE_GSE)
intel_opregion_gse_intr(dev);
@@ -1840,7 +1854,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
/* enable kind of interrupts always enabled */
u32 display_mask = DE_MASTER_IRQ_CONTROL | DE_GSE | DE_PCH_EVENT |
- DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE;
+ DE_PLANEA_FLIP_DONE | DE_PLANEB_FLIP_DONE |
+ DE_AUX_CHANNEL_A;
u32 render_irqs;
u32 hotplug_mask;
@@ -1875,7 +1890,8 @@ static int ironlake_irq_postinstall(struct drm_device *dev)
SDE_PORTB_HOTPLUG_CPT |
SDE_PORTC_HOTPLUG_CPT |
SDE_PORTD_HOTPLUG_CPT |
- SDE_GMBUS_CPT);
+ SDE_GMBUS_CPT |
+ SDE_AUX_MASK_CPT);
} else {
hotplug_mask = (SDE_CRT_HOTPLUG |
SDE_PORTB_HOTPLUG |
@@ -1912,7 +1928,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
DE_MASTER_IRQ_CONTROL | DE_GSE_IVB | DE_PCH_EVENT_IVB |
DE_PLANEC_FLIP_DONE_IVB |
DE_PLANEB_FLIP_DONE_IVB |
- DE_PLANEA_FLIP_DONE_IVB;
+ DE_PLANEA_FLIP_DONE_IVB |
+ DE_AUX_CHANNEL_A_IVB;
u32 render_irqs;
u32 hotplug_mask;
@@ -1942,7 +1959,8 @@ static int ivybridge_irq_postinstall(struct drm_device *dev)
SDE_PORTB_HOTPLUG_CPT |
SDE_PORTC_HOTPLUG_CPT |
SDE_PORTD_HOTPLUG_CPT |
- SDE_GMBUS_CPT);
+ SDE_GMBUS_CPT |
+ SDE_AUX_MASK_CPT);
dev_priv->pch_irq_mask = ~hotplug_mask;
I915_WRITE(SDEIIR, I915_READ(SDEIIR));
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH 8/8] drm/i915: irq-drive the dp aux communication
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
` (5 preceding siblings ...)
2012-09-09 9:00 ` [PATCH 7/8] drm/i915: wire up do aux channel done interrupt Daniel Vetter
@ 2012-09-09 9:00 ` Daniel Vetter
2012-09-09 9:29 ` [PATCH] " Daniel Vetter
2012-09-09 10:24 ` [PATCH] drm/i915: use _NOTRACE for gmbus/dp aux wait loops Daniel Vetter
7 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:00 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
At least on the platforms that have a dp aux irq and also have it
enabled - vlv/hsw should have one, too. But I don't have a machine to
test this on, and the current code doesn't support dp yet anyway on
those platforms. Judging from docs there's no dp aux interrupt for gm45.
Also, I only have an ivb cpu edp machine, so the dp aux A code for
snb/ilk is untested.
For dpcd probing when nothing is connected it slashes about 5ms of cpu
time (cpu time is now negligible), which agrees with 3 * 5 400 usec
timeouts.
A previous version of this patch increases the time required to go
through the dp_detect cycle (which includes reading the edid) from
around 33 ms to around 40 ms. Experiments indicated that this is
purely due to the irq latency - the hw doesn't allow us to queue up
dp aux transactions and hence irq latency directly affects throughput.
gmbus is much better, there we have a 8 byte buffer, and we get the
irq once another 4 bytes can be queued up.
But by using the pm_qos interface to request the lowest possible cpu
wake-up latency this slowdown completely disappeared.
Since all our output detection logic is single-threaded with the
mode_config mutex right now anyway, I've decide not ot play fancy and
to just reuse the gmbus wait queue. But this would definitely prep the
way to run dp detection on different ports in parallel
v2: Add a timeout for dp aux transfers when using interrupts - the hw
_does_ prevent this with the hw-based 400 usec timeout, but if the
irq somehow doesn't arrive we're screwed. Lesson learned while
developing this ;-)
v3: While at it also convert the busy-loop to wait_for_atomic, so that
we don't run the risk of an infinite loop any more.
v4: Ensure we have the smallest possible irq latency by using the
pm_qos interface.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 4 +++
drivers/gpu/drm/i915/i915_irq.c | 6 +++++
drivers/gpu/drm/i915/intel_dp.c | 55 +++++++++++++++++++++++++++++++----------
4 files changed, 53 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 068b49b..568913b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1761,6 +1761,7 @@ int i915_driver_unload(struct drm_device *dev)
intel_teardown_mchbar(dev);
destroy_workqueue(dev_priv->wq);
+ pm_qos_remove_request(&dev_priv->pm_qos);
pci_dev_put(dev_priv->bridge_dev);
kfree(dev->dev_private);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 13b9e6a..50c6edf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -40,6 +40,7 @@
#include <linux/backlight.h>
#include <linux/intel-iommu.h>
#include <linux/kref.h>
+#include <linux/pm_qos.h>
/* General customization:
*/
@@ -437,6 +438,9 @@ typedef struct drm_i915_private {
/* protects the irq masks */
spinlock_t irq_lock;
+ /* To control wakeup latency, e.g. for irq-driven dp aux transfers. */
+ struct pm_qos_request pm_qos;
+
/* DPIO indirect register protection */
spinlock_t dpio_lock;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c483531..bcb2624 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -607,7 +607,11 @@ static void gmbus_irq_handler(struct drm_device *dev)
static void dp_aux_irq_handler(struct drm_device *dev)
{
+ struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
DRM_DEBUG_DRIVER("AUX channel interrupt\n");
+
+ wake_up_all(&dev_priv->gmbus_wait_queue);
}
static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
@@ -2718,6 +2722,8 @@ void intel_irq_init(struct drm_device *dev)
INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
+ pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, 0);
+
dev->driver->get_vblank_counter = i915_get_vblank_counter;
dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f28353d..5d98a53 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -340,6 +340,28 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
}
}
+static uint32_t
+intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
+{
+ struct drm_device *dev = intel_dp->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint32_t ch_ctl = intel_dp->output_reg + 0x10;
+ uint32_t status;
+ bool done;
+
+#define C (((status = I915_READ(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
+ if (has_aux_irq)
+ done = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
+ else
+ done = wait_for_atomic(C, 10) == 0;
+ if (!done)
+ DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
+ has_aux_irq);
+#undef C
+
+ return status;
+}
+
static int
intel_dp_aux_ch(struct intel_dp *intel_dp,
uint8_t *send, int send_bytes,
@@ -350,11 +372,13 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t ch_ctl = output_reg + 0x10;
uint32_t ch_data = ch_ctl + 4;
- int i;
- int recv_bytes;
+ int i, ret, recv_bytes;
uint32_t status;
uint32_t aux_clock_divider;
int try, precharge;
+ bool has_aux_irq = INTEL_INFO(dev)->gen >= 5;
+
+ pm_qos_update_request(&dev_priv->pm_qos, 0);
intel_dp_check_edp(intel_dp);
/* The clock divider is based off the hrawclk,
@@ -390,7 +414,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
if (try == 3) {
WARN(1, "dp_aux_ch not started status 0x%08x\n",
I915_READ(ch_ctl));
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
/* Must try at least 3 times according to DP spec */
@@ -403,6 +428,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
/* Send the command and wait for it to complete */
I915_WRITE(ch_ctl,
DP_AUX_CH_CTL_SEND_BUSY |
+ (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
DP_AUX_CH_CTL_TIME_OUT_400us |
(send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
(precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
@@ -410,12 +436,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
DP_AUX_CH_CTL_DONE |
DP_AUX_CH_CTL_TIME_OUT_ERROR |
DP_AUX_CH_CTL_RECEIVE_ERROR);
- for (;;) {
- status = I915_READ(ch_ctl);
- if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
- break;
- udelay(100);
- }
+
+ status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
/* Clear done status and any errors */
I915_WRITE(ch_ctl,
@@ -433,7 +455,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
if ((status & DP_AUX_CH_CTL_DONE) == 0) {
DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
/* Check for timeout or receive error.
@@ -441,14 +464,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
*/
if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
DRM_ERROR("dp_aux_ch receive error status 0x%08x\n", status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
/* Timeouts occur when the device isn't connected, so they're
* "normal" -- don't fill the kernel log with these */
if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
DRM_DEBUG_KMS("dp_aux_ch timeout status 0x%08x\n", status);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto out;
}
/* Unload any bytes sent back from the other side */
@@ -461,7 +486,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
unpack_aux(I915_READ(ch_data + i),
recv + i, recv_bytes - i);
- return recv_bytes;
+ ret = recv_bytes;
+out:
+ pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+ return ret;
}
/* Write data to the aux channel in native mode */
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH] drm/i915: irq-drive the dp aux communication
2012-09-09 9:00 ` [PATCH 8/8] drm/i915: irq-drive the dp aux communication Daniel Vetter
@ 2012-09-09 9:29 ` Daniel Vetter
2012-09-09 11:24 ` Chris Wilson
0 siblings, 1 reply; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 9:29 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
At least on the platforms that have a dp aux irq and also have it
enabled - vlv/hsw should have one, too. But I don't have a machine to
test this on, and the current code doesn't support dp yet anyway on
those platforms. Judging from docs there's no dp aux interrupt for gm45.
Also, I only have an ivb cpu edp machine, so the dp aux A code for
snb/ilk is untested.
For dpcd probing when nothing is connected it slashes about 5ms of cpu
time (cpu time is now negligible), which agrees with 3 * 5 400 usec
timeouts.
A previous version of this patch increases the time required to go
through the dp_detect cycle (which includes reading the edid) from
around 33 ms to around 40 ms. Experiments indicated that this is
purely due to the irq latency - the hw doesn't allow us to queue up
dp aux transactions and hence irq latency directly affects throughput.
gmbus is much better, there we have a 8 byte buffer, and we get the
irq once another 4 bytes can be queued up.
But by using the pm_qos interface to request the lowest possible cpu
wake-up latency this slowdown completely disappeared.
Since all our output detection logic is single-threaded with the
mode_config mutex right now anyway, I've decide not ot play fancy and
to just reuse the gmbus wait queue. But this would definitely prep the
way to run dp detection on different ports in parallel
v2: Add a timeout for dp aux transfers when using interrupts - the hw
_does_ prevent this with the hw-based 400 usec timeout, but if the
irq somehow doesn't arrive we're screwed. Lesson learned while
developing this ;-)
v3: While at it also convert the busy-loop to wait_for_atomic, so that
we don't run the risk of an infinite loop any more.
v4: Ensure we have the smallest possible irq latency by using the
pm_qos interface.
v5: Add a comment to the code to explain why we frob pm_qos. Suggested
by Chris Wilson.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/i915_dma.c | 1 +
drivers/gpu/drm/i915/i915_drv.h | 4 +++
drivers/gpu/drm/i915/i915_irq.c | 6 +++++
drivers/gpu/drm/i915/intel_dp.c | 59 ++++++++++++++++++++++++++++++++---------
4 files changed, 57 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 068b49b..568913b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1761,6 +1761,7 @@ int i915_driver_unload(struct drm_device *dev)
intel_teardown_mchbar(dev);
destroy_workqueue(dev_priv->wq);
+ pm_qos_remove_request(&dev_priv->pm_qos);
pci_dev_put(dev_priv->bridge_dev);
kfree(dev->dev_private);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 13b9e6a..50c6edf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -40,6 +40,7 @@
#include <linux/backlight.h>
#include <linux/intel-iommu.h>
#include <linux/kref.h>
+#include <linux/pm_qos.h>
/* General customization:
*/
@@ -437,6 +438,9 @@ typedef struct drm_i915_private {
/* protects the irq masks */
spinlock_t irq_lock;
+ /* To control wakeup latency, e.g. for irq-driven dp aux transfers. */
+ struct pm_qos_request pm_qos;
+
/* DPIO indirect register protection */
spinlock_t dpio_lock;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index c483531..bcb2624 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -607,7 +607,11 @@ static void gmbus_irq_handler(struct drm_device *dev)
static void dp_aux_irq_handler(struct drm_device *dev)
{
+ struct drm_i915_private *dev_priv = (drm_i915_private_t *) dev->dev_private;
+
DRM_DEBUG_DRIVER("AUX channel interrupt\n");
+
+ wake_up_all(&dev_priv->gmbus_wait_queue);
}
static void ibx_irq_handler(struct drm_device *dev, u32 pch_iir)
@@ -2718,6 +2722,8 @@ void intel_irq_init(struct drm_device *dev)
INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
+ pm_qos_add_request(&dev_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, 0);
+
dev->driver->get_vblank_counter = i915_get_vblank_counter;
dev->max_vblank_count = 0xffffff; /* only 24 bits of frame count */
if (IS_G4X(dev) || INTEL_INFO(dev)->gen >= 5) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f28353d..0ddc9be 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -340,6 +340,28 @@ intel_dp_check_edp(struct intel_dp *intel_dp)
}
}
+static uint32_t
+intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
+{
+ struct drm_device *dev = intel_dp->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint32_t ch_ctl = intel_dp->output_reg + 0x10;
+ uint32_t status;
+ bool done;
+
+#define C (((status = I915_READ(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
+ if (has_aux_irq)
+ done = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
+ else
+ done = wait_for_atomic(C, 10) == 0;
+ if (!done)
+ DRM_ERROR("dp aux hw did not signal timeout (has irq: %i)!\n",
+ has_aux_irq);
+#undef C
+
+ return status;
+}
+
static int
intel_dp_aux_ch(struct intel_dp *intel_dp,
uint8_t *send, int send_bytes,
@@ -350,11 +372,17 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
struct drm_i915_private *dev_priv = dev->dev_private;
uint32_t ch_ctl = output_reg + 0x10;
uint32_t ch_data = ch_ctl + 4;
- int i;
- int recv_bytes;
+ int i, ret, recv_bytes;
uint32_t status;
uint32_t aux_clock_divider;
int try, precharge;
+ bool has_aux_irq = INTEL_INFO(dev)->gen >= 5;
+
+ /* dp aux is extremely sensitive to irq latency, hence request the
+ * lowest possible wakeup latency and so prevent the cpu from going into
+ * deep sleep states.
+ */
+ pm_qos_update_request(&dev_priv->pm_qos, 0);
intel_dp_check_edp(intel_dp);
/* The clock divider is based off the hrawclk,
@@ -390,7 +418,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
if (try == 3) {
WARN(1, "dp_aux_ch not started status 0x%08x\n",
I915_READ(ch_ctl));
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
/* Must try at least 3 times according to DP spec */
@@ -403,6 +432,7 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
/* Send the command and wait for it to complete */
I915_WRITE(ch_ctl,
DP_AUX_CH_CTL_SEND_BUSY |
+ (has_aux_irq ? DP_AUX_CH_CTL_INTERRUPT : 0) |
DP_AUX_CH_CTL_TIME_OUT_400us |
(send_bytes << DP_AUX_CH_CTL_MESSAGE_SIZE_SHIFT) |
(precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
@@ -410,12 +440,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
DP_AUX_CH_CTL_DONE |
DP_AUX_CH_CTL_TIME_OUT_ERROR |
DP_AUX_CH_CTL_RECEIVE_ERROR);
- for (;;) {
- status = I915_READ(ch_ctl);
- if ((status & DP_AUX_CH_CTL_SEND_BUSY) == 0)
- break;
- udelay(100);
- }
+
+ status = intel_dp_aux_wait_done(intel_dp, has_aux_irq);
/* Clear done status and any errors */
I915_WRITE(ch_ctl,
@@ -433,7 +459,8 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
if ((status & DP_AUX_CH_CTL_DONE) == 0) {
DRM_ERROR("dp_aux_ch not done status 0x%08x\n", status);
- return -EBUSY;
+ ret = -EBUSY;
+ goto out;
}
/* Check for timeout or receive error.
@@ -441,14 +468,16 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
*/
if (status & DP_AUX_CH_CTL_RECEIVE_ERROR) {
DRM_ERROR("dp_aux_ch receive error status 0x%08x\n", status);
- return -EIO;
+ ret = -EIO;
+ goto out;
}
/* Timeouts occur when the device isn't connected, so they're
* "normal" -- don't fill the kernel log with these */
if (status & DP_AUX_CH_CTL_TIME_OUT_ERROR) {
DRM_DEBUG_KMS("dp_aux_ch timeout status 0x%08x\n", status);
- return -ETIMEDOUT;
+ ret = -ETIMEDOUT;
+ goto out;
}
/* Unload any bytes sent back from the other side */
@@ -461,7 +490,11 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
unpack_aux(I915_READ(ch_data + i),
recv + i, recv_bytes - i);
- return recv_bytes;
+ ret = recv_bytes;
+out:
+ pm_qos_update_request(&dev_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
+
+ return ret;
}
/* Write data to the aux channel in native mode */
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH] drm/i915: irq-drive the dp aux communication
2012-09-09 9:29 ` [PATCH] " Daniel Vetter
@ 2012-09-09 11:24 ` Chris Wilson
0 siblings, 0 replies; 12+ messages in thread
From: Chris Wilson @ 2012-09-09 11:24 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
On Sun, 9 Sep 2012 11:29:24 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> At least on the platforms that have a dp aux irq and also have it
> enabled - vlv/hsw should have one, too. But I don't have a machine to
> test this on, and the current code doesn't support dp yet anyway on
> those platforms. Judging from docs there's no dp aux interrupt for gm45.
>
> Also, I only have an ivb cpu edp machine, so the dp aux A code for
> snb/ilk is untested.
Typing this on a ilk pch dp, it seems to be holding up fine.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] drm/i915: use _NOTRACE for gmbus/dp aux wait loops
2012-09-09 9:00 [PATCH 1/8] drm/i915: reorder setup sequence to have irqs for output setup Daniel Vetter
` (6 preceding siblings ...)
2012-09-09 9:00 ` [PATCH 8/8] drm/i915: irq-drive the dp aux communication Daniel Vetter
@ 2012-09-09 10:24 ` Daniel Vetter
7 siblings, 0 replies; 12+ messages in thread
From: Daniel Vetter @ 2012-09-09 10:24 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter
Less clutter in the traces. And in both cases we yell rather loud
into the logs if we time out. Patch suggested by Chris Wilson.
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_dp.c | 2 +-
drivers/gpu/drm/i915/intel_i2c.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0ddc9be..1aae1a1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -349,7 +349,7 @@ intel_dp_aux_wait_done(struct intel_dp *intel_dp, bool has_aux_irq)
uint32_t status;
bool done;
-#define C (((status = I915_READ(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
+#define C (((status = I915_READ_NOTRACE(ch_ctl)) & DP_AUX_CH_CTL_SEND_BUSY) == 0)
if (has_aux_irq)
done = wait_event_timeout(dev_priv->gmbus_wait_queue, C, 10);
else
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index 4a38a30..31b1a65 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -224,7 +224,7 @@ gmbus_wait_hw_status(struct drm_i915_private *dev_priv,
prepare_to_wait(&dev_priv->gmbus_wait_queue, &wait,
TASK_UNINTERRUPTIBLE);
- gmbus2 = I915_READ(GMBUS2 + reg_offset);
+ gmbus2 = I915_READ_NOTRACE(GMBUS2 + reg_offset);
if (gmbus2 & (GMBUS_SATOER | gmbus2_status))
break;
@@ -247,7 +247,7 @@ gmbus_wait_idle(struct drm_i915_private *dev_priv)
int ret;
int reg_offset = dev_priv->gpio_mmio_base;
-#define C ((I915_READ(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0)
+#define C ((I915_READ_NOTRACE(GMBUS2 + reg_offset) & GMBUS_ACTIVE) == 0)
if (!HAS_GMBUS_IRQ(dev_priv->dev))
return wait_for(C, 10);
--
1.7.11.2
^ permalink raw reply related [flat|nested] 12+ messages in thread