Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-10 Thread Takashi Iwai 
At Thu, 10 Mar 2011 08:49:37 +0100,
Takashi Iwai wrote:
 
 At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
 Indan Zupancic wrote:
  
  Hello,
  
  On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
   Alex, can you confirm that the revert of 951f3512dba5 plus the
   one-liner patch from Takashi that Indan quoted also works for you?
  
 Linus
  
   On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
  
   So please revert my patch and apply Takashi Iwai's, which fixes the
   most immediate bug without changing anything else. This should go
   in stable too.
  
  
  I found another backlight bug:
  
  When suspending intel_panel_disable_backlight() is never called,
  but intel_panel_enable_backlight() is called at resume. With the
  effect that if the brightness was ever changed after screen
  blanking, the wrong brightness gets restored.
  
  This explains the weird behaviour I've seen. I didn't see it with
  combination mode, because then the brightness is always the same
  (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
  send a patch in a moment.
  
  Alternative for reverting the combination mode removal (I can also
  redo the patch against the revert and Takashi's patch, if that's
  preferred):
  
  --
  
  drm/i915: Do handle backlight combination mode specially
  
  Add back the combination mode check, but with slightly cleaner code
  and the weirdness removed: No val = 1, but also no val = ~1. The
  old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
  The other change is clearer calculations: Just check for zero level
  explicitly instead of avoiding the divide-by-zero.
  
  Signed-off-by: Indan Zupancic in...@nul.nu
  
  ---
  
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index d860abe..b05631a 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -30,6 +30,10 @@
  
   #include intel_drv.h
  
  +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
  +#define BLM_COMBINATION_MODE (1  30)
  +#define BLM_LEGACY_MODE (1  16)
  +
   void
   intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 struct drm_display_mode *adjusted_mode)
  @@ -110,6 +114,22 @@ done:
  dev_priv-pch_pf_size = (width  16) | height;
   }
  
  +/*
  + * What about gen 3? If there are no gen 3 systems with ASLE,
  + * then it doesn't matter, as we don't need to change the
  + * brightness. But then the gen 2 check can be removed too.
  + */
  +static int is_backlight_combination_mode(struct drm_device *dev)
  +{
  +   struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +   if (INTEL_INFO(dev)-gen = 4)
  +   return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
  +   if (IS_GEN2(dev))
  +   return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
  +   return 0;
  +}
  +
   static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
   {
  u32 val;
  @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
  *dev)
  max = 17;
  } else {
  max = 16;
  +   /* Ignore BLM_LEGACY_MODE bit */
  if (INTEL_INFO(dev)-gen  4)
  max = ~1;
  }
  +   if (is_backlight_combination_mode(dev))
  +   max *= 0xff;
  }
  
  DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
  @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
  val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
  if (IS_PINEVIEW(dev))
  val = 1;
  +   if (is_backlight_combination_mode(dev)){
  +   u8 lbpc;
  +
  +   pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
  +   val *= lbpc;
  +   }
  }
  
  DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
  @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, 
  u32 level)
  
  if (HAS_PCH_SPLIT(dev))
  return intel_pch_panel_set_backlight(dev, level);
  +
  +   if (level  is_backlight_combination_mode(dev)){
  +   u32 max = intel_panel_get_max_backlight(dev);
  +   u8 lpbc;
  +
  +   lpbc = level * 0xff / max;
  +   level /= lpbc;
 
 Hmm, I don't think this calculation is correct.  This would result
 in level of opregion over its limit.  For example, assume the level
 max = 100, so total max = 25500.  Passing level=150 here will be:
 
   lbpc = 150 * 0xff / 25500 = 1.5 = 1
   level = 150 / 1 = 150, which is over limit.
 
 More worse, lbpc can be zero when level is below 100 in the case
 above...

That is, Chris' original code in that portion was correct:

if (is_backlight_combination_mode(dev)){
u32 max = intel_panel_get_max_backlight(dev);
u8 lpbc;

lpbc = level * 0xfe / max

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-10 Thread Indan Zupancic 
On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
 At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
 Indan Zupancic wrote:

 Hello,

 On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
  Alex, can you confirm that the revert of 951f3512dba5 plus the
  one-liner patch from Takashi that Indan quoted also works for you?
 
Linus
 
  On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
 
  So please revert my patch and apply Takashi Iwai's, which fixes the
  most immediate bug without changing anything else. This should go
  in stable too.
 

 I found another backlight bug:

 When suspending intel_panel_disable_backlight() is never called,
 but intel_panel_enable_backlight() is called at resume. With the
 effect that if the brightness was ever changed after screen
 blanking, the wrong brightness gets restored.

 This explains the weird behaviour I've seen. I didn't see it with
 combination mode, because then the brightness is always the same
 (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
 send a patch in a moment.

 Alternative for reverting the combination mode removal (I can also
 redo the patch against the revert and Takashi's patch, if that's
 preferred):

 --

 drm/i915: Do handle backlight combination mode specially

 Add back the combination mode check, but with slightly cleaner code
 and the weirdness removed: No val = 1, but also no val = ~1. The
 old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
 The other change is clearer calculations: Just check for zero level
 explicitly instead of avoiding the divide-by-zero.

 Signed-off-by: Indan Zupancic in...@nul.nu

 ---

 diff --git a/drivers/gpu/drm/i915/intel_panel.c 
 b/drivers/gpu/drm/i915/intel_panel.c
 index d860abe..b05631a 100644
 --- a/drivers/gpu/drm/i915/intel_panel.c
 +++ b/drivers/gpu/drm/i915/intel_panel.c
 @@ -30,6 +30,10 @@

  #include intel_drv.h

 +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
 +#define BLM_COMBINATION_MODE (1  30)
 +#define BLM_LEGACY_MODE (1  16)
 +
  void
  intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
 struct drm_display_mode *adjusted_mode)
 @@ -110,6 +114,22 @@ done:
  dev_priv-pch_pf_size = (width  16) | height;
  }

 +/*
 + * What about gen 3? If there are no gen 3 systems with ASLE,
 + * then it doesn't matter, as we don't need to change the
 + * brightness. But then the gen 2 check can be removed too.
 + */
 +static int is_backlight_combination_mode(struct drm_device *dev)
 +{
 +struct drm_i915_private *dev_priv = dev-dev_private;
 +
 +if (INTEL_INFO(dev)-gen = 4)
 +return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
 +if (IS_GEN2(dev))
 +return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
 +return 0;
 +}
 +
  static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
  {
  u32 val;
 @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
 *dev)
  max = 17;
  } else {
  max = 16;
 +/* Ignore BLM_LEGACY_MODE bit */
  if (INTEL_INFO(dev)-gen  4)
  max = ~1;
  }
 +if (is_backlight_combination_mode(dev))
 +max *= 0xff;
  }

  DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
 @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
  val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
  if (IS_PINEVIEW(dev))
  val = 1;
 +if (is_backlight_combination_mode(dev)){
 +u8 lbpc;
 +
 +pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
 +val *= lbpc;
 +}
  }

  DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
 @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, 
 u32 level)

  if (HAS_PCH_SPLIT(dev))
  return intel_pch_panel_set_backlight(dev, level);
 +
 +if (level  is_backlight_combination_mode(dev)){
 +u32 max = intel_panel_get_max_backlight(dev);
 +u8 lpbc;
 +
 +lpbc = level * 0xff / max;
 +level /= lpbc;

 Hmm, I don't think this calculation is correct.  This would result
 in level of opregion over its limit.  For example, assume the level
 max = 100, so total max = 25500.  Passing level=150 here will be:

   lbpc = 150 * 0xff / 25500 = 1.5 = 1
   level = 150 / 1 = 150, which is over limit.

 More worse, lbpc can be zero when level is below 100 in the case
 above...

Yes, you're right. It seems that any simplification I try to do
creates a new bug.

Do you have any bright idea why the old code did val = ~1; too?
It seems obvious it's related to val = 1, but...

Thanks,

Indan


___
dri-devel mailing list
dri-devel@lists.freedesktop.org

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-10 Thread Indan Zupancic 
On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
 At Thu, 10 Mar 2011 08:49:37 +0100,
 Takashi Iwai wrote:

 At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
 Indan Zupancic wrote:
 
  Hello,
 
  On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
   Alex, can you confirm that the revert of 951f3512dba5 plus the
   one-liner patch from Takashi that Indan quoted also works for you?
  
 Linus
  
   On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
  
   So please revert my patch and apply Takashi Iwai's, which fixes the
   most immediate bug without changing anything else. This should go
   in stable too.
  
 
  I found another backlight bug:
 
  When suspending intel_panel_disable_backlight() is never called,
  but intel_panel_enable_backlight() is called at resume. With the
  effect that if the brightness was ever changed after screen
  blanking, the wrong brightness gets restored.
 
  This explains the weird behaviour I've seen. I didn't see it with
  combination mode, because then the brightness is always the same
  (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
  send a patch in a moment.
 
  Alternative for reverting the combination mode removal (I can also
  redo the patch against the revert and Takashi's patch, if that's
  preferred):
 
  --
 
  drm/i915: Do handle backlight combination mode specially
 
  Add back the combination mode check, but with slightly cleaner code
  and the weirdness removed: No val = 1, but also no val = ~1. The
  old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
  The other change is clearer calculations: Just check for zero level
  explicitly instead of avoiding the divide-by-zero.
 
  Signed-off-by: Indan Zupancic in...@nul.nu
 
  ---
 
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index d860abe..b05631a 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -30,6 +30,10 @@
 
   #include intel_drv.h
 
  +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
  +#define BLM_COMBINATION_MODE (1  30)
  +#define BLM_LEGACY_MODE (1  16)
  +
   void
   intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode)
  @@ -110,6 +114,22 @@ done:
 dev_priv-pch_pf_size = (width  16) | height;
   }
 
  +/*
  + * What about gen 3? If there are no gen 3 systems with ASLE,
  + * then it doesn't matter, as we don't need to change the
  + * brightness. But then the gen 2 check can be removed too.
  + */
  +static int is_backlight_combination_mode(struct drm_device *dev)
  +{
  +  struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +  if (INTEL_INFO(dev)-gen = 4)
  +  return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
  +  if (IS_GEN2(dev))
  +  return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
  +  return 0;
  +}
  +
   static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
   {
 u32 val;
  @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
  *dev)
 max = 17;
 } else {
 max = 16;
  +  /* Ignore BLM_LEGACY_MODE bit */
 if (INTEL_INFO(dev)-gen  4)
 max = ~1;
 }
  +  if (is_backlight_combination_mode(dev))
  +  max *= 0xff;
 }
 
 DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
  @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
 if (IS_PINEVIEW(dev))
 val = 1;
  +  if (is_backlight_combination_mode(dev)){
  +  u8 lbpc;
  +
  +  pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
  +  val *= lbpc;
  +  }
 }
 
 DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
  @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device 
  *dev, u32 level)
 
 if (HAS_PCH_SPLIT(dev))
 return intel_pch_panel_set_backlight(dev, level);
  +
  +  if (level  is_backlight_combination_mode(dev)){
  +  u32 max = intel_panel_get_max_backlight(dev);
  +  u8 lpbc;
  +
  +  lpbc = level * 0xff / max;
  +  level /= lpbc;

 Hmm, I don't think this calculation is correct.  This would result
 in level of opregion over its limit.  For example, assume the level
 max = 100, so total max = 25500.  Passing level=150 here will be:

  lbpc = 150 * 0xff / 25500 = 1.5 = 1
  level = 150 / 1 = 150, which is over limit.

 More worse, lbpc can be zero when level is below 100 in the case
 above...

 That is, Chris' original code in that portion was correct:

   if (is_backlight_combination_mode(dev)){
   u32 max = intel_panel_get_max_backlight(dev);
   u8 lpbc;

   lpbc = level * 0xfe / max + 1;

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-10 Thread Takashi Iwai 
At Thu, 10 Mar 2011 09:45:18 +0100 (CET),
Indan Zupancic wrote:
 
 On Thu, March 10, 2011 08:49, Takashi Iwai wrote:
  At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
  Indan Zupancic wrote:
 
  Hello,
 
  On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
   Alex, can you confirm that the revert of 951f3512dba5 plus the
   one-liner patch from Takashi that Indan quoted also works for you?
  
 Linus
  
   On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
  
   So please revert my patch and apply Takashi Iwai's, which fixes the
   most immediate bug without changing anything else. This should go
   in stable too.
  
 
  I found another backlight bug:
 
  When suspending intel_panel_disable_backlight() is never called,
  but intel_panel_enable_backlight() is called at resume. With the
  effect that if the brightness was ever changed after screen
  blanking, the wrong brightness gets restored.
 
  This explains the weird behaviour I've seen. I didn't see it with
  combination mode, because then the brightness is always the same
  (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
  send a patch in a moment.
 
  Alternative for reverting the combination mode removal (I can also
  redo the patch against the revert and Takashi's patch, if that's
  preferred):
 
  --
 
  drm/i915: Do handle backlight combination mode specially
 
  Add back the combination mode check, but with slightly cleaner code
  and the weirdness removed: No val = 1, but also no val = ~1. The
  old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
  The other change is clearer calculations: Just check for zero level
  explicitly instead of avoiding the divide-by-zero.
 
  Signed-off-by: Indan Zupancic in...@nul.nu
 
  ---
 
  diff --git a/drivers/gpu/drm/i915/intel_panel.c 
  b/drivers/gpu/drm/i915/intel_panel.c
  index d860abe..b05631a 100644
  --- a/drivers/gpu/drm/i915/intel_panel.c
  +++ b/drivers/gpu/drm/i915/intel_panel.c
  @@ -30,6 +30,10 @@
 
   #include intel_drv.h
 
  +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
  +#define BLM_COMBINATION_MODE (1  30)
  +#define BLM_LEGACY_MODE (1  16)
  +
   void
   intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
struct drm_display_mode *adjusted_mode)
  @@ -110,6 +114,22 @@ done:
 dev_priv-pch_pf_size = (width  16) | height;
   }
 
  +/*
  + * What about gen 3? If there are no gen 3 systems with ASLE,
  + * then it doesn't matter, as we don't need to change the
  + * brightness. But then the gen 2 check can be removed too.
  + */
  +static int is_backlight_combination_mode(struct drm_device *dev)
  +{
  +  struct drm_i915_private *dev_priv = dev-dev_private;
  +
  +  if (INTEL_INFO(dev)-gen = 4)
  +  return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
  +  if (IS_GEN2(dev))
  +  return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
  +  return 0;
  +}
  +
   static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
   {
 u32 val;
  @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
  *dev)
 max = 17;
 } else {
 max = 16;
  +  /* Ignore BLM_LEGACY_MODE bit */
 if (INTEL_INFO(dev)-gen  4)
 max = ~1;
 }
  +  if (is_backlight_combination_mode(dev))
  +  max *= 0xff;
 }
 
 DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
  @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
 if (IS_PINEVIEW(dev))
 val = 1;
  +  if (is_backlight_combination_mode(dev)){
  +  u8 lbpc;
  +
  +  pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
  +  val *= lbpc;
  +  }
 }
 
 DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
  @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device 
  *dev, u32 level)
 
 if (HAS_PCH_SPLIT(dev))
 return intel_pch_panel_set_backlight(dev, level);
  +
  +  if (level  is_backlight_combination_mode(dev)){
  +  u32 max = intel_panel_get_max_backlight(dev);
  +  u8 lpbc;
  +
  +  lpbc = level * 0xff / max;
  +  level /= lpbc;
 
  Hmm, I don't think this calculation is correct.  This would result
  in level of opregion over its limit.  For example, assume the level
  max = 100, so total max = 25500.  Passing level=150 here will be:
 
  lbpc = 150 * 0xff / 25500 = 1.5 = 1
  level = 150 / 1 = 150, which is over limit.
 
  More worse, lbpc can be zero when level is below 100 in the case
  above...
 
 Yes, you're right. It seems that any simplification I try to do
 creates a new bug.
 
 Do you have any bright idea why the old code did val = ~1; too?
 It seems obvious it's related to val = 1, but...

I guess it's for the case

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-10 Thread Takashi Iwai 
At Thu, 10 Mar 2011 11:06:28 +0100 (CET),
Indan Zupancic wrote:
 
 On Thu, March 10, 2011 09:25, Takashi Iwai wrote:
  At Thu, 10 Mar 2011 08:49:37 +0100,
  Takashi Iwai wrote:
 
  At Thu, 10 Mar 2011 06:50:09 +0100 (CET),
  Indan Zupancic wrote:
  
   Hello,
  
   On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
Alex, can you confirm that the revert of 951f3512dba5 plus the
one-liner patch from Takashi that Indan quoted also works for you?
   
  Linus
   
On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:
   
So please revert my patch and apply Takashi Iwai's, which fixes the
most immediate bug without changing anything else. This should go
in stable too.
   
  
   I found another backlight bug:
  
   When suspending intel_panel_disable_backlight() is never called,
   but intel_panel_enable_backlight() is called at resume. With the
   effect that if the brightness was ever changed after screen
   blanking, the wrong brightness gets restored.
  
   This explains the weird behaviour I've seen. I didn't see it with
   combination mode, because then the brightness is always the same
   (zero or the maximum, the BIOS only uses LBPC on my system.) I'll
   send a patch in a moment.
  
   Alternative for reverting the combination mode removal (I can also
   redo the patch against the revert and Takashi's patch, if that's
   preferred):
  
   --
  
   drm/i915: Do handle backlight combination mode specially
  
   Add back the combination mode check, but with slightly cleaner code
   and the weirdness removed: No val = 1, but also no val = ~1. The
   old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
   The other change is clearer calculations: Just check for zero level
   explicitly instead of avoiding the divide-by-zero.
  
   Signed-off-by: Indan Zupancic in...@nul.nu
  
   ---
  
   diff --git a/drivers/gpu/drm/i915/intel_panel.c 
   b/drivers/gpu/drm/i915/intel_panel.c
   index d860abe..b05631a 100644
   --- a/drivers/gpu/drm/i915/intel_panel.c
   +++ b/drivers/gpu/drm/i915/intel_panel.c
   @@ -30,6 +30,10 @@
  
#include intel_drv.h
  
   +#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
   +#define BLM_COMBINATION_MODE (1  30)
   +#define BLM_LEGACY_MODE (1  16)
   +
void
intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
   struct drm_display_mode *adjusted_mode)
   @@ -110,6 +114,22 @@ done:
