Message-ID: <87sevzf9pw.fsf@intel.com>
Date: Wed, 24 Jul 2024 11:57:47 +0300
From: Jani Nikula <jani.nikula@...ux.intel.com>
To: Hans de Goede <hdegoede@...hat.com>, 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

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.

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