Re: [PATCH] drm/i915: Initialise min/max frequencies before updating RPS registers

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Sep 24, 2013 at 12:12:54PM +0100, Chris Wilson wrote:
> 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@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.

Yeah, that sounds useful. Maybe shovel it all into the aggregate info file
we already have in debugfs.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch