[Intel-gfx] [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 at 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 at gmail.com>
>      Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=79699
>      Signed-off-by: Chris Wilson <chris at 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