* [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
@ 2021-03-19 5:44 Evan Benn
2021-03-19 14:22 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Evan Benn @ 2021-03-19 5:44 UTC (permalink / raw)
To: LKML
Cc: Anand, Evan Benn, Alex Deucher, Aurabindo Pillai,
Bas Nieuwenhuizen, Bhawanpreet Lakha, Christian König,
Daniel Vetter, David Airlie, Eryk Brol, Harry Wentland, Leo Li,
Nicholas Kazlauskas, Rodrigo Siqueira, Simon Ser, Stylon Wang,
amd-gfx, dri-devel
AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
to ensure no display backlight will flicker at low user brightness
settings. However this value is quite bright, so for devices that do not
implement the ACPI ATIF
ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
functionality the user cannot set the brightness to a low level even if
the display would support such a low PWM.
This ATIF feature is not implemented on for example AMD grunt chromebooks.
Signed-off-by: Evan Benn <evanbenn@chromium.org>
---
I could not find a justification for the reason for the value. It has
caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439
Maybe this can be either user controlled or userspace configured, but
preventing users from turning their backlight dim seems wrong.
Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 573cf17262da..0129bd69b94e 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
return 0;
}
-#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
+#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
#define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
#define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
--
2.31.0.291.g576ba9dcdaf-goog
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-19 5:44 [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0 Evan Benn
@ 2021-03-19 14:22 ` Alex Deucher
2021-03-19 15:07 ` Harry Wentland
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2021-03-19 14:22 UTC (permalink / raw)
To: Evan Benn
Cc: LKML, Stylon Wang, Eryk Brol, David Airlie, Rodrigo Siqueira,
amd-gfx list, Nicholas Kazlauskas, Leo Li, Aurabindo Pillai,
Maling list - DRI developers, Alex Deucher, Bhawanpreet Lakha,
Christian König, Anand
On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
>
> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
> to ensure no display backlight will flicker at low user brightness
> settings. However this value is quite bright, so for devices that do not
> implement the ACPI ATIF
> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
> functionality the user cannot set the brightness to a low level even if
> the display would support such a low PWM.
>
> This ATIF feature is not implemented on for example AMD grunt chromebooks.
>
> Signed-off-by: Evan Benn <evanbenn@chromium.org>
>
> ---
> I could not find a justification for the reason for the value. It has
> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439
>
> Maybe this can be either user controlled or userspace configured, but
> preventing users from turning their backlight dim seems wrong.
My understanding is that some panels flicker if you set the min to a
value too low. This was a safe minimum if the platform didn't specify
it's own safe minimum. I think we'd just be trading one bug for
another (flickering vs not dim enough). Maybe a whitelist or
blacklist would be a better solution?
Alex
>
> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377
>
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 573cf17262da..0129bd69b94e 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> return 0;
> }
>
> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
>
> --
> 2.31.0.291.g576ba9dcdaf-goog
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-19 14:22 ` Alex Deucher
@ 2021-03-19 15:07 ` Harry Wentland
2021-03-19 21:31 ` Evan Benn
0 siblings, 1 reply; 7+ messages in thread
From: Harry Wentland @ 2021-03-19 15:07 UTC (permalink / raw)
To: Alex Deucher, Evan Benn
Cc: Stylon Wang, Eryk Brol, David Airlie, Rodrigo Siqueira, LKML,
amd-gfx list, Christian König, Leo Li, Aurabindo Pillai,
Maling list - DRI developers, Alex Deucher, Bhawanpreet Lakha,
Nicholas Kazlauskas, Anand
On 2021-03-19 10:22 a.m., Alex Deucher wrote:
> On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
>>
>> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
>> to ensure no display backlight will flicker at low user brightness
>> settings. However this value is quite bright, so for devices that do not
>> implement the ACPI ATIF
>> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
>> functionality the user cannot set the brightness to a low level even if
>> the display would support such a low PWM.
>>
>> This ATIF feature is not implemented on for example AMD grunt chromebooks.
>>
>> Signed-off-by: Evan Benn <evanbenn@chromium.org>
>>
>> ---
>> I could not find a justification for the reason for the value. It has
>> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439>>>
>> Maybe this can be either user controlled or userspace configured, but
>> preventing users from turning their backlight dim seems wrong.
>
> My understanding is that some panels flicker if you set the min to a
> value too low. This was a safe minimum if the platform didn't specify
> it's own safe minimum. I think we'd just be trading one bug for
> another (flickering vs not dim enough). Maybe a whitelist or
> blacklist would be a better solution?
>
Yeah, this is a NACK from me as-is for the reasons Alex described.
I agree a whitelist approach might be best.
Is this fix perhaps for OLED panels? If so we could use a different
min-value for OLED panels that don't do PWM, but use 12 for everything else.
Harry
> Alex
>
>
>>
>> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>>
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 573cf17262da..0129bd69b94e 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
>> return 0;
>> }
>>
>> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
>> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
>> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
>> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
>>
>> --
>> 2.31.0.291.g576ba9dcdaf-goog
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-19 15:07 ` Harry Wentland
@ 2021-03-19 21:31 ` Evan Benn
2021-03-19 21:35 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Evan Benn @ 2021-03-19 21:31 UTC (permalink / raw)
To: Harry Wentland
Cc: Alex Deucher, Evan Benn, Stylon Wang, Eryk Brol, David Airlie,
Rodrigo Siqueira, LKML, amd-gfx list, Christian König,
Leo Li, Aurabindo Pillai, Maling list - DRI developers,
Alex Deucher, Bhawanpreet Lakha, Nicholas Kazlauskas, Anand
On Sat, 20 Mar 2021 at 02:10, Harry Wentland <harry.wentland@amd.com> wrote:
> On 2021-03-19 10:22 a.m., Alex Deucher wrote:
> > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
> >>
> >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
> >> to ensure no display backlight will flicker at low user brightness
> >> settings. However this value is quite bright, so for devices that do not
> >> implement the ACPI ATIF
> >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
> >> functionality the user cannot set the brightness to a low level even if
> >> the display would support such a low PWM.
> >>
> >> This ATIF feature is not implemented on for example AMD grunt chromebooks.
> >>
> >> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> >>
> >> ---
> >> I could not find a justification for the reason for the value. It has
> >> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439>>>
> >> Maybe this can be either user controlled or userspace configured, but
> >> preventing users from turning their backlight dim seems wrong.
> >
> > My understanding is that some panels flicker if you set the min to a
> > value too low. This was a safe minimum if the platform didn't specify
> > it's own safe minimum. I think we'd just be trading one bug for
> > another (flickering vs not dim enough). Maybe a whitelist or
> > blacklist would be a better solution?
> >
>
> Yeah, this is a NACK from me as-is for the reasons Alex described.
Thanks Harry + Alex,
I agree this solution is not the best.
>
> I agree a whitelist approach might be best.
Do you have any idea what an allowlist could be keyed on?
Is the flickering you observed here a function of the panel or the gpu
or some other component?
Maybe we could move the minimum level into the logic for that hardware.
>
> Is this fix perhaps for OLED panels? If so we could use a different
> min-value for OLED panels that don't do PWM, but use 12 for everything else.
All the chromebooks I have worked with LCD + LED backlight have been
fine with a backlight set to 0.
We do have OLED panels too, but I'm not aware of what they do.
> Harry
>
> > Alex
> >
> >
> >>
> >> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>>
> >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> index 573cf17262da..0129bd69b94e 100644
> >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> >> return 0;
> >> }
> >>
> >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
> >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
> >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
> >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
> >>
> >> --
> >> 2.31.0.291.g576ba9dcdaf-goog
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel>>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-19 21:31 ` Evan Benn
@ 2021-03-19 21:35 ` Alex Deucher
2021-03-22 0:12 ` Evan Benn
0 siblings, 1 reply; 7+ messages in thread
From: Alex Deucher @ 2021-03-19 21:35 UTC (permalink / raw)
To: Evan Benn
Cc: Harry Wentland, Evan Benn, Stylon Wang, Eryk Brol, David Airlie,
Rodrigo Siqueira, LKML, amd-gfx list, Christian König,
Leo Li, Aurabindo Pillai, Maling list - DRI developers,
Alex Deucher, Bhawanpreet Lakha, Nicholas Kazlauskas, Anand
On Fri, Mar 19, 2021 at 5:31 PM Evan Benn <evanbenn@gmail.com> wrote:
>
> On Sat, 20 Mar 2021 at 02:10, Harry Wentland <harry.wentland@amd.com> wrote:
> > On 2021-03-19 10:22 a.m., Alex Deucher wrote:
> > > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
> > >>
> > >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
> > >> to ensure no display backlight will flicker at low user brightness
> > >> settings. However this value is quite bright, so for devices that do not
> > >> implement the ACPI ATIF
> > >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
> > >> functionality the user cannot set the brightness to a low level even if
> > >> the display would support such a low PWM.
> > >>
> > >> This ATIF feature is not implemented on for example AMD grunt chromebooks.
> > >>
> > >> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> > >>
> > >> ---
> > >> I could not find a justification for the reason for the value. It has
> > >> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439>>>
> > >> Maybe this can be either user controlled or userspace configured, but
> > >> preventing users from turning their backlight dim seems wrong.
> > >
> > > My understanding is that some panels flicker if you set the min to a
> > > value too low. This was a safe minimum if the platform didn't specify
> > > it's own safe minimum. I think we'd just be trading one bug for
> > > another (flickering vs not dim enough). Maybe a whitelist or
> > > blacklist would be a better solution?
> > >
> >
> > Yeah, this is a NACK from me as-is for the reasons Alex described.
>
> Thanks Harry + Alex,
>
> I agree this solution is not the best.
>
> >
> > I agree a whitelist approach might be best.
>
> Do you have any idea what an allowlist could be keyed on?
> Is the flickering you observed here a function of the panel or the gpu
> or some other component?
> Maybe we could move the minimum level into the logic for that hardware.
>
Maybe the panel string from the EDID? Either that or something from
dmi data? Harry would probably have a better idea.
Alex
> >
> > Is this fix perhaps for OLED panels? If so we could use a different
> > min-value for OLED panels that don't do PWM, but use 12 for everything else.
>
> All the chromebooks I have worked with LCD + LED backlight have been
> fine with a backlight set to 0.
> We do have OLED panels too, but I'm not aware of what they do.
>
> > Harry
> >
> > > Alex
> > >
> > >
> > >>
> > >> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>>
> > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > >> index 573cf17262da..0129bd69b94e 100644
> > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> > >> return 0;
> > >> }
> > >>
> > >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
> > >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
> > >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
> > >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
> > >>
> > >> --
> > >> 2.31.0.291.g576ba9dcdaf-goog
> > >>
> > >> _______________________________________________
> > >> dri-devel mailing list
> > >> dri-devel@lists.freedesktop.org
> > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel>>
> >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-19 21:35 ` Alex Deucher
@ 2021-03-22 0:12 ` Evan Benn
2021-03-22 14:54 ` Alex Deucher
0 siblings, 1 reply; 7+ messages in thread
From: Evan Benn @ 2021-03-22 0:12 UTC (permalink / raw)
To: Alex Deucher
Cc: Evan Benn, Harry Wentland, Stylon Wang, Eryk Brol, David Airlie,
Rodrigo Siqueira, LKML, amd-gfx list, Christian König,
Leo Li, Aurabindo Pillai, Maling list - DRI developers,
Alex Deucher, Bhawanpreet Lakha, Nicholas Kazlauskas, Anand
On Sat, Mar 20, 2021 at 8:36 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Fri, Mar 19, 2021 at 5:31 PM Evan Benn <evanbenn@gmail.com> wrote:
> >
> > On Sat, 20 Mar 2021 at 02:10, Harry Wentland <harry.wentland@amd.com> wrote:
> > > On 2021-03-19 10:22 a.m., Alex Deucher wrote:
> > > > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
> > > >>
> > > >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
> > > >> to ensure no display backlight will flicker at low user brightness
> > > >> settings. However this value is quite bright, so for devices that do not
> > > >> implement the ACPI ATIF
> > > >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
> > > >> functionality the user cannot set the brightness to a low level even if
> > > >> the display would support such a low PWM.
> > > >>
> > > >> This ATIF feature is not implemented on for example AMD grunt chromebooks.
> > > >>
> > > >> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> > > >>
> > > >> ---
> > > >> I could not find a justification for the reason for the value. It has
> > > >> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439>>>
> > > >> Maybe this can be either user controlled or userspace configured, but
> > > >> preventing users from turning their backlight dim seems wrong.
> > > >
> > > > My understanding is that some panels flicker if you set the min to a
> > > > value too low. This was a safe minimum if the platform didn't specify
> > > > it's own safe minimum. I think we'd just be trading one bug for
> > > > another (flickering vs not dim enough). Maybe a whitelist or
> > > > blacklist would be a better solution?
> > > >
> > >
> > > Yeah, this is a NACK from me as-is for the reasons Alex described.
> >
> > Thanks Harry + Alex,
> >
> > I agree this solution is not the best.
> >
> > >
> > > I agree a whitelist approach might be best.
> >
> > Do you have any idea what an allowlist could be keyed on?
> > Is the flickering you observed here a function of the panel or the gpu
> > or some other component?
> > Maybe we could move the minimum level into the logic for that hardware.
> >
>
> Maybe the panel string from the EDID? Either that or something from
> dmi data? Harry would probably have a better idea.
One problem with keying from panel EDID is that for example the grunt chromebook
platform has more than 100 different panels already shipped. Add to that that
repair centers or people repairing their own device will use 'compatible'
panels. I'm sure the AMD windows laptops have even more variety!
>
> Alex
>
> > >
> > > Is this fix perhaps for OLED panels? If so we could use a different
> > > min-value for OLED panels that don't do PWM, but use 12 for everything else.
> >
> > All the chromebooks I have worked with LCD + LED backlight have been
> > fine with a backlight set to 0.
> > We do have OLED panels too, but I'm not aware of what they do.
> >
> > > Harry
> > >
> > > > Alex
> > > >
> > > >
> > > >>
> > > >> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>>
> > > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > >> index 573cf17262da..0129bd69b94e 100644
> > > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> > > >> return 0;
> > > >> }
> > > >>
> > > >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
> > > >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
> > > >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
> > > >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
> > > >>
> > > >> --
> > > >> 2.31.0.291.g576ba9dcdaf-goog
> > > >>
> > > >> _______________________________________________
> > > >> dri-devel mailing list
> > > >> dri-devel@lists.freedesktop.org
> > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel>>
> > >
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0
2021-03-22 0:12 ` Evan Benn
@ 2021-03-22 14:54 ` Alex Deucher
0 siblings, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2021-03-22 14:54 UTC (permalink / raw)
To: Evan Benn
Cc: Evan Benn, Harry Wentland, Stylon Wang, Eryk Brol, David Airlie,
Rodrigo Siqueira, LKML, amd-gfx list, Christian König,
Leo Li, Aurabindo Pillai, Maling list - DRI developers,
Alex Deucher, Bhawanpreet Lakha, Nicholas Kazlauskas, Anand
On Sun, Mar 21, 2021 at 8:12 PM Evan Benn <evanbenn@chromium.org> wrote:
>
> On Sat, Mar 20, 2021 at 8:36 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Fri, Mar 19, 2021 at 5:31 PM Evan Benn <evanbenn@gmail.com> wrote:
> > >
> > > On Sat, 20 Mar 2021 at 02:10, Harry Wentland <harry.wentland@amd.com> wrote:
> > > > On 2021-03-19 10:22 a.m., Alex Deucher wrote:
> > > > > On Fri, Mar 19, 2021 at 3:23 AM Evan Benn <evanbenn@chromium.org> wrote:
> > > > >>
> > > > >> AMDGPU_DM_DEFAULT_MIN_BACKLIGHT was set to the value of 12
> > > > >> to ensure no display backlight will flicker at low user brightness
> > > > >> settings. However this value is quite bright, so for devices that do not
> > > > >> implement the ACPI ATIF
> > > > >> ATIF_FUNCTION_QUERY_BRIGHTNESS_TRANSFER_CHARACTERISTICS
> > > > >> functionality the user cannot set the brightness to a low level even if
> > > > >> the display would support such a low PWM.
> > > > >>
> > > > >> This ATIF feature is not implemented on for example AMD grunt chromebooks.
> > > > >>
> > > > >> Signed-off-by: Evan Benn <evanbenn@chromium.org>
> > > > >>
> > > > >> ---
> > > > >> I could not find a justification for the reason for the value. It has
> > > > >> caused some noticable regression for users: https://bugzilla.kernel.org/show_bug.cgi?id=203439>>>
> > > > >> Maybe this can be either user controlled or userspace configured, but
> > > > >> preventing users from turning their backlight dim seems wrong.
> > > > >
> > > > > My understanding is that some panels flicker if you set the min to a
> > > > > value too low. This was a safe minimum if the platform didn't specify
> > > > > it's own safe minimum. I think we'd just be trading one bug for
> > > > > another (flickering vs not dim enough). Maybe a whitelist or
> > > > > blacklist would be a better solution?
> > > > >
> > > >
> > > > Yeah, this is a NACK from me as-is for the reasons Alex described.
> > >
> > > Thanks Harry + Alex,
> > >
> > > I agree this solution is not the best.
> > >
> > > >
> > > > I agree a whitelist approach might be best.
> > >
> > > Do you have any idea what an allowlist could be keyed on?
> > > Is the flickering you observed here a function of the panel or the gpu
> > > or some other component?
> > > Maybe we could move the minimum level into the logic for that hardware.
> > >
> >
> > Maybe the panel string from the EDID? Either that or something from
> > dmi data? Harry would probably have a better idea.
>
> One problem with keying from panel EDID is that for example the grunt chromebook
> platform has more than 100 different panels already shipped. Add to that that
> repair centers or people repairing their own device will use 'compatible'
> panels. I'm sure the AMD windows laptops have even more variety!
>
Do all of those "compatible" panels work with the min backlight level
of 0? If so, maybe something platform specific like a DMI string
would make more sense.
Alex
> >
> > Alex
> >
> > > >
> > > > Is this fix perhaps for OLED panels? If so we could use a different
> > > > min-value for OLED panels that don't do PWM, but use 12 for everything else.
> > >
> > > All the chromebooks I have worked with LCD + LED backlight have been
> > > fine with a backlight set to 0.
> > > We do have OLED panels too, but I'm not aware of what they do.
> > >
> > > > Harry
> > > >
> > > > > Alex
> > > > >
> > > > >
> > > > >>
> > > > >> Also reviewed here: https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/2748377>>>
> > > > >> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
> > > > >> 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >>
> > > > >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > >> index 573cf17262da..0129bd69b94e 100644
> > > > >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > > >> @@ -3151,7 +3151,7 @@ static int amdgpu_dm_mode_config_init(struct amdgpu_device *adev)
> > > > >> return 0;
> > > > >> }
> > > > >>
> > > > >> -#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 12
> > > > >> +#define AMDGPU_DM_DEFAULT_MIN_BACKLIGHT 0
> > > > >> #define AMDGPU_DM_DEFAULT_MAX_BACKLIGHT 255
> > > > >> #define AUX_BL_DEFAULT_TRANSITION_TIME_MS 50
> > > > >>
> > > > >> --
> > > > >> 2.31.0.291.g576ba9dcdaf-goog
> > > > >>
> > > > >> _______________________________________________
> > > > >> dri-devel mailing list
> > > > >> dri-devel@lists.freedesktop.org
> > > > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel>> _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel>>
> > > >
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-03-22 14:56 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-19 5:44 [PATCH] drm/amd/display: Set AMDGPU_DM_DEFAULT_MIN_BACKLIGHT to 0 Evan Benn
2021-03-19 14:22 ` Alex Deucher
2021-03-19 15:07 ` Harry Wentland
2021-03-19 21:31 ` Evan Benn
2021-03-19 21:35 ` Alex Deucher
2021-03-22 0:12 ` Evan Benn
2021-03-22 14:54 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®