Message-ID: <CADnq5_PC4u_nTKsB0dwr2UyXNu9+2EJd+KEYjBdpbBototkwOQ@mail.gmail.com>
Date: Wed, 24 Jul 2024 11:53:41 -0400
From: Alex Deucher <alexdeucher@...il.com>
To: Jani Nikula <jani.nikula@...ux.intel.com>
Cc: Hans de Goede <hdegoede@...hat.com>, Thomas Weißschuh <linux@...ssschuh.net>, 
	Alex Deucher <alexander.deucher@....com>, Christian König <christian.koenig@....com>, 
	David Airlie <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>, 
	Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, Maxime Ripard <mripard@...nel.org>, 
	Thomas Zimmermann <tzimmermann@...e.de>, Harry Wentland <harry.wentland@....com>, 
	Leo Li <sunpeng.li@....com>, Rodrigo Siqueira <Rodrigo.Siqueira@....com>, 
	Mario Limonciello <mario.limonciello@....com>, Matt Hartley <matt.hartley@...il.com>, 
	Kieran Levin <ktl@...mework.net>, amd-gfx@...ts.freedesktop.org, 
	dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	Dustin Howett <dustin@...ett.net>
Subject: Re: [PATCH v2 0/3] drm: backlight quirk infrastructure and lower
 minimum for Framework AMD 13

On Wed, Jul 24, 2024 at 4:58 AM Jani Nikula <jani.nikula@...ux.intel.com> wrote:
>
> On Thu, 18 Jul 2024, Hans de Goede <hdegoede@...hat.com> wrote:
> > Hi Thomas,
> >
> > On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
> >> Hi Hans!
> >>
> >> thanks for your feedback!
> >>
> >> On 2024-06-24 11:11:40+0000, Hans de Goede wrote:
> >>> On 6/23/24 10:51 AM, Thomas Weißschuh wrote:
> >>>> The value of "min_input_signal" returned from ATIF on a Framework AMD 13
> >>>> is "12". This leads to a fairly bright minimum display backlight.
> >>>>
> >>>> Add a generic quirk infrastructure for backlight configuration to
> >>>> override the settings provided by the firmware.
> >>>> Also add amdgpu as a user of that infrastructure and a quirk for the
> >>>> Framework 13 matte panel.
> >>>> Most likely this will also work for the glossy panel, but I can't test
> >>>> that.
> >>>>
> >>>> One solution would be a fixed firmware version, but given that the
> >>>> problem exists since the release of the hardware, it has been known for
> >>>> a month that the hardware can go lower and there was no acknowledgment
> >>>> from Framework in any way, I'd like to explore this alternative
> >>>> way forward.
> >>>
> >>> There are many panels where the brightness can go lower then the advertised
> >>> minimum brightness by the firmware (e.g. VBT for i915). For most users
> >>> the minimum brightness is fine, especially since going lower often may lead
> >>> to an unreadable screen when indoors (not in the full sun) during daylight
> >>> hours. And some users get confused by the unreadable screen and find it
> >>> hard to recover things from this state.
> >>
> >> There are a fair amount of complaints on the Framework forums about this.
> >> And that specific panel is actually readable even at 0% PWM.
> >
> > If a lot of Framework users are complaining about this, then maybe Framework
> > should fix their VBT in a BIOS update ?  That seems like a better solution
> > then quirking this in the kernel.
> >
> >>
> >>> So IMHO we should not be overriding the minimum brightness from the firmware
> >>> using quirks because:
> >>>
> >>> a) This is going to be an endless game of whack-a-mole
> >>
> >> Indeed, but IMO it is better to maintain the list in the kernel than
> >> forcing all users to resort to random forum advise and fiddle with
> >> lowlevel system configuration.
> >
> > One of the problem is that what is an acceptable minimum brightness
> > value is subjective. One person's "still too bright" is another
> > person's "barely readable"
>
> Side note, IIRC the minimum brightness in VBT was not originally about
> subjective minimums, but rather to avoid electrical issues that 0% PWM
> caused in some board designs.

It's the same on AMD.  There was undesirable behavior on some panels
if the level dropped below a certain threshold.

Alex

>
> BR,
> Jani.
>
>
> >
> >>> b) The new value may be too low for certain users / use-cases
> >>
> >> The various userspace wrappers already are applying a safety
> >> threshold to not go to "0".
> >> At least gnome-settings-daemon and brightnessctl do not go below 1% of
> >> brightness_max. They already have to deal with panels that can go
> >> completely dark.
> >
> > Right, something which was added because the minimum brightness value
> > on VBTs often is broken. Either it is missing or (subjectively) it is
> > too high.
> >
> >
> >>> With that said I realize that there are also many users who want to have
> >>> a lower minimum brightness value for use in the evening, since they find
> >>> the available minimum value still too bright. I know some people want this
> >>> for e.g. various ThinkPad models too.
> >>
> >> From my experience with ThinkPads, the default brightness range there
> >> was fine for me. But on the Framework 13 AMD it is not.
> >>
> >>> So rather then quirking this, with the above mentioned disadvantages I believe
> >>> that it would be better to extend the existing video=eDP-1:.... kernel
> >>> commandline parsing to allow overriding the minimum brightness in a driver
> >>> agnostic way.
> >>
> >> I'm not a fan. It seems much too complicated for most users.
> >
> > Wanting lower minimum brightness really is mostly a power-user thing
> > and what is the right value is somewhat subjective and this is an often
> > heard complained. I really believe that the kernel should NOT get in
> > the business of adding quirks for this. OTOH given that this is an often
> > heard complaint having some generic mechanism to override the VBT value
> > would be good to have.
> >
> > As for this being too complicated, I fully agree that ideally things
> > should just work 100% OOTB, which is why I believe that a firmware fix
> > from Framework would be good. But when things do not work 100% adding
> > a kernel cmdline option is something which is regularly asked from users /
> > found in support questions on fora so I don't think this is overly
> > complicated. I agree it is not ideal but IMHO it is workable.
> >
> > E.g. on Fedora it would simply be a question of users having to run:
> >
> > sudo grubby --update-kernel=ALL --args="video=eDP-1:min-brightness=1"
> >
> > will add the passed in argument to all currently installed (and
> > future) kernels.
> >
> >> Some more background to the Framework 13 AMD case:
> >> The same panel on the Intel variant already goes darker.
> >> The last responses we got from Framework didn't indicate that the high
> >> minimum brightness was intentional [0], [1].
> >> Coincidentally the "12" returned from ATIF matches
> >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT, so maybe the firmware is just not set
> >> up completely.
> >
> > Right, so I think this should be investigated closer and then get
> > framework to issue a BIOS fix, not add a quirk mechanism to the kernel.
> >
> > IIRC the amdgpu driver will use AMDGPU_DM_DEFAULT_MIN_BACKLIGHT when
> > that setting is 0 in the VBT.
> >
> >>
> >>> The minimum brightness override set this way will still need hooking up
> >>> in each driver separately but by using the video=eDP-1:... mechanism
> >>> we can document how to do this in driver independent manner. since
> >>> I know there have been multiple requests for something like this in
> >>> the past I believe that having a single uniform way for users to do this
> >>> will be good.
> >>>
> >>> Alternatively we could have each driver have a driver specific module-
> >>> parameter for this. Either way I think we need some way for users to
> >>> override this as a config/setting tweak rather then use quirks for this.
> >>
> >> This also seems much too complicated for normal users.
> >
> > I agree that having a uniform way is better then having per driver
> > module options.
> >
> > Regards,
> >
> > Hans
> >
>
> --
> Jani Nikula, Intel

Powered by blists - more mailing lists