dev_priv-pch_pf_size = (width  16) | height;
}
  
   +/*
   + * What about gen 3? If there are no gen 3 systems with ASLE,
   + * then it doesn't matter, as we don't need to change the
   + * brightness. But then the gen 2 check can be removed too.
   + */
   +static int is_backlight_combination_mode(struct drm_device *dev)
   +{
   +struct drm_i915_private *dev_priv = dev-dev_private;
   +
   +if (INTEL_INFO(dev)-gen = 4)
   +return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
   +if (IS_GEN2(dev))
   +return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
   +return 0;
   +}
   +
static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
{
u32 val;
   @@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device 
   *dev)
max = 17;
} else {
max = 16;
   +/* Ignore BLM_LEGACY_MODE bit */
if (INTEL_INFO(dev)-gen  4)
max = ~1;
}
   +if (is_backlight_combination_mode(dev))
   +max *= 0xff;
}
  
DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
   @@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device 
   *dev)
val = I915_READ(BLC_PWM_CTL)  
   BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val = 1;
   +if (is_backlight_combination_mode(dev)){
   +u8 lbpc;
   +
   +pci_read_config_byte(dev-pdev, PCI_LBPC, 
   lbpc);
   +val *= lbpc;
   +}
}
  
DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
   @@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device 
   *dev, u32 level)
  
