From: Ben Widawsky <ben@bwidawsk.net>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 17/24] drm/i915: kill dev_priv->rps.lock
Date: Thu, 27 Jun 2013 20:35:17 -0700 [thread overview]
Message-ID: <20130628033517.GA1067@bwidawsk.net> (raw)
In-Reply-To: <1371037046-3732-18-git-send-email-daniel.vetter@ffwll.ch>
On Wed, Jun 12, 2013 at 01:37:19PM +0200, Daniel Vetter wrote:
> Now that the rps interrupt locking isn't clearly separated (at elast
> conceptually) from all the other interrupt locking having a different
> lock stopped making sense. With this we can (again) unifiy the
> ringbuffer irq refcounts without causing a massive confusion, but
> that's for the next patch.
I think the original reason for the RPS lock still makes sense, so I'd
say your assertion should be that it's not worth the extra complexity,
which I can agree with. (The original use of the rps_lock was just for
the rps workqueue, it expanded a bunch since then).
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 1 -
> drivers/gpu/drm/i915/i915_drv.h | 8 ++++----
> drivers/gpu/drm/i915/i915_irq.c | 12 ++++++------
> drivers/gpu/drm/i915/intel_pm.c | 16 ++++++++--------
> drivers/gpu/drm/i915/intel_ringbuffer.c | 8 ++++----
> drivers/gpu/drm/i915/intel_ringbuffer.h | 2 +-
> 6 files changed, 23 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index b913b3d..14c3e9c 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1612,7 +1612,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->gpu_error.lock);
> - spin_lock_init(&dev_priv->rps.lock);
> spin_lock_init(&dev_priv->backlight.lock);
> mutex_init(&dev_priv->dpio_lock);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 07954b2..42d1363 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -691,12 +691,12 @@ struct i915_suspend_saved_registers {
> };
>
> struct intel_gen6_power_mgmt {
> + /* work and pm_iir are protected by dev_priv->irq_lock */
> struct work_struct work;
> - struct delayed_work vlv_work;
> u32 pm_iir;
> - /* lock - irqsave spinlock that protectects the work_struct and
> - * pm_iir. */
> - spinlock_t lock;
> +
> + /* On vlv we need to manually drop to Vmin with a delayed work. */
> + struct delayed_work vlv_work;
A bit unrelated, but meh.
>
> /* The below variables an all the rps hw state are protected by
> * dev->struct mutext. */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 752b98d..cd7135d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -696,13 +696,13 @@ static void gen6_pm_rps_work(struct work_struct *work)
> u32 pm_iir, pm_imr;
> u8 new_delay;
>
> - spin_lock_irq(&dev_priv->rps.lock);
> + spin_lock_irq(&dev_priv->irq_lock);
> pm_iir = dev_priv->rps.pm_iir;
> dev_priv->rps.pm_iir = 0;
> pm_imr = I915_READ(GEN6_PMIMR);
> /* Make sure not to corrupt PMIMR state used by ringbuffer code */
> I915_WRITE(GEN6_PMIMR, pm_imr & ~GEN6_PM_RPS_EVENTS);
> - spin_unlock_irq(&dev_priv->rps.lock);
> + spin_unlock_irq(&dev_priv->irq_lock);
>
> if ((pm_iir & GEN6_PM_RPS_EVENTS) == 0)
> return;
> @@ -856,11 +856,11 @@ static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv,
> * The mask bit in IMR is cleared by dev_priv->rps.work.
> */
>
> - spin_lock(&dev_priv->rps.lock);
> + spin_lock(&dev_priv->irq_lock);
> dev_priv->rps.pm_iir |= pm_iir;
> I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
> POSTING_READ(GEN6_PMIMR);
> - spin_unlock(&dev_priv->rps.lock);
> + spin_unlock(&dev_priv->irq_lock);
>
> queue_work(dev_priv->wq, &dev_priv->rps.work);
> }
> @@ -933,12 +933,12 @@ static void hsw_pm_irq_handler(struct drm_i915_private *dev_priv,
> u32 pm_iir)
> {
> if (pm_iir & GEN6_PM_RPS_EVENTS) {
> - spin_lock(&dev_priv->rps.lock);
> + spin_lock(&dev_priv->irq_lock);
> dev_priv->rps.pm_iir |= pm_iir & GEN6_PM_RPS_EVENTS;
> 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_RPS_EVENTS);
> - spin_unlock(&dev_priv->rps.lock);
> + spin_unlock(&dev_priv->irq_lock);
>
> queue_work(dev_priv->wq, &dev_priv->rps.work);
> }
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index adc44e4..6a5cfb5 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3128,9 +3128,9 @@ static void gen6_disable_rps(struct drm_device *dev)
> * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
>
> - spin_lock_irq(&dev_priv->rps.lock);
> + spin_lock_irq(&dev_priv->irq_lock);
> dev_priv->rps.pm_iir = 0;
> - spin_unlock_irq(&dev_priv->rps.lock);
> + spin_unlock_irq(&dev_priv->irq_lock);
>
> I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> }
> @@ -3147,9 +3147,9 @@ static void valleyview_disable_rps(struct drm_device *dev)
> * register (PMIMR) to mask PM interrupts. The only risk is in leaving
> * stale bits in PMIIR and PMIMR which gen6_enable_rps will clean up. */
>
> - spin_lock_irq(&dev_priv->rps.lock);
> + spin_lock_irq(&dev_priv->irq_lock);
> dev_priv->rps.pm_iir = 0;
> - spin_unlock_irq(&dev_priv->rps.lock);
> + spin_unlock_irq(&dev_priv->irq_lock);
>
> I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
>
> @@ -3314,13 +3314,13 @@ static void gen6_enable_rps(struct drm_device *dev)
>
> /* requires MSI enabled */
> I915_WRITE(GEN6_PMIER, I915_READ(GEN6_PMIER) | GEN6_PM_RPS_EVENTS);
> - spin_lock_irq(&dev_priv->rps.lock);
> + spin_lock_irq(&dev_priv->irq_lock);
> /* FIXME: Our interrupt enabling sequence is bonghits.
> * dev_priv->rps.pm_iir really should be 0 here. */
> dev_priv->rps.pm_iir = 0;
> I915_WRITE(GEN6_PMIMR, I915_READ(GEN6_PMIMR) & ~GEN6_PM_RPS_EVENTS);
> I915_WRITE(GEN6_PMIIR, GEN6_PM_RPS_EVENTS);
> - spin_unlock_irq(&dev_priv->rps.lock);
> + spin_unlock_irq(&dev_priv->irq_lock);
> /* unmask all PM interrupts */
> I915_WRITE(GEN6_PMINTRMSK, 0);
>
> @@ -3585,10 +3585,10 @@ static void valleyview_enable_rps(struct drm_device *dev)
>
> /* requires MSI enabled */
> I915_WRITE(GEN6_PMIER, GEN6_PM_RPS_EVENTS);
> - spin_lock_irq(&dev_priv->rps.lock);
> + spin_lock_irq(&dev_priv->irq_lock);
> WARN_ON(dev_priv->rps.pm_iir != 0);
> I915_WRITE(GEN6_PMIMR, 0);
> - spin_unlock_irq(&dev_priv->rps.lock);
> + spin_unlock_irq(&dev_priv->irq_lock);
> /* enable all PM interrupts */
> I915_WRITE(GEN6_PMINTRMSK, 0);
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 1ef081c..a7c9934 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1053,14 +1053,14 @@ hsw_vebox_get_irq(struct intel_ring_buffer *ring)
> if (!dev->irq_enabled)
> return false;
>
> - spin_lock_irqsave(&dev_priv->rps.lock, flags);
> + spin_lock_irqsave(&dev_priv->irq_lock, flags);
> if (ring->irq_refcount.pm++ == 0) {
> u32 pm_imr = I915_READ(GEN6_PMIMR);
> I915_WRITE_IMR(ring, ~ring->irq_enable_mask);
> I915_WRITE(GEN6_PMIMR, pm_imr & ~ring->irq_enable_mask);
> POSTING_READ(GEN6_PMIMR);
> }
> - spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
> + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
>
> return true;
> }
> @@ -1075,14 +1075,14 @@ hsw_vebox_put_irq(struct intel_ring_buffer *ring)
> if (!dev->irq_enabled)
> return;
>
> - spin_lock_irqsave(&dev_priv->rps.lock, flags);
> + spin_lock_irqsave(&dev_priv->irq_lock, flags);
> if (--ring->irq_refcount.pm == 0) {
> u32 pm_imr = I915_READ(GEN6_PMIMR);
> I915_WRITE_IMR(ring, ~0);
> I915_WRITE(GEN6_PMIMR, pm_imr | ring->irq_enable_mask);
> POSTING_READ(GEN6_PMIMR);
> }
> - spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
> + spin_unlock_irqrestore(&dev_priv->irq_lock, flags);
> }
>
> static int
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index efc403d..f960805 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -76,7 +76,7 @@ struct intel_ring_buffer {
>
> struct {
> u32 gt; /* protected by dev_priv->irq_lock */
> - u32 pm; /* protected by dev_priv->rps.lock (sucks) */
> + u32 pm; /* protected by dev_priv->irq_lock */
> } irq_refcount;
> u32 irq_enable_mask; /* bitmask to enable ring interrupt */
> u32 trace_irq_seqno;
> --
> 1.8.1.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ben Widawsky, Intel Open Source Technology Center
next prev parent reply other threads:[~2013-06-28 3:32 UTC|newest]
Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-06-12 11:37 [PATCH 00/24] irq locking review Daniel Vetter
2013-06-12 11:37 ` [PATCH 01/24] drm/i915: fix locking around ironlake_enable|disable_display_irq Daniel Vetter
2013-06-25 12:26 ` [PATCH] " Daniel Vetter
2013-06-26 21:15 ` Paulo Zanoni
2013-06-27 10:37 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 02/24] drm/i915: close tiny race in the ilk pcu even interrupt setup Daniel Vetter
2013-06-26 21:20 ` Paulo Zanoni
2013-06-12 11:37 ` [PATCH 03/24] drm/i915: assert_spin_locked for pipestat interrupt enable/disable Daniel Vetter
2013-06-26 21:44 ` Paulo Zanoni
2013-06-12 11:37 ` [PATCH 04/24] drm/i915: s/hotplug_irq_storm_detect/intel_hpd_irq_handler/ Daniel Vetter
2013-06-12 14:22 ` Egbert Eich
2013-06-12 11:37 ` [PATCH 05/24] drm/i915: fold the hpd_irq_setup call into intel_hpd_irq_handler Daniel Vetter
[not found] ` <20920.34096.704203.67316@linux-qknr.site>
2013-06-12 15:00 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 06/24] drm/i915: fold the queue_work " Daniel Vetter
2013-06-12 14:29 ` Egbert Eich
2013-06-12 11:37 ` [PATCH 07/24] drm/i915: fold the no-irq check " Daniel Vetter
2013-06-12 14:33 ` Egbert Eich
2013-06-26 22:35 ` Paulo Zanoni
2013-06-27 10:39 ` Daniel Vetter
2013-06-27 11:44 ` [PATCH 1/8] drm/i915: fix locking around ironlake_enable|disable_display_irq Daniel Vetter
2013-06-27 11:44 ` [PATCH 2/8] drm/i915: close tiny race in the ilk pcu even interrupt setup Daniel Vetter
2013-06-27 11:45 ` [PATCH 3/8] drm/i915: assert_spin_locked for pipestat interrupt enable/disable Daniel Vetter
2013-06-27 11:45 ` [PATCH 4/8] drm/i915: s/hotplug_irq_storm_detect/intel_hpd_irq_handler/ Daniel Vetter
2013-06-27 11:45 ` [PATCH 5/8] drm/i915: fold the hpd_irq_setup call into intel_hpd_irq_handler Daniel Vetter
2013-06-27 11:45 ` [PATCH 6/8] drm/i915: fold the queue_work " Daniel Vetter
2013-06-27 12:13 ` Chris Wilson
2013-06-27 11:45 ` [PATCH 7/8] drm/i915: fold the no-irq check " Daniel Vetter
2013-06-27 12:14 ` Chris Wilson
2013-06-27 11:45 ` [PATCH 8/8] drm/i915: fix hpd interrupt register locking Daniel Vetter
2013-06-27 14:41 ` Paulo Zanoni
2013-06-27 15:52 ` [PATCH 1/6] drm/i915: assert_spin_locked for pipestat interrupt enable/disable Daniel Vetter
2013-06-27 15:52 ` [PATCH 2/6] drm/i915: s/hotplug_irq_storm_detect/intel_hpd_irq_handler/ Daniel Vetter
2013-06-27 15:52 ` [PATCH 3/6] drm/i915: fold the hpd_irq_setup call into intel_hpd_irq_handler Daniel Vetter
2013-06-27 15:52 ` [PATCH 4/6] drm/i915: fold the queue_work " Daniel Vetter
2013-06-27 15:52 ` [PATCH 5/6] drm/i915: fold the no-irq check " Daniel Vetter
2013-06-27 15:52 ` [PATCH 6/6] drm/i915: fix hpd interrupt register locking Daniel Vetter
2013-07-04 19:22 ` [PATCH 1/6] drm/i915: assert_spin_locked for pipestat interrupt enable/disable Daniel Vetter
2013-06-27 17:44 ` [PATCH 8/8] drm/i915: fix hpd interrupt register locking Daniel Vetter
2013-06-12 11:37 ` [PATCH 08/24] " Daniel Vetter
2013-06-12 14:59 ` Egbert Eich
2013-06-12 15:10 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 09/24] drm/i915: extract ibx_display_interrupt_update Daniel Vetter
2013-06-25 12:27 ` [PATCH] " Daniel Vetter
2013-06-12 11:37 ` [PATCH 10/24] drm/i915: remove SERR_INT clearing in the postinstall hook Daniel Vetter
2013-06-27 19:34 ` Paulo Zanoni
2013-07-04 19:49 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 11/24] drm/i915: improve SERR_INT clearing for fifo underrun reporting Daniel Vetter
2013-06-27 20:19 ` Paulo Zanoni
2013-07-04 19:55 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 12/24] drm/i915: improve GEN7_ERR_INT " Daniel Vetter
2013-06-12 11:37 ` [PATCH 13/24] drm/i915: kill lpt pch transcoder->crtc mapping code for fifo underruns Daniel Vetter
2013-06-12 13:04 ` Paulo Zanoni
2013-06-12 14:46 ` [PATCH] " Daniel Vetter
2013-06-27 20:45 ` Paulo Zanoni
2013-07-04 20:41 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 14/24] drm/i915: irq handlers don't need interrupt-safe spinlocks Daniel Vetter
2013-06-25 12:27 ` [PATCH] " Daniel Vetter
2013-06-27 21:14 ` Paulo Zanoni
2013-06-27 22:40 ` Daniel Vetter
2013-06-28 16:57 ` Paulo Zanoni
2013-06-12 11:37 ` [PATCH 15/24] drm/i915: streamline hsw_pm_irq_handler Daniel Vetter
2013-06-25 12:28 ` [PATCH] " Daniel Vetter
2013-06-12 11:37 ` [PATCH 16/24] drm/i915: queue work outside spinlock in hsw_pm_irq_handler Daniel Vetter
2013-06-12 11:37 ` [PATCH 17/24] drm/i915: kill dev_priv->rps.lock Daniel Vetter
2013-06-28 3:35 ` Ben Widawsky [this message]
2013-06-28 3:35 ` Ben Widawsky
2013-06-12 11:37 ` [PATCH 18/24] drm/i915: unify ring irq refcounts (again) Daniel Vetter
2013-06-28 17:24 ` Ben Widawsky
2013-07-04 20:52 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 19/24] drm/i915: don't enable PM_VEBOX_CS_ERROR_INTERRUPT Daniel Vetter
2013-06-12 17:13 ` Ben Widawsky
2013-06-12 17:18 ` Daniel Vetter
2013-06-12 18:19 ` Ben Widawsky
2013-06-12 18:32 ` Daniel Vetter
2013-06-12 18:51 ` Ben Widawsky
2013-06-28 17:25 ` Ben Widawsky
2013-06-12 11:37 ` [PATCH 20/24] drm/i915: kill bogus GTIIR clearing in vlv_preinstall hook Daniel Vetter
2013-06-28 17:01 ` Ben Widawsky
2013-07-04 20:56 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 21/24] drm/i915: unify PM interrupt preinstall sequence Daniel Vetter
2013-06-28 17:26 ` Ben Widawsky
2013-07-04 21:03 ` Daniel Vetter
2013-06-12 11:37 ` [PATCH 22/24] drm/i915: unify GT/PM irq postinstall code Daniel Vetter
2013-06-12 11:37 ` [PATCH 23/24] drm/i915: extract rps interrupt enable/disable helpers Daniel Vetter
2013-06-12 11:37 ` [PATCH 24/24] drm/i915: simplify rps interrupt enabling/disabling sequence Daniel Vetter
2013-06-12 22:32 ` Ben Widawsky
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=20130628033517.GA1067@bwidawsk.net \
--to=ben@bwidawsk.net \
--cc=daniel.vetter@ffwll.ch \
--cc=intel-gfx@lists.freedesktop.org \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox