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

[Thomas Weißschuh]

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