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

[Tim Gardner]

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 at 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 at canonical.com>

-- 
Tim Gardner tim.gardner at canonical.com