if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
   +
   +if (level  is_backlight_combination_mode(dev)){
   +u32 max = intel_panel_get_max_backlight(dev);
   +u8 lpbc;
   +
   +lpbc = level * 0xff / max;
   +level /= lpbc;
 
  Hmm, I don't think this calculation is correct.  This would result
  in level of opregion over its limit.  For example, assume the level
  max = 100, so total max = 25500.  Passing

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-09 Thread Indan Zupancic 
Hello,

On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
 Alex, can you confirm that the revert of 951f3512dba5 plus the
 one-liner patch from Takashi that Indan quoted also works for you?

   Linus

 On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:

 So please revert my patch and apply Takashi Iwai's, which fixes the
 most immediate bug without changing anything else. This should go
 in stable too.


I found another backlight bug:

When suspending intel_panel_disable_backlight() is never called,
but intel_panel_enable_backlight() is called at resume. With the
effect that if the brightness was ever changed after screen
blanking, the wrong brightness gets restored.

This explains the weird behaviour I've seen. I didn't see it with
combination mode, because then the brightness is always the same
(zero or the maximum, the BIOS only uses LBPC on my system.) I'll
send a patch in a moment.

Alternative for reverting the combination mode removal (I can also
redo the patch against the revert and Takashi's patch, if that's
preferred):

--

drm/i915: Do handle backlight combination mode specially

Add back the combination mode check, but with slightly cleaner code
and the weirdness removed: No val = 1, but also no val = ~1. The
old code probably confused bit 0 with BLM_LEGACY_MODE, which is bit 16.
The other change is clearer calculations: Just check for zero level
explicitly instead of avoiding the divide-by-zero.

Signed-off-by: Indan Zupancic in...@nul.nu

---

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index d860abe..b05631a 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -30,6 +30,10 @@

 #include intel_drv.h

+#define PCI_LBPC 0xf4 /* legacy/combination backlight modes */
+#define BLM_COMBINATION_MODE (1  30)
+#define BLM_LEGACY_MODE (1  16)
+
 void
 intel_fixed_panel_mode(struct drm_display_mode *fixed_mode,
   struct drm_display_mode *adjusted_mode)
@@ -110,6 +114,22 @@ done:
dev_priv-pch_pf_size = (width  16) | height;
 }

