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-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@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); > >>>>>> } > >>>>> > >>>> > >>> > >> > > >
