At Wed, 16 Nov 2011 22:15:41 -0800, Keith Packard wrote: > > On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai at 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