+/*
+ * What about gen 3? If there are no gen 3 systems with ASLE,
+ * then it doesn't matter, as we don't need to change the
+ * brightness. But then the gen 2 check can be removed too.
+ */
+static int is_backlight_combination_mode(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = dev-dev_private;
+
+   if (INTEL_INFO(dev)-gen = 4)
+   return I915_READ(BLC_PWM_CTL2)  BLM_COMBINATION_MODE;
+   if (IS_GEN2(dev))
+   return I915_READ(BLC_PWM_CTL)  BLM_LEGACY_MODE;
+   return 0;
+}
+
 static u32 i915_read_blc_pwm_ctl(struct drm_i915_private *dev_priv)
 {
u32 val;
@@ -163,9 +183,12 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
max = 17;
} else {
max = 16;
+   /* Ignore BLM_LEGACY_MODE bit */
if (INTEL_INFO(dev)-gen  4)
max = ~1;
}
+   if (is_backlight_combination_mode(dev))
+   max *= 0xff;
}

DRM_DEBUG_DRIVER(max backlight PWM = %d\n, max);
@@ -183,6 +206,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
val = I915_READ(BLC_PWM_CTL)  BACKLIGHT_DUTY_CYCLE_MASK;
if (IS_PINEVIEW(dev))
val = 1;
+   if (is_backlight_combination_mode(dev)){
+   u8 lbpc;
+
+   pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
+   val *= lbpc;
+   }
}

