Bug 97486 - Backlight does not change when adjust it higher than 50% after S3
Summary: Backlight does not change when adjust it higher than 50% after S3
Status: CLOSED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Intel (show other bugs)
Version: DRI git
Hardware: x86-64 (AMD64) Linux (All)
: medium normal
Assignee: Intel GFX Bugs mailing list
QA Contact: Intel GFX Bugs mailing list
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-26 03:34 UTC by Alex Hung
Modified: 2017-07-24 22:40 UTC (History)
7 users (show)

See Also:
i915 platform: KBL
i915 features: display/backlight


Attachments
Output of lspci (23.06 KB, text/plain)
2016-08-26 06:13 UTC, Alex Hung
no flags Details
/proc/cpuinfo (4.46 KB, text/plain)
2016-08-26 06:14 UTC, Alex Hung
no flags Details
dmesg (135.65 KB, text/plain)
2016-08-26 06:16 UTC, Alex Hung
no flags Details
intel_reg dump before S3 (17.16 KB, text/x-log)
2016-08-30 06:11 UTC, AceLan Kao
no flags Details
intel_reg dump after S3 (17.15 KB, text/x-log)
2016-08-30 06:14 UTC, AceLan Kao
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Hung 2016-08-26 03:34:00 UTC
Steps:
1. Boot into OS
2. suspend system
3. resume system from S3
4. Try to adjust brightness level

Expected results: Backlight should be changed when adjust it

Actual results: Backlight does not change when adjust it higher than 50% after S3
Comment 1 Alex Hung 2016-08-26 06:03:17 UTC
The kernel source code used is from https://code.launchpad.net/~ubuntu-kernel-test/ubuntu/+source/linux/+git/mirror-drm-intel/+ref/drm-intel-nightly, and kernel version shown is 4.8.0-rc3)

The debian binary is available @ http://people.canonical.com/~alexhung/LP1597612/
Comment 2 Alex Hung 2016-08-26 06:13:54 UTC
Created attachment 126045 [details]
Output of lspci
Comment 3 Alex Hung 2016-08-26 06:14:33 UTC
Created attachment 126046 [details]
/proc/cpuinfo
Comment 4 Alex Hung 2016-08-26 06:16:11 UTC
Created attachment 126047 [details]
dmesg

dmesg with the following:
1. kernel parameter with drm.debug=0x6
2. boot, suspend and resume
3. brightness by hotkeys from max to min, and from min to max
Comment 5 XiongZhang 2016-08-26 07:52:04 UTC
From ODM's debug result, the root cause of the issue is PWM signal is changed after suspend and resume in Linux no mater which panel is used. However BOE's panel spec does not support well when PWM signal is 1.6K.
Why PWM signal is changed to 1.6K after suspend and resume?

=================================
In Ubuntu Environment: 
1. When system boot up to the desktop, PWM signal is 220K
2. After system suspend and resume, PWM signal is 1.6K

In WIndows 10 Environment:
1. When system boot up to the desktop, PWM signal is 220K
2. After system suspend and resume, PWM signal is 220K
===================================
Comment 6 Jani Nikula 2016-08-26 08:43:31 UTC
A register dump using 'intel_reg dump' before suspend and after resume might be interesting. intel_reg is part of intel-gpu-tools http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/
Comment 7 AceLan Kao 2016-08-30 06:11:00 UTC
Created attachment 126114 [details]
intel_reg dump before S3
Comment 8 AceLan Kao 2016-08-30 06:14:20 UTC
Created attachment 126115 [details]
intel_reg dump after S3
Comment 9 AceLan Kao 2016-08-30 07:17:17 UTC
I tried to increase the value of reg 0xc8254 after S3, it seems help.
The display can become brighter after the brightness value be adjusted above 50%, and after set the reg value up to 0x05f00000, the brightness variation can be seen while changing the brightness value from 0% to 100%
Comment 10 Shawn Lee 2016-08-31 09:19:58 UTC
Hi AceLan Kao,
Could you also help to read register 0xc2004 before and after S3?
Comment 11 AceLan Kao 2016-09-01 02:55:50 UTC
The value of reg 0xC2004 before and after S3 is 0x01, it doesn't change.
Comment 12 Shawn Lee 2016-09-01 05:27:08 UTC
Hi AceLan Kao,
Please also dump register 0xc2000 before and after S3. Thanks!
Comment 13 AceLan Kao 2016-09-02 02:52:55 UTC
Before S3, 0xc2000 is 0x01
After S3, 0xc2000 is 0x00
So, after S3 I write back the value, it works.
Comment 14 Shawn Lee 2016-09-06 07:03:46 UTC
Hi AceLan Kao,

Could you help to apply the change in below to monitor this symptom still be replicated? Thanks!

