| Message ID | 1401976118-8780-1-git-send-email-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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); } }
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(-)