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? 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) > > >
