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

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

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