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

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

[Hans de Goede]

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

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

[Jani Nikula]

On Thu, 05 Jun 2014, Hans de Goede <hdegoede@redhat.com> wrote:
> 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.

I'm not at all familiar with the code base you're changing, and I'm just
speculating here, but this seems a little odd.

My guess is that the sna_output_backlight_get and/or
intel_output_backlight_get functions read the actual_brightness sysfs
file, which reads back the value from hardware. This is the contract for
backlight class device. The acpi video implementation returns the cached
value if there's no BQC or BCQ method.

I think perhaps either the current brightness should be read before
switching off the output, or the brightness sysfs file should be used
(which returns the cached current value). Or, perhaps, your patch is the
right answer, as I think we should save the value across disable/enable
anyway.

Chris?

BR,
Jani.


>
> 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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Chris Wilson]

On Thu, Jun 05, 2014 at 05:01:23PM +0300, Jani Nikula wrote:
> On Thu, 05 Jun 2014, Hans de Goede <hdegoede@redhat.com> wrote:
> > 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.
> 
> I'm not at all familiar with the code base you're changing, and I'm just
> speculating here, but this seems a little odd.
> 
> My guess is that the sna_output_backlight_get and/or
> intel_output_backlight_get functions read the actual_brightness sysfs
> file, which reads back the value from hardware. This is the contract for
> backlight class device. The acpi video implementation returns the cached
> value if there's no BQC or BCQ method.
> 
> I think perhaps either the current brightness should be read before
> switching off the output.

It is read before we switch off the CRTC (and thus the output). So where
it is getting the zero from is a puzzle - it should be the current state
of the hardware, ergo what the user set by some other path.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Hans de Goede]

Hi,

On 06/05/2014 04:29 PM, Chris Wilson wrote:
> On Thu, Jun 05, 2014 at 05:01:23PM +0300, Jani Nikula wrote:
>> On Thu, 05 Jun 2014, Hans de Goede <hdegoede@redhat.com> wrote:
>>> 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.
>>
>> I'm not at all familiar with the code base you're changing, and I'm just
>> speculating here, but this seems a little odd.
>>
>> My guess is that the sna_output_backlight_get and/or
>> intel_output_backlight_get functions read the actual_brightness sysfs
>> file, which reads back the value from hardware. This is the contract for
>> backlight class device. The acpi video implementation returns the cached
>> value if there's no BQC or BCQ method.

Hmm, interesting, I just checked, and xxx_output_backlight_get in the
2.21.15 driver which is where this is seen indeed reads actual_brightness
not just brightness. Where as current master uses backlight not
actual_backlight, but some of the reporting users have tried with
2.99.x driver versions and they still had the problem.

Still I'll prepare a scratch build for these users using just backlight
to see if that helps.


>>
>> I think perhaps either the current brightness should be read before
>> switching off the output.
> 
> It is read before we switch off the CRTC (and thus the output). So where
> it is getting the zero from is a puzzle - it should be the current state
> of the hardware, ergo what the user set by some other path.

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 ?

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'll write a patch doing this tomorrow and ask the user to test
(I'm calling it a day for now). If you've a better idea please let
me know before I write and submit this patch :)

Regards,

Hans

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

[Chris Wilson]

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.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Hans de Goede]

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.

Ok, FWIW attached is a patch which I'm asking the user to test to confirm that
the above steps 1-4 are the problem of his suspend / resume problems.

About solving this differently / more completely why is the driver saving
the brightness at all? IMHO the driver / xrandr property should be the canonical
source of the brightness level once X is running, so we should only read it once
on init and then just always use backlight_active_level, this way the code becomes
simpler and we won't have any of this issues.

I really so no use-case where we want something to change the brightness underneath
us and then for the driver to correctly pick up this change.

One last question, should I ask users with this problem to re-test using the brightness
sysfs file in the xxx_output_get_property methods instead of the actual_brightness
sysfs file as the 2.21.15 version they have is doing ?

Regards,

Hans

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

[Hans de Goede]

