From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH] drm/i915: minor corner case fix to respect user's backlight setting Date: Fri, 27 Jan 2017 16:10:20 +0200 Message-ID: <871svo4mr7.fsf@intel.com> References: <1485521860-682-1-git-send-email-harry.pan@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <1485521860-682-1-git-send-email-harry.pan@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: LKML Cc: intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Harry Pan , daniel.vetter@intel.com, gs0622@gmail.com List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCAyNyBKYW4gMjAxNywgSGFycnkgUGFuIDxoYXJyeS5wYW5AaW50ZWwuY29tPiB3cm90 ZToKPiBXaGVuIGVuYWJsaW5nIHBhbmVsIGJhY2tsaWdodCwgaWYgdGhlIGN1cnJlbnQgYmFja2xp Z2h0IGxldmVsCj4gc2V0dGluZyBtYXRjaGVzIHRoZSBwYW5lbCdzIG1pbmltYWwsIGl0IHdvdWxk IGFwcGx5IGRlZmF1bHQgcG9saWN5IHRvCj4gb3ZlcnJpZGUgdGhlIGN1cnJlbnQgbGV2ZWwgYnkg dGhlIHBhbmVsJ3MgbWF4aW11bSB1bnRpbCBuZXh0IHJlcXVlc3QKPiB0byB1cGRhdGUgYnJpZ2h0 bmVzcywgdGhpcyBsZWFkcyB1bmV4cGVjdGVkIHVzZXIgY29uZnVzaW9uIHdpdGgKPiB0ZW1wb3Jh cnkgZnVsbCBwb3dlciBiYWNrbGlnaHQuCj4KPiBUaGlzIG9kZCBjb3VsZCBiZSByZXByb2R1Y2Vk IGFzIGNvbW1hbmRzIGxpa2UgdGhlc2U6Cj4gICQgeGJhY2tsaWdodCAtc2V0IDAKPiAgJCBzdWRv IHNoIC1jICdlY2hvIG1lbSA+IC9zeXMvcG93ZXIvc3RhdGUnCj4gIChyZXN1bWUpCj4KPiBUbyBm aXggdGhpcywgc2xpZ2h0bHkgdGlua2VyIHRoZSBiYWNrbGlnaHQgbGV2ZWwgY29tcGFyaXNvbiBm cm9tCj4gJ2xlc3MtYW5kLWVxdWFsLXRvJyB0byAnbGVzcy10aGFuJy4KPgo+IEJlZm9yZTogKGRt ZXNnIHwgZ3JlcCBiYWNrbGlnaHQgIyB3aXRoIGRybS5kZWJ1Zz0weGUpCj4gWyAgIDgyLjI0OTI2 NV0gW2RybTppbnRlbF9iYWNrbGlnaHRfZGV2aWNlX3VwZGF0ZV9zdGF0dXMgW2k5MTVdXSB1cGRh dGluZyBpbnRlbF9iYWNrbGlnaHQsIGJyaWdodG5lc3M9MC81MjczCj4gWyAgIDgyLjI0OTI4Ml0g W2RybTppbnRlbF9wYW5lbF9hY3R1YWxseV9zZXRfYmFja2xpZ2h0IFtpOTE1XV0gc2V0IGJhY2ts aWdodCBQV00gPSAyMDcKPiBbICAgODIuMjQ5MzA2XSBbZHJtOmludGVsX2VkcF9iYWNrbGlnaHRf cG93ZXIgW2k5MTVdXSBwYW5lbCBwb3dlciBjb250cm9sIGJhY2tsaWdodCBkaXNhYmxlCj4gWyAg IDkyLjA2NjA0MV0gW2RybTppbnRlbF9lZHBfYmFja2xpZ2h0X29mZiBbaTkxNV1dCj4gWyAgIDky LjI3MDQ4OV0gW2RybTppbnRlbF9wYW5lbF9hY3R1YWxseV9zZXRfYmFja2xpZ2h0IFtpOTE1XV0g c2V0IGJhY2tsaWdodCBQV00gPSAwCj4gWyAgIDk0LjA4MDQzNF0gW2RybTppbnRlbF9lZHBfYmFj a2xpZ2h0X29uLnBhcnQuMjUgW2k5MTVdXQo+IFsgICA5NC4wODA0NzZdIFtkcm06aW50ZWxfcGFu ZWxfZW5hYmxlX2JhY2tsaWdodCBbaTkxNV1dIHBpcGUgQQo+IFsgICA5NC4wODA1MzldIFtkcm06 aW50ZWxfcGFuZWxfYWN0dWFsbHlfc2V0X2JhY2tsaWdodCBbaTkxNV1dIHNldCBiYWNrbGlnaHQg UFdNID0gNTI3Mwo+Cj4gQWZ0ZXI6Cj4gWyDCoMKgNzIuODc0NDY1XSBbZHJtOmludGVsX2JhY2ts aWdodF9kZXZpY2VfdXBkYXRlX3N0YXR1cyBbaTkxNV1dIHVwZGF0aW5nIGludGVsX2JhY2tsaWdo dCwgYnJpZ2h0bmVzcz0wLzUyNzMKPiBbIMKgwqA3Mi44NzQ0OTldIFtkcm06aW50ZWxfcGFuZWxf YWN0dWFsbHlfc2V0X2JhY2tsaWdodCBbaTkxNV1dIHNldCBiYWNrbGlnaHQgUFdNID0gMjA3Cj4g WyDCoMKgNzIuODc0NTQwXSBbZHJtOmludGVsX2VkcF9iYWNrbGlnaHRfcG93ZXIgW2k5MTVdXSBw YW5lbCBwb3dlciBjb250cm9sIGJhY2tsaWdodCBkaXNhYmxlCj4gWyDCoMKgODYuODA3OTI4XSBb ZHJtOmludGVsX2VkcF9iYWNrbGlnaHRfb2ZmIFtpOTE1XV0KPiBbIMKgwqA4Ny4wMTMyMjddIFtk cm06aW50ZWxfcGFuZWxfYWN0dWFsbHlfc2V0X2JhY2tsaWdodCBbaTkxNV1dIHNldCBiYWNrbGln aHQgUFdNID0gMAo+IFsgwqDCoDg5LjAwMTgyOV0gW2RybTppbnRlbF9lZHBfYmFja2xpZ2h0X29u LnBhcnQuMjUgW2k5MTVdXQo+IFsgwqDCoDg5LjAwMTg1OV0gW2RybTppbnRlbF9wYW5lbF9lbmFi bGVfYmFja2xpZ2h0IFtpOTE1XV0gcGlwZSBBCj4gWyDCoMKgODkuMDAxOTI2XSBbZHJtOmludGVs X3BhbmVsX2FjdHVhbGx5X3NldF9iYWNrbGlnaHQgW2k5MTVdXSBzZXQgYmFja2xpZ2h0IFBXTSA9 IDIwNwo+Cj4gRml4ZXM6IDEzZjNmYmU4MjdkMCAoImZpeCBpbmNvbnNpc3RlbnQgYnJpZ2h0bmVz cyBhZnRlciByZXN1bWUiKQoKVGhhdCByZWZlcmVuY2UgaXMgbm90IHJlYWxseSB0cnVlLiBXZSd2 ZSBoYWQgdGhpcyBwb2xpY3kgb2Ygc2V0dGluZyB0aGUKYmFja2xpZ2h0IHRvIG1heCBhdCBlbmFi bGUgaWYgaXQgd2FzIHByZXZpb3VzbHkgemVybyBmb3IgZW9ucy4gWWVzLCBpdCdzCnBvbGljeSwg bm90IG1lY2hhbmlzbSwgYnV0IGl0J3MgYmFzaWNhbGx5IEFCSS4KCkZvciBzb21lIHJlYXNvbiB0 aGUgZXhwZWN0YXRpb24gaXMgdGhhdCB0aGUgc2VxdWVuY2U6CgoxLiBzZXQgYmFja2xpZ2h0IHRv IDAKMi4gZHBtcyBvZmYKMy4gZHBtcyBvbgoKZG9lcyBub3QgbGVhZCB0byBhIGJsYWNrIHNjcmVl biByZWdhcmRsZXNzIG9mIHRoZSB1c2VyIHJlcXVlc3QgdG8gaGF2ZSAwCmJhY2tsaWdodC4gWW91 ciBjaGFuZ2UgYnJlYWtzIHRoaXMuCgo+IFNpZ25lZC1vZmYtYnk6IEhhcnJ5IFBhbiA8aGFycnku cGFuQGludGVsLmNvbT4gLS0tCj5kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9wYW5lbC5jIHwg MiArLSAxIGZpbGUgY2hhbmdlZCwgMQo+aW5zZXJ0aW9uKCspLCAxIGRlbGV0aW9uKC0pCj4KPiBk aWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfcGFuZWwuYyBiL2RyaXZlcnMv Z3B1L2RybS9pOTE1L2ludGVsX3BhbmVsLmMKPiBpbmRleCAwOGFiNmQ3Li5lODgyMTM5IDEwMDY0 NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX3BhbmVsLmMKPiArKysgYi9kcml2 ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9wYW5lbC5jCj4gQEAgLTExMDQsNyArMTEwNCw3IEBAIHZv aWQgaW50ZWxfcGFuZWxfZW5hYmxlX2JhY2tsaWdodChzdHJ1Y3QgaW50ZWxfY29ubmVjdG9yICpj b25uZWN0b3IpCj4gIAo+ICAJV0FSTl9PTihwYW5lbC0+YmFja2xpZ2h0Lm1heCA9PSAwKTsKPiAg Cj4gLQlpZiAocGFuZWwtPmJhY2tsaWdodC5sZXZlbCA8PSBwYW5lbC0+YmFja2xpZ2h0Lm1pbikg ewo+ICsJaWYgKHBhbmVsLT5iYWNrbGlnaHQubGV2ZWwgPCBwYW5lbC0+YmFja2xpZ2h0Lm1pbikg ewo+ICAJCXBhbmVsLT5iYWNrbGlnaHQubGV2ZWwgPSBwYW5lbC0+YmFja2xpZ2h0Lm1heDsKCklm IHdlIGNoYW5nZWQgdGhpcyB0byBmb2xsb3cgeW91ciBsb2dpYywgdGhlIHNlbnNpYmxlIHRoaW5n IHRvIGRvIHdvdWxkCmJlIHRvIHNldCB0aGUgYmFja2xpZ2h0IHRvIG1pbiwgbm90IG1heCwgaW4g dGhpcyBjYXNlLgoKQnV0IHRoZSBwb2ludCBpcyBtb290LiBJIGRvbid0IHdhbnQgdG8gZGVhbCB3 aXRoIHRoZSByZWdyZXNzaW9ucyB0aGF0IEkKcHJlZGljdCB0aGUgY2hhbmdlIHdpbGwgaW5ldml0 YWJseSBjYXVzZS4KCgpCUiwKSmFuaS4KCgo+ICAJCWlmIChwYW5lbC0+YmFja2xpZ2h0LmRldmlj ZSkKPiAgCQkJcGFuZWwtPmJhY2tsaWdodC5kZXZpY2UtPnByb3BzLmJyaWdodG5lc3MgPQoKLS0g CkphbmkgTmlrdWxhLCBJbnRlbCBPcGVuIFNvdXJjZSBUZWNobm9sb2d5IENlbnRlcgpfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGlu ZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVl ZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK 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