Patchwork [Karmic] SRU: [PATCH] drm-i915-remove-loop-in-Ironlake-interrupt-handler

login
register
mail settings
Submitter Steve Conklin
Date Jan. 20, 2010, 3:42 a.m.
Message ID <4B567B9D.6070100@canonical.com>
Download mbox | patch
Permalink /patch/43259/
State Accepted
Delegated to: Stefan Bader
Headers show

Comments

Steve Conklin - Jan. 20, 2010, 3:42 a.m.
The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
Linus's tree. There has been an urgent request made by intel to apply this to
the stable tree, and it should be in Karmic as well.

I welcome additional reviews of this patch, as it required some tweaking to
apply and it's ISR code.

As far as I know, this is the last remaining patch for the moment that is
required in Karmic for Ironlake.

Steve
Tim Gardner - Jan. 20, 2010, 1:45 p.m.
Steve Conklin wrote:
> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
> Linus's tree. There has been an urgent request made by intel to apply this to
> the stable tree, and it should be in Karmic as well.
> 
> I welcome additional reviews of this patch, as it required some tweaking to
> apply and it's ISR code.
> 
> As far as I know, this is the last remaining patch for the moment that is
> required in Karmic for Ironlake.
> 
> Steve
> 

I noticed 3 things about the resulting patch.

1) This handler is not very efficient. There is typically one or more
bits in a single register that indicates if this instance of the card
actually generated the interrupt. In this case it looks like DEIER has
that information. Does the handler really need to do 6 register reads
and 2 register writes simply to determine it has nothing to do.

2) Depending on the HW design, there may be a race between when the IIR
registers are read, and when their conditions are cleared. For example,
DEIIR could go active after having been read, but before its cleared at
the end of the handler.

3) Why is there an additional variable introduced 'u32 segno' ?

rtg
Steve Conklin - Jan. 20, 2010, 5:45 p.m.
On 01/20/2010 07:45 AM, Tim Gardner wrote:
> Steve Conklin wrote:
>> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
>> Linus's tree. There has been an urgent request made by intel to apply this to
>> the stable tree, and it should be in Karmic as well.
>>
>> I welcome additional reviews of this patch, as it required some tweaking to
>> apply and it's ISR code.
>>
>> As far as I know, this is the last remaining patch for the moment that is
>> required in Karmic for Ironlake.
>>
>> Steve
>>
> 
> I noticed 3 things about the resulting patch.
> 
> 1) This handler is not very efficient. There is typically one or more
> bits in a single register that indicates if this instance of the card
> actually generated the interrupt. In this case it looks like DEIER has
> that information. Does the handler really need to do 6 register reads
> and 2 register writes simply to determine it has nothing to do.
> 
> 2) Depending on the HW design, there may be a race between when the IIR
> registers are read, and when their conditions are cleared. For example,
> DEIIR could go active after having been read, but before its cleared at
> the end of the handler.
> 
> 3) Why is there an additional variable introduced 'u32 segno' ?
> 
> rtg

Upon further review, I see some divergence from both upstream and what's in
Linus's tree that worry me. I'm going to investigate but in the meantime, this
patch should be withdrawn from consideration for Karmic.

Steve
Steve Conklin - Jan. 20, 2010, 9:49 p.m.
On 01/20/2010 07:45 AM, Tim Gardner wrote:
> Steve Conklin wrote:
>> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
>> Linus's tree. There has been an urgent request made by intel to apply this to
>> the stable tree, and it should be in Karmic as well.
>>
>> I welcome additional reviews of this patch, as it required some tweaking to
>> apply and it's ISR code.
>>
>> As far as I know, this is the last remaining patch for the moment that is
>> required in Karmic for Ironlake.
>>
>> Steve
>>
> 
> I noticed 3 things about the resulting patch.
> 
> 1) This handler is not very efficient. There is typically one or more
> bits in a single register that indicates if this instance of the card
> actually generated the interrupt. In this case it looks like DEIER has
> that information. Does the handler really need to do 6 register reads
> and 2 register writes simply to determine it has nothing to do.
> 
> 2) Depending on the HW design, there may be a race between when the IIR
> registers are read, and when their conditions are cleared. For example,
> DEIIR could go active after having been read, but before its cleared at
> the end of the handler.
> 
> 3) Why is there an additional variable introduced 'u32 segno' ?
> 
> rtg