[-- 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

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

[Chris Wilson]

On Fri, Jun 06, 2014 at 04:37:36PM +0200, Hans de Goede wrote:
> 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.
> 
> Ok, FWIW attached is a patch which I'm asking the user to test to confirm that
> the above steps 1-4 are the problem of his suspend / resume problems.

It should be fixed upstream now.
 
> About solving this differently / more completely why is the driver saving
> the brightness at all? IMHO the driver / xrandr property should be the canonical
> source of the brightness level once X is running, so we should only read it once
> on init and then just always use backlight_active_level, this way the code becomes
> simpler and we won't have any of this issues.
> 
> I really so no use-case where we want something to change the brightness underneath
> us and then for the driver to correctly pick up this change.

ACPI buttons may modify the brightness directly and we need to propagate
those changes back to the RandR clients. See the current tree for how
that works.

commit c6cd10f536e099277cdc46643725a5a50ea8b525
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Jun 5 22:43:37 2014 +0100

    sna: Hook up a backlight udev monitor for external changes
    
    Changes to the backlights are notified through uevents. Hooking up a
    udev monitor to listen out for external changes to the backlight (e.g.
    through ACPI function keys, or by the user writing to
    /sys/class/backlight directly) is easier than enabling polling on the
    backlight sysfs file using X's select() mechanism.
    
    Since we listen to backlight changes, we have to be careful not to
    confuse the side-effects of disabling connectors (which may cause either
    ourselves or the kernel to turn off the backlight) with the user value.
    
    Many thanks to Alexander Mezin for the suggestion to use udev for
    tracking the notifications for external changes to the backlight.
    
    Reported-by: Alexander Mezin <mezin.alexander@gmail.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79699
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

 
> One last question, should I ask users with this problem to re-test using the brightness
> sysfs file in the xxx_output_get_property methods instead of the actual_brightness
> sysfs file as the 2.21.15 version they have is doing ?

No, that seems to be a separate issue. We may want to use brightness
just so that the values we set and we read are idempotent.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Hans de Goede]

Hi,

On 06/06/2014 04:51 PM, Chris Wilson wrote:
> On Fri, Jun 06, 2014 at 04:37:36PM +0200, Hans de Goede wrote:
>> 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.
>>
>> Ok, FWIW attached is a patch which I'm asking the user to test to confirm that
>> the above steps 1-4 are the problem of his suspend / resume problems.
>
> It should be fixed upstream now.
>
>> About solving this differently / more completely why is the driver saving
>> the brightness at all? IMHO the driver / xrandr property should be the canonical
>> source of the brightness level once X is running, so we should only read it once
>> on init and then just always use backlight_active_level, this way the code becomes
>> simpler and we won't have any of this issues.
>>
>> I really so no use-case where we want something to change the brightness underneath
>> us and then for the driver to correctly pick up this change.
>
> ACPI buttons may modify the brightness directly and we need to propagate
> those changes back to the RandR clients. See the current tree for how
> that works.
>
> commit c6cd10f536e099277cdc46643725a5a50ea8b525
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Jun 5 22:43:37 2014 +0100
>
>      sna: Hook up a backlight udev monitor for external changes
>
>      Changes to the backlights are notified through uevents. Hooking up a
>      udev monitor to listen out for external changes to the backlight (e.g.
>      through ACPI function keys, or by the user writing to
>      /sys/class/backlight directly) is easier than enabling polling on the
>      backlight sysfs file using X's select() mechanism.
>
>      Since we listen to backlight changes, we have to be careful not toack the backli
>      confuse the side-effects of disabling connectors (which may cause either
>      ourselves or the kernel to turn off the backlight) with the user value.
>
>      Many thanks to Alexander Mezin for the suggestion to use udev for
>      tracking the notifications for external changes to the backlight.
>
>      Reported-by: Alexander Mezin <mezin.alexander@gmail.com>
>      Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79699
>      Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Thanks, I fail to see how this addresses the original problem though,
current master still reads back the backlight from the kernel at DPMS
off, so the problem my original patch in this thread tries to fix
still exists AFAIK, we will still read back 0 on DPMS off in the
scenario my patch tries to address.

You've cleaned up the code in sna_output_dpms, but the logic is unchanged.

Possibly this even makes the problem bigger, because if the modeset
changes the brightness before the dpms off (as it seems to do) then an
udev event may get triggered and process before sna_output_dpms changes
the sna_output->dpms_mode and the

if (sna_output->dpms_mode != DPMSModeOn)

Guard in sna_backlight_uevent won't protect against storing the 0 brightness
value caused by the modeset to end up in active_level

Regards,

Hans

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

[Chris Wilson]

