On 2024-09-13 15:48:55+0000, Mario Limonciello wrote: > 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 said that enabling panel power savings would slightly adjust the brightness. But that difference is very tiny and nearly not perceptible. Surely it is not "not a good experience" on my machine. Also it turns out I my testing was incomplete. The same effect also occurs with the default min backlight of AMDGPU_DM_DEFAULT_MIN_BACKLIGHT=12. > 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) > > > > > > > >
