tuned icon indicating copy to clipboard operation
tuned copied to clipboard

Add a plugin for amdgpu tuning of the `panel_power_savings` attribute

Open superm1 opened this issue 1 year ago • 18 comments

This attribute accepts a range from 0 through 4 where larger values will also have larger panel power savings.

Using this has a trade off for color accuracy, and it is only applied when the system is currently operating on battery.

The plugin uses upower to get a signal when battery changes so that it will react instantly.

Intentionally the plugin will check what values are already programmed to the sysfs file to avoid unnecessary writes. Writing the sysfs file will cause a modeset which isn't necessary if writing the same value twice.

The default values are applied to the profiles that are used in power-profiles-daemon compatbility. They also match the values used in that software.

superm1 avatar Feb 11 '24 02:02 superm1

I'm not sure what I think about enabling this by default for the main stock profiles, mainly due to the new upower dependency and the fact that it basically performs a kind of "dynamic tuning", which TuneD has now abandoned by default. @yarda, what are your thoughts on this?

Regarding "dynamic tuning" it's not a blocker, because I think we need per plugin configuration, e.g. something like:

tuned-main.conf:
dynamic_tuning=no, yes:amdgpu, yes:scheduler

This was discussed earlier, but never implemented :). This is for another PR. Regarding the scheduler plugin, the dynamic tuning should do the same what currently the runtime=1 hack do.

yarda avatar Feb 21 '24 10:02 yarda

I think it should reflect the enable_dbus main conf and --no-dbus command line option and also work without DBus (e.g. with limited functionality or with plugin disabled), because there are products which runs by default without DBus, e.g. OCP and this shouldn't break it.

yarda avatar Feb 21 '24 10:02 yarda

I'm not sure what I think about enabling this by default for the main stock profiles, mainly due to the new upower dependency

Regarding the upower, it should detect whether upower is available and if yes use it. This is what other plugin do (e.g. disk plugin with hdparm) then we could softdep upower and it will not break consumers without upower.

yarda avatar Feb 21 '24 10:02 yarda

Also what about adding this functionality into the video plugin?

yarda avatar Feb 21 '24 13:02 yarda

I'm not sure what I think about enabling this by default for the main stock profiles, mainly due to the new upower dependency

Regarding the upower, it should detect whether upower is available and if yes use it. This is what other plugin do (e.g. disk plugin with hdparm) then we could softdep upower and it will not break consumers without upower.

If it's acceptable I would prefer to make power mandatory for the plugin. If it's not available or masked then fail the plugin initialization/load or make it a noop.

I'll double check behavior for the plugin with it masked to make sure I got it right.

superm1 avatar Feb 21 '24 13:02 superm1

Also what about adding this functionality into the video plugin?

I was thinking dedicated amdgpu plugin with the idea that we could extend it to other amdgpu stuff later maybe.

superm1 avatar Feb 21 '24 13:02 superm1

I think it should reflect the enable_dbus main conf and --no-dbus command line option and also work without DBus (e.g. with limited functionality or with plugin disabled), because there are products which runs by default without DBus, e.g. OCP and this shouldn't break it.

I think plugin should be disabled for this case.

superm1 avatar Feb 21 '24 14:02 superm1

I think it should reflect the enable_dbus main conf and --no-dbus command line option and also work without DBus (e.g. with limited functionality or with plugin disabled), because there are products which runs by default without DBus, e.g. OCP and this shouldn't break it.

I think plugin should be disabled for this case.

I believe the modifications I've made will effectively cause this behavior if upower can't be accessed for any reason (such as dbus is disabled). Let me know if you disagree.

superm1 avatar Feb 21 '24 15:02 superm1

Also what about adding this functionality into the video plugin?

I was thinking dedicated amdgpu plugin with the idea that we could extend it to other amdgpu stuff later maybe.

The current functionality of the video plugin is also AMD-specific.

zacikpa avatar Feb 22 '24 08:02 zacikpa

Also what about adding this functionality into the video plugin?

I was thinking dedicated amdgpu plugin with the idea that we could extend it to other amdgpu stuff later maybe.

The current functionality of the video plugin is also AMD-specific.

I think it could be good extension of the video plugin, because it currently doesn't have dynamic tuning and maybe easier and more user friendly to write more generic:

[video]
...

than HW specific:

[video]
...
[amdgpu]
...

