Add a plugin for amdgpu tuning of the panel_power_savings attribute

[superm1]

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.

[zacikpa]

Thanks for the contribution, Mario, it looks interesting. I haven't yet tested this properly, as I do not have any device with an AMD GPU at hand.

On devices without AMD GPUs, TuneD now reports errors due to the missing file /sys/class/drm/card1-eDP-1/amdgpu/panel_power_savings. Make sure to check that the file exists or add a flag that disables the functionality of the plugin entirely if the device doesn't have an AMD card.

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?

[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 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]

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]

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]

Also what about adding this functionality into the video plugin?

[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

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]

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]

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]

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.

[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.

[zacikpa]

Thanks for the update. Now, this seems to work as expected (i.e., no-op) on machines without AMD GPUs or without upower on the dbus. I'll try to find an AMD GPU machine to test it fully.

[yarda]

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]
...

[superm1]

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]

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]

ping?

[zacikpa]

@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?

[superm1]

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]

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]

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]

[zacikpa]

Thanks for the update. I've got one more change request and when that is fixed, this looks mergeable to me.

[zacikpa]

Managed to find one other thing still.

[zacikpa]

Thanks. LGTM, so approving, but leaving the final merge decision on @yarda.

[superm1]

@yarda ping?

[zacikpa]

As discussed in #616, the response to battery/AC changes should be handled by tuned-ppd and thus moved to that PR.

[zacikpa]

Thanks, please delete the remnants of the dbus functionality according to the comments.

[yarda]

Thanks, please remove the upower mention from the commit 9d10869 description:

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

Otherwise OK.

[superm1]

Thanks, please remove the upower mention from the commit 9d10869 description:

Rebased on main and done now.

[yarda]

Thanks.