DRM_DEBUG_DRIVER(get backlight PWM = %d\n, val);
@@ -205,6 +234,15 @@ void intel_panel_set_backlight(struct drm_device *dev, u32 
level)

if (HAS_PCH_SPLIT(dev))
return intel_pch_panel_set_backlight(dev, level);
+
+   if (level  is_backlight_combination_mode(dev)){
+   u32 max = intel_panel_get_max_backlight(dev);
+   u8 lpbc;
+
+   lpbc = level * 0xff / max;
+   level /= lpbc;
+   pci_write_config_byte(dev-pdev, PCI_LBPC, lpbc);
+   }
tmp = I915_READ(BLC_PWM_CTL);
if (IS_PINEVIEW(dev)) {
tmp = ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-06 Thread Alex Riesen 
On Fri, Mar 4, 2011 at 19:47, Linus Torvalds
torva...@linux-foundation.org wrote:
 Alex, can you confirm that the revert of 951f3512dba5 plus the
 one-liner patch from Takashi that Indan quoted also works for you?

I afraid mine is a different case, because backlight in this system
works properly even with Indan's patch reverted.

Sorry for the late reply...

 On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:

 So please revert my patch and apply Takashi Iwai's, which fixes the
 most immediate bug without changing anything else. This should go
 in stable too.


  Linus

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-04 Thread Linus Torvalds 
Alex, can you confirm that the revert of 951f3512dba5 plus the
one-liner patch from Takashi that Indan quoted also works for you?

  Linus

On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:

 So please revert my patch and apply Takashi Iwai's, which fixes the
 most immediate bug without changing anything else. This should go
 in stable too.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-04 Thread Indan Zupancic 
On Fri, March 4, 2011 19:47, Linus Torvalds wrote:
 Alex, can you confirm that the revert of 951f3512dba5 plus the
 one-liner patch from Takashi that Indan quoted also works for you?

   Linus

 On Thu, Mar 3, 2011 at 10:53 PM, Indan Zupancic in...@nul.nu wrote:

 So please revert my patch and apply Takashi Iwai's, which fixes the
 most immediate bug without changing anything else. This should go
 in stable too.


For what it's worth, doing the above does prevent the regression for the
ASLE case, but for me with gen 2 hardware the brightness isn't quite
right after suspend/resume, while it is with my patch applied. So
assuming that there are no gen 2 systems with ASLE out there, the best
solution may be the remove the combination mode check for gen 2 hardware
and leave it for gen =4. Would be nice if Jesse or Chris could confirm
that there are no gen 2 ASLE systems out there though.

I'm going camping, I'll send a patch next week.

Greetings,

Indan



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-04 Thread Peter Stuge 
Indan Zupancic wrote:
 What I don't understand is how BIOS makers could know about those bits.

They have relationships with Intel since 30 years, ie. they get what
they need in one form or other.


//Peter
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-03-03 Thread Indan Zupancic 
Hello,

On Wed, February 23, 2011 02:09, Linus Torvalds wrote:
 On Tue, Feb 22, 2011 at 2:31 PM, Tino Keitel tino.kei...@tikei.de wrote:

 I just tried 2.6.38-rc6 on my ThinkPad X61s without any other DRM
 related patches, and my backlight issue is gone.

 I applied Indan's fix in -rc6 (commit 951f3512dba5), since it had
 several testers and seemed to simplify the code nicely too.

Sadly, as so often in life, it's not correct. At this point I'm not sure
if it's better to revert that patch and add a correct one, or to just
fix it up. The end result is the same I suppose. I've also found more
documentation, namely: ACPI_IGD_OpRegion_Spec.pdf, which has the ASLE
stuff in intel_opregion.c, and VOL_1_graphics_core.pdf, which mentions
LBPC (I was looking at 3 before). Apparently the undocumented stuff
the old code did was correct. What I don't understand is how BIOS
makers could know about those bits.

The good side is that that big warning in my patch description is
invalid, something else was going on: The BIOS used LBPC to set and
restore brightness, while the driver only used BLC_PWM_CTL after my
patch.

All credits to Intel for making something simple as backlight control
as stupid and complex as possible:

- It has two registers to control brightness, sometimes one is used,
sometimes the other, sometimes both, and it's unknown what the BIOS
uses, and it's undefined what registers are restored by the BIOS after
reboot/resume.

- When using ACPI and ASLE, the kernel requests a brightness change
via a standard ACPI method, which in turns lets the BIOS generate an
ASLE interrupt, which is handled by the driver. The brightness to set
is between 0 and 255, and the driver is supposed to store the current
brightness in another register. That register stores the brightness in
percentages, which is used by the BIOS to restore brightness. How it
does that is undefined, so it can use either register. So the BIOS
obviously knows how to change the brightness, and it's still seemed
like a good idea to bother the driver with it. The ASLE interface is
a mess.

All in all, after my patch, systems using ASLE and a BIOS storing
the brightness in LBPC stopped working. The reason it works without
ASLE is because the brightness is never changed by the driver, the
backlight is only enabled or disabled.

I'd love to clean up the whole backlight mess, but it's too late in
the RC cycle to do that.

So please revert my patch and apply Takashi Iwai's, which fixes the
most immediate bug without changing anything else. This should go
in stable too.

From f6b8a45b9544072e6ddbb944a4c03a9ec8cbca3a Mon Sep 17 00:00:00 2001 From: 
Takashi
Iwai ti...@suse.de
Date: Mon, 21 Feb 2011 14:19:27 +0100
Subject: [PATCH] drm/i915: Fix calculation of backlight value in combined mode

The commit a95735569312f2ab0c80425e2cd1e5cb0b4e1870
drm/i915: Refactor panel backlight controls
causes a regression for GM45 that is using the combined mode for
controlling the backlight brightness.  The commit introduced a wrong bit shift 
for
computing the current backlight level.

Bugzilla: https://bugzilla.novell.com/show_bug.cgi?id=672946
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=34524

Signed-off-by: Takashi Iwai ti...@suse.de
Cc: sta...@kernel.org
---
 drivers/gpu/drm/i915/intel_panel.c |1 -
 1 file changed, 1 deletion(-)

--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -176,7 +176,6 @@
val = ~1;
pci_read_config_byte(dev-pdev, PCI_LBPC, lbpc);
val *= lbpc;
-   val = 1;
}
}


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-02-19 Thread Alex Riesen 
Sorry for this late answer. I only get a very little time for this.

