Re: [PATCH] backlight: Don't read back backlight setting from kernel on DPMS off

[Hans de Goede <hdegoede@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1408 bytes --]

Hi,

On 06/05/2014 10:24 PM, Chris Wilson wrote:
> On Thu, Jun 05, 2014 at 09:08:33PM +0200, Hans de Goede wrote:
>> Note that it is read after the framebuffer has been resized and a new mode
>> has been set on "pipe 0 using LVDS1", could this perhaps cause the 0 to be
>> read when using actual_brightness ?
> 
> Indeed, that is likely the explanation, and shows the fallacy in the
> current approach. And also explains why acpi_backlight works with the
> current code, but that the kernel interfering with intel_backlight does
> not.
>  
>> Also I've just had a user who has been testing this patch come back to
>> me it does help, but he still has a suspend/resume issue. It seems that
>> some X app / gnome-component is doing the following:
>>
>> 1) DPMS off
>> 2) Read backlight xrandr property -> this will now return 0
>> 3) Set backlight xrandr property value to the value just read, aka 0
>> 4) DPMS on -> "restores" backlight to 0 because of the property set
>>
>> I believe the best way to fix this will be to make
>> xxx_output_get_property("backlight") return backlight_active_level
>> when in DPMS off, rather then calling xxx_output_backlight_get.
> 
> I had the same thought when reviewing the code following your email. I
> modified sna, but I think I want to restructure how backlight is saved
> around modesets.

And now a mail with the promised patch really attached ...

Regards,

Hans


[-- Attachment #2: 0001-backlight-Don-t-read-back-backlight-setting-from-ker.patch --]
[-- Type: text/x-patch, Size: 3168 bytes --]

>From c39c9f3cded6e47ae7b1899362a85bc94926a1d5 Mon Sep 17 00:00:00 2001
From: Hans de Goede <hdegoede@redhat.com>
Date: Thu, 5 Jun 2014 14:56:56 +0200
Subject: [PATCH] backlight: Don't read back backlight setting from kernel on
 DPMS off

We've several reports from users where the backlight comes up turned off
on starting X, when using video.use_native_backlight=1 (true dmi quirks, will
be the default for 3.16), in combination with having an external display
plugged in:

https://bugzilla.redhat.com/show_bug.cgi?id=1032978
https://bugzilla.redhat.com/show_bug.cgi?id=1103806

This seems to be caused by /sys/class/backlight/intel_backlight/brightness
reading back 0 when re-initializing the outputs. Unlike
/sys/class/backlight/acpi_video0/brightness which is used without the
video.use_native_backlight=1 param, which keeps returning the value from before

Here is an excerpt from Xorg.log when this happens:

[28225]: (II) intel(0): resizing framebuffer to 3286x1080
[28225]: (II) intel(0): switch to mode 1366x768@59.8 on pipe 0 using LVDS1, position (0, 0), rotation normal
[28225]: (II) intel(0): switch to mode 1920x1080@60.0 on pipe 0 using HDMI2, position (1366, 0), rotation normal
[28225]: (II) intel(0): DPMS off mode 3, saved backlight level 0
^^^ This is an extra debug line I added, mode == the mode parameter to
 xxx_output_dpms_backlight, saved backlight level is the value of
 backlight_active_level after the xxx_output_backlight_get call.

Note how backlight_active_level becomes 0 here.

[28225]: (II) intel(0): switch to mode 1366x768@59.8 on pipe 1 using LVDS1, position (0, 0), rotation normal
[28225]: (II) intel(0): DPMS on mode 0, setting backlight to 0

And here we restore the backlight to backlight_active_level which now is 0.

This commit fixes this by not reading back the backlight setting from the
kernel on DPMS off.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 src/sna/sna_display.c   | 3 ---
 src/uxa/intel_display.c | 3 ---
 2 files changed, 6 deletions(-)

diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c
index 13dbf64..c9d4b08 100644
--- a/src/sna/sna_display.c
+++ b/src/sna/sna_display.c
@@ -2323,9 +2323,6 @@ sna_output_dpms_backlight(xf86OutputPtr output, int oldmode, int mode)
 			sna_output_backlight_set(output,
 						   sna_output->backlight_active_level);
 	} else {
-		/* Only save the current backlight value if we're going from on to off. */
-		if (oldmode == DPMSModeOn)
-			sna_output->backlight_active_level = sna_output_backlight_get(output);
 		sna_output_backlight_set(output, 0);
 	}
 }
diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c
index 95ddcc8..62902f4 100644
--- a/src/uxa/intel_display.c
+++ b/src/uxa/intel_display.c
@@ -963,9 +963,6 @@ intel_output_dpms_backlight(xf86OutputPtr output, int oldmode, int mode)
 			intel_output_backlight_set(output,
 						   intel_output->backlight_active_level);
 	} else {
-		/* Only save the current backlight value if we're going from on to off. */
-		if (oldmode == DPMSModeOn)
-			intel_output->backlight_active_level = intel_output_backlight_get(output);
 		intel_output_backlight_set(output, 0);
 	}
 }
-- 
2.0.0


[-- Attachment #3: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx