[Intel-gfx] [PATCH 4/5] drm/i915: move gen6 rps handling to workqueue
Chris Wilson
chris at chris-wilson.co.uk
Thu Apr 21 08:34:08 CEST 2011
On Wed, 20 Apr 2011 16:53:18 -0700, Ben Widawsky <ben at bwidawsk.net> wrote:
> The render P-state handling code requires reading from a GT register.
> This means that FORCEWAKE must be written to, a resource which is shared
> and should be protected by struct_mutex.
>
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_dma.c | 1 +
> drivers/gpu/drm/i915/i915_drv.h | 4 +++
> drivers/gpu/drm/i915/i915_irq.c | 47 +++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_reg.h | 5 +++-
> 4 files changed, 47 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 7273037..855355e 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -2025,6 +2025,7 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>
> spin_lock_init(&dev_priv->irq_lock);
> spin_lock_init(&dev_priv->error_lock);
> + spin_lock_init(&dev_priv->rps_lock);
>
> if (IS_MOBILE(dev) || !IS_GEN2(dev))
> dev_priv->num_pipe = 2;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1cd5a76..4faa7e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -676,6 +676,10 @@ typedef struct drm_i915_private {
>
> bool mchbar_need_disable;
>
> + struct work_struct rps_work;
> + spinlock_t rps_lock;
> + u32 pm_iir;
> +
> u8 cur_delay;
> u8 min_delay;
> u8 max_delay;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index c6062c3..2d9f751 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -367,22 +367,30 @@ static void notify_ring(struct drm_device *dev,
> jiffies + msecs_to_jiffies(DRM_I915_HANGCHECK_PERIOD));
> }
>
> -static void gen6_pm_irq_handler(struct drm_device *dev)
> +static void gen6_pm_rps_work(struct work_struct *work)
> {
> - drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> + drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> + rps_work);
> u8 new_delay = dev_priv->cur_delay;
> - u32 pm_iir;
> + u32 pm_iir, pm_imr;
> +
> + spin_lock_irq(&dev_priv->rps_lock);
> + pm_iir = dev_priv->pm_iir;
> + dev_priv->pm_iir = 0;
> + pm_imr = I915_READ(GEN6_PMIMR);
> + spin_unlock_irq(&dev_priv->rps_lock);
>
> - pm_iir = I915_READ(GEN6_PMIIR);
> if (!pm_iir)
> return;
>
> + mutex_lock(&dev_priv->dev->struct_mutex);
> if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> if (dev_priv->cur_delay != dev_priv->max_delay)
> new_delay = dev_priv->cur_delay + 1;
> if (new_delay > dev_priv->max_delay)
> new_delay = dev_priv->max_delay;
> } else if (pm_iir & (GEN6_PM_RP_DOWN_THRESHOLD | GEN6_PM_RP_DOWN_TIMEOUT)) {
> + gen6_gt_force_wake_get(dev_priv);
> if (dev_priv->cur_delay != dev_priv->min_delay)
> new_delay = dev_priv->cur_delay - 1;
> if (new_delay < dev_priv->min_delay) {
> @@ -396,13 +404,18 @@ static void gen6_pm_irq_handler(struct drm_device *dev)
> I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> I915_READ(GEN6_RP_INTERRUPT_LIMITS) & ~0x3f0000);
> }
> + gen6_gt_force_wake_put(dev_priv);
> }
>
> - gen6_set_rps(dev, new_delay);
> + gen6_set_rps(dev_priv->dev, new_delay);
> dev_priv->cur_delay = new_delay;
> + mutex_unlock(&dev_priv->dev->struct_mutex);
>
> - I915_WRITE(GEN6_PMIIR, pm_iir);
> -}
> + /*
> + * Lock not held here, because clearing is non-destructive, and
> + * the interrupt handler is the only other place where it is written.
> + */
> + I915_WRITE(GEN6_PMIMR, pm_imr & ~pm_iir); }
But does this register use __gen6_gt_wait_for_fifo? *That* requires a
lock. I'm starting to see a need for I915_READ_IRQ, I915_WRITE_IRQ, so
that the circumstances are explicit and we acknowledge them when modifying
the read/write routines.
> static void pch_irq_handler(struct drm_device *dev)
> {
> @@ -525,13 +538,28 @@ static irqreturn_t ironlake_irq_handler(struct drm_device *dev)
> i915_handle_rps_change(dev);
> }
>
> - if (IS_GEN6(dev))
> - gen6_pm_irq_handler(dev);
> + if (IS_GEN6(dev) && pm_iir) {
> + if (pm_iir & (GEN6_PM_DEFERRED_EVENTS)) {
if (IS_GEN6(dev) && pm_iir & GEN6_PM_DEFERRED_EVENTS) {
roll the two ifs into one to remove a surplus level of nesting and kill
the redundant brackets!
> + unsigned long flags;
> + /* Make sure no new interrupts come in */
> + spin_lock_irqsave(&dev_priv->rps_lock, flags);
> + I915_WRITE(GEN6_PMIMR, pm_iir);
> +
> + /* Add the new ones */
> + BUG_ON(dev_priv->pm_iir & pm_iir);
Bah. The comments are absolutely useless. Really. What you need to
describe is how the use of IMR and IIR is split between the interrupt
handler and the tasklet.
Or maybe they did their job, because I had to go back and read much more
carefully what you were doing...
> + dev_priv->pm_iir |= pm_iir;
> + spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
> +
> + /* queue it up */
> + queue_work(dev_priv->wq, &dev_priv->rps_work);
> + }
> + }
>
> /* should clear PCH hotplug event before clear CPU irq */
> I915_WRITE(SDEIIR, pch_iir);
> I915_WRITE(GTIIR, gt_iir);
> I915_WRITE(DEIIR, de_iir);
> + I915_WRITE(GEN6_PMIIR, pm_iir);
>
> done:
> I915_WRITE(DEIER, de_ier);
> @@ -1658,6 +1686,7 @@ void i915_driver_irq_preinstall(struct drm_device * dev)
>
> INIT_WORK(&dev_priv->hotplug_work, i915_hotplug_work_func);
> INIT_WORK(&dev_priv->error_work, i915_error_work_func);
> + INIT_WORK(&dev_priv->rps_work, gen6_pm_rps_work);
>
> if (HAS_PCH_SPLIT(dev)) {
> ironlake_irq_preinstall(dev);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index f39ac3a..9774a2e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3386,7 +3386,7 @@
> #define GEN6_PMINTRMSK 0xA168
>
> #define GEN6_PMISR 0x44020
> -#define GEN6_PMIMR 0x44024
> +#define GEN6_PMIMR 0x44024 /* Protected by rps_lock */
> #define GEN6_PMIIR 0x44028
> #define GEN6_PMIER 0x4402C
> #define GEN6_PM_MBOX_EVENT (1<<25)
> @@ -3396,6 +3396,9 @@
> #define GEN6_PM_RP_DOWN_THRESHOLD (1<<4)
> #define GEN6_PM_RP_UP_EI_EXPIRED (1<<2)
> #define GEN6_PM_RP_DOWN_EI_EXPIRED (1<<1)
> +#define GEN6_PM_DEFERRED_EVENTS (GEN6_PM_RP_UP_THRESHOLD | \
> + GEN6_PM_RP_DOWN_THRESHOLD | \
> + GEN6_PM_RP_DOWN_TIMEOUT)
>
> #define GEN6_PCODE_MAILBOX 0x138124
> #define GEN6_PCODE_READY (1<<31)
> --
> 1.7.3.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Chris Wilson, Intel Open Source Technology Centre
More information about the Intel-gfx
mailing list