On Fri, Feb 18, 2011 at 05:57, Indan Zupancic in...@nul.nu wrote:
 On Thu, February 17, 2011 23:13, Tino Keitel wrote:
 with kernel 2.6.37, the display brightness of my ThinkPad X61s was
 always reduced after lid open, resume from suspend etc.  With this
 patch on top of 2.6.38-rc5, the problem is gone.  Thanks.

 Tino, I think Alex's patch only hides the problem and doesn't properly solve

Could well be. I don't understand what the code is supposed to do.
The patch was created just be looking at diffs.

 the real bug. Can you confirm that this is the bit that fixes it for you?

 diff --git a/drivers/gpu/drm/i915/intel_panel.c 
 b/drivers/gpu/drm/i915/intel_panel.c
 index c65992d..c4b1ca4 100644
 --- a/drivers/gpu/drm/i915/intel_panel.c
 +++ b/drivers/gpu/drm/i915/intel_panel.c
 @@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
  {
        struct drm_i915_private *dev_priv = dev-dev_private;

 +       if (dev_priv-backlight_enabled)
 +               return;
 +
        if (dev_priv-backlight_level == 0)
                dev_priv-backlight_level = intel_panel_get_max_backlight(dev);

 (Alex's patch edited by hand, offsets might be wrong.)

It is not enough, at least for me.

 The other bits either don't change the logic, or should be harmless, or are
 plain wrong, like setting the brightness to maximum at bootup.

I am not absolutely sure, but I don't think this is what happens on this laptop.

 Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 
 fixes
 it for you too? (Make sure you're at max brightness before rebooting.)

I'll try it now.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-02-19 Thread Alex Riesen 
On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.l...@gmail.com wrote:
 Lastly, could you verify that my patch at 
 https://lkml.org/lkml/2011/2/16/447 fixes
 it for you too? (Make sure you're at max brightness before rebooting.)

 I'll try it now.


I can confirm that it does fix backlight in my case (Dell XPS 1330,
LVDS panel, GM965/GL960).

Tested-by: Alex Riesen raa.l...@gmail.com
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-02-19 Thread Linus Torvalds 
On Sat, Feb 19, 2011 at 4:26 AM, Alex Riesen raa.l...@gmail.com wrote:
 On Sat, Feb 19, 2011 at 13:11, Alex Riesen raa.l...@gmail.com wrote:
 Lastly, could you verify that my patch at 
 https://lkml.org/lkml/2011/2/16/447 fixes
 it for you too? (Make sure you're at max brightness before rebooting.)

 I'll try it now.


 I can confirm that it does fix backlight in my case (Dell XPS 1330,
 LVDS panel, GM965/GL960).

 Tested-by: Alex Riesen raa.l...@gmail.com

Guys, should I apply this, or will I get it through somebody's pull?

  Linus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-02-17 Thread Tino Keitel 
On Wed, Feb 16, 2011 at 20:26:58 +0100, Alex Riesen wrote:
 Signed-off-by: Alex Riesen raa.l...@gmail.com
 
 ---
 Linus Torvalds, Wed, Feb 16, 2011 05:16:01 +0100:
  Most of the changes are pretty spread out and small, with drivers/gpu
  (radeon and i915) somewhat standing out from the pack. ... 
 
 The backlight level on this Dell XPS M1330 reduces every time I reopen the
 lid, and BIOS does not seem to know anything about that (the keyboard
 shortcuts to set backlight brightness cause it to jump to the level next to
 the one set before closing the lid).

Hi,

with kernel 2.6.37, the display brightness of my ThinkPad X61s was
always reduced after lid open, resume from suspend etc.  With this
patch on top of 2.6.38-rc5, the problem is gone.  Thanks.

Regards,
Tino

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] fix backlight brightness on intel LVDS panel after reopening lid

2011-02-17 Thread Indan Zupancic 
On Thu, February 17, 2011 23:13, Tino Keitel wrote:
 with kernel 2.6.37, the display brightness of my ThinkPad X61s was
 always reduced after lid open, resume from suspend etc.  With this
 patch on top of 2.6.38-rc5, the problem is gone.  Thanks.

Tino, I think Alex's patch only hides the problem and doesn't properly solve
the real bug. Can you confirm that this is the bit that fixes it for you?

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index c65992d..c4b1ca4 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -267,6 +235,9 @@ void intel_panel_enable_backlight(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev-dev_private;

+   if (dev_priv-backlight_enabled)
+   return;
+
if (dev_priv-backlight_level == 0)
dev_priv-backlight_level = intel_panel_get_max_backlight(dev);

(Alex's patch edited by hand, offsets might be wrong.)

The other bits either don't change the logic, or should be harmless, or are
plain wrong, like setting the brightness to maximum at bootup.

If the above bit fixes it then it's because intel_panel_set_backlight() is 
called
less often, as that's the buggy function the problem doesn't show up (or is less
clear). Calling intel_panel_set_backlight() with the same value should keep the
brightness the same. Because of the buggy combination code it doesn't always.

Also, try suspending/resuming or xset dpms force off/on often in a loop with 
both
highest and lowest brightness and check if it works correctly with just Alex's 
patch.

Lastly, could you verify that my patch at https://lkml.org/lkml/2011/2/16/447 
fixes
it for you too? (Make sure you're at max brightness before rebooting.)

That said, the above bit of Alex's patch should be fine to apply, because it 
avoids
unnecessary register fiddling either way.

Greetings,

Indan


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel