[PATCH] drm/amd/display: Validate backlight caps are sane

[Mario Limonciello]

On 9/13/2024 15:36, Alex Deucher wrote:
> On Fri, Sep 13, 2024 at 2:51 PM Mario Limonciello
> <mario.limonciello at amd.com> wrote:
>>
>> On 9/13/2024 13:47, Harry Wentland wrote:
>>>
>>>
>>> On 2024-09-13 14:00, Mario Limonciello wrote:
>>>> Currently amdgpu takes backlight caps provided by the ACPI tables
>>>> on systems as is.  If the firmware sets maximums that are too low
>>>> this means that users don't get a good experience.
>>>>
>>>> To avoid having to maintain a quirk list of such systems, do a sanity
>>>> check on the values.  Check that the spread is at least half of the
>>>> values that amdgpu would use if no ACPI table was found and if not
>>>> use the amdgpu defaults.
>>>>
>>>> Closes: https://gitlab.freedesktop.org/drm/amd/-/issues/3020
>>>> Signed-off-by: Mario Limonciello <mario.limonciello at amd.com>
>>>> ---
>>>>    .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 16 ++++++++++++++++
>>>>    1 file changed, 16 insertions(+)
>>>>
>>>> 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 5942fc4e1c86..ad66f09cd0bb 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -4428,6 +4428,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>>>>
>>>>    #define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
>>>>    #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
>>>> +#define AMDGPU_DM_MIN_SPREAD ((AMDGPU_DM_DEFAULT_MAX_BACKLIGHT - AMDGPU_DM_DEFAULT_MIN_BACKLIGHT) / 2)
>>>>    #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
>>>>
>>>>    static void amdgpu_dm_update_backlight_caps(struct amdgpu_display_manager *dm,
>>>> @@ -4442,6 +4443,21 @@ static void amdgpu_dm_update_backlight_caps(struct amdgpu_display_manager *dm,
>>>>               return;
>>>>
>>>>       amdgpu_acpi_get_backlight_caps(&caps);
>>>> +
>>>> +    /* validate the firmware value is sane */
>>>> +    if (caps.caps_valid) {
>>>> +            int spread = caps.max_input_signal - caps.min_input_signal;
>>>> +
>>>> +            if (caps.max_input_signal > AMDGPU_DM_DEFAULT_MAX_BACKLIGHT ||
>>>> +                caps.min_input_signal < AMDGPU_DM_DEFAULT_MIN_BACKLIGHT ||
>>>
>>> Would we still want to allow signals below AMDGPU_DM_DEFAULT_MIN_BACKLIGHT
>>> (but above 0)? The min backlight value of 12 is a bit conservative and
>>> I wouldn't be surprised if systems set it lower via ACPI.
>>>
>>> The rest looks like great checks that we should absolutely have.
>>
>> I'm waffling about that one because Thomas' testing showed that there
>> was some problems with panel power savings when he quirked the Framework
>> panels below their ACPI default (12) in his v6 series of the Framework
>> quirks.
> 
> What about systems without the need for a quirk?  E.g., the vendor put
> a value less than AMDGPU_DM_DEFAULT_MIN_BACKLIGHT in the ACPI tables
> because they validated it and it works.  Won't this break that?
> 

 From what I can tell from the observations that Thomas had (mentioned 
in this commit message) setting it below 12 causes panel power saving to 
not work properly; the issue should be specifically in how DC/DMCUB 
handles that case.

I think once that's fixed we should open it up further.

> Alex
> 
>>
>> So my thought process was we should put in an explicit check for now and
>> then when we have panel power savings working without a modeset landed
>> then we can also add code to the backlight set callback to turn off
>> panel power savings when set below AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to
>> prevent the issue he found.
>>
>>>
>>> Harry
>>>
>>>> +                spread > AMDGPU_DM_DEFAULT_MAX_BACKLIGHT ||
>>>> +                spread < AMDGPU_DM_MIN_SPREAD) {
>>>> +                    DRM_DEBUG_KMS("DM: Invalid backlight caps: min=%d, max=%d\n",
>>>> +                                  caps.min_input_signal, caps.max_input_signal);
>>>> +                    caps.caps_valid = false;
>>>> +            }
>>>> +    }
>>>> +
>>>>       if (caps.caps_valid) {
>>>>               dm->backlight_caps[bl_idx].caps_valid = true;
>>>>               if (caps.aux_support)
>>>
>>