[Intel-gfx] [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers

[Chris Wilson]

On Tue, Sep 24, 2013 at 12:47:21PM +0200, Daniel Vetter wrote:
> On Mon, Aug 26, 2013 at 04:15:16PM +0300, Ville Syrjälä wrote:
> > On Mon, Aug 26, 2013 at 01:45:56PM +0100, Chris Wilson wrote:
> > > The RPS register writing routines use the current value of min/max to
> > > set certain limits and interrupt gating. If we set those afterwards, we
> > > risk setting up the hw incorrectly and losing power management events,
> > > and worse, trigger some internal assertions.
> > > 
> > > Reorder the calling sequences to be correct, and remove the then
> > > unrequired clamping from inside set_rps(). And for a bonus, fix the bug
> > > of calling gen6_set_rps() from Valleyview.
> > > 
> > > Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c |  2 +-
> > >  drivers/gpu/drm/i915/i915_sysfs.c   | 16 ++++++++--------
> > >  drivers/gpu/drm/i915/intel_pm.c     | 19 +++++--------------
> > >  3 files changed, 14 insertions(+), 23 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 2a276c8..b2b1730 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -2113,7 +2113,7 @@ i915_max_freq_set(void *data, u64 val)
> > >  	if (IS_VALLEYVIEW(dev)) {
> > >  		val = vlv_freq_opcode(dev_priv->mem_freq, val);
> > >  		dev_priv->rps.max_delay = val;
> > > -		gen6_set_rps(dev, val);
> > > +		valleyview_set_rps(dev, val);
> > 
> > Not caused by your patch, but why on earth are we telling the GPU
> > to switch to the new max_freq here?
> > 
> > In the old way of doing things I presume this should have been
> > set_rps(cur_delay). And in the new way we should add the 
> > same 'cur_delay > val' check here that we have in i915_sysfs.
> > 
> > Maybe we should just have some kind of
> > rps_set_minmax(new_min, new_max) func that takes care of
> > this stuff in a single location.
> 
> We might as well just rip out the debugfs interfaces now that we have all
> this stuff in sysfs.

Actually, the debugfs can serve a purpose for giving us hw values
instead of our bookkeeping values (which is what sysfs should provide).
So lose the ability to write values, but keep the low level reads
intact.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre