drm/amd: Only allow one entity to control ABM

Commit Message

Mario Limonciello Feb. 16, 2024, 2:07 p.m. UTC

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)
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 <[e-mail]>
---
Cc: Hamza Mahfooz <[e-mail]>
Cc: Harry Wentland <[e-mail]>
Cc: Sun peng (Leo) Li <[e-mail]>
 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(-)

Comments

Christian König Feb. 16, 2024, 2:38 p.m. UTC | #1

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?

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 <[e-mail]>
> ---
> Cc: Hamza Mahfooz <[e-mail]>
> Cc: Harry Wentland <[e-mail]>
> Cc: Sun peng (Leo) Li <[e-mail]>
>   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);
>   	}

Mario Limonciello Feb. 16, 2024, 2:42 p.m. UTC | #2

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.

> 
> 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 <[e-mail]>
>> ---
>> Cc: Hamza Mahfooz <[e-mail]>
>> Cc: Harry Wentland <[e-mail]>
>> Cc: Sun peng (Leo) Li <[e-mail]>
>>   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);
>>       }
>

Christian König Feb. 16, 2024, 2:47 p.m. UTC | #3

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.

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 <[e-mail]>
>>> ---
>>> Cc: Hamza Mahfooz <[e-mail]>
>>> Cc: Harry Wentland <[e-mail]>
>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>   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);
>>>       }
>>
>

Harry Wentland Feb. 16, 2024, 3:05 p.m. UTC | #4

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.
> 

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.

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 <[e-mail]>
>>>> ---
>>>> Cc: Hamza Mahfooz <[e-mail]>
>>>> Cc: Harry Wentland <[e-mail]>
>>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>>   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);
>>>>       }
>>>
>>
>

Mario Limonciello Feb. 16, 2024, 3:12 p.m. UTC | #5

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.

> 
> 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?

> 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 <[e-mail]>
>>>>> ---
>>>>> Cc: Hamza Mahfooz <[e-mail]>
>>>>> Cc: Harry Wentland <[e-mail]>
>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>>>    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);
>>>>>        }
>>>>
>>>
>>
>

Christian König Feb. 16, 2024, 3:41 p.m. UTC | #6

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.

>>
>> 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 <[e-mail]>
>>>>>> ---
>>>>>> Cc: Hamza Mahfooz <[e-mail]>
>>>>>> Cc: Harry Wentland <[e-mail]>
>>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>>>>    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);
>>>>>>        }
>>>>>
>>>>
>>>
>>
>

Mario Limonciello Feb. 16, 2024, 3:47 p.m. UTC | #7

On 2/16/2024 09:41, Christian König 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.
> 

It's more than AC vs battery.  It's user preference of how they want to 
use the system.

Here's some screenshots of how it all works:

https://linuxconfig.org/how-to-manage-power-profiles-over-d-bus-with-power-profiles-daemon-on-linux

>>>
>>> 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.
> 

It's user wants system in "power-saving" state or they want it in 
"balanced" state or they want it in "performance" state.

User picks that state in a client and there is a designated ABM policy 
value that goes with it.  Daemon programs the ABM value.

Alex Deucher Feb. 16, 2024, 6:37 p.m. UTC | #8

On Fri, Feb 16, 2024 at 10:42 AM Christian König
<[e-mail]> 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 <[e-mail]>
> >>>>>> ---
> >>>>>> Cc: Hamza Mahfooz <[e-mail]>
> >>>>>> Cc: Harry Wentland <[e-mail]>
> >>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
> >>>>>>    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);
> >>>>>>        }
> >>>>>
> >>>>
> >>>
> >>
> >
>

Christian König Feb. 19, 2024, 3:19 p.m. UTC | #9

Am 16.02.24 um 19:37 schrieb Alex Deucher:
> On Fri, Feb 16, 2024 at 10:42 AM Christian König
> <[e-mail]> 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 <[e-mail]>
>>>>>>>> ---
>>>>>>>> Cc: Hamza Mahfooz <[e-mail]>
>>>>>>>> Cc: Harry Wentland <[e-mail]>
>>>>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>>>>>>     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);
>>>>>>>>         }

Alex Deucher Feb. 19, 2024, 3:28 p.m. UTC | #10

On Mon, Feb 19, 2024 at 10:19 AM Christian König
<[e-mail]> wrote:
>
> Am 16.02.24 um 19:37 schrieb Alex Deucher:
> > On Fri, Feb 16, 2024 at 10:42 AM Christian König
> > <[e-mail]> 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.

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 <[e-mail]>
> >>>>>>>> ---
> >>>>>>>> Cc: Hamza Mahfooz <[e-mail]>
> >>>>>>>> Cc: Harry Wentland <[e-mail]>
> >>>>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
> >>>>>>>>     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);
> >>>>>>>>         }
>

Christian König Feb. 20, 2024, 2:09 p.m. UTC | #11

Am 19.02.24 um 16:28 schrieb Alex Deucher:
> On Mon, Feb 19, 2024 at 10:19 AM Christian König
> <[e-mail]> wrote:
>> Am 16.02.24 um 19:37 schrieb Alex Deucher:
>>> On Fri, Feb 16, 2024 at 10:42 AM Christian König
>>> <[e-mail]> 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 <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>> Cc: Hamza Mahfooz <[e-mail]>
>>>>>>>>>> Cc: Harry Wentland <[e-mail]>
>>>>>>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
>>>>>>>>>>      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);
>>>>>>>>>>          }

Alex Deucher Feb. 20, 2024, 2:18 p.m. UTC | #12

[Public]

> -----Original Message-----
> From: amd-gfx <[e-mail]> On Behalf Of
> Christian König
> Sent: Tuesday, February 20, 2024 9:10 AM
> To: Alex Deucher <[e-mail]>
> Cc: Limonciello, Mario <[e-mail]>; Wentland, Harry
> <[e-mail]>; [e-mail]; dri-
> [e-mail]; Mahfooz, Hamza <[e-mail]>;
> Li, Sun peng (Leo) <[e-mail]>
> 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
> > <[e-mail]> wrote:
> >> Am 16.02.24 um 19:37 schrieb Alex Deucher:
> >>> On Fri, Feb 16, 2024 at 10:42 AM Christian König
> >>> <[e-mail]> 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 <[e-mail]>
> >>>>>>>>>> ---
> >>>>>>>>>> Cc: Hamza Mahfooz <[e-mail]>
> >>>>>>>>>> Cc: Harry Wentland <[e-mail]>
> >>>>>>>>>> Cc: Sun peng (Leo) Li <[e-mail]>
> >>>>>>>>>>      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);
> >>>>>>>>>>          }

Gergo Koteles April 13, 2024, 8:51 a.m. UTC | #13

Hi> 

> 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.

I tried this with 6.9 and it looks weird with an OLED panel used with
dark UI settings.
The dark is no longer dark, everything is brighter.
I turned this feature off with amdgpu.abmlevel=0.

Best regards,
Gergo

Mario Limonciello April 26, 2024, 2:44 p.m. UTC | #14

On 4/13/2024 03:51, Gergo Koteles wrote:
> Hi>
> 
>> 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.
> 
> I tried this with 6.9 and it looks weird with an OLED panel used with
> dark UI settings.
> The dark is no longer dark, everything is brighter.
> I turned this feature off with amdgpu.abmlevel=0.
> 
> Best regards,
> Gergo
> 

Would you mind filing a bug with the details please?  This was something 
that was actually explicitly checked against an OLED panel.  ABM 
shouldn't be applying to OLED today so this very likely points at a bug 
somewhere in the stack.

Patch

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