Re: [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips

From: Takashi Iwai <tiwai@suse.de>
To: Keith Packard <keithp@keithp.com>
Cc: Daniel Mack <zonque@gmail.com>,
	Chris Wilson <chris@chris-wilson.co.uk>,
	Jesse Barnes <jbarnes@virtuousgeek.org>,
	harald@redhat.com, intel-gfx@lists.freedesktop.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips
Date: Thu, 17 Nov 2011 17:33:50 +0100	[thread overview]
Message-ID: <s5hlirelndd.wl%tiwai@suse.de> (raw)
In-Reply-To: <yunsjlnl1f6.fsf@aiko.keithp.com>

At Wed, 16 Nov 2011 22:15:41 -0800,
Keith Packard wrote:
> 
> On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > While refactoring of backlight control code in commit [a95735569:
> > drm/i915: Refactor panel backlight controls], the handling of the bit
> > 0 of duty-cycle was gone except for pineview.  This resulted in invalid
> > register values for old chips like 915GM.  When the bit 0 is set, the
> > backlight is turned off suddenly.
> 
> I'm looking at the mentioned patch and I don't see how that managed to
> correctly handle bit 0; is this the patch that managed to break this?

Hrm, then I must have been confused.  The gen < 4 check (formerly
!IS_I965G()) was firstly introduced in this refactoring.  I thought
this was done before, but apparently not.

Looking more carefully again, actually the IS_PINEVIEW() part (former
IS_GND()) was introduced in the commit [078a033f: drm/i915: fix
opregion backlight chip detect and range].  Thus the same bug must
have been present even in the earlier version, but didn't come up by
some reason.

So, I'll have to correct the patch commit log.  Sorry for that.

Now, a question is whether the condition (gen < 4) is really correct.
I *guess* the original code in the refactoring was intended to cover
it:
	if (!IS_I965G(dev))
		max &= ~1;

But this rule wasn't applied to set_backlight().
The bit-0-clear above implies that it's a similar behavior like
pineview.  And indeed Daniel's machine with 915GM hits this problem.
If the above rule should be applied to all places, it'd make more
sense to handle both in the same way like pineview.

Unfortunately I have no old machines to test this, and the only
material to judge was Daniel's case.  (Is Harald's machine also
915GM?)

If it's only with 915GM, we'll just need to change IS_PINEVEW() to
IS_PINEVIEW() || IS_I915GM().  This might be a safer option at this
moment unless we check all cases or specs...

thanks,

Takashi