From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757946Ab1KQQdx (ORCPT ); Thu, 17 Nov 2011 11:33:53 -0500 Received: from cantor2.suse.de ([195.135.220.15]:33626 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753809Ab1KQQdw (ORCPT ); Thu, 17 Nov 2011 11:33:52 -0500 Date: Thu, 17 Nov 2011 17:33:50 +0100 Message-ID: From: Takashi Iwai To: Keith Packard Cc: Daniel Mack , Chris Wilson , Jesse Barnes , 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 In-Reply-To: References: User-Agent: Wanderlust/2.15.6 (Almost Unreal) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.7 Emacs/23.3 (x86_64-suse-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org At Wed, 16 Nov 2011 22:15:41 -0800, Keith Packard wrote: > > On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai 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