yarda avatar Feb 22 '24 10:02 yarda

OK let me see if I can squeeze it into that plugin instead. Two questions:

  1. do you mind if I reformat the whole file with black in an earlier commit ? It's got some pretty funky (IMO) whitespace. As I started to merge the two it became really apparent.

  2. Should the option be called amdgpu_panel_power_savings or panel_power_savings? Radeon will never pick this feature up, it's specific to the modern amdgpu hardware that runs DCN.

superm1 avatar Feb 22 '24 19:02 superm1

For now I've force pushed using the old configuration option name and kept the existing style for that plugin. In general though, I think it would be better to reformat it with black if you're open to it.

superm1 avatar Feb 22 '24 20:02 superm1

ping?

superm1 avatar Mar 04 '24 21:03 superm1

@superm1, sorry for my longer inactivity.

I'm still looking for a suitable device for testing. You mentioned that it has to run DCN, is there any specific version of DCN required? Will I be able to test this on Raphael, Renoir, or Ridge?

zacikpa avatar Mar 12 '24 09:03 zacikpa

The important part is that it is a mobile design with Vega or newer. I tested it on Phoenix and Rembrandt but I would expect it to be exposed on Renoir or newer.

superm1 avatar Mar 12 '24 12:03 superm1

One more thing:

Backport these 2 kernel patches from drm-next to 6.8 kernels to enable access to ABM:

  • https://gitlab.freedesktop.org/agd5f/linux/-/commit/63d0b87213a0ba241b3fcfba3fe7b0aed0cd1cc5
  • https://gitlab.freedesktop.org/agd5f/linux/-/commit/040fdcde288a2830edc31dd507963d6aadf990d2

They'll be part of 6.9-rc1.

superm1 avatar Mar 12 '24 12:03 superm1

I still haven't been able to test this, because I can't easily patch the kernel on the AMD GPU machines that I can access.

The patches are going to be in 6.9-rc1 if that makes it easier.

Here is sample output with the PR with a battery unplugged at startup, plugged in, and then unplugged, then powersave, then back to balanced.

$ sudo ./tuned.py  --debug 2>&1 | grep plugin_video
2024-03-18 10:56:21,786 DEBUG    tuned.utils.plugin_loader: loading module tuned.plugins.plugin_video
2024-03-18 10:56:22,038 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:56:22,038 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:56:22,040 DEBUG    tuned.plugins.plugin_video: 🔋: True, 🎯: 1
2024-03-18 10:56:22,040 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 1 [🔋: True]
2024-03-18 10:56:34,022 DEBUG    tuned.plugins.plugin_video: 🔋: False, 🎯: 1
2024-03-18 10:56:34,023 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 0 [🔋: False]
2024-03-18 10:56:36,758 DEBUG    tuned.plugins.plugin_video: 🔋: True, 🎯: 1
2024-03-18 10:56:36,758 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 1 [🔋: True]
2024-03-18 10:59:06,934 DEBUG    tuned.utils.plugin_loader: loading module tuned.plugins.plugin_video
2024-03-18 10:59:07,806 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:59:07,806 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:59:07,809 DEBUG    tuned.plugins.plugin_video: 🔋: True, 🎯: 3
2024-03-18 10:59:07,809 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 3 [🔋: True]
2024-03-18 10:59:17,192 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 0 [🔋: True]
2024-03-18 10:59:17,967 DEBUG    tuned.utils.plugin_loader: loading module tuned.plugins.plugin_video
2024-03-18 10:59:18,231 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:59:18,231 DEBUG    tuned.plugins.plugin_video: radeon_powersave is not supported on 'card0-eDP-1'
2024-03-18 10:59:18,232 DEBUG    tuned.plugins.plugin_video: 🔋: True, 🎯: 1
2024-03-18 10:59:18,232 INFO     tuned.plugins.plugin_video: card0-eDP-1 panel_power_savings -> 1 [🔋: True]

superm1 avatar Mar 18 '24 12:03 superm1

@yarda ping?

superm1 avatar Mar 29 '24 05:03 superm1

Thanks, please remove the upower mention from the commit https://github.com/redhat-performance/tuned/commit/9d108694d623fb7d32c7dde40881041e215ff6ce description:

Rebased on main and done now.

superm1 avatar May 23 '24 15:05 superm1

Thanks.

yarda avatar May 23 '24 16:05 yarda