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