[PATCH] drm/amd: Only allow one entity to control ABM
Alex Deucher
alexdeucher at gmail.com
Fri Feb 16 18:37:43 UTC 2024
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.
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