From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue Date: Tue, 20 Aug 2013 16:26:44 +0200 Message-ID: <20130820142644.GU776@phenom.ffwll.local> References: <1375826239-3060-1-git-send-email-przanoni@gmail.com> <1376078677-24901-1-git-send-email-przanoni@gmail.com> <1376078677-24901-2-git-send-email-przanoni@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f169.google.com (mail-ea0-f169.google.com [209.85.215.169]) by gabe.freedesktop.org (Postfix) with ESMTP id 7CA5BE6A22 for ; Tue, 20 Aug 2013 07:26:34 -0700 (PDT) Received: by mail-ea0-f169.google.com with SMTP id k11so260056eaj.28 for ; Tue, 20 Aug 2013 07:26:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1376078677-24901-2-git-send-email-przanoni@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Paulo Zanoni Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, Aug 09, 2013 at 05:04:36PM -0300, Paulo Zanoni wrote: > From: Paulo Zanoni > > It seems we've been doing this ever since we started processing the > RPS events on a work queue, on commit "drm/i915: move gen6 rps > handling to workqueue", 4912d04193733a825216b926ffd290fada88ab07. > > The problem is: when we add work to the queue, instead of just masking > the bits we queued and leaving all the others on their current state, > we mask the bits we queued and unmask all the others. This basically > means we'll be unmasking a bunch of interrupts we're not going to > process. And if you look at gen6_pm_rps_work, we unmask back only > GEN6_PM_RPS_EVENTS, which means the bits we unmasked when adding work > to the queue will remain unmasked after we process the queue. > > Notice that even though we unmask those unrelated interrupts, we never > enable them on IER, so they don't fire our interrupt handler, they > just stay there on IIR waiting to be cleared when something else > triggers the interrupt handler. > > So this patch does what seems to make more sense: mask only the bits > we add to the queue, without unmasking anything else, and so we'll > unmask them after we process the queue. > > As a side effect we also have to remove that WARN, because it is not > only making sure we don't mask useful interrupts, it is also making > sure we do unmask useless interrupts! That piece of code should not be > responsible for knowing which bits should be unmasked, so just don't > assert anything, and trust that snb_disable_pm_irq should be doing the > right thing. > > With i915.enable_pc8=1 I was getting ocasional "GEN6_PMIIR is not 0" > error messages due to the fact that we unmask those unrelated > interrupts but don't enable them. > > Note: if bugs start bisecting to this patch, then it probably means > someone was relying on the fact that we unmask everything by accident, > then we should fix gen5_gt_irq_postinstall or whoever needs the > accidentally unmasked interrupts. Or maybe I was just wrong and we > need to revert this patch :) > > Signed-off-by: Paulo Zanoni I've added another note here explaining that with the addition of VEBOX this little bug started to matter: After the first rps interrupt we'll never mask the VEBOX user interrupt again and so blow through cpu time needlessly when running video workloads using VEBOX. -Daniel > --- > drivers/gpu/drm/i915/i915_irq.c | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 5b51c43..d9ebfb6 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -167,11 +167,6 @@ void snb_disable_pm_irq(struct drm_i915_private *dev_priv, uint32_t mask) > snb_update_pm_irq(dev_priv, mask, 0); > } > > -static void snb_set_pm_irq(struct drm_i915_private *dev_priv, uint32_t val) > -{ > - snb_update_pm_irq(dev_priv, 0xffffffff, ~val); > -} > - > static bool ivb_can_enable_err_int(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -960,7 +955,7 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, > > spin_lock(&dev_priv->irq_lock); > dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS; > - snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir); > + snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS); > spin_unlock(&dev_priv->irq_lock); > > queue_work(dev_priv->wq, &dev_priv->rps.work); > @@ -1043,9 +1038,7 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv, > if (pm_iir & GEN6_PM_RPS_EVENTS) { > spin_lock(&dev_priv->irq_lock); > dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS; > - snb_set_pm_irq(dev_priv, dev_priv->rps.pm_iir); > - /* never want to mask useful interrupts. */ > - WARN_ON(I915_READ_NOTRACE(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS); > + snb_disable_pm_irq(dev_priv, pm_iir & GEN6_PM_RPS_EVENTS); > spin_unlock(&dev_priv->irq_lock); > > queue_work(dev_priv->wq, &dev_priv->rps.work); > -- > 1.8.1.2 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch