[PATCH] drm/amd: Only allow one entity to control ABM
Christian König
christian.koenig at amd.com
Mon Feb 19 15:19:45 UTC 2024
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?
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