| Message ID | 20240623-amdgpu-min-backlight-quirk-v2-0-cecf7f49da9b@weissschuh.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm: backlight quirk infrastructure and lower minimum for Framework AMD 13 | expand |
Hi Thomas, 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. 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 b) The new value may be too low for certain users / use-cases 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. 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. 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. Regards, Hans > > Notes: > > * Should the quirk infrastructure be part of drm_edid.c? > * The current allocation of struct drm_edid in amdgpu is bad. > But it is done the same way in other parts of amdgpu. > I do have patches migrating amdgpu to proper usage of struct drm_edid [0] > > Mario: > > I intentionally left out the consideration of the firmware version. > The quirk will stay correct even if the firmware starts reporting > correct values. > If there are strong opinions it would be easy to add, though. > > Based on amdgpu/drm-next. > > [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/ > > --- > Changes in v2: > - Introduce proper drm backlight quirk infrastructure > - Quirk by EDID and DMI instead of only DMI > - Limit quirk to only single Framework 13 matte panel > - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net > > --- > Thomas Weißschuh (3): > drm: Add panel backlight quirks > drm: panel-backlight-quirks: Add Framework 13 matte panel > drm/amd/display: Add support backlight quirks > > Documentation/gpu/drm-kms-helpers.rst | 3 + > drivers/gpu/drm/Kconfig | 4 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++ > drivers/gpu/drm/drm_panel_backlight_quirks.c | 76 +++++++++++++++++++++++ > include/drm/drm_utils.h | 11 ++++ > 7 files changed, 124 insertions(+) > --- > base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a > > Best regards,
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. > 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. > 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. > 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. 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. > 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. [0] https://community.frame.work/t/25711/26 [1] https://community.frame.work/t/47036/7
On 6/23/2024 3:51, 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. > > Notes: > > * Should the quirk infrastructure be part of drm_edid.c? > * The current allocation of struct drm_edid in amdgpu is bad. > But it is done the same way in other parts of amdgpu. > I do have patches migrating amdgpu to proper usage of struct drm_edid [0] > > Mario: > > I intentionally left out the consideration of the firmware version. > The quirk will stay correct even if the firmware starts reporting > correct values. > If there are strong opinions it would be easy to add, though. > > Based on amdgpu/drm-next. > > [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/ > Thomas, Are you doing any testing of this lower backlight value specifically with panel power savings enabled? If not can you please check that? Specifically write the maximum value of "4" to the sysfs file: /sys/class/drm/card0-eDP-1/amdgpu/panel_power_savings Thanks, > --- > Changes in v2: > - Introduce proper drm backlight quirk infrastructure > - Quirk by EDID and DMI instead of only DMI > - Limit quirk to only single Framework 13 matte panel > - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net > > --- > Thomas Weißschuh (3): > drm: Add panel backlight quirks > drm: panel-backlight-quirks: Add Framework 13 matte panel > drm/amd/display: Add support backlight quirks > > Documentation/gpu/drm-kms-helpers.rst | 3 + > drivers/gpu/drm/Kconfig | 4 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++ > drivers/gpu/drm/drm_panel_backlight_quirks.c | 76 +++++++++++++++++++++++ > include/drm/drm_utils.h | 11 ++++ > 7 files changed, 124 insertions(+) > --- > base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a > > Best regards,
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" >> 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
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
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
On Thu, 18 Jul 2024, Hans de Goede <hdegoede@redhat.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 >
On Wed, Jul 24, 2024 at 4:58 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Thu, 18 Jul 2024, Hans de Goede <hdegoede@redhat.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. 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
Hi Thomas, 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. > > Notes: > > * Should the quirk infrastructure be part of drm_edid.c? > * The current allocation of struct drm_edid in amdgpu is bad. > But it is done the same way in other parts of amdgpu. > I do have patches migrating amdgpu to proper usage of struct drm_edid [0] So now that we have agreed to move forward with this approach one generic design issue / question which popped into my head is: What happens if i915 also gets support for the minimum brightness quirk? Specifically the panel used on the framework 13 will match the quirk independent of which GPU driver is used. But does say a minimum value of "5" have the same meaning / results in the same minimum duty-cycle when used by the i915 code vs the amdgpu code ? I know that the actual quirk sets the min pwm to 0, which presumably results in a 0% duty cycle on both i915 and amdgpu, but I'm worried what happens if we see the same panel used in designs which can have it attached to different vendor GPUs, how should the GPU driver interpret the pwm_min_brightness value so that it is interpretted consistently between drivers ? I guess this is covered by the docbook saying that min-brightness is on a 0-255 brightness scale (with 255 being 100%) though ? Just making sure that everyone has agreement on that being the scale and that drivers should not directly take this value but scale it as necessary. This should also (scale it as necessary) be explicitly mentioned in the docs IMHO. Regards, Hans > Mario: > > I intentionally left out the consideration of the firmware version. > The quirk will stay correct even if the firmware starts reporting > correct values. > If there are strong opinions it would be easy to add, though. > > Based on amdgpu/drm-next. > > [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/ > > --- > Changes in v2: > - Introduce proper drm backlight quirk infrastructure > - Quirk by EDID and DMI instead of only DMI > - Limit quirk to only single Framework 13 matte panel > - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net > > --- > Thomas Weißschuh (3): > drm: Add panel backlight quirks > drm: panel-backlight-quirks: Add Framework 13 matte panel > drm/amd/display: Add support backlight quirks > > Documentation/gpu/drm-kms-helpers.rst | 3 + > drivers/gpu/drm/Kconfig | 4 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + > drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++ > drivers/gpu/drm/drm_panel_backlight_quirks.c | 76 +++++++++++++++++++++++ > include/drm/drm_utils.h | 11 ++++ > 7 files changed, 124 insertions(+) > --- > base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b > change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a > > Best regards,
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. Notes: * Should the quirk infrastructure be part of drm_edid.c? * The current allocation of struct drm_edid in amdgpu is bad. But it is done the same way in other parts of amdgpu. I do have patches migrating amdgpu to proper usage of struct drm_edid [0] Mario: I intentionally left out the consideration of the firmware version. The quirk will stay correct even if the firmware starts reporting correct values. If there are strong opinions it would be easy to add, though. Based on amdgpu/drm-next. [0] https://lore.kernel.org/lkml/20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net/ --- Changes in v2: - Introduce proper drm backlight quirk infrastructure - Quirk by EDID and DMI instead of only DMI - Limit quirk to only single Framework 13 matte panel - Link to v1: https://lore.kernel.org/r/20240610-amdgpu-min-backlight-quirk-v1-1-8459895a5b2a@weissschuh.net --- Thomas Weißschuh (3): drm: Add panel backlight quirks drm: panel-backlight-quirks: Add Framework 13 matte panel drm/amd/display: Add support backlight quirks Documentation/gpu/drm-kms-helpers.rst | 3 + drivers/gpu/drm/Kconfig | 4 ++ drivers/gpu/drm/Makefile | 1 + drivers/gpu/drm/amd/amdgpu/Kconfig | 1 + drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 28 +++++++++ drivers/gpu/drm/drm_panel_backlight_quirks.c | 76 +++++++++++++++++++++++ include/drm/drm_utils.h | 11 ++++ 7 files changed, 124 insertions(+) --- base-commit: 1ecef5589320fd56af599b624d59c355d162ac7b change-id: 20240610-amdgpu-min-backlight-quirk-8402fd8e736a Best regards,