On 2024-09-13 14:51, Mario Limonciello 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.limoncie...@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.
>
Ah, didn't know about that one. In that case this is
Reviewed-by: Harry Wentland <harry.wentl...@amd.com>
> 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.
>
Sounds reasonable.
Harry
>>
>> 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)
>>
>
