Re: [PATCH 1/4] drm/i915: save/restore the legacy backlight control

[Daniel Vetter <daniel@ffwll.ch>]

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