Message-ID: <a050aad4-d195-42e6-8a84-02170a4f9835@t-8ch.de>
Date: Sat, 20 Jul 2024 09:31:37 +0200
From: Thomas Weißschuh <linux@...ssschuh.net>
To: Hans de Goede <hdegoede@...hat.com>
Cc: 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

Hi Hans,

On 2024-07-18 10:25:18+0000, Hans de Goede wrote:
> On 6/24/24 6:15 PM, Thomas Weißschuh wrote:
> > 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.

Framework has now stated that they will update the VBT for their 13' device. [0]
It won't happen for the 16' one as its out of spec there, although it
has been reported to work.

<snip>

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

Thanks for taking the time for your explanations.
I came around to agree with your proposal for a cmdline override.

What to you think about:

void drm_connector_get_cmdline_backlight_overrides(struct drm_connector *connector,
						   struct drm_backlight_override *overrides);

struct drm_backlight_override would look like
struct drm_panel_backlight_quirk from this patch.

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

This is not my reading of the code.
To me it seems "0" will be accepted, which is also why the second "fix"
from [1] works.

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

Ack.

[0] https://community.frame.work/t/responded-amd-framework-minimum-brightness-too-high-now-with-measurements/47036/12
[1] https://community.frame.work/t/resolved-even-lower-screen-brightness/25711/42

Powered by blists - more mailing lists