On Sat, Jun 07, 2014 at 12:18:35PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/06/2014 04:51 PM, Chris Wilson wrote:
> >commit c6cd10f536e099277cdc46643725a5a50ea8b525
> >Author: Chris Wilson <chris@chris-wilson.co.uk>
> >Date:   Thu Jun 5 22:43:37 2014 +0100
> 
> Thanks, I fail to see how this addresses the original problem though,
> current master still reads back the backlight from the kernel at DPMS
> off, so the problem my original patch in this thread tries to fix
> still exists AFAIK, we will still read back 0 on DPMS off in the
> scenario my patch tries to address.

It changes the sequence in which output->funcs->dpms is called to
prevent the bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Hans de Goede]

Hi,

On 06/07/2014 01:12 PM, Chris Wilson wrote:
> On Sat, Jun 07, 2014 at 12:18:35PM +0200, Hans de Goede wrote:
>> Hi,
>>
>> On 06/06/2014 04:51 PM, Chris Wilson wrote:
>>> commit c6cd10f536e099277cdc46643725a5a50ea8b525
>>> Author: Chris Wilson <chris@chris-wilson.co.uk>
>>> Date:   Thu Jun 5 22:43:37 2014 +0100
>>
>> Thanks, I fail to see how this addresses the original problem though,
>> current master still reads back the backlight from the kernel at DPMS
>> off, so the problem my original patch in this thread tries to fix
>> still exists AFAIK, we will still read back 0 on DPMS off in the
>> scenario my patch tries to address.
> 
> It changes the sequence in which output->funcs->dpms is called to
> prevent the bug.

Thanks, I don't see that being changed in the above commit though,
and I cannot find the commit in which it does change, can you point
me to the commit where this is changed ?

Regards,

Hans

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

[Chris Wilson]

On Fri, Jun 13, 2014 at 10:49:12AM +0200, Hans de Goede wrote:
> Hi,
> 
> On 06/07/2014 01:12 PM, Chris Wilson wrote:
> > On Sat, Jun 07, 2014 at 12:18:35PM +0200, Hans de Goede wrote:
> >> Hi,
> >>
> >> On 06/06/2014 04:51 PM, Chris Wilson wrote:
> >>> commit c6cd10f536e099277cdc46643725a5a50ea8b525
> >>> Author: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Date:   Thu Jun 5 22:43:37 2014 +0100
> >>
> >> Thanks, I fail to see how this addresses the original problem though,
> >> current master still reads back the backlight from the kernel at DPMS
> >> off, so the problem my original patch in this thread tries to fix
> >> still exists AFAIK, we will still read back 0 on DPMS off in the
> >> scenario my patch tries to address.
> > 
> > It changes the sequence in which output->funcs->dpms is called to
> > prevent the bug.
> 
> Thanks, I don't see that being changed in the above commit though,
> and I cannot find the commit in which it does change, can you point
> me to the commit where this is changed ?


commit c6cd10f536e099277cdc46643725a5a50ea8b525
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Jun 5 22:43:37 2014 +0100

    sna: Hook up a backlight udev monitor for external changes
    
    Changes to the backlights are notified through uevents. Hooking up a
    udev monitor to listen out for external changes to the backlight (e.g.
    through ACPI function keys, or by the user writing to
    /sys/class/backlight directly) is easier than enabling polling on the
    backlight sysfs file using X's select() mechanism.
    
    Since we listen to backlight changes, we have to be careful not to
    confuse the side-effects of disabling connectors (which may cause either
    ourselves or the kernel to turn off the backlight) with the user value.
    
    Many thanks to Alexander Mezin for the suggestion to use udev for
    tracking the notifications for external changes to the backlight.
    
    Reported-by: Alexander Mezin <mezin.alexander@gmail.com>
    Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79699
    Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

@@ -820,6 +949,14 @@ sna_crtc_apply(xf86CrtcPtr crtc)
        for (i = 0; i < config->num_output; i++) {
                xf86OutputPtr output = config->output[i];
 
+               /* Make sure we mark the output as off (and save the backlight)
+                * before the kernel turns it off due to changing the pipe.
+                * This is necessary as the kernel may turn off the backlight
+                * and we lose track of the user settings.
+                */
+               if (output->crtc == NULL)
+                       output->funcs->dpms(output, DPMSModeOff);
+
                if (output->crtc != crtc)
                        continue;

It does depend on those other outputs being marked as disconnected first
when switching pipes, which seems true looking at xf86RandR12CrtcSet().
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre