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
