Re: [PATCH] drm/i915: Fix invalid backpanel values for GEN3 or older chips

[Keith Packard <keithp@keithp.com>]

[-- Attachment #1: Type: text/plain, Size: 3838 bytes --]

On Thu, 17 Nov 2011 17:33:50 +0100, Takashi Iwai <tiwai@suse.de> wrote:

> 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...

I read through the hardware docs yesterday and figured out what was
going on. On all pre-gen4 hardware, the maximum backlight value has the
lowest bit (of 16) hard-wired to zero. This means that the possible
backlight duty cycle values are limited to 0 .. 0xfffe.

On older hardware, this was managed by reporting this true range. This
meant that the set_backlight path didn't need any special code; simply
setting the values as provided should have worked just fine.

On Pineview, this was changed (for reasons unknown) to use only 15 bits
for backlight levels. The range of possible values is then
0 .. 0x7fff. In this case, values have to be shifted by one to convert
between the advertised range of 0 .. 0x7fff and the hardware range of
0 .. 0xfffe.

Exposing the range of 0 .. 0xfffe introduces a potential problem --
write a value of 0xffff and the hardware gets mightily confused,
and probably ends up turning the backlight off. Using the range of
0 .. 0x7fff avoids this issue completely.

Using the narrower range does limit the precision of the backlight level
setting, but it seems like 32767 possible values is more than sufficient...

Here's a patch which just uses the pineview version everywhere (and
cleans that up at the same time).

From e06789f688dc7ab1331f5f15ad3d60326b5139b4 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Fri, 18 Nov 2011 11:09:24 -0800
Subject: [PATCH] drm/i915: Treat pre-gen4 backlight duty cycle value
 consistently

For i945 and earlier chips, the backlight frequency value had the low
bit (of 16) fixed to zero. The Pineview code path handled this by just
exposing the backlight range as 15 bits while other chips had the
backlight range limited to 0 .. 0xfffe.

This patch makes everyone take the pineview code path, providing 15
bits of backlight duty cycle range which seems more than sufficient to me.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_panel.c |   16 +++++-----------
 1 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 21f60b7..04d79fd 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -178,13 +178,10 @@ u32 intel_panel_get_max_backlight(struct drm_device *dev)
 	if (HAS_PCH_SPLIT(dev)) {
 		max >>= 16;
 	} else {
-		if (IS_PINEVIEW(dev)) {
+		if (INTEL_INFO(dev)->gen < 4)
 			max >>= 17;
-		} else {
+		else
 			max >>= 16;
-			if (INTEL_INFO(dev)->gen < 4)
-				max &= ~1;
-		}
 
 		if (is_backlight_combination_mode(dev))
 			max *= 0xff;
@@ -203,13 +200,12 @@ u32 intel_panel_get_backlight(struct drm_device *dev)
 		val = I915_READ(BLC_PWM_CPU_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
 	} else {
 		val = I915_READ(BLC_PWM_CTL) & BACKLIGHT_DUTY_CYCLE_MASK;
-		if (IS_PINEVIEW(dev))
+		if (INTEL_INFO(dev)->gen < 4)
 			val >>= 1;
 
 		if (is_backlight_combination_mode(dev)) {
 			u8 lbpc;
 
-			val &= ~1;
 			pci_read_config_byte(dev->pdev, PCI_LBPC, &lbpc);
 			val *= lbpc;
 		}
@@ -246,11 +242,9 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	}
 
 	tmp = I915_READ(BLC_PWM_CTL);
-	if (IS_PINEVIEW(dev)) {
-		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
+	if (INTEL_INFO(dev)->gen < 4) 
 		level <<= 1;
-	} else
-		tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
+	tmp &= ~BACKLIGHT_DUTY_CYCLE_MASK;
 	I915_WRITE(BLC_PWM_CTL, tmp | level);
 }
 
-- 
1.7.7.3



-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]