Re: [PATCH 16/26] drm/i915: turbo & RC6 support for VLV

[Jesse Barnes <jbarnes@virtuousgeek.org>]

On Wed, 6 Mar 2013 16:21:03 +0530 (IST)
Rohit Jain <rohit@intel.com> wrote:

> 
> 
> On Sat, 2 Mar 2013, Jesse Barnes wrote:
> 
> > From: Ben Widawsky <ben@bwidawsk.net>
> >
> > Uses slightly different interfaces than other platforms.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> > drivers/gpu/drm/i915/intel_pm.c |  148 +++++++++++++++++++++++++++++++++++++--
> > 1 file changed, 144 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e3947cb..d16f4f40 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -2477,6 +2477,47 @@ void gen6_set_rps(struct drm_device *dev, u8 val)
> > 	trace_intel_gpu_freq_change(val * 50);
> > }
> >
> > +void valleyview_set_rps(struct drm_device *dev, u8 val)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(100);
> > +	u32 limits = gen6_rps_limits(dev_priv, &val);
> > +	u32 pval;
> > +
> > +	WARN_ON(!mutex_is_locked(&dev_priv->rps.hw_lock));
> > +	WARN_ON(val > dev_priv->rps.max_delay);
> > +	WARN_ON(val < dev_priv->rps.min_delay);
> > +
> > +	if (val == dev_priv->rps.cur_delay)
> > +		return;
> > +
> > +	valleyview_punit_write(dev_priv, PUNIT_REG_GPU_FREQ_REQ, val);
> > +
> > +	do {
> > +		valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval);
> > +		if (time_after(jiffies, timeout)) {
> > +			DRM_DEBUG_DRIVER("timed out waiting for Punit\n");
> > +			break;
> > +		}
> > +		udelay(10);
> > +	} while (pval & 1);
> > +
> > +	valleyview_punit_read(dev_priv, PUNIT_REG_GPU_FREQ_STS, &pval);
> > +	if ((pval >> 8) != val)
> > +		DRM_DEBUG_DRIVER("punit overrode freq: %d requested, but got %d\n",
> > +			  val, pval >> 8);
> > +
> > +	/* Make sure we continue to get interrupts
> > +	 * until we hit the minimum or maximum frequencies.
> > +	 */
> > +	I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, limits);
> > +
> > +	dev_priv->rps.cur_delay = val;
> 
> Shouldn't we store pval >> 8 instead of val in cur_delay in order to
> reclaim the rps state? If we store val here, the requested frequency will
> eventually exceed max_delay if punit overrides with a lower frequency.
> 

Yeah we should track the current freq here instead.  But we clamp to
max_delay in the caller right?  And yeah I missed the update to
i915_irq.c, I fixed that too.

-- 
Jesse Barnes, Intel Open Source Technology Center