[PATCH] drm/amd: Only allow one entity to control ABM
Deucher, Alexander
Alexander.Deucher at amd.com
Tue Feb 20 14:18:21 UTC 2024
[Public]
> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces at lists.freedesktop.org> On Behalf Of
> Christian König
> Sent: Tuesday, February 20, 2024 9:10 AM
> To: Alex Deucher <alexdeucher at gmail.com>
> Cc: Limonciello, Mario <Mario.Limonciello at amd.com>; Wentland, Harry
> <Harry.Wentland at amd.com>; amd-gfx at lists.freedesktop.org; dri-
> devel at lists.freedesktop.org; Mahfooz, Hamza <Hamza.Mahfooz at amd.com>;
> Li, Sun peng (Leo) <Sunpeng.Li at amd.com>
> Subject: Re: [PATCH] drm/amd: Only allow one entity to control ABM
>
> Am 19.02.24 um 16:28 schrieb Alex Deucher:
> > On Mon, Feb 19, 2024 at 10:19 AM Christian König
> > <christian.koenig at amd.com> wrote:
> >> Am 16.02.24 um 19:37 schrieb Alex Deucher:
> >>> On Fri, Feb 16, 2024 at 10:42 AM Christian König
> >>> <christian.koenig at amd.com> wrote:
> >>>> Am 16.02.24 um 16:12 schrieb Mario Limonciello:
> >>>>> On 2/16/2024 09:05, Harry Wentland wrote:
> >>>>>> On 2024-02-16 09:47, Christian König wrote:
> >>>>>>> Am 16.02.24 um 15:42 schrieb Mario Limonciello:
> >>>>>>>> On 2/16/2024 08:38, Christian König wrote:
> >>>>>>>>> Am 16.02.24 um 15:07 schrieb Mario Limonciello:
> >>>>>>>>>> By exporting ABM to sysfs it's possible that DRM master and
> >>>>>>>>>> software controlling the sysfs file fight over the value programmed
> for ABM.
> >>>>>>>>>>
> >>>>>>>>>> Adjust the module parameter behavior to control who control
> ABM:
> >>>>>>>>>> -2: DRM
> >>>>>>>>>> -1: sysfs (IE via software like power-profiles-daemon)
> >>>>>>>>> Well that sounds extremely awkward. Why should a
> >>>>>>>>> power-profiles-deamon has control over the panel power saving
> >>>>>>>>> features?
> >>>>>>>>>
> >>>>>>>>> I mean we are talking about things like reducing backlight
> >>>>>>>>> level when the is inactivity, don't we?
> >>>>>>>> We're talking about activating the ABM algorithm when the
> >>>>>>>> system is in power saving mode; not from inactivity. This
> >>>>>>>> allows the user to squeeze out some extra power "just" in that
> situation.
> >>>>>>>>
> >>>>>>>> But given the comments on the other patch, I tend to agree with
> >>>>>>>> Harry's proposal instead that we just drop the DRM property
> >>>>>>>> entirely as there are no consumers of it.
> >>>>>>> Yeah, but even then the design to let this be controlled by an
> >>>>>>> userspace deamon is questionable. Stuff like that is handled
> >>>>>>> inside the kernel and not exposed to userspace usually.
> >>>>>>>
> >>>>> Regarding the "how" and "why" of PPD; besides this panel power
> >>>>> savings sysfs file there are two other things that are nominally changed.
> >>>>>
> >>>>> ACPI platform profile:
> >>>>> https://www.kernel.org/doc/html/latest/userspace-api/sysfs-platfor
> >>>>> m_profile.html
> >>>>>
> >>>>> AMD-Pstate EPP value:
> >>>>> https://www.kernel.org/doc/html//latest/admin-guide/pm/amd-
> pstate.
> >>>>> html
> >>>>>
> >>>>> When a user goes into "power saving" mode both of those are tweaked.
> >>>>> Before we introduced the EPP tweaking in PPD we did discuss a
> >>>>> callback within the kernel so that userspace could change "just"
> >>>>> the ACPI platform profile and everything else would react. There
> >>>>> was pushback on this, and so instead knobs are offered for things
> >>>>> that should be tweaked and the userspace daemon can set up policy
> >>>>> for what to do when a a user uses a userspace client (such as
> >>>>> GNOME or KDE) to change the desired system profile.
> >>>> Ok, well who came up with the idea of the userspace deamon? Cause I
> >>>> think there will be even more push back on this approach.
> >>>>
> >>>> Basically when we go from AC to battery (or whatever) the drivers
> >>>> usually handle that all inside the kernel today. Involving
> >>>> userspace is only done when there is a need for that, e.g.
> >>>> inactivity detection or similar.
> >>> Well, we don't want policy in the kernel unless it's a platform or
> >>> hardware requirement. Kernel should provide the knobs and then
> >>> userspace can set them however they want depending on user preference.
> >> Well, you not have the policy itself but usually the handling inside
> >> the kernel.
> >>
> >> In other words when I connect/disconnect AC from my laptop I can hear
> >> the fan changing, which is a switch in power state. Only the beep
> >> which comes out of the speakers as conformation is handled in userspace I
> think.
> >>
> >> And IIRC changing background light is also handled completely inside
> >> the kernel and when I close the lid the display turns off on its own
> >> and not because of some userspace deamon.
> >>
> >> So why is for this suddenly a userspace deamon involved?
> > It's a user preference. Some people won't like ABM, some will. They
> > set the policy from user space. It's similar to the backlight level.
> > Some users always prefer a bright backlight regardless of AC/DC state,
> > others want the backlight to get brighter when on AC power. The
> > kernel provides the knobs to set the ABM level and then user space can
> > specify the level and also device when they want it enabled (never,
> > only on DC, etc.). The kernel driver for the backlight doesn't change
> > the backlight at AC/DC switch, userspace gets an event when the power
> > source changes and it then talks to the kernel to change the backlight
> > level. I think lid is handled the same way. Userspace gets a lid
> > event and it turns off the displays and maybe enters suspend or maybe
> > not depending on what the user wants.
>
> Ok, well that comes as a surprise. Which userspace component is responsible
> for this?
IIRC, Systemd-logind handles most of the power events.
Alex
>
> Christian.
>
> >
> > Alex
> >
> >> Christian.
> >>
> >>> Alex
> >>>
> >>>
> >>>>>> I think we'll need a bit in our kernel docs describing ABM.
> >>>>>> Assumptions around what it is and does seem to lead to confusion.
> >>>>>>
> >>>>>> The thing is that ABM has a visual impact that not all users
> >>>>>> like. It would make sense for users to be able to change the ABM
> >>>>>> level as desired, and/or configure their power profiles to select
> >>>>>> a certain ABM level.
> >>>>>>
> >>>>>> ABM will reduce the backlight and compensate by adjusting
> >>>>>> brightness and contrast of the image. It has 5 levels: 0, 1, 2,
> >>>>>> 3, 4. 0 means off. 4 means maximum backlight reduction. IMO, 1
> >>>>>> and 2 look okay. 3 and 4 can be quite impactful, both to power and
> visual fidelity.
> >>>>>>
> >>>>> Aside from this patch Hamza did make some improvements to the kdoc
> >>>>> in the recent patches, can you read through again and see if you
> >>>>> think it still needs improvements?
> >>>> Well what exactly is the requirement? That we have different ABM
> >>>> settings for AC and battery? If yes providing different DRM
> >>>> properties would sound like the right approach to me.
> >>>>
> >>>> Regards,
> >>>> Christian.
> >>>>
> >>>>>> Harry
> >>>>>>
> >>>>>>> Regards,
> >>>>>>> Christian.
> >>>>>>>
> >>>>>>>>> Regards,
> >>>>>>>>> Christian.
> >>>>>>>>>
> >>>>>>>>>> 0-4: User via command line
> >>>>>>>>>>
> >>>>>>>>>> Also introduce a Kconfig option that allows distributions to
> >>>>>>>>>> choose the default policy that is appropriate for them.
> >>>>>>>>>>
> >>>>>>>>>> Fixes: f97e4303da16 ("drm/amd/display: add
> >>>>>>>>>> panel_power_savings sysfs entry to eDP connectors")
> >>>>>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
> >>>>>>>>>> ---
> >>>>>>>>>> Cc: Hamza Mahfooz <Hamza.Mahfooz at amd.com>
> >>>>>>>>>> Cc: Harry Wentland <Harry.Wentland at amd.com>
> >>>>>>>>>> Cc: Sun peng (Leo) Li <Sunpeng.Li at amd.com>
> >>>>>>>>>> drivers/gpu/drm/amd/amdgpu/Kconfig | 72
> >>>>>>>>>> +++++++++++++++++++
> >>>>>>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 23 +++---
> >>>>>>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 +-
> >>>>>>>>>> 3 files changed, 90 insertions(+), 11 deletions(-)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig
> >>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/Kconfig
> >>>>>>>>>> index 22d88f8ef527..2ab57ccf6f21 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
> >>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
> >>>>>>>>>> @@ -80,6 +80,78 @@ config DRM_AMDGPU_WERROR
> >>>>>>>>>> Add -Werror to the build flags for amdgpu.ko.
> >>>>>>>>>> Only enable this if you are warning code for amdgpu.ko.
> >>>>>>>>>> +choice
> >>>>>>>>>> + prompt "Amdgpu panel power Savings"
> >>>>>>>>>> + default AMDGPU_SYSFS_ABM
> >>>>>>>>>> + help
> >>>>>>>>>> + Control the default behavior for adaptive panel
> >>>>>>>>>> +power
> >>>>>>>>>> savings.
> >>>>>>>>>> +
> >>>>>>>>>> + Panel power savings features will sacrifice color accuracy
> >>>>>>>>>> + in exchange for power savings.
> >>>>>>>>>> +
> >>>>>>>>>> + This can be configured for:
> >>>>>>>>>> + - dynamic control by the DRM master
> >>>>>>>>>> + - dynamic control by sysfs nodes
> >>>>>>>>>> + - statically by the user at kernel compile time
> >>>>>>>>>> +
> >>>>>>>>>> + This value can also be overridden by the amdgpu.abmlevel
> >>>>>>>>>> + module parameter.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_DRM_ABM
> >>>>>>>>>> + bool "DRM Master control"
> >>>>>>>>>> + help
> >>>>>>>>>> + Export a property called 'abm_level' that can be
> >>>>>>>>>> + manipulated by the DRM master for supported hardware.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_SYSFS_ABM
> >>>>>>>>>> + bool "sysfs control"
> >>>>>>>>>> + help
> >>>>>>>>>> + Export a sysfs file 'panel_power_savings' that can be
> >>>>>>>>>> + manipulated by userspace for supported hardware.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_HARDCODE_ABM0
> >>>>>>>>>> + bool "No Panel power savings"
> >>>>>>>>>> + help
> >>>>>>>>>> + Disable panel power savings.
> >>>>>>>>>> + It can only overridden by the kernel command line.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_HARDCODE_ABM1
> >>>>>>>>>> + bool "25% Panel power savings"
> >>>>>>>>>> + help
> >>>>>>>>>> + Set the ABM panel power savings algorithm to 25%.
> >>>>>>>>>> + It can only overridden by the kernel command line.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_HARDCODE_ABM2
> >>>>>>>>>> + bool "50% Panel power savings"
> >>>>>>>>>> + help
> >>>>>>>>>> + Set the ABM panel power savings algorithm to 50%.
> >>>>>>>>>> + It can only overridden by the kernel command line.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_HARDCODE_ABM3
> >>>>>>>>>> + bool "75% Panel power savings"
> >>>>>>>>>> + help
> >>>>>>>>>> + Set the ABM panel power savings algorithm to 75%.
> >>>>>>>>>> + It can only overridden by the kernel command line.
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_HARDCODE_ABM4
> >>>>>>>>>> + bool "100% Panel power savings"
> >>>>>>>>>> + help
> >>>>>>>>>> + Set the ABM panel power savings algorithm to 100%.
> >>>>>>>>>> + It can only overridden by the kernel command line.
> >>>>>>>>>> +endchoice
> >>>>>>>>>> +
> >>>>>>>>>> +config AMDGPU_ABM_POLICY
> >>>>>>>>>> + int
> >>>>>>>>>> + default -2 if AMDGPU_DRM_ABM
> >>>>>>>>>> + default -1 if AMDGPU_SYSFS_ABM
> >>>>>>>>>> + default 0 if AMDGPU_HARDCODE_ABM0
> >>>>>>>>>> + default 1 if AMDGPU_HARDCODE_ABM1
> >>>>>>>>>> + default 2 if AMDGPU_HARDCODE_ABM2
> >>>>>>>>>> + default 3 if AMDGPU_HARDCODE_ABM3
> >>>>>>>>>> + default 4 if AMDGPU_HARDCODE_ABM4
> >>>>>>>>>> + default -1
> >>>>>>>>>> +
> >>>>>>>>>> +
> >>>>>>>>>> source "drivers/gpu/drm/amd/acp/Kconfig"
> >>>>>>>>>> source "drivers/gpu/drm/amd/display/Kconfig"
> >>>>>>>>>> source "drivers/gpu/drm/amd/amdkfd/Kconfig"
> >>>>>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>>>>>>> index af7fae7907d7..00d6c8b58716 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >>>>>>>>>> @@ -844,17 +844,24 @@ module_param_named(visualconfirm,
> >>>>>>>>>> amdgpu_dc_visual_confirm, uint, 0444);
> >>>>>>>>>> * DOC: abmlevel (uint)
> >>>>>>>>>> * Override the default ABM (Adaptive Backlight
> >>>>>>>>>> Management) level used for DC
> >>>>>>>>>> * enabled hardware. Requires DMCU to be supported and
> loaded.
> >>>>>>>>>> - * Valid levels are 0-4. A value of 0 indicates that ABM
> >>>>>>>>>> should be disabled by
> >>>>>>>>>> - * default. Values 1-4 control the maximum allowable
> >>>>>>>>>> brightness reduction via
> >>>>>>>>>> - * the ABM algorithm, with 1 being the least reduction and 4
> >>>>>>>>>> being the most
> >>>>>>>>>> - * reduction.
> >>>>>>>>>> + * Valid levels are -2 through 4.
> >>>>>>>>>> *
> >>>>>>>>>> - * Defaults to -1, or disabled. Userspace can only override
> >>>>>>>>>> this level after
> >>>>>>>>>> - * boot if it's set to auto.
> >>>>>>>>>> + * -2: indicates that ABM should be controlled by DRM
> >>>>>>>>>> + property
> >>>>>>>>>> 'abm_level.
> >>>>>>>>>> + * -1: indicates that ABM should be controlled by the sysfs file
> >>>>>>>>>> + * 'panel_power_savings'.
> >>>>>>>>>> + * 0: indicates that ABM should be disabled.
> >>>>>>>>>> + * 1-4: control the maximum allowable brightness reduction via
> >>>>>>>>>> + * the ABM algorithm, with 1 being the least reduction and
> >>>>>>>>>> 4 being the most
> >>>>>>>>>> + * reduction.
> >>>>>>>>>> + *
> >>>>>>>>>> + * Both the DRM property 'abm_level' and the sysfs file
> >>>>>>>>>> 'panel_power_savings'
> >>>>>>>>>> + * will only be available on supported hardware configurations.
> >>>>>>>>>> + *
> >>>>>>>>>> + * The default value is configured by kernel configuration
> >>>>>>>>>> option AMDGPU_ABM_POLICY
> >>>>>>>>>> */
> >>>>>>>>>> -int amdgpu_dm_abm_level = -1;
> >>>>>>>>>> +int amdgpu_dm_abm_level = CONFIG_AMDGPU_ABM_POLICY;
> >>>>>>>>>> MODULE_PARM_DESC(abmlevel,
> >>>>>>>>>> - "ABM level (0 = off, 1-4 = backlight reduction level,
> >>>>>>>>>> -1 auto (default))");
> >>>>>>>>>> + "ABM level (0 = off, 1-4 = backlight reduction
> >>>>>>>>>> + level,
> >>>>>>>>>> -1 = sysfs control, -2 = drm control");
> >>>>>>>>>> module_param_named(abmlevel, amdgpu_dm_abm_level, int,
> 0444);
> >>>>>>>>>> int amdgpu_backlight = -1; diff --git
> >>>>>>>>>> a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>>>>>> index b9ac3d2f8029..147fe744f82e 100644
> >>>>>>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>>>>>> +++
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >>>>>>>>>> @@ -6515,7 +6515,7 @@ static void
> >>>>>>>>>> amdgpu_dm_connector_unregister(struct drm_connector
> *connector)
> >>>>>>>>>> struct amdgpu_dm_connector *amdgpu_dm_connector =
> >>>>>>>>>> to_amdgpu_dm_connector(connector);
> >>>>>>>>>> if (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP &&
> >>>>>>>>>> - amdgpu_dm_abm_level < 0)
> >>>>>>>>>> + amdgpu_dm_abm_level == -1)
> >>>>>>>>>> sysfs_remove_group(&connector->kdev->kobj,
> &amdgpu_group);
> >>>>>>>>>> drm_dp_aux_unregister(&amdgpu_dm_connector-
> >dm_dp_aux.aux);
> >>>>>>>>>> @@ -6623,7 +6623,7 @@
> >>>>>>>>>> amdgpu_dm_connector_late_register(struct
> >>>>>>>>>> drm_connector *connector)
> >>>>>>>>>> int r;
> >>>>>>>>>> if (connector->connector_type ==
> DRM_MODE_CONNECTOR_eDP &&
> >>>>>>>>>> - amdgpu_dm_abm_level < 0) {
> >>>>>>>>>> + amdgpu_dm_abm_level == -1) {
> >>>>>>>>>> r = sysfs_create_group(&connector->kdev->kobj,
> >>>>>>>>>> &amdgpu_group);
> >>>>>>>>>> if (r)
> >>>>>>>>>> @@ -7654,7 +7654,7 @@ void
> >>>>>>>>>> amdgpu_dm_connector_init_helper(struct
> >>>>>>>>>> amdgpu_display_manager *dm,
> >>>>>>>>>> if (connector_type == DRM_MODE_CONNECTOR_eDP &&
> >>>>>>>>>> (dc_is_dmcu_initialized(adev->dm.dc) ||
> >>>>>>>>>> - adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level
> < 0) {
> >>>>>>>>>> + adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level
> >>>>>>>>>> + ==
> >>>>>>>>>> -2) {
> >>>>>>>>>> drm_object_attach_property(&aconnector->base.base,
> >>>>>>>>>> adev->mode_info.abm_level_property, 0);
> >>>>>>>>>> }
More information about the amd-gfx
mailing list