---
 drivers/gpu/drm/i915/i915_drv.h     |    1 +
 drivers/gpu/drm/i915/i915_suspend.c |    6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c413587..003cbf2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1086,6 +1086,7 @@ struct i915_suspend_saved_registers {
 	uint64_t saveFENCE[I915_MAX_NUM_FENCES];
 	u32 savePCH_PORT_HOTPLUG;
 	u16 saveGCDGMBUS;
+	u32 savePWM_GRANULARITY;
 };
 
 struct vlv_s0ix_state {
diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index a0af170..5bf8b88 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -40,6 +40,9 @@ static void i915_save_display(struct drm_device *dev)
 	/* save FBC interval */
 	if (HAS_FBC(dev) && INTEL_INFO(dev)->gen <= 4 && !IS_G4X(dev))
 		dev_priv->regfile.saveFBC_CONTROL = I915_READ(FBC_CONTROL);
+
+	if (HAS_PCH_SPT(dev_priv))
+		dev_priv->regfile.savePWM_GRANULARITY = I915_READ(SOUTH_CHICKEN1);
 }
 
 static void i915_restore_display(struct drm_device *dev)
@@ -58,6 +61,9 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(FBC_CONTROL, dev_priv->regfile.saveFBC_CONTROL);
 
 	i915_redisable_vga(dev);
+
+	if (HAS_PCH_SPT(dev_priv))
+		I915_WRITE(SOUTH_CHICKEN1, dev_priv->regfile.savePWM_GRANULARITY);
 }
 
 int i915_save_state(struct drm_device *dev)
--
Comment 15 Jani Nikula 2016-09-06 13:53:57 UTC
(In reply to Shawn Lee from comment #14)
> Could you help to apply the change in below to monitor this symptom still be
> replicated? Thanks!

This may be tried as a debug patch, but *no* new register saves/restores should be added to save/restore. Instead, the backlight enable hooks in intel_panel.c should make sure it contains all state that is required.
Comment 16 AceLan Kao 2016-09-07 03:28:36 UTC
Hi Shawn Lee,
The patch works for me.
Comment 17 yann 2016-09-08 10:16:18 UTC
Patch is under review and available here : https://patchwork.freedesktop.org/series/12165/
Comment 18 AceLan Kao 2016-09-14 00:58:21 UTC
Any updates of the upstreaming progress of this patch?
Comment 19 Jani Nikula 2016-09-14 09:35:14 UTC
Pending another version from Shawn to address review comments.
Comment 20 Yuan Chao 2016-09-18 22:09:17 UTC
Regarding a long out-standing bug on Maabook Air 6,2 (2013), there is a similar behavior on back light after S3. [Bug 67454] With the suggested test on Comment 10 to 14 here, I found that instead of SOUTH_CHICKEN1 reg., SOUTH_CHICKEN2 reg. needs to be kept and restored. The output of 'lspci -nns 2' is 

00:02.0 VGA compatible controller [0300]: Intel Corporation Haswell-ULT Integrated Graphics Controller [8086:0a26] (rev 09)

Would the proposed patch be extended and cover the issue on MBA? Thanks!
Comment 21 Jani Nikula 2016-09-19 08:28:40 UTC
(In reply to Yuan Chao from comment #20)
> Regarding a long out-standing bug on Maabook Air 6,2 (2013), there is a
> similar behavior on back light after S3. [Bug 67454] With the suggested test
> on Comment 10 to 14 here, I found that instead of SOUTH_CHICKEN1 reg.,
> SOUTH_CHICKEN2 reg. needs to be kept and restored. The output of 'lspci -nns
> 2' is 
> 
> 00:02.0 VGA compatible controller [0300]: Intel Corporation Haswell-ULT
> Integrated Graphics Controller [8086:0a26] (rev 09)
> 
> Would the proposed patch be extended and cover the issue on MBA? Thanks!

It already covered your hardware. Did you actually try?

Anyway, here's two patches addressing the review comments:

https://patchwork.freedesktop.org/patch/111128/
https://patchwork.freedesktop.org/patch/111129/

Please test.
Comment 22 Jani Nikula 2016-09-20 09:16:29 UTC
Fixed by commits

commit 32b421e79e6b546da1d469f1229403ac9142d695
Author: Jani Nikula <jani.nikula@intel.com>
Date:   Mon Sep 19 13:35:25 2016 +0300

    drm/i915/backlight: setup and cache pwm alternate increment value

and

commit e29aff05f239f8dd24e9ee7816fd96726e20105a
Author: Shawn Lee <shawn.c.lee@intel.com>
Date:   Mon Sep 19 13:35:26 2016 +0300

    drm/i915/backlight: setup backlight pwm alternate increment on backlight enable

in drm-intel-next-queued.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.