[Public] > -----Original Message----- > From: amd-gfx <amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of > Christian König > Sent: Tuesday, February 20, 2024 9:10 AM > To: Alex Deucher <alexdeucher@xxxxxxxxx> > Cc: Limonciello, Mario <Mario.Limonciello@xxxxxxx>; Wentland, Harry > <Harry.Wentland@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; dri- > devel@xxxxxxxxxxxxxxxxxxxxx; Mahfooz, Hamza <Hamza.Mahfooz@xxxxxxx>; > Li, Sun peng (Leo) <Sunpeng.Li@xxxxxxx> > 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@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> > >>>>>>>>>> --- > >>>>>>>>>> Cc: Hamza Mahfooz <Hamza.Mahfooz@xxxxxxx> > >>>>>>>>>> Cc: Harry Wentland <Harry.Wentland@xxxxxxx> > >>>>>>>>>> Cc: Sun peng (Leo) Li <Sunpeng.Li@xxxxxxx> > >>>>>>>>>> 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); > >>>>>>>>>> }
