On Wed, Jul 24, 2024 at 4:58 AM Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx> wrote: > > On Thu, 18 Jul 2024, Hans de Goede <hdegoede@xxxxxxxxxx> 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. It's the same on AMD. There was undesirable behavior on some panels if the level dropped below a certain threshold. Alex > > 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
