On Tue, 24 Jul 2012 23:33:45 +0200
Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> This way it's easier so see what belongs together, and what is used
> by the ilk ips code. Also add some comments that explain the locking.
>
> v2: Missed one place that the dev_priv->ips change caught ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
With a few comments below
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 11 +++----
> drivers/gpu/drm/i915/i915_dma.c | 2 +-
> drivers/gpu/drm/i915/i915_drv.h | 18 ++++++++++--
> drivers/gpu/drm/i915/i915_irq.c | 32 ++++++++++-----------
> drivers/gpu/drm/i915/intel_display.c | 2 +-
> drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++-----------------
> 6 files changed, 65 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 2499610..05087fa 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1287,7 +1287,8 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>
> seq_printf(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\n");
>
> - for (gpu_freq = dev_priv->min_delay; gpu_freq <= dev_priv->max_delay;
> + for (gpu_freq = dev_priv->rps.min_delay;
> + gpu_freq <= dev_priv->rps.max_delay;
> gpu_freq++) {
> I915_WRITE(GEN6_PCODE_DATA, gpu_freq);
> I915_WRITE(GEN6_PCODE_MAILBOX, GEN6_PCODE_READY |
> @@ -1715,7 +1716,7 @@ i915_max_freq_read(struct file *filp,
>
> mutex_lock(&dev->struct_mutex);
> len = snprintf(buf, sizeof(buf),
> - "max freq: %d\n", dev_priv->max_delay * 50);
> + "max freq: %d\n", dev_priv->rps.max_delay * 50);
> mutex_unlock(&dev->struct_mutex);
>
> if (len > sizeof(buf))
> @@ -1755,7 +1756,7 @@ i915_max_freq_write(struct file *filp,
> /*
> * Turbo will still be enabled, but won't go above the set value.
> */
> - dev_priv->max_delay = val / 50;
> + dev_priv->rps.max_delay = val / 50;
>
> gen6_set_rps(dev, val / 50);
> mutex_unlock(&dev->struct_mutex);
> @@ -1785,7 +1786,7 @@ i915_min_freq_read(struct file *filp, char __user *ubuf, size_t max,
>
> mutex_lock(&dev->struct_mutex);
> len = snprintf(buf, sizeof(buf),
> - "min freq: %d\n", dev_priv->min_delay * 50);
> + "min freq: %d\n", dev_priv->rps.min_delay * 50);
> mutex_unlock(&dev->struct_mutex);
>
> if (len > sizeof(buf))
> @@ -1823,7 +1824,7 @@ i915_min_freq_write(struct file *filp, const char __user *ubuf, size_t cnt,
> /*
> * Turbo will still be enabled, but won't go below the set value.
> */
> - dev_priv->min_delay = val / 50;
> + dev_priv->rps.min_delay = val / 50;
>
> gen6_set_rps(dev, val / 50);
> mutex_unlock(&dev->struct_mutex);
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 5e20f11..5f63638 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1586,7 +1586,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);
> + spin_lock_init(&dev_priv->rps.lock);
>
> if (IS_IVYBRIDGE(dev) || IS_HASWELL(dev))
> dev_priv->num_pipe = 3;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f176589..2496d60 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -793,9 +793,21 @@ typedef struct drm_i915_private {
>
> bool mchbar_need_disable;
>
> - struct work_struct rps_work;
> - spinlock_t rps_lock;
> - u32 pm_iir;
> + /* gen6+ rps state */
> + struct {
> + struct work_struct work;
> + u32 pm_iir;
> + /* lock - irqsave spinlock that protectects the work_struct and
> + * pm_iir. */
> + spinlock_t lock;
> +
> + /* The below variables an all the rps hw state are protected by
> + * dev->struct mutext. */
> + u8 cur_delay;
> + u8 min_delay;
> + u8 max_delay;
> + } rps;
> +
>
> u8 cur_delay;
> u8 min_delay;
Could you add the reason for adding new cur/min/max delays to the commit
message? From just this hunk it would seem we'd want to remove the old
cur/min/max.
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 41ed41d..6e3f43c 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -349,16 +349,16 @@ static void notify_ring(struct drm_device *dev,
> static void gen6_pm_rps_work(struct work_struct *work)
> {
> drm_i915_private_t *dev_priv = container_of(work, drm_i915_private_t,
> - rps_work);
> + rps.work);
> u32 pm_iir, pm_imr;
> u8 new_delay;
>
> - spin_lock_irq(&dev_priv->rps_lock);
> - pm_iir = dev_priv->pm_iir;
> - dev_priv->pm_iir = 0;
> + spin_lock_irq(&dev_priv->rps.lock);
> + pm_iir = dev_priv->rps.pm_iir;
> + dev_priv->rps.pm_iir = 0;
> pm_imr = I915_READ(GEN6_PMIMR);
> I915_WRITE(GEN6_PMIMR, 0);
> - spin_unlock_irq(&dev_priv->rps_lock);
> + spin_unlock_irq(&dev_priv->rps.lock);
>
> if ((pm_iir & GEN6_PM_DEFERRED_EVENTS) == 0)
> return;
> @@ -366,9 +366,9 @@ static void gen6_pm_rps_work(struct work_struct *work)
> mutex_lock(&dev_priv->dev->struct_mutex);
>
> if (pm_iir & GEN6_PM_RP_UP_THRESHOLD)
> - new_delay = dev_priv->cur_delay + 1;
> + new_delay = dev_priv->rps.cur_delay + 1;
> else
> - new_delay = dev_priv->cur_delay - 1;
> + new_delay = dev_priv->rps.cur_delay - 1;
>
> gen6_set_rps(dev_priv->dev, new_delay);
>
> @@ -488,20 +488,20 @@ static void gen6_queue_rps_work(struct drm_i915_private *dev_priv,
> * IIR bits should never already be set because IMR should
> * prevent an interrupt from being shown in IIR. The warning
> * displays a case where we've unsafely cleared
> - * dev_priv->pm_iir. Although missing an interrupt of the same
> + * dev_priv->rps.pm_iir. Although missing an interrupt of the same
> * type is not a problem, it displays a problem in the logic.
> *
> - * The mask bit in IMR is cleared by rps_work.
> + * The mask bit in IMR is cleared by dev_priv->rps.work.
> */
>
> - spin_lock_irqsave(&dev_priv->rps_lock, flags);
> - WARN(dev_priv->pm_iir & pm_iir, "Missed a PM interrupt\n");
> - dev_priv->pm_iir |= pm_iir;
> - I915_WRITE(GEN6_PMIMR, dev_priv->pm_iir);
> + spin_lock_irqsave(&dev_priv->rps.lock, flags);
> + WARN(dev_priv->rps.pm_iir & pm_iir, "Missed a PM interrupt\n");
> + dev_priv->rps.pm_iir |= pm_iir;
> + I915_WRITE(GEN6_PMIMR, dev_priv->rps.pm_iir);
> POSTING_READ(GEN6_PMIMR);
> - spin_unlock_irqrestore(&dev_priv->rps_lock, flags);
> + spin_unlock_irqrestore(&dev_priv->rps.lock, flags);
>
> - queue_work(dev_priv->wq, &dev_priv->rps_work);
> + queue_work(dev_priv->wq, &dev_priv->rps.work);
> }
>
> static irqreturn_t valleyview_irq_handler(DRM_IRQ_ARGS)
> @@ -2649,7 +2649,7 @@ void intel_irq_init(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);
> + INIT_WORK(&dev_priv->rps.work, gen6_pm_rps_work);
> INIT_WORK(&dev_priv->parity_error_work, ivybridge_parity_work);
>
> dev->driver->get_vblank_counter = i915_get_vblank_counter;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index be45b92..00a90e1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7220,7 +7220,7 @@ void intel_modeset_cleanup(struct drm_device *dev)
> * enqueue unpin/hotplug work. */
> drm_irq_uninstall(dev);
> cancel_work_sync(&dev_priv->hotplug_work);
> - cancel_work_sync(&dev_priv->rps_work);
> + cancel_work_sync(&dev_priv->rps.work);
>
> /* flush any delayed tasks or pending work */
> flush_scheduled_work();
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index d5af41b..6c9925d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2275,17 +2275,17 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> WARN_ON(!mutex_is_locked(&dev->struct_mutex));
>
> limits = 0;
> - if (val >= dev_priv->max_delay)
> - val = dev_priv->max_delay;
> + if (val >= dev_priv->rps.max_delay)
> + val = dev_priv->rps.max_delay;
> else
> - limits |= dev_priv->max_delay << 24;
> + limits |= dev_priv->rps.max_delay << 24;
>
> - if (val <= dev_priv->min_delay)
> - val = dev_priv->min_delay;
> + if (val <= dev_priv->rps.min_delay)
> + val = dev_priv->rps.min_delay;
> else
> - limits |= dev_priv->min_delay << 16;
> + limits |= dev_priv->rps.min_delay << 16;
>
> - if (val == dev_priv->cur_delay)
> + if (val == dev_priv->rps.cur_delay)
> return;
>
> I915_WRITE(GEN6_RPNSWREQ,
> @@ -2298,7 +2298,7 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> */
> I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
>
> - dev_priv->cur_delay = val;
> + dev_priv->rps.cur_delay = val;
> }
>
> static void gen6_disable_rps(struct drm_device *dev)
> @@ -2314,9 +2314,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);
> - dev_priv->pm_iir = 0;
> - spin_unlock_irq(&dev_priv->rps_lock);
> + spin_lock_irq(&dev_priv->rps.lock);
> + dev_priv->rps.pm_iir = 0;
> + spin_unlock_irq(&dev_priv->rps.lock);
>
> I915_WRITE(GEN6_PMIIR, I915_READ(GEN6_PMIIR));
> }
> @@ -2385,9 +2385,9 @@ static void gen6_enable_rps(struct drm_device *dev)
> gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
>
> /* In units of 100MHz */
> - dev_priv->max_delay = rp_state_cap & 0xff;
> - dev_priv->min_delay = (rp_state_cap & 0xff0000) >> 16;
> - dev_priv->cur_delay = 0;
> + dev_priv->rps.max_delay = rp_state_cap & 0xff;
> + dev_priv->rps.min_delay = (rp_state_cap & 0xff0000) >> 16;
> + dev_priv->rps.cur_delay = 0;
>
> /* disable the counters and set deterministic thresholds */
> I915_WRITE(GEN6_RC_CONTROL, 0);
> @@ -2440,8 +2440,8 @@ static void gen6_enable_rps(struct drm_device *dev)
>
> I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 1000000);
> I915_WRITE(GEN6_RP_INTERRUPT_LIMITS,
> - dev_priv->max_delay << 24 |
> - dev_priv->min_delay << 16);
> + dev_priv->rps.max_delay << 24 |
> + dev_priv->rps.min_delay << 16);
>
> if (IS_HASWELL(dev)) {
> I915_WRITE(GEN6_RP_UP_THRESHOLD, 59400);
> @@ -2486,7 +2486,7 @@ static void gen6_enable_rps(struct drm_device *dev)
> 500))
> DRM_ERROR("timeout waiting for pcode mailbox to finish\n");
> if (pcu_mbox & (1<<31)) { /* OC supported */
> - dev_priv->max_delay = pcu_mbox & 0xff;
> + dev_priv->rps.max_delay = pcu_mbox & 0xff;
> DRM_DEBUG_DRIVER("overclocking supported, adjusting frequency max to %dMHz\n", pcu_mbox * 50);
> }
>
> @@ -2494,10 +2494,10 @@ static void gen6_enable_rps(struct drm_device *dev)
>
> /* requires MSI enabled */
> I915_WRITE(GEN6_PMIER, GEN6_PM_DEFERRED_EVENTS);
> - spin_lock_irq(&dev_priv->rps_lock);
> - WARN_ON(dev_priv->pm_iir != 0);
> + spin_lock_irq(&dev_priv->rps.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->rps.lock);
> /* enable all PM interrupts */
> I915_WRITE(GEN6_PMINTRMSK, 0);
>
> @@ -2529,9 +2529,9 @@ static void gen6_update_ring_freq(struct drm_device *dev)
> * to use for memory access. We do this by specifying the IA frequency
> * the PCU should use as a reference to determine the ring frequency.
> */
> - for (gpu_freq = dev_priv->max_delay; gpu_freq >= dev_priv->min_delay;
> + for (gpu_freq = dev_priv->rps.max_delay; gpu_freq >= dev_priv->rps.min_delay;
> gpu_freq--) {
> - int diff = dev_priv->max_delay - gpu_freq;
> + int diff = dev_priv->rps.max_delay - gpu_freq;
>
> /*
> * For GPU frequencies less than 750MHz, just use the lowest
> @@ -2974,7 +2974,7 @@ unsigned long i915_gfx_val(struct drm_i915_private *dev_priv)
>
> assert_spin_locked(&mchdev_lock);
>
> - pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->cur_delay * 4));
> + pxvid = I915_READ(PXVFREQ_BASE + (dev_priv->rps.cur_delay * 4));
> pxvid = (pxvid >> 24) & 0x7f;
> ext_v = pvid_to_extvid(dev_priv, pxvid);
>
> @@ -3772,11 +3772,11 @@ static void gen6_sanitize_pm(struct drm_device *dev)
> * until we hit the minimum or maximum frequencies.
> */
> limits &= ~(0x3f << 16 | 0x3f << 24);
> - delay = dev_priv->cur_delay;
> + delay = dev_priv->rps.cur_delay;
> if (delay < dev_priv->max_delay)
> limits |= (dev_priv->max_delay & 0x3f) << 24;
> - if (delay > dev_priv->min_delay)
> - limits |= (dev_priv->min_delay & 0x3f) << 16;
> + if (delay > dev_priv->rps.min_delay)
> + limits |= (dev_priv->rps.min_delay & 0x3f) << 16;
>
> if (old != limits) {
> /* Note that the known failure case is to read back 0. */
--
Ben Widawsky, Intel Open Source Technology Center
