Message-ID: <d1d57a98-dcba-43d0-aa90-016c4f85a32f@redhat.com>
Date: Mon, 22 Jul 2024 13:53:17 +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 7/20/24 9:31 AM, Thomas Weißschuh wrote:
> 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.

I'm not entirely convinced that we need the struct drm_backlight_override
abstraction right away. Maybe we can start with just a
drm_connector_get_cmdline_min_brightness_override() which just returns an int?

If you prefer to keep the struct drm_backlight_override that is fine too,
we can see what others think when you submit a new version for review.


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

I have not looked at that code i quite a while, so you're probably right.

Regards,

Hans

Powered by blists - more mailing lists