From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH] backlight: Don't read back backlight setting from kernel on DPMS off Date: Fri, 06 Jun 2014 16:37:36 +0200 Message-ID: <5391D230.30700@redhat.com> References: <1401976118-8780-1-git-send-email-hdegoede@redhat.com> <8761kfe9qk.fsf@intel.com> <20140605142912.GB22214@nuc-i3427.alporthouse.com> <5390C031.4090500@redhat.com> <20140605202427.GC22214@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by gabe.freedesktop.org (Postfix) with ESMTP id 1E0FD6E38B for ; Fri, 6 Jun 2014 07:37:43 -0700 (PDT) In-Reply-To: <20140605202427.GC22214@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson , Jani Nikula , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org 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