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.vet...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
