[Intel-gfx] [PATCH 0/5] Add power feature debugfs disabling

[Chris Wilson]

On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee at intel.com wrote:
> From: Jeff McGee <jeff.mcgee at intel.com>
> 
> This series has recently been accepted into the Haswell Android kernel and
> helps with debugging and profiling these power features. I would like it
> to be considered for upstream incorporation. The patches here have been
> rebased (minimal changes required) and compile-tested only.
> 
> Broad device support is provided, accept for RPS and RC6 with Broadwell
> and Valleyview. Both of these were somewhat of a moving target and I
> didn't have devices to work with. Support can of course be added with
> help from appropriate folks.
> 
> The hooks introduce some amount of overhead as an additional check is
> often needed to determine whether the feature is on or off - similar to
> the module parameters that already exist. I felt that the overhead was
> minimal enough and didn't want to ugly up the code with CONFIG_DEBUG_FS
> compile conditionals. But I'm open to the list's thoughts on this.
> 
> IGT tests of these new interfaces can certainly be added. I wanted to
> make sure there was sufficient interest in having these interfaces before
> starting on the tests. So please provide feedback.

I can see the value of adding this for power testing and the code looks
quite neat and self-contained. (The one bikeshed I have is that I would
like the parameter check and debug.disable check combined into a single
function call, similar to intel_enable_rc6() so that all the similar
logic is together, well commented and easy to verify, and hard for
callers to get wrong.) Longer term, should we not consider this for
our /sys/drm/card0/power API? i.e. do we see value beyond debugging and
testing?

In the RPS autoadjust disabling code, it looks like you could go much
further and shutdown the interrupts entirely saving wakeups and power.
And put the restart logic into a single function (it requires too much
inner knowledge of the RPS logic to spread around).

I think the rest looked ok - but I did have a moment of puzzlement that
IPS did not turn off intel_ips ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre