Re: [PATCH] drm/amd: Only allow one entity to control ABM

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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 <mario.limonciello@xxxxxxx>
>>>> ---
>>>> Cc: Hamza Mahfooz <Hamza.Mahfooz@xxxxxxx>
>>>> Cc: Harry Wentland <Harry.Wentland@xxxxxxx>
>>>> Cc: Sun peng (Leo) Li <Sunpeng.Li@xxxxxxx>
>>>>   drivers/gpu/drm/amd/amdgpu/Kconfig            | 72 +++++++++++++++++++
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       | 23 +++---
>>>>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  6 +-
>>>>   3 files changed, 90 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/Kconfig b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>>> index 22d88f8ef527..2ab57ccf6f21 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/Kconfig
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/Kconfig
>>>> @@ -80,6 +80,78 @@ config DRM_AMDGPU_WERROR
>>>>         Add -Werror to the build flags for amdgpu.ko.
>>>>         Only enable this if you are warning code for amdgpu.ko.
>>>> +choice
>>>> +    prompt "Amdgpu panel power Savings"
>>>> +    default AMDGPU_SYSFS_ABM
>>>> +    help
>>>> +        Control the default behavior for adaptive panel power savings.
>>>> +
>>>> +        Panel power savings features will sacrifice color accuracy
>>>> +        in exchange for power savings.
>>>> +
>>>> +        This can be configured for:
>>>> +        - dynamic control by the DRM master
>>>> +        - dynamic control by sysfs nodes
>>>> +        - statically by the user at kernel compile time
>>>> +
>>>> +        This value can also be overridden by the amdgpu.abmlevel
>>>> +        module parameter.
>>>> +
>>>> +config AMDGPU_DRM_ABM
>>>> +    bool "DRM Master control"
>>>> +    help
>>>> +        Export a property called 'abm_level' that can be
>>>> +        manipulated by the DRM master for supported hardware.
>>>> +
>>>> +config AMDGPU_SYSFS_ABM
>>>> +    bool "sysfs control"
>>>> +    help
>>>> +        Export a sysfs file 'panel_power_savings' that can be
>>>> +        manipulated by userspace for supported hardware.
>>>> +
>>>> +config AMDGPU_HARDCODE_ABM0
>>>> +    bool "No Panel power savings"
>>>> +    help
>>>> +        Disable panel power savings.
>>>> +        It can only overridden by the kernel command line.
>>>> +
>>>> +config AMDGPU_HARDCODE_ABM1
>>>> +    bool "25% Panel power savings"
>>>> +    help
>>>> +        Set the ABM panel power savings algorithm to 25%.
>>>> +        It can only overridden by the kernel command line.
>>>> +
>>>> +config AMDGPU_HARDCODE_ABM2
>>>> +    bool "50% Panel power savings"
>>>> +    help
>>>> +        Set the ABM panel power savings algorithm to 50%.
>>>> +        It can only overridden by the kernel command line.
>>>> +
>>>> +config AMDGPU_HARDCODE_ABM3
>>>> +    bool "75% Panel power savings"
>>>> +    help
>>>> +        Set the ABM panel power savings algorithm to 75%.
>>>> +        It can only overridden by the kernel command line.
>>>> +
>>>> +config AMDGPU_HARDCODE_ABM4
>>>> +    bool "100% Panel power savings"
>>>> +    help
>>>> +        Set the ABM panel power savings algorithm to 100%.
>>>> +        It can only overridden by the kernel command line.
>>>> +endchoice
>>>> +
>>>> +config AMDGPU_ABM_POLICY
>>>> +    int
>>>> +    default -2 if AMDGPU_DRM_ABM
>>>> +    default -1 if AMDGPU_SYSFS_ABM
>>>> +    default 0 if AMDGPU_HARDCODE_ABM0
>>>> +    default 1 if AMDGPU_HARDCODE_ABM1
>>>> +    default 2 if AMDGPU_HARDCODE_ABM2
>>>> +    default 3 if AMDGPU_HARDCODE_ABM3
>>>> +    default 4 if AMDGPU_HARDCODE_ABM4
>>>> +    default -1
>>>> +
>>>> +
>>>>   source "drivers/gpu/drm/amd/acp/Kconfig"
>>>>   source "drivers/gpu/drm/amd/display/Kconfig"
>>>>   source "drivers/gpu/drm/amd/amdkfd/Kconfig"
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> index af7fae7907d7..00d6c8b58716 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>>> @@ -844,17 +844,24 @@ module_param_named(visualconfirm, amdgpu_dc_visual_confirm, uint, 0444);
>>>>    * DOC: abmlevel (uint)
>>>>    * Override the default ABM (Adaptive Backlight Management) level used for DC
>>>>    * enabled hardware. Requires DMCU to be supported and loaded.
>>>> - * Valid levels are 0-4. A value of 0 indicates that ABM should be disabled by
>>>> - * default. Values 1-4 control the maximum allowable brightness reduction via
>>>> - * the ABM algorithm, with 1 being the least reduction and 4 being the most
>>>> - * reduction.
>>>> + * Valid levels are -2 through 4.
>>>>    *
>>>> - * Defaults to -1, or disabled. Userspace can only override this level after
>>>> - * boot if it's set to auto.
>>>> + *  -2: indicates that ABM should be controlled by DRM property 'abm_level.
>>>> + *  -1: indicates that ABM should be controlled by the sysfs file
>>>> + *      'panel_power_savings'.
>>>> + *   0: indicates that ABM should be disabled.
>>>> + * 1-4: control the maximum allowable brightness reduction via
>>>> + *      the ABM algorithm, with 1 being the least reduction and 4 being the most
>>>> + *      reduction.
>>>> + *
>>>> + * Both the DRM property 'abm_level' and the sysfs file 'panel_power_savings'
>>>> + * will only be available on supported hardware configurations.
>>>> + *
>>>> + * The default value is configured by kernel configuration option AMDGPU_ABM_POLICY
>>>>    */
>>>> -int amdgpu_dm_abm_level = -1;
>>>> +int amdgpu_dm_abm_level = CONFIG_AMDGPU_ABM_POLICY;
>>>>   MODULE_PARM_DESC(abmlevel,
>>>> -         "ABM level (0 = off, 1-4 = backlight reduction level, -1 auto (default))");
>>>> +         "ABM level (0 = off, 1-4 = backlight reduction level, -1 = sysfs control, -2 = drm control");
>>>>   module_param_named(abmlevel, amdgpu_dm_abm_level, int, 0444);
>>>>   int amdgpu_backlight = -1;
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index b9ac3d2f8029..147fe744f82e 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -6515,7 +6515,7 @@ static void amdgpu_dm_connector_unregister(struct drm_connector *connector)
>>>>       struct amdgpu_dm_connector *amdgpu_dm_connector = to_amdgpu_dm_connector(connector);
>>>>       if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
>>>> -        amdgpu_dm_abm_level < 0)
>>>> +        amdgpu_dm_abm_level == -1)
>>>>           sysfs_remove_group(&connector->kdev->kobj, &amdgpu_group);
>>>> drm_dp_aux_unregister(&amdgpu_dm_connector->dm_dp_aux.aux);
>>>> @@ -6623,7 +6623,7 @@ amdgpu_dm_connector_late_register(struct drm_connector *connector)
>>>>       int r;
>>>>       if (connector->connector_type == DRM_MODE_CONNECTOR_eDP &&
>>>> -        amdgpu_dm_abm_level < 0) {
>>>> +        amdgpu_dm_abm_level == -1) {
>>>>           r = sysfs_create_group(&connector->kdev->kobj,
>>>>                          &amdgpu_group);
>>>>           if (r)
>>>> @@ -7654,7 +7654,7 @@ void amdgpu_dm_connector_init_helper(struct amdgpu_display_manager *dm,
>>>>       if (connector_type == DRM_MODE_CONNECTOR_eDP &&
>>>>           (dc_is_dmcu_initialized(adev->dm.dc) ||
>>>> -         adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level < 0) {
>>>> +         adev->dm.dc->ctx->dmub_srv) && amdgpu_dm_abm_level == -2) {
>>>> drm_object_attach_property(&aconnector->base.base,
>>>>                   adev->mode_info.abm_level_property, 0);
>>>>       }
>>>
>>
> 




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux