From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756062Ab1ALMH3 (ORCPT ); Wed, 12 Jan 2011 07:07:29 -0500 Received: from mga09.intel.com ([134.134.136.24]:42940 "EHLO mga09.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751901Ab1ALMH1 (ORCPT ); Wed, 12 Jan 2011 07:07:27 -0500 Message-Id: X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.60,312,1291622400"; d="scan'208";a="592177905" Date: Wed, 12 Jan 2011 12:07:23 +0000 To: Indan Zupancic Subject: Re: Linux 2.6.37 Cc: Michal Hocko , Linus Torvalds , Linux Kernel Mailing List , dri-devel@lists.freedesktop.org In-Reply-To: References: <20110106104816.GA4026@tiehlicka.suse.cz> <20110111171744.GA3854@tiehlicka.suse.cz> <1bdc18$j7rfh5@fmsmga002.fm.intel.com> From: Chris Wilson Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 12 Jan 2011 01:35:49 +0100 (CET), "Indan Zupancic" wrote: > Yeah, the second patch is a bit of a desperate attempt because Larry reported that > it didn't fix his problem. > > About your patch, you still do: > > +void intel_panel_setup_backlight(struct drm_device *dev) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + > + dev_priv->backlight_level = intel_panel_get_max_backlight(dev); > + dev_priv->backlight_enabled = dev_priv->backlight_level != 0; > +} > > While my patch changes that to: > > + u32 level; > > - if (dev_priv->backlight_level == 0) > - dev_priv->backlight_level = intel_panel_get_max_backlight(dev); > + if (dev_priv->backlight_level == 0) { > + level = intel_panel_get_backlight(dev); > + if (level == 0) > + level = intel_panel_get_max_backlight(dev); > + dev_priv->backlight_level = level; > + } > > Your patch uses intel_panel_get_max_backlight() to check if the backlight is > enabled. Is this always correct, or may it cause bugs in the future? That was a typo, cut'n'pasting the line from above. > Anyway, my main concern with unconditionally setting the backlight level to > the maximum is that any stored brightness level (by the BIOS or whatever) may > be lost, and that the screen is set to maximum brightness at each boot. This > is certainly the behaviour I've seen with an unpatched kernel. So I propose to > do what my patch does and set it to intel_panel_get_backlight(dev) if that > returns non-zero. Something like this: Sure, s/intel_panel_get_max_backlight/intel_panel_get_backlight/ and we get the behaviour we both want - preserving the current backlight unless none is set. > While I'm glad this problem is being fixed upstream, it would be nice to get > some credit for finding the source of the problem. Sorry. You found the bug but I felt your rationale was off. However, I was amiss in not giving you the credit you fully deserved. -Chris commit 9c1c388a53e5df8819e898106a64e34d0994a5d4 Author: Indan Zupancic Date: Wed Jan 12 11:59:19 2011 +0000 drm/i915/panel: The backlight is enabled if the current value is non-zero ... and not if the maximum is non-zero. This fixes the typo introduced in 47356eb6728501452 and preserves the backlight value from boot. [ickle: My thanks also to Indan Zupancic for diagnosing the original regression and suggesting the appropriate fix.] Signed-off-by: Chris Wilson Cc: stable@kernel.org # after 47356eb6728501452 diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_pan index e00d200..27c79c7 100644 --- a/drivers/gpu/drm/i915/intel_panel.c +++ b/drivers/gpu/drm/i915/intel_panel.c @@ -278,6 +278,6 @@ void intel_panel_setup_backlight(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; - dev_priv->backlight_level = intel_panel_get_max_backlight(dev); + dev_priv->backlight_level = intel_panel_max_backlight(dev); dev_priv->backlight_enabled = dev_priv->backlight_level != 0; } -- Chris Wilson, Intel Open Source Technology Centre