I'm convinced that the patch is good. It leaves this function identical to
what's now in Linus's upstream tree. As for the questions above, I've pasted the
complete patched function below, and my answers are:

1) There aren't really any extra reads. The "disable master interrupt" bit
accesses the DEIER register correctly, and looks pretty standard. The throwaway
read isn't documented as far as I can find in the hardware documentation but
it's been in the driver for a very long time.

To determine whether there is an interrupt pending that we care about, and to
determine the source, requires reading three registers - DEIIR, GTIIR, and
SDEIIR. If these three are zero, then we finish out and are done. The registers
are only read once.

2)  Also, the only bits which cleared are the ones which were set when we got
interrupted, which should all have been handled. Without the documentation about
the specifics of this I think we have to assume it's correct. It may be in the
manuals, I haven't found it.

3) That variable was added by an earlier patch which makes tracing the driver
easier - the description frmo the commit to Linus's tree is:

commit 1c5d22f76dc721f3acb7a3dadc657a221e487fb7
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Aug 25 11:15:50 2009 +0100

    drm/i915: Add tracepoints

    By adding tracepoint equivalents for WATCH_BUF/EXEC we are able to monitor
    the lifetimes of objects, requests and significant events. These events can
    then be probed using the tracing frameworks, such as systemtap and, in
    particular, perf.

I hereby renew my request to have this pulled in to SRU proposed for Karmic.

Steve

-----------------------------

irqreturn_t igdng_irq_handler(struct drm_device *dev)
{
        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;
        struct drm_i915_master_private *master_priv;

        /* disable master interrupt before clearing iir  */
        de_ier = I915_READ(DEIER);
        I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
        (void)I915_READ(DEIER);

        de_iir = I915_READ(DEIIR);
        gt_iir = I915_READ(GTIIR);
        pch_iir = I915_READ(SDEIIR);

        if (de_iir == 0 && gt_iir == 0 && pch_iir == 0)
                goto done;

        ret = IRQ_HANDLED;

        if (dev->primary->master) {
                master_priv = dev->primary->master->driver_priv;
                if (master_priv->sarea_priv)
                        master_priv->sarea_priv->last_dispatch =
                                READ_BREADCRUMB(dev_priv);
        }

        if (gt_iir & GT_USER_INTERRUPT) {
                u32 seqno = i915_get_gem_seqno(dev);
                dev_priv->mm.irq_gem_seqno = seqno;
                trace_i915_gem_request_complete(dev, seqno);
                DRM_WAKEUP(&dev_priv->irq_queue);
                dev_priv->hangcheck_count = 0;
                mod_timer(&dev_priv->hangcheck_timer, jiffies +
DRM_I915_HANGCHECK_PERIOD);
        }

        if (de_iir & DE_GSE)
                ironlake_opregion_gse_intr(dev);

        /* check event from PCH */
        if ((de_iir & DE_PCH_EVENT) &&
            (pch_iir & SDE_HOTPLUG_MASK)) {
                queue_work(dev_priv->wq, &dev_priv->hotplug_work);
        }

        /* should clear PCH hotplug event before clear CPU irq */
        I915_WRITE(SDEIIR, pch_iir);
        I915_WRITE(GTIIR, gt_iir);
        I915_WRITE(DEIIR, de_iir);

done:
        I915_WRITE(DEIER, de_ier);
        (void)I915_READ(DEIER);

        return ret;
}
Stefan Bader - Jan. 21, 2010, 10:13 a.m.
Steve Conklin wrote:
> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
> Linus's tree. There has been an urgent request made by intel to apply this to
> the stable tree, and it should be in Karmic as well.
> 
> I welcome additional reviews of this patch, as it required some tweaking to
> apply and it's ISR code.
> 
> As far as I know, this is the last remaining patch for the moment that is
> required in Karmic for Ironlake.
> 
> Steve
> 

