[PATCH 4/9] drm/i915: move all rps state into dev_priv->rps

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux