On Fri, Jan 31, 2014 at 03:42:47PM -0600, jeff.mcgee@xxxxxxxxx wrote: > From: Jeff McGee <jeff.mcgee@xxxxxxxxx> > > 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