This one is queued for upstream stable 2.6.32.5. It looks ok to me. The only
slight difference seems to be that it moves writing into the IIRs after the
other actions, but this might have no impact at all.

Steve, I need a (public) bug number for this request. I did not see any. For the
patch part:

Acked-by: Stefan Bader <stefan.bader@canonical.com>
Tim Gardner - Jan. 21, 2010, 12:59 p.m.
Steve Conklin wrote:
> On 01/20/2010 07:45 AM, Tim Gardner wrote:
>> Steve Conklin wrote:
>>> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
>>> Linus's tree. There has been an urgent request made by intel to apply this to
>>> the stable tree, and it should be in Karmic as well.
>>>
>>> I welcome additional reviews of this patch, as it required some tweaking to
>>> apply and it's ISR code.
>>>
>>> As far as I know, this is the last remaining patch for the moment that is
>>> required in Karmic for Ironlake.
>>>
>>> Steve
>>>
>> I noticed 3 things about the resulting patch.
>>
>> 1) This handler is not very efficient. There is typically one or more
>> bits in a single register that indicates if this instance of the card
>> actually generated the interrupt. In this case it looks like DEIER has
>> that information. Does the handler really need to do 6 register reads
>> and 2 register writes simply to determine it has nothing to do.
>>
>> 2) Depending on the HW design, there may be a race between when the IIR
>> registers are read, and when their conditions are cleared. For example,
>> DEIIR could go active after having been read, but before its cleared at
>> the end of the handler.
>>
>> 3) Why is there an additional variable introduced 'u32 segno' ?
>>
>> rtg
> 
> I'm convinced that the patch is good. It leaves this function identical to
> what's now in Linus's upstream tree. As for the questions above, I've pasted the
> complete patched function below, and my answers are:
> 
> 1) There aren't really any extra reads. The "disable master interrupt" bit
> accesses the DEIER register correctly, and looks pretty standard. The throwaway
> read isn't documented as far as I can find in the hardware documentation but
> it's been in the driver for a very long time.
> 
> To determine whether there is an interrupt pending that we care about, and to
> determine the source, requires reading three registers - DEIIR, GTIIR, and
> SDEIIR. If these three are zero, then we finish out and are done. The registers
> are only read once.
> 
> 2)  Also, the only bits which cleared are the ones which were set when we got
> interrupted, which should all have been handled. Without the documentation about
> the specifics of this I think we have to assume it's correct. It may be in the
> manuals, I haven't found it.
> 
> 3) That variable was added by an earlier patch which makes tracing the driver
> easier - the description frmo the commit to Linus's tree is:
> 
> commit 1c5d22f76dc721f3acb7a3dadc657a221e487fb7
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Aug 25 11:15:50 2009 +0100
> 
>     drm/i915: Add tracepoints
> 
>     By adding tracepoint equivalents for WATCH_BUF/EXEC we are able to monitor
>     the lifetimes of objects, requests and significant events. These events can
>     then be probed using the tracing frameworks, such as systemtap and, in
>     particular, perf.
> 
> I hereby renew my request to have this pulled in to SRU proposed for Karmic.
> 
> Steve
> 
> -----------------------------
> 
> irqreturn_t igdng_irq_handler(struct drm_device *dev)
> {
>         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;
>         struct drm_i915_master_private *master_priv;
> 
>         /* disable master interrupt before clearing iir  */
>         de_ier = I915_READ(DEIER);
>         I915_WRITE(DEIER, de_ier & ~DE_MASTER_IRQ_CONTROL);
>         (void)I915_READ(DEIER);
> 
>         de_iir = I915_READ(DEIIR);
>         gt_iir = I915_READ(GTIIR);
>         pch_iir = I915_READ(SDEIIR);
> 
>         if (de_iir == 0 && gt_iir == 0 && pch_iir == 0)
>                 goto done;
> 
>         ret = IRQ_HANDLED;
> 
>         if (dev->primary->master) {
>                 master_priv = dev->primary->master->driver_priv;
>                 if (master_priv->sarea_priv)
>                         master_priv->sarea_priv->last_dispatch =
>                                 READ_BREADCRUMB(dev_priv);
>         }
> 
>         if (gt_iir & GT_USER_INTERRUPT) {
>                 u32 seqno = i915_get_gem_seqno(dev);
>                 dev_priv->mm.irq_gem_seqno = seqno;
>                 trace_i915_gem_request_complete(dev, seqno);
>                 DRM_WAKEUP(&dev_priv->irq_queue);
>                 dev_priv->hangcheck_count = 0;
>                 mod_timer(&dev_priv->hangcheck_timer, jiffies +
> DRM_I915_HANGCHECK_PERIOD);
>         }
> 
>         if (de_iir & DE_GSE)
>                 ironlake_opregion_gse_intr(dev);
> 
>         /* check event from PCH */
>         if ((de_iir & DE_PCH_EVENT) &&
>             (pch_iir & SDE_HOTPLUG_MASK)) {
>                 queue_work(dev_priv->wq, &dev_priv->hotplug_work);
>         }
> 
>         /* should clear PCH hotplug event before clear CPU irq */
>         I915_WRITE(SDEIIR, pch_iir);
>         I915_WRITE(GTIIR, gt_iir);
>         I915_WRITE(DEIIR, de_iir);
> 
> done:
>         I915_WRITE(DEIER, de_ier);
>         (void)I915_READ(DEIER);
> 
>         return ret;
> }
> 

