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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <871svo4mr7.fsf@intel.com>
Date:   Fri, 27 Jan 2017 16:10:20 +0200
From:   Jani Nikula <jani.nikula@...ux.intel.com>
To:     Harry Pan <harry.pan@...el.com>,
        LKML <linux-kernel@...r.kernel.org>
Cc:     gs0622@...il.com, Harry Pan <harry.pan@...el.com>,
        daniel.vetter@...el.com, airlied@...ux.ie,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org
Subject: Re: [PATCH] drm/i915: minor corner case fix to respect user's backlight setting

On Fri, 27 Jan 2017, Harry Pan <harry.pan@...el.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@...el.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