[PATCH] drm/amd: Only allow one entity to control ABM
Christian König
christian.koenig at amd.com
Tue Feb 20 14:09:31 UTC 2024
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-platform_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?
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