Acked-by: Tim Gardner <tim.gardner@canonical.com>
Steve Conklin - Jan. 21, 2010, 4:09 p.m.
On 01/21/2010 04:13 AM, Stefan Bader wrote:
> Steve Conklin wrote:
>> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
>> Linus's tree. There has been an urgent request made by intel to apply this to
>> the stable tree, and it should be in Karmic as well.
>>
>> I welcome additional reviews of this patch, as it required some tweaking to
>> apply and it's ISR code.
>>
>> As far as I know, this is the last remaining patch for the moment that is
>> required in Karmic for Ironlake.
>>
>> Steve
>>
> 
> This one is queued for upstream stable 2.6.32.5. It looks ok to me. The only
> slight difference seems to be that it moves writing into the IIRs after the
> other actions, but this might have no impact at all.
> 
> Steve, I need a (public) bug number for this request. I did not see any. For the
> patch part:
> 
> Acked-by: Stefan Bader <stefan.bader@canonical.com>

Bug number 510722
Stefan Bader - Jan. 22, 2010, 3:48 p.m.
Steve Conklin wrote:
> The attached patch is derived from c7c85101afd0cb8ce497456d12ee1cad4aad152f in
> Linus's tree. There has been an urgent request made by intel to apply this to
> the stable tree, and it should be in Karmic as well.
> 
> I welcome additional reviews of this patch, as it required some tweaking to
> apply and it's ISR code.
> 
> As far as I know, this is the last remaining patch for the moment that is
> required in Karmic for Ironlake.
> 
> Steve
> 
Applied to Karmic. Seen this included in 2.6.32.5, so next Lucid pull of stable
should close it there.

Patch

From 3134a9e28609358fb0e24e02bdbb2da83bdb285e Mon Sep 17 00:00:00 2001
From: Zou Nan hai <Nanhai.zou@intel.com>
Date: Fri, 15 Jan 2010 10:29:06 +0800
Subject: [PATCH] drm/i915: remove loop in Ironlake interrupt handler

On Ironlake, there is an interrupt master control bit. With the bit
disabled before clearing IIR, we do not need to handle extra interrupt
in a loop. This patch removes the loop in Ironlake interrupt handler.
It fixed irq lost issue on some Ironlake platforms.

