On 9/13/2024 15:36, Alex Deucher wrote:
On Fri, Sep 13, 2024 at 2:51 PM Mario Limonciello
<mario.limonciello@xxxxxxx> 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@xxxxxxx>
---
.../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)
