Re: [PATCH] drm/amd/display: Only restore backlight after amdgpu_dm_init or dm_resume

[Matthew Schwartz <matthew.schwartz@linux.dev>]

On 9/10/25 2:32 PM, Mario Limonciello wrote:
> On 9/10/25 4:27 PM, Matthew Schwartz wrote:
>> On clients that utilize AMD_PRIVATE_COLOR properties for HDR support,
>> brightness sliders can include a hardware controlled portion and a
>> gamma-based portion. This is the case on the Steam Deck OLED when using
>> gamescope with Steam as a client.
>>
>> When a user sets a brightness level while HDR is active, the gamma-based
>> portion and/or hardware portion are adjusted to achieve the desired
>> brightness. However, when a modeset takes place while the gamma-based
>> portion is in-use, restoring the hardware brightness level overrides the
>> user's overall brightness level and results in a mismatch between what
>> the slider reports and the display's current brightness.
>>
>> To avoid overriding gamma-based brightness, only restore HW backlight
>> level after boot or resume. This ensures that the backlight level is
>> set correctly after the DC layer resets it while avoiding interference
>> with subsequent modesets.
>>
>> Fixes: 7875afafba84 ("drm/amd/display: Fix brightness level not retained over reboot")
>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/4551
>> Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 11 +++++++++--
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  7 +++++++
>>   2 files changed, 16 insertions(+), 2 deletions(-)
>>
>> 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 7808a647a306c..3651315760759 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -2037,6 +2037,8 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>>         dc_hardware_init(adev->dm.dc);
>>   +    adev->dm.restore_backlight = true;
>> +
>>       adev->dm.hpd_rx_offload_wq = hpd_rx_irq_create_workqueue(adev);
>>       if (!adev->dm.hpd_rx_offload_wq) {
>>           drm_err(adev_to_drm(adev), "failed to create hpd rx offload workqueue.\n");
>> @@ -3407,6 +3409,7 @@ static int dm_resume(struct amdgpu_ip_block *ip_block)
>>           dc_set_power_state(dm->dc, DC_ACPI_CM_POWER_STATE_D0);
>>             dc_resume(dm->dc);
>> +        adev->dm.restore_backlight = true;
>>             amdgpu_dm_irq_resume_early(adev);
>>   @@ -9922,7 +9925,8 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_state *state,
>>               acrtc->hw_mode = new_crtc_state->mode;
>>               crtc->hwmode = new_crtc_state->mode;
>>               mode_set_reset_required = true;
>> -            set_backlight_level = true;
>> +            if (dm->restore_backlight)
>> +                set_backlight_level = true;
>>           } else if (modereset_required(new_crtc_state)) {
>>               drm_dbg_atomic(dev,
>>                          "Atomic commit: RESET. crtc id %d:[%p]\n",
>> @@ -9979,13 +9983,16 @@ static void amdgpu_dm_commit_streams(struct drm_atomic_state *state,
>>        * to fix a flicker issue.
>>        * It will cause the dm->actual_brightness is not the current panel brightness
>>        * level. (the dm->brightness is the correct panel level)
>> -     * So we set the backlight level with dm->brightness value after set mode
>> +     * So we set the backlight level with dm->brightness value after initial
>> +     * set mode. Use restore_backlight flag to avoid setting backlight level
>> +     * for every subsequent mode set.
>>        */
>>       if (set_backlight_level) {
> 
> Is there really still a need for the local variable in this function 'set_backlight_level'?
> 

No, you're correct and it's a bit cleaner that way. I'll fix this in v2.

> It seems to me with this change you can just use dm->restore_backlight for state tracking at all times.
>>           for (i = 0; i < dm->num_of_edps; i++) {
>>               if (dm->backlight_dev[i])
>>                   amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
>>           }
>> +        dm->restore_backlight = false;
>>       }
>>   }
>>   diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index b937da0a4e4a0..6aae51c1beb36 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -610,6 +610,13 @@ struct amdgpu_display_manager {
>>        */
>>       u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
>>   +    /**
>> +     * @restore_backlight:
>> +     *
>> +     * Flag to indicate whether to restore backlight after modeset.
>> +     */
>> +    bool restore_backlight;
>> +
>>       /**
>>        * @aux_hpd_discon_quirk:
>>        *
>