Refactored slightly for karmic because it wouldn't apply directly.

Signed-off-by: Zou Nan hai <Nanhai.zou@intel.com>
Signed-off-by: Zhenyu Wang <zhenyuw@linux.intel.com>
Signed-off-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Steve Conklin <sconklin@canonical.com>
---
 drivers/gpu/drm/i915/i915_irq.c |   61 ++++++++++++++++----------------------
 1 files changed, 26 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5caf737..84a47c7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -268,7 +268,6 @@  irqreturn_t igdng_irq_handler(struct drm_device *dev)
 	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;
-	u32 new_de_iir, new_gt_iir, new_pch_iir;
 	struct drm_i915_master_private *master_priv;
 
 	/* disable master interrupt before clearing iir  */
@@ -280,47 +279,39 @@  irqreturn_t igdng_irq_handler(struct drm_device *dev)
 	gt_iir = I915_READ(GTIIR);
 	pch_iir = I915_READ(SDEIIR);
 
-	for (;;) {
-		if (de_iir == 0 && gt_iir == 0 && pch_iir == 0)
-			break;
-
-		ret = IRQ_HANDLED;
-
-		/* should clear PCH hotplug event before clear CPU irq */
-		I915_WRITE(SDEIIR, pch_iir);
-		new_pch_iir = I915_READ(SDEIIR);
+	if (de_iir == 0 && gt_iir == 0 && pch_iir == 0)
+		goto done;
 
-		I915_WRITE(DEIIR, de_iir);
-		new_de_iir = I915_READ(DEIIR);
-		I915_WRITE(GTIIR, gt_iir);
-		new_gt_iir = I915_READ(GTIIR);
-
-		if (dev->primary->master) {
-			master_priv = dev->primary->master->driver_priv;
-			if (master_priv->sarea_priv)
-				master_priv->sarea_priv->last_dispatch =
-					READ_BREADCRUMB(dev_priv);
-		}
+	ret = IRQ_HANDLED;
 
-		if (gt_iir & GT_USER_INTERRUPT) {
-			dev_priv->mm.irq_gem_seqno = i915_get_gem_seqno(dev);
-			DRM_WAKEUP(&dev_priv->irq_queue);
-		}
+	if (dev->primary->master) {
+		master_priv = dev->primary->master->driver_priv;
+		if (master_priv->sarea_priv)
+			master_priv->sarea_priv->last_dispatch =
+				READ_BREADCRUMB(dev_priv);
+	}
 
-		if (de_iir & DE_GSE)
-			ironlake_opregion_gse_intr(dev);
+	if (gt_iir & GT_USER_INTERRUPT) {
+		u32 seqno = i915_get_gem_seqno(dev);
+		dev_priv->mm.irq_gem_seqno = seqno;
+		DRM_WAKEUP(&dev_priv->irq_queue);
+	}
 
-		/* check event from PCH */
-		if ((de_iir & DE_PCH_EVENT) &&
-			(pch_iir & SDE_HOTPLUG_MASK)) {
-			queue_work(dev_priv->wq, &dev_priv->hotplug_work);
-		}
+	if (de_iir & DE_GSE)
+		ironlake_opregion_gse_intr(dev);
 
-		de_iir = new_de_iir;
-		gt_iir = new_gt_iir;
-		pch_iir = new_pch_iir;
+	/* check event from PCH */
+	if ((de_iir & DE_PCH_EVENT) &&
+	    (pch_iir & SDE_HOTPLUG_MASK)) {
+		queue_work(dev_priv->wq, &dev_priv->hotplug_work);
 	}
 
+	/* should clear PCH hotplug event before clear CPU irq */
+	I915_WRITE(SDEIIR, pch_iir);
+	I915_WRITE(GTIIR, gt_iir);
+	I915_WRITE(DEIIR, de_iir);
+
+done:
 	I915_WRITE(DEIER, de_ier);
 	(void)I915_READ(DEIER);
 
-- 
1.6.3.3