On Wed, May 29, 2013 at 09:19:25PM +0200, Daniel Vetter wrote:
> On Tue, May 28, 2013 at 07:22:24PM -0700, Ben Widawsky wrote:
> > HSW has some special requirements for the VEBOX. Splitting out the
> > interrupt handler will make the code a bit nicer and less error prone
> > when we begin to handle those.
> >
> > The slight functional change in this patch (queueing work while holding
> > the spinlock) is intentional as it makes a subsequent patch a bit nicer.
> > The change should also only effect HSW platforms.
> >
> > Reviewed-by: Damien Lespiau <damien.lespiau at intel.com>
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 557acd3..c7b51c2 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -842,6 +842,7 @@ static void snb_gt_irq_handler(struct drm_device *dev,
> > ivybridge_handle_parity_error(dev);
> > }
> >
> > +/* Legacy way of handling PM interrupts */
> > static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> > u32 pm_iir)
> > {
> > @@ -921,6 +922,31 @@ static void dp_aux_irq_handler(struct drm_device *dev)
> > wake_up_all(&dev_priv->gmbus_wait_queue);
> > }
> >
> > +/* Unlike gen6_queue_rps_work() from which this function is originally derived,
> > + * we must be able to deal with other PM interrupts. This is complicated because
> > + * of the way in which we use the masks to defer the RPS work (which for
> > + * posterity is necessary because of forcewake).
> > + */
> > +static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
> > + u32 pm_iir)
> > +{
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&dev_priv->rps.lock, flags);
> > + dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_DEFERRED_EVENTS;
> > + if (dev_priv->rps.pm_iir) {
> > + I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
> > + /* never want to mask useful interrupts. (also posting read) */
> > + WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_DEFERRED_EVENTS);
> > + /* TODO: if queue_work is slow, move it out of the spinlock */
> > + queue_work(dev_priv->wq, &dev_priv->rps.work);
>
> Not happy how hsw and gen6 rps queueing now differs ever so slightly. It's
> rather completely irrelevant where we actually queue the work since:
> - interrupt handlers are non-reentrant
> - queue_work is irq safe anyway.
> So smells like cargo-culting too me.
>
> So I've killed your TODO and moved it out. If there's indeed a good reason
> later on we can reconsider.
Dropped this change again since it affects correctness later on.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
