[Intel-gfx] [PATCH 3/3] drm/i915: Revert backlight cargo-culting

[Takashi Iwai]

At Fri, 22 Mar 2013 12:22:42 +0100,
Daniel Vetter wrote:
> 
> On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai at suse.de> wrote:
> > At Fri, 22 Mar 2013 11:22:57 +0100,
> > Daniel Vetter wrote:
> >> On Fri, Mar 22, 2013 at 11:13 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> >> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai at suse.de> wrote:
> >> >> At Fri, 22 Mar 2013 10:53:41 +0100,
> >> >> Daniel Vetter wrote:
> >> >>>
> >> >>> This reverts the following commits:
> >> >>>
> >> >>> cf0a6584aa6d382f802 drm/i915: write backlight harder
> >> >>> 770c12312ad617172b1 drm/i915: Fix blank panel at reopening lid
> >> >>>
> >> >>> We've come full-circle in this mess and now broke the originally fixed
> >> >>> machines again with the new trick. So remove it all and start over.
> >> >>>
> >> >>> References: http://www.mail-archive.com/intel-gfx@lists.freedesktop.org/msg18788.html
> >> >>> Cc: Jani Nikula <jani.nikula at intel.com>
> >> >>> Cc: Takashi Iwai <tiwai at suse.de>
> >> >>> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> >> >>
> >> >> Well, although I fully agree that it's better to clear all voodoo once
> >> >> and fix from scratch, my concern is that it'll cause mess for stable
> >> >> kernel.  This path won't fix anything by itself, so it's not suitable
> >> >> for stable alone.  If any, it should be together with the real fix
> >> >> patch.
> >> >>
> >> >> I guess you'll submit the proper fix patch(es) to stable later on?
> >> >
> >> > Oops, I've missed that 770c12312ad617172 itself is a regression fix,
> >> > but lacks the relevant commit citations. I'll resend with more
> >> > aggressive reverting.
> >>
> >> Actually the only other patch I've found is 6db65cbb941f, but no one
> >> reported a regression against that one yet. So I think we can leave
> >> that in for now. So I still think that this is the patch I want for
> >> 3.9&stable, since oldest regression wins.
> >
> > Yeah, the situation is messy.  Let me write down as far as I remember:
> >
> > - 770c12312ad617172 itself fixed the backlight at resume originally
> >   with 3.6 kernel.  The backlight worked at boot time, IIRC.  It had
> >   no mention of regressing commit since it was a series of changes,
> >   IIRC.  It fixed the same problem for a few Lenovo and HP laptops.
> 
> I think the important question is whether those Lenovo/HPs ever
> worked?

IIRC, the boot screen worked on 3.6-rc but the resume was broken -- at
least on HP machines.  On Lenovo, it might be different.  Sorry, I
don't remember.  Need to dig into LKML archive.

> If 770c is a regression fix, too, we'll need to revert more I
> think.

Yes, but a revert wasn't trivial at that time because of large changes
in i915 code in 3.6-rc1.  Maybe we can try it again...

> >   However, this turned out to break a backlight on a Dell machine.
> >
> > - cf0a6584aa6d382f802 is a try to fix the Dell backlight, so it went
> >   in 3.9 and propagated to stable.  But it hadn't been tested on
> >   Lenovo nor HP machines.
> >
> >   Now this broke HP machines (not sure about Lenovo) again.
> >   Interestingly, at this time, it broke the backlight at boot time.
> >   Maybe the recent changes of initialization sequence at boot made
> >   difference?
> >
> > - 6db65cbb941f is slightly different fix from other commits.
> >   AFAIK, this was found only on SandyBridge eDP (HP Z-something
> >   model), and this happens only at resume, again.
> >
> >   Unfortunately I have no longer access to this machine, so I can't
> >   check the current status.
> 
> Iirc we've used 6db6 as inspiration/justification for the above commits.

Well, not from the beginning; it was reported on different machines as
an individual issue.  But the end fix looks similar, so very likely
related.

Another point is that the fixes are targeted to different ranges:

The fix 770c was for a variety of machines and all were LVDS.  (But
now we see it's affecting to HSW and IVY eDP, too.)

The fix 6db6 is for SNB eDP only.  We didn't see the issue on LVDS at
that time.


> > Now, you revert both first two, that is, the backlight handling is
> > like 3.6-rc1.  A Dell laptop should still work, but HP/Lenovo are
> > likely broken yet.  That's why I wonder whether it's suitable for
> > stable.  It cleans up the code but doesn't fix actual bugs.
> >
> > Apart from that, a big question is why the commit 770c12312ad617172
> > broke the Dell laptop.  It just writes BLC_PWM_CPU_CTL again.  If this
> > really breaks, won't further changing the backlight (or even setting
> > the same level) break again even with the "fix" patch?
> > Since intel_panel_set_backlight() just updates  BLC_PWM_CPU_CTL, the
> > effect should be identical...
> 
> One of the reasons why I want to go back to the beginning and rip out
> all the hacks is that I've noticed that we completely lack locking in
> the backlight code. And we have at least 3 concurrent useres:
> - modeset code
> - sysfs backlight interface
> - asle interrupt handler

Yeah, that looks fragile.

> Especially the last one is known to fire when touching some of the
> blc/panel registers through some smm magic. So imo before we add more
> hacks again, we should fix up the locking (and generally make the code
> a bit less messy, Jani has started with some patches), and then check
> which bugs are left.
> 
> Yeah, this will suck a bit for distros :(

Hehe, that's hard life :p


Takashi