[Intel-gfx] [PATCH 0/5] PowerManagement Toggle for PowerTOP

[Alexandra Yates]

On 04/14/2016 02:48 AM, Daniel Vetter wrote:
> On Wed, Apr 13, 2016 at 10:06:57PM +0000, Vivi, Rodrigo wrote:
>> On Wed, 2016-04-13 at 22:46 +0200, Daniel Vetter wrote:
>>> On Wed, Apr 13, 2016 at 10:38 PM, Vivi, Rodrigo <
>>> rodrigo.vivi at intel.com> wrote:
>>>> On Wed, 2016-04-13 at 16:50 +0200, Daniel Vetter wrote:
>>>>> On Wed, Apr 13, 2016 at 12:59:18PM +0000, Zanoni, Paulo R wrote:
>>>>>> Em Ter, 2016-04-12 às 12:18 -0700, Alexandra Yates escreveu:
>>>>>>> This project is explained in detail on the HAS
>>>>>>> https://docs.google.com/a/intel.com/document/d/1E-en_xqfHgCnh
>>>>>>> D1Te
>>>>>>> s3f0
>>>>>>> 8UYrOc-etv2W-pU0ZErKdE/edit?usp=sharing
>>>>>>>
>>>>>>> Summary:
>>>>>>> Permits the user to identify and toggle values for PSR, FBC,
>>>>>>> RC6,
>>>>>>> DRRS, and IPS
>>>>>>> under /sys/class/drm/card0/power/.  By enabling these
>>>>>>> features
>>>>>>> I'm
>>>>>>> looking to
>>>>>>> empower our customers, such as, power team, chrome OS, and
>>>>>>> platform
>>>>>>> integration teams
>>>>>>> to debug graphics power management features.
>>>>>>>
>>>>>>>  From the current patchset PSR, FBC, RC6, and, DRRS are
>>>>>>> complete
>>>>>>> and
>>>>>>> past BAT, modset,
>>>>>>> and suspend/resume tests.  For IPS I'm looking for feedback
>>>>>>> since
>>>>>>> IPS
>>>>>>> seems to change
>>>>>>> during runtime dynamically.  Additionally, as a future
>>>>>>> project
>>>>>>> IPS
>>>>>>> would be better
>>>>>>> served if it is implemented by using the atomic check, pre
>>>>>>> -commit,
>>>>>>> commit, post-commit,
>>>>>>> a good example of this is the PSR enable/disable
>>>>>>> implementation.
>>>>>>>
>>>>>>> For this project to be completed needs to have in place the
>>>>>>> following
>>>>>>> components:
>>>>>>> (Need to be developed)
>>>>>>> * IPS toggle interface flesh-out
>>>>>>> * Tests added to intel-gpu-tools repo
>>>>>>> * Documentation for all sysfs added interfaces
>>>>>>> * PowerTOP component named: GFX-TOP. With the following
>>>>>>> requirements:
>>>>>>>    * It would be available only to developers
>>>>>>>    * It would allow the developers to toggle the interfaces
>>>>>>> from
>>>>>>>      the PowerTOP user interface.
>>>>>>>    * It would show the Power Consumption impact of toggling on
>>>>>>> and
>>>>>>> off
>>>>>>>      these settings.
>>>>>>>
>>>>>>
>>>>>> A small suggestion:
>>>>>>
>>>>>> In the past, I've seen people testing i915.ko power saving
>>>>>> features
>>>>>> just by "toggling" the features through the i915 module
>>>>>> parameters
>>>>>> without doing anything else. In most of these cases, the
>>>>>> machine
>>>>>> was
>>>>>> not properly configured for power savings, so the conclusion
>>>>>> was
>>>>>> often
>>>>>> that the feature didn't save any power. While this was true
>>>>>> (toggling
>>>>>> the feature didn't save any power), these people would have
>>>>>> reached
>>>>>> different conclusions if they had, for example, changed their
>>>>>> disk
>>>>>> power management policies. Do we have any sort of plan to try
>>>>>> to
>>>>>> educate the powertop/gfxtop users regarding these things?
>>>>
>>>> But the toggling the module parameter doesn't change anything
>>>> because
>>>> it depends on the next modeset to have any effect o nmnost of pm
>>>> features.
>>>> The idea is that we don't need this with the sysfs and the toggle
>>>> should represent an immediate change on the feature behaviour.
>>>> Although
>>>> it is a fact that the features also depends on other conditions
>>>> like
>>>> PSR depends on a fully idle screen for few frames at least.
>>>
>>> Well that part where the knob needs to kick the system into working
>>> correctly (what if we need an expensive w/a that's hard to reset at
>>> runtime) is what scares me.
>>>
>>>>>> Maybe the powertop/gfxtop interface could expose some very-easy
>>>>>> -to-
>>>>>> reach text explaining these things.
>>>>
>>>> Good idea.
>>>>
>>>>>
>>>>> I think trying to extract/reuse those from the igt testcases
>>>>> might be
>>>>> really useful. I.e. add docs that explain
>>>>> a) what igt must past of a feature to be considered working
>>>>> b) add some functions to igt testcases for manually
>>>>> enabling/disabling a
>>>>> given rpm feature. The tests already know how to do that, and
>>>>> most
>>>>> have
>>>>> nice explanations about what all should be changed on top to make
>>>>> things
>>>>> work.
>>>>
>>>> Actually I had the opposite direction in mind. I was expecting we
>>>> could
>>>> make igt tests to start using this sysfs toggles.
>>>> The problem I see with current igt tests way is that we need to
>>>> change
>>>> the parameter and depend on the next full modeset happening. Well
>>>> it is
>>>> true that we need to have the modeset on the tests anyway, but I
>>>> believe we could reduce that restriction and also powertop wouldn't
>>>> do
>>>> any modeset.
>>>
>>> So taking more steps backwards: The goal here is to help debug
>>> runtime
>>> pm issues. The proposed solutions is that you can toggel stuff in
>>> powertop.
>>>
>>> Is this really how the experts debug runtime pm issues?
>>
>> I've seen many experts (non-gfx devs) around using the powertop for
>> debuging PM related issues and checking PC states, etc.
>>
>> One very good case to have at least the informative tab there is
>> exactly help this use case: One developer in the power lab cannot see
>> deep PC state residencies with screen on and look to the gfx tab and
>> see if DMC is enabled or not and PSR is enabled or not. And the extra
>> is if is not enabled but possible it would just toggle directly from
>> that interface.
>>
>> Or at least the informative with everything together they would know if
>> i915 is somehow the culprit for blocking PC states without need to hunt
>> different debugfs interfaces they are not familiar with.
>
> I'm fully on board with you on the "figure out that i915 is the culprit".
> I don't get why we need to be able to toggle stuff from within powertop to
> do that. Information-only should be enough. Maybe even some of the hints
> Paulo suggested (like "you haven't loaded DMC, pls install").
>
> Adding toggles into powertop - I have no idea how that helps with
> debugging, beyond the modparams we have already. This is also because I'm
> strongly opposed as "powertop the solution to enable rpm stuff by
> default", and that's seems to be the reason a lot of folks want these
> knobs.
>
> Imo if something doesn't work that should, the next step is to file a bug
> report with the i915 experts to fix this. Not knobs and hacks in powertop
> ...
>
> /me fighting a bit a holy war
>
>>> At least when I hack around my first questions are:
>>> - do we get residenency/how much, and maybe what exactly is blocking
>>> it. I think we discussed this at the meeting and agreed that
>>> more/officially exposed residency counters.
>>
>> The problem is that we don't have this counters when power wells are
>> down. So we lost all DC counters and PSR counter when DC entry.
>> So no residencies. But I still like the idea of the status.
>
> And I guess no way to at least somewhat fake those in software?
>
>>> - once I have a suspect I keep digging into that direction, using
>>> debugfs/unsafe module options, debugfs information, whatever.
>>>
>>> I fully agree that we can try to do a better job on documenting this
>>> stuff, and maybe exposing more information through stable interfaces.
>>>
>>> But adding new abi to enable/disable stuff sounds like jumping into
>>> the middle of the use-case story, ignoring everything else. And I
>>> think that was roughly what we agreed at that meeting - we can add
>>> knobs when it makes sense, but only when it makes sense. I'm still
>>> not
>>> sold on the "debug by powertop" solution.
>>
>> what about at least informative for now?
>
> Useful information sounds good. I'm freaking out about the maintenance of
> adding knobs and stuff that's supposed to work.
>
>> Watermark is my bad. I forgot to include it and also to toggle it we
>> would need to add support for disabling it what we don't have at all
>> but I believe we should have anyways.
>
> Watermarks was a bit a trick example, since imo adding code to
> fake-disable low-power watermark levels is nonsense. It's only going to
> help with very specific bugs, and if you don't understand the wm
> programming model for your platforms it's going to be totally useless
> information. Misconfiguring the driver to make it crap doesn't really tell
> you a lot ...
> -Daniel
>

Thank you for all the reviews and the constructive feedback.  I'm 
pleased the patches got your attention.

I'll spin today the patches to show only the state of the features on 
sysfs.  I still think there is some value on toggling the interfaces, 
however the stability of the driver for the end user seems to win that 
battle for now :)

I see the benefit on adding more documentation around each feature and 
how they impact RPM.  I think the platform enabling engineer would find 
this kind of information interesting when debugging the many GFX bugs 
that often are the culprit when reaching deep C states.

-- 
Thank you,
<Alexandra>