[PATCH] drm/i915: minor corner case fix to respect user's backlight setting

[PATCH] drm/i915: minor corner case fix to respect user's backlight setting

[Harry Pan]

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

Re: [PATCH] drm/i915: minor corner case fix to respect user's backlight setting

[Jani Nikula]

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

✓ Fi.CI.BAT: success for drm/i915: minor corner case fix to respect user's backlight setting

[Patchwork]

== Series Details ==

Series: drm/i915: minor corner case fix to respect user's backlight setting
URL   : https://patchwork.freedesktop.org/series/18685/
State : success

== Summary ==

Series 18685v1 drm/i915: minor corner case fix to respect user's backlight setting
https://patchwork.freedesktop.org/api/1.0/series/18685/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b-frame-sequence:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:78   pass:65   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:223  dwarn:0   dfail:0   fail:2   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:221  dwarn:4   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

dc47d8d6ed4ea669aeccad104fde843fe039b9a5 drm-tip: 2017y-01m-30d-09h-12m-47s UTC integration manifest
269b261f drm/i915: minor corner case fix to respect user's backlight setting

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3632/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx