From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932786AbdA0OOX convert rfc822-to-8bit (ORCPT ); Fri, 27 Jan 2017 09:14:23 -0500 Received: from mga06.intel.com ([134.134.136.31]:61449 "EHLO mga06.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932544AbdA0OOT (ORCPT ); Fri, 27 Jan 2017 09:14:19 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.33,295,1477983600"; d="scan'208";a="814037165" From: Jani Nikula To: Harry Pan , LKML Cc: gs0622@gmail.com, Harry Pan , daniel.vetter@intel.com, airlied@linux.ie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH] drm/i915: minor corner case fix to respect user's backlight setting In-Reply-To: <1485521860-682-1-git-send-email-harry.pan@intel.com> Organization: Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo References: <1485521860-682-1-git-send-email-harry.pan@intel.com> Date: Fri, 27 Jan 2017 16:10:20 +0200 Message-ID: <871svo4mr7.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 27 Jan 2017, Harry Pan 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 --- >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