From mboxrd@z Thu Jan 1 00:00:00 1970 From: Paulo Zanoni Subject: [PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue Date: Fri, 9 Aug 2013 17:04:36 -0300 Message-ID: <1376078677-24901-2-git-send-email-przanoni@gmail.com> References: <1375826239-3060-1-git-send-email-przanoni@gmail.com> <1376078677-24901-1-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-gh0-f181.google.com (mail-gh0-f181.google.com [209.85.160.181]) by gabe.freedesktop.org (Postfix) with ESMTP id A1A36E5FFC for ; Fri, 9 Aug 2013 13:05:21 -0700 (PDT) Received: by mail-gh0-f181.google.com with SMTP id z12so173203ghb.12 for ; Fri, 09 Aug 2013 13:05:21 -0700 (PDT) In-Reply-To: <1376078677-24901-1-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: intel-gfx@lists.freedesktop.org Cc: Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org 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 --- 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