On Wed, 12 Jan 2011 01:35:49 +0100 (CET), "Indan Zupancic" <indan@xxxxxx> 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 <indan@xxxxxx> 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 <chris@xxxxxxxxxxxxxxxxxx> Cc: stable@xxxxxxxxxx # 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 _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel