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

From: Daniel Vetter <daniel.vetter@ffwll.ch>
To: Takashi Iwai <tiwai@suse.de>
Cc: Jani Nikula <jani.nikula@intel.com>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>
Subject: Re: [PATCH 3/3] drm/i915: Revert backlight cargo-culting
Date: Fri, 22 Mar 2013 12:22:42 +0100	[thread overview]
Message-ID: <CAKMK7uHD40UJFXzuncaPhfr-+KDjYC7MDFob6caqXwe3QOh5nA@mail.gmail.com> (raw)
In-Reply-To: <s5htxo3af77.wl%tiwai@suse.de>

On Fri, Mar 22, 2013 at 11:42 AM, Takashi Iwai <tiwai@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@ffwll.ch> wrote:
>> > On Fri, Mar 22, 2013 at 11:00 AM, Takashi Iwai <tiwai@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@intel.com>
>> >>> Cc: Takashi Iwai <tiwai@suse.de>
>> >>> Signed-off-by: Daniel Vetter <daniel.vetter@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? If 770c is a regression fix, too, we'll need to revert more I
think.

>   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.

> 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

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 :(

Yours, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch