[PATCH] Revert "drm/i915: allow PCH PWM override on IVB"

[PATCH] Revert "drm/i915: allow PCH PWM override on IVB"

[Daniel Vetter]

This reverts commit f82cfb6bcda164ef3a66b8c3fc549b1f9bdd09ad.

This breaks the backlight controls on my IVB asus zenbook with an eDP
panel.

I guess the right fix would be to read this bit and use either the pch
or the cpu register to frob the backlight values. But that is stuff
for -next.

Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c |   16 ----------------
 1 file changed, 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a7c727d..a8538ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6921,19 +6921,6 @@ static void i915_disable_vga(struct drm_device *dev)
 	POSTING_READ(vga_reg);
 }
 
-static void ivb_pch_pwm_override(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	/*
-	 * IVB has CPU eDP backlight regs too, set things up to let the
-	 * PCH regs control the backlight
-	 */
-	I915_WRITE(BLC_PWM_CPU_CTL2, PWM_ENABLE);
-	I915_WRITE(BLC_PWM_CPU_CTL, 0);
-	I915_WRITE(BLC_PWM_PCH_CTL1, PWM_ENABLE | (1<<30));
-}
-
 void intel_modeset_init_hw(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -6950,9 +6937,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
 		gen6_enable_rps(dev_priv);
 		gen6_update_ring_freq(dev_priv);
 	}
-
-	if (IS_IVYBRIDGE(dev))
-		ivb_pch_pwm_override(dev);
 }
 
 void intel_modeset_init(struct drm_device *dev)
-- 
1.7.10.2

Re: [PATCH] Revert "drm/i915: allow PCH PWM override on IVB"

[Daniel Vetter]

On Wed, Jun 27, 2012 at 04:43:36PM +0200, Daniel Vetter wrote:
> This reverts commit f82cfb6bcda164ef3a66b8c3fc549b1f9bdd09ad.
> 
> This breaks the backlight controls on my IVB asus zenbook with an eDP
> panel.
> 
> I guess the right fix would be to read this bit and use either the pch
> or the cpu register to frob the backlight values. But that is stuff
> for -next.
> 
> Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Jesse acked this on irc, so I've queued this one for -fixes.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   16 ----------------
>  1 file changed, 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a7c727d..a8538ac 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6921,19 +6921,6 @@ static void i915_disable_vga(struct drm_device *dev)
>  	POSTING_READ(vga_reg);
>  }
>  
> -static void ivb_pch_pwm_override(struct drm_device *dev)
> -{
> -	struct drm_i915_private *dev_priv = dev->dev_private;
> -
> -	/*
> -	 * IVB has CPU eDP backlight regs too, set things up to let the
> -	 * PCH regs control the backlight
> -	 */
> -	I915_WRITE(BLC_PWM_CPU_CTL2, PWM_ENABLE);
> -	I915_WRITE(BLC_PWM_CPU_CTL, 0);
> -	I915_WRITE(BLC_PWM_PCH_CTL1, PWM_ENABLE | (1<<30));
> -}
> -
>  void intel_modeset_init_hw(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -6950,9 +6937,6 @@ void intel_modeset_init_hw(struct drm_device *dev)
>  		gen6_enable_rps(dev_priv);
>  		gen6_update_ring_freq(dev_priv);
>  	}
> -
> -	if (IS_IVYBRIDGE(dev))
> -		ivb_pch_pwm_override(dev);
>  }
>  
>  void intel_modeset_init(struct drm_device *dev)
> -- 
> 1.7.10.2
> 

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] Revert "drm/i915: allow PCH PWM override on IVB"

[Paulo Zanoni]

Hi

2012/6/27 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This reverts commit f82cfb6bcda164ef3a66b8c3fc549b1f9bdd09ad.
>
> This breaks the backlight controls on my IVB asus zenbook with an eDP
> panel.
>
> I guess the right fix would be to read this bit and use either the pch
> or the cpu register to frob the backlight values. But that is stuff
> for -next.
>

I was about to propose *exactly* this patch and just noticed it is
here, but for -fixes instead of dinq.

After playing with the CPU/PCH backlight registers, this is what I concluded:

 - If you want to control backlight just using the PCH:
   - You have to turn on BLM_PCH_PWM_ENABLE (PCH backlight enable)
   - You have to turn on the BLM_PCH_OVERRIDE_ENABLE bit
   - You have to use BLC_PWM_PCH_CTL2 to set the backlight value
   - Just ignore the CPU registers

 - If you want to control backlight using the CPU:
   - You have to turn on BLM_PWM_ENABLE (CPU backlight enable)
   - You have to turn on BLM_PCH_PWM_ENABLE (PCH backlight enable)
   - You have to turn *OFF* the BLM_PCH_OVERRIDE_ENABLE bit
   - You have to use BLC_PWM_CPU_CTL2 to set the backlight value

This ivb_pcm_pwm_override function turns the override bit ON, so it
completely relies on the PCH registers. The problem is that our code
only changes the CPU registers... It was probably relying on the
values set by the bios, and I would guess that changing the backlight
levels was probably not working at all (I don't have an IVB machine
with LVDS to test).

If people start complaining that your revert causes problems my
suggestion would be:
  - Undo your revert, and instead of removing the whole function, just
remove the code that turns the override bit on (1 << 30). Explicitly
turn it off.
  - Then resend the patch that kills the whole function to dinq and
apply it on top of the patch I just sent a few minutes ago. Your
rework of the backlight registers + my fix should probably make it
safe to completely remove ivb_pch_pwm.


-- 
Paulo Zanoni