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

From: Takashi Iwai <tiwai@suse.de>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
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 11:42:20 +0100	[thread overview]
Message-ID: <s5htxo3af77.wl%tiwai@suse.de> (raw)
In-Reply-To: <CAKMK7uEBNwQJJeBqbZrR=LGcp-AckpSLg2L6f7iGDD_DdrLZjw@mail.gmail.com>

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.

  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.

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

Takashi