Message-ID: <6db5abf9-cbdd-4ec0-b669-5df23de6c2ad@redhat.com>
Date: Thu, 18 Jul 2024 10:25:18 +0200
From: Hans de Goede <hdegoede@...hat.com>
To: Thomas Weißschuh <linux@...ssschuh.net>
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 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"

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

Powered by blists - more mailing lists