All of lore.kernel.org
 help / color / mirror / Atom feed
From: Paulo Zanoni <przanoni@gmail.com>
To: intel-gfx@lists.freedesktop.org
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
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	[thread overview]
Message-ID: <1376078677-24901-2-git-send-email-przanoni@gmail.com> (raw)
In-Reply-To: <1376078677-24901-1-git-send-email-przanoni@gmail.com>

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 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

  reply	other threads:[~2013-08-09 20:05 UTC|newest]

Thread overview: 50+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-06 21:57 [PATCH 0/9] Haswell Package C8+ Paulo Zanoni
2013-08-06 21:57 ` [PATCH 1/9] drm/i915: add the FCLK case to intel_ddi_get_cdclk_freq Paulo Zanoni
2013-08-14 18:42   ` Rodrigo Vivi
2013-08-06 21:57 ` [PATCH 2/9] drm/i915: wrap GTIMR changes Paulo Zanoni
2013-08-15  0:19   ` Rodrigo Vivi
2013-08-15 13:21     ` Paulo Zanoni
2013-08-06 21:57 ` [PATCH 3/9] drm/i915: wrap GEN6_PMIMR changes Paulo Zanoni
2013-08-15  0:22   ` Rodrigo Vivi
2013-08-15 13:23     ` Paulo Zanoni
2013-08-06 21:57 ` [PATCH 4/9] drm/i915: don't update GEN6_PMIMR when it's not needed Paulo Zanoni
2013-08-07  0:35   ` Chris Wilson
2013-08-07 13:34     ` Paulo Zanoni
2013-08-07 14:14       ` Chris Wilson
2013-08-20 14:18         ` Daniel Vetter
2013-08-15  0:28   ` Rodrigo Vivi
2013-08-06 21:57 ` [PATCH 5/9] drm/i915: add dev_priv->pm_irq_mask Paulo Zanoni
2013-08-15  0:36   ` Rodrigo Vivi
2013-08-15 13:31     ` Paulo Zanoni
2013-08-06 21:57 ` [PATCH 6/9] drm/i915: don't disable/reenable IVB error interrupts when not needed Paulo Zanoni
2013-08-15  0:41   ` Rodrigo Vivi
2013-08-20 14:21   ` Daniel Vetter
2013-08-20 14:43     ` Paulo Zanoni
2013-08-20 15:11       ` Daniel Vetter
2013-08-20 18:07         ` Paulo Zanoni
2013-08-06 21:57 ` [PATCH 7/9] drm/i915: allow package C8+ states on Haswell (disabled) Paulo Zanoni
2013-08-07  0:54   ` Chris Wilson
2013-08-07 13:38     ` Paulo Zanoni
2013-08-07 14:20       ` Chris Wilson
2013-08-07 16:02         ` Daniel Vetter
2013-08-09 20:10           ` Paulo Zanoni
2013-08-09 20:32             ` Chris Wilson
2013-08-09 21:34               ` Paulo Zanoni
2013-08-10  7:55                 ` Daniel Vetter
2013-08-10  8:04                   ` Chris Wilson
2013-08-12 22:02                     ` Paulo Zanoni
2013-08-09 20:42             ` Chris Wilson
2013-08-09 21:25               ` Paulo Zanoni
2013-08-06 21:57 ` [PATCH 8/9] drm/i915: add i915_pc8_status debugfs file Paulo Zanoni
2013-08-06 21:57 ` [PATCH 9/9] drm/i915: enable Package C8+ by default Paulo Zanoni
2013-08-06 22:31 ` [PATCH 0/9] Haswell Package C8+ Daniel Vetter
2013-08-07 13:30   ` Paulo Zanoni
2013-08-09 20:04 ` [PATCH 6.1/9] drm/i915: don't queue PM events we won't process Paulo Zanoni
2013-08-09 20:04   ` Paulo Zanoni [this message]
2013-08-20 14:26     ` [PATCH 6.2/9] drm/i915: fix how we mask PMIMR when adding work to the queue Daniel Vetter
2013-08-09 20:04   ` [PATCH 6.3/9] drm/i915: merge HSW and SNB PM irq handlers Paulo Zanoni
2013-08-14 19:21     ` Ben Widawsky
2013-08-15 14:51       ` Paulo Zanoni
2013-08-20 14:27         ` Daniel Vetter
2013-08-14 18:36   ` [PATCH 6.1/9] drm/i915: don't queue PM events we won't process Ben Widawsky
2013-08-15 14:50     ` Paulo Zanoni

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1376078677-24901-2-git-send-email-przanoni@gmail.com \
    --to=przanoni@gmail.com \
    --cc=intel-gfx@lists.freedesktop.org \
    --cc=paulo.r.zanoni@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.