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 at ffwll.ch Mobile: +41 (0)79 365 57 48
