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
