[Intel-gfx] [PATCH 16/26] drm/i915: turbo & RC6 support for VLV

[Jain, Rohit]

> -----Original Message-----
> From: intel-gfx-bounces+rohit.jain=intel.com at lists.freedesktop.org
> [mailto:intel-gfx-bounces+rohit.jain=intel.com at lists.freedesktop.org]
> On Behalf Of Jesse Barnes
> Sent: Friday, March 08, 2013 3:58 AM
> To: Rohit Jain
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH 16/26] drm/i915: turbo & RC6 support
> for VLV
> 
> On Wed, 6 Mar 2013 16:21:03 +0530 (IST)
> Rohit Jain <rohit at intel.com> wrote:
> 
> >
> >
> > On Sat, 2 Mar 2013, Jesse Barnes wrote:
> >
> > > From: Ben Widawsky <ben at bwidawsk.net>
> > >
> > > Uses slightly different interfaces than other platforms.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes at 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.

Cool! On my board, max_delay gets set to 255 while the punit refuses to go
above 222 in practice. In this case, before we can clamp to max_delay, cur_delay
overflows and gets set to min_delay instead :)

Fixing it like this solves this problem neatly.

Cheers,
Rohit