From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control Date: Thu, 30 Aug 2012 10:50:06 +0200 Message-ID: <20120830085006.GA18384@phenom.ffwll.local> References: <179a19e78c99201a9ec18967c122a5a3aedbf555.1346136448.git.jani.nikula@intel.com> <20120828141438.GC5125@phenom.ffwll.local> <4f206dd76bb01291ea6740c90ac392cc.squirrel@webmail.greenhost.nl> <20120828151534.GE5125@phenom.ffwll.local> <058d0fbebf0fcda2648f59b033e514dc.squirrel@webmail.greenhost.nl> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-we0-f177.google.com (mail-we0-f177.google.com [74.125.82.177]) by gabe.freedesktop.org (Postfix) with ESMTP id EEB4C9E9E0 for ; Thu, 30 Aug 2012 01:49:57 -0700 (PDT) Received: by weyr3 with SMTP id r3so894444wey.36 for ; Thu, 30 Aug 2012 01:49:57 -0700 (PDT) Content-Disposition: inline In-Reply-To: <058d0fbebf0fcda2648f59b033e514dc.squirrel@webmail.greenhost.nl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Indan Zupancic Cc: Jani Nikula , Takashi Iwai , intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Thu, Aug 30, 2012 at 10:32:48AM +0200, Indan Zupancic wrote: > On Tue, August 28, 2012 17:15, Daniel Vetter wrote: > > On Tue, Aug 28, 2012 at 04:49:15PM +0200, Indan Zupancic wrote: > >> By the way, saving LBPC only makes sense if it's done before it was > >> set to 0 to disable the panel. I don't know if the current code does > >> the right thing, I haven't looked at it for a while. > > > > I think we can coax it into doing the right thing, see my other mail. If > > your completely sure that lbpc /should/ be handled by the bios across s/r > > I think we can drop this. But tbh I have no idea how this really is > > supposed to work, and unfortunately we're not allowed to cross-check with > > the windows driver codebase :( > > Of course I'm not completely sure. But I agree with Chris' reasoning, > why else have two ways of controlling the backlight? To me LBPC gives > the impression of being a system specific thing the OS shouldn't need > to worry about. Partly because it wasn't documented in the old docs at > all, and its address isn't mentioned in the 965 vol3 manual, but also > because it's from around the time that the BIOS used to do a lot more > directly (APM versus ACPI etc.) It also gives a way to lower the max > output voltage (via PWM) to the backlight hardware. > > These kind of problems are caused by us not having exactly the same > info as the BIOS/machine makers got. Because that's what guided them > into making the stuff as it is, not how it's supposed to work. > > The code could check if LBPC has a reasonable value after resume: Do > nothing if it has and restore the last known good value if it doesn't. > > Writing 0 to LBPC before suspend seems unnecessary because the hardware > is going into sleep mode anyway. So the PWM signal should become 0 anyway, > or if it doesn't, the BIOS has a chance to disable the backlight. But I'm > not sure if that's really true. > > So to get back to this patch, saving the LBPC value only makes sense if > it's done before it's set to 0. I think saving it at i915_save_display() > time is too late if the panel got disabled. Then the code is always saving > and restoring zeros. Well, if we use that backlight enable bit for panel on/off, we should no longer clear neither lbpc nor the backlight registers. Hence I think this might just work, without us saving lbpc. But we can easily add that later on I think (if it's required on some kind of crazy machine). > BTW, inverted sense of the PWM register could be related to the polarity > of the PWM signal. That should be controlled with bit 28 in BLC_PWM_CTL2. Well, I've tried that and it didn't work. I suspect that this entire inverted backlight quirk mess we currently have is just an ugly hack to prevent our code from setting the backlight to 0 and doesn't actually work as advertised. Imo it was bad judgment to merge it ... Thanks, Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48