* [PATCH] drm/i915: minor corner case fix to respect user's backlight setting
@ 2017-01-27 12:57 Harry Pan
2017-01-27 14:10 ` Jani Nikula
0 siblings, 1 reply; 2+ messages in thread
From: Harry Pan @ 2017-01-27 12:57 UTC (permalink / raw)
To: LKML; +Cc: airlied, intel-gfx, Harry Pan, dri-devel, daniel.vetter, gs0622
When enabling panel backlight, if the current backlight level
setting matches the panel's minimal, it would apply default policy to
override the current level by the panel's maximum until next request
to update brightness, this leads unexpected user confusion with
temporary full power backlight.
This odd could be reproduced as commands like these:
$ xbacklight -set 0
$ sudo sh -c 'echo mem > /sys/power/state'
(resume)
To fix this, slightly tinker the backlight level comparison from
'less-and-equal-to' to 'less-than'.
Before: (dmesg | grep backlight # with drm.debug=0xe)
[ 82.249265] [drm:intel_backlight_device_update_status [i915]] updating intel_backlight, brightness=0/5273
[ 82.249282] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
[ 82.249306] [drm:intel_edp_backlight_power [i915]] panel power control backlight disable
[ 92.066041] [drm:intel_edp_backlight_off [i915]]
[ 92.270489] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 0
[ 94.080434] [drm:intel_edp_backlight_on.part.25 [i915]]
[ 94.080476] [drm:intel_panel_enable_backlight [i915]] pipe A
[ 94.080539] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 5273
After:
[ 72.874465] [drm:intel_backlight_device_update_status [i915]] updating intel_backlight, brightness=0/5273
[ 72.874499] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
[ 72.874540] [drm:intel_edp_backlight_power [i915]] panel power control backlight disable
[ 86.807928] [drm:intel_edp_backlight_off [i915]]
[ 87.013227] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 0
[ 89.001829] [drm:intel_edp_backlight_on.part.25 [i915]]
[ 89.001859] [drm:intel_panel_enable_backlight [i915]] pipe A
[ 89.001926] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
Fixes: 13f3fbe827d0 ("fix inconsistent brightness after resume")
Signed-off-by: Harry Pan <harry.pan@intel.com>
---
drivers/gpu/drm/i915/intel_panel.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 08ab6d7..e882139 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1104,7 +1104,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
WARN_ON(panel->backlight.max == 0);
- if (panel->backlight.level <= panel->backlight.min) {
+ if (panel->backlight.level < panel->backlight.min) {
panel->backlight.level = panel->backlight.max;
if (panel->backlight.device)
panel->backlight.device->props.brightness =
--
2.6.6
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 2+ messages in thread* Re: [PATCH] drm/i915: minor corner case fix to respect user's backlight setting
2017-01-27 12:57 [PATCH] drm/i915: minor corner case fix to respect user's backlight setting Harry Pan
@ 2017-01-27 14:10 ` Jani Nikula
0 siblings, 0 replies; 2+ messages in thread
From: Jani Nikula @ 2017-01-27 14:10 UTC (permalink / raw)
To: LKML; +Cc: intel-gfx, dri-devel, Harry Pan, daniel.vetter, gs0622
On Fri, 27 Jan 2017, Harry Pan <harry.pan@intel.com> wrote:
> When enabling panel backlight, if the current backlight level
> setting matches the panel's minimal, it would apply default policy to
> override the current level by the panel's maximum until next request
> to update brightness, this leads unexpected user confusion with
> temporary full power backlight.
>
> This odd could be reproduced as commands like these:
> $ xbacklight -set 0
> $ sudo sh -c 'echo mem > /sys/power/state'
> (resume)
>
> To fix this, slightly tinker the backlight level comparison from
> 'less-and-equal-to' to 'less-than'.
>
> Before: (dmesg | grep backlight # with drm.debug=0xe)
> [ 82.249265] [drm:intel_backlight_device_update_status [i915]] updating intel_backlight, brightness=0/5273
> [ 82.249282] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
> [ 82.249306] [drm:intel_edp_backlight_power [i915]] panel power control backlight disable
> [ 92.066041] [drm:intel_edp_backlight_off [i915]]
> [ 92.270489] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 0
> [ 94.080434] [drm:intel_edp_backlight_on.part.25 [i915]]
> [ 94.080476] [drm:intel_panel_enable_backlight [i915]] pipe A
> [ 94.080539] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 5273
>
> After:
> [ 72.874465] [drm:intel_backlight_device_update_status [i915]] updating intel_backlight, brightness=0/5273
> [ 72.874499] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
> [ 72.874540] [drm:intel_edp_backlight_power [i915]] panel power control backlight disable
> [ 86.807928] [drm:intel_edp_backlight_off [i915]]
> [ 87.013227] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 0
> [ 89.001829] [drm:intel_edp_backlight_on.part.25 [i915]]
> [ 89.001859] [drm:intel_panel_enable_backlight [i915]] pipe A
> [ 89.001926] [drm:intel_panel_actually_set_backlight [i915]] set backlight PWM = 207
>
> Fixes: 13f3fbe827d0 ("fix inconsistent brightness after resume")
That reference is not really true. We've had this policy of setting the
backlight to max at enable if it was previously zero for eons. Yes, it's
policy, not mechanism, but it's basically ABI.
For some reason the expectation is that the sequence:
1. set backlight to 0
2. dpms off
3. dpms on
does not lead to a black screen regardless of the user request to have 0
backlight. Your change breaks this.
> Signed-off-by: Harry Pan <harry.pan@intel.com> ---
>drivers/gpu/drm/i915/intel_panel.c | 2 +- 1 file changed, 1
>insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
> index 08ab6d7..e882139 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1104,7 +1104,7 @@ void intel_panel_enable_backlight(struct intel_connector *connector)
>
> WARN_ON(panel->backlight.max == 0);
>
> - if (panel->backlight.level <= panel->backlight.min) {
> + if (panel->backlight.level < panel->backlight.min) {
> panel->backlight.level = panel->backlight.max;
If we changed this to follow your logic, the sensible thing to do would
be to set the backlight to min, not max, in this case.
But the point is moot. I don't want to deal with the regressions that I
predict the change will inevitably cause.
BR,
Jani.
> if (panel->backlight.device)
> panel->backlight.device->props.brightness =
--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2017-01-27 14:10 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-27 12:57 [PATCH] drm/i915: minor corner case fix to respect user's backlight setting Harry Pan
2017-01-27 14:10 ` Jani Nikula
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox