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

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

[Takashi Iwai]

While refactoring of backlight control code in commit [a95735569:
drm/i915: Refactor panel backlight controls], the handling of the bit
0 of duty-cycle was gone except for pineview.  This resulted in invalid
register values for old chips like 915GM.  When the bit 0 is set, the
backlight is turned off suddenly.

This patch changes the bit-0 check by replacing with the condition of
gen < 4 (pineview is included in this condition, too).

Reported-and-tested-by: Daniel Mack <zonque@gmail.com>
Cc: <stable@kernel.org>
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 drivers/gpu/drm/i915/intel_panel.c |    8 +++-----
 1 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 499d4c0..737d00f 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -178,12 +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 {
 			max >>= 16;
-			if (INTEL_INFO(dev)->gen < 4)
-				max &= ~1;
 		}
 
 		if (is_backlight_combination_mode(dev))
@@ -203,7 +201,7 @@ 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)) {
@@ -246,7 +244,7 @@ static void intel_panel_actually_set_backlight(struct drm_device *dev, u32 level
 	}
 
 	tmp = I915_READ(BLC_PWM_CTL);
-	if (IS_PINEVIEW(dev)) {
+	if (INTEL_INFO(dev)->gen < 4) {
 		tmp &= ~(BACKLIGHT_DUTY_CYCLE_MASK - 1);
 		level <<= 1;
 	} else
-- 
1.7.7

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

[Keith Packard]

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

On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:

> While refactoring of backlight control code in commit [a95735569:
> drm/i915: Refactor panel backlight controls], the handling of the bit
> 0 of duty-cycle was gone except for pineview.  This resulted in invalid
> register values for old chips like 915GM.  When the bit 0 is set, the
> backlight is turned off suddenly.

I'm looking at the mentioned patch and I don't see how that managed to
correctly handle bit 0; is this the patch that managed to break this?

-- 
keith.packard@intel.com

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

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

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 579 bytes --]

On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:

> While refactoring of backlight control code in commit [a95735569:
> drm/i915: Refactor panel backlight controls], the handling of the bit
> 0 of duty-cycle was gone except for pineview.  This resulted in invalid
> register values for old chips like 915GM.  When the bit 0 is set, the
> backlight is turned off suddenly.

I'm looking at the mentioned patch and I don't see how that managed to
correctly handle bit 0; is this the patch that managed to break this?

-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Takashi Iwai]

At Wed, 16 Nov 2011 22:15:41 -0800,
Keith Packard wrote:
> 
> On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > While refactoring of backlight control code in commit [a95735569:
> > drm/i915: Refactor panel backlight controls], the handling of the bit
> > 0 of duty-cycle was gone except for pineview.  This resulted in invalid
> > register values for old chips like 915GM.  When the bit 0 is set, the
> > backlight is turned off suddenly.
> 
> I'm looking at the mentioned patch and I don't see how that managed to
> correctly handle bit 0; is this the patch that managed to break this?

Hrm, then I must have been confused.  The gen < 4 check (formerly
!IS_I965G()) was firstly introduced in this refactoring.  I thought
this was done before, but apparently not.

Looking more carefully again, actually the IS_PINEVIEW() part (former
IS_GND()) was introduced in the commit [078a033f: drm/i915: fix
opregion backlight chip detect and range].  Thus the same bug must
have been present even in the earlier version, but didn't come up by
some reason.

So, I'll have to correct the patch commit log.  Sorry for that.

Now, a question is whether the condition (gen < 4) is really correct.
I *guess* the original code in the refactoring was intended to cover
it:
	if (!IS_I965G(dev))
		max &= ~1;

But this rule wasn't applied to set_backlight().
The bit-0-clear above implies that it's a similar behavior like
pineview.  And indeed Daniel's machine with 915GM hits this problem.
If the above rule should be applied to all places, it'd make more
sense to handle both in the same way like pineview.

Unfortunately I have no old machines to test this, and the only
material to judge was Daniel's case.  (Is Harald's machine also
915GM?)

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


thanks,

Takashi

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

[Daniel Mack]

On Thu, Nov 17, 2011 at 5:33 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Wed, 16 Nov 2011 22:15:41 -0800,
> Keith Packard wrote:
>>
>> On Wed, 16 Nov 2011 18:14:55 +0100, Takashi Iwai <tiwai@suse.de> wrote:
>>
>> > While refactoring of backlight control code in commit [a95735569:
>> > drm/i915: Refactor panel backlight controls], the handling of the bit
>> > 0 of duty-cycle was gone except for pineview.  This resulted in invalid
>> > register values for old chips like 915GM.  When the bit 0 is set, the
>> > backlight is turned off suddenly.
>>
>> I'm looking at the mentioned patch and I don't see how that managed to
>> correctly handle bit 0; is this the patch that managed to break this?
>
> Hrm, then I must have been confused.  The gen < 4 check (formerly
> !IS_I965G()) was firstly introduced in this refactoring.  I thought
> this was done before, but apparently not.
>
> Looking more carefully again, actually the IS_PINEVIEW() part (former
> IS_GND()) was introduced in the commit [078a033f: drm/i915: fix
> opregion backlight chip detect and range].  Thus the same bug must
> have been present even in the earlier version, but didn't come up by
> some reason.
>
> So, I'll have to correct the patch commit log.  Sorry for that.
>
> Now, a question is whether the condition (gen < 4) is really correct.
> I *guess* the original code in the refactoring was intended to cover
> it:
>        if (!IS_I965G(dev))
>                max &= ~1;
>
> But this rule wasn't applied to set_backlight().
> The bit-0-clear above implies that it's a similar behavior like
> pineview.  And indeed Daniel's machine with 915GM hits this problem.
> If the above rule should be applied to all places, it'd make more
> sense to handle both in the same way like pineview.
>
> Unfortunately I have no old machines to test this, and the only
> material to judge was Daniel's case.  (Is Harald's machine also
> 915GM?)

I can test more patches if you want me to, no problem. Just let me know.


Daniel

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

[Keith Packard]

[-- 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 --]

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

[Daniel Mack]

On Fri, Nov 18, 2011 at 8:25 PM, Keith Packard <keithp@keithp.com> wrote:
> 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>

Yep, that works as well. Thanks.

Reported-and-tested-by: Daniel Mack <zonque@gmail.com>
Cc: stable@kernel.org


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

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

[Takashi Iwai]

At Fri, 18 Nov 2011 11:25:48 -0800,
Keith Packard wrote:
> 
> 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).

Thanks!  It's pretty similar with my patch in the end, so in case
needed:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>

Maybe it'd be better to mention that actually setting bit-0 caused a
blank screen on some machines.


Takashi

> 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

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

[Takashi Iwai]

At Fri, 18 Nov 2011 11:25:48 -0800,
Keith Packard wrote:
> 
> 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).

Thanks!  It's pretty similar with my patch in the end, so in case
needed:
  Reviewed-by: Takashi Iwai <tiwai@suse.de>

Maybe it'd be better to mention that actually setting bit-0 caused a
blank screen on some machines.


Takashi

> 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

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

[Keith Packard]

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

On Sat, 19 Nov 2011 11:05:05 +0100, Takashi Iwai <tiwai@suse.de> wrote:

> Maybe it'd be better to mention that actually setting bit-0 caused a
> blank screen on some machines.

Was that caused by *just* setting bit zero? Or was it caused by setting
the duty cycle to 0xffff, in which case it would be larger than the
maximum value?

I'll clean up the commit log message with your answer and then push this out.

-- 
keith.packard@intel.com

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

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

[Takashi Iwai]

At Sat, 19 Nov 2011 10:34:12 -0800,
Keith Packard wrote:
> 
> [1  <text/plain (quoted-printable)>]
> On Sat, 19 Nov 2011 11:05:05 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Maybe it'd be better to mention that actually setting bit-0 caused a
> > blank screen on some machines.
> 
> Was that caused by *just* setting bit zero? Or was it caused by setting
> the duty cycle to 0xffff, in which case it would be larger than the
> maximum value?
> 
> I'll clean up the commit log message with your answer and then push this out.

According to Daniels' original post:

On 11/04/2011 03:36 PM, Daniel Mack wrote:
> I'm facing a bug on a Samsung X20 notebook which features an i915
> chipset (output of 'lspci -v' attached).
>
> The effect is that setting the backlight to odd values causes the value
> to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook
> (I don't recall which model it was).
>
> So this will turn the backlight to full brightness:
>
> # cat /sys/class/backlight/intel_backlight/max_brightness
> 29750
> # echo 29750 > /sys/class/backlight/intel_backlight/brightness
>
> However, writing 29749 will turn the display backlight off, and 29748
> appears to be the next valid lower value.

So, writing bit-0 caused a problem, as it seems.


Takashi

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

[Takashi Iwai]

At Sat, 19 Nov 2011 10:34:12 -0800,
Keith Packard wrote:
> 
> [1  <text/plain (quoted-printable)>]
> On Sat, 19 Nov 2011 11:05:05 +0100, Takashi Iwai <tiwai@suse.de> wrote:
> 
> > Maybe it'd be better to mention that actually setting bit-0 caused a
> > blank screen on some machines.
> 
> Was that caused by *just* setting bit zero? Or was it caused by setting
> the duty cycle to 0xffff, in which case it would be larger than the
> maximum value?
> 
> I'll clean up the commit log message with your answer and then push this out.

According to Daniels' original post:

On 11/04/2011 03:36 PM, Daniel Mack wrote:
> I'm facing a bug on a Samsung X20 notebook which features an i915
> chipset (output of 'lspci -v' attached).
>
> The effect is that setting the backlight to odd values causes the value
> to be misinterpreted. Harald Hoyer (cc:) had the same thing on a Netbook
> (I don't recall which model it was).
>
> So this will turn the backlight to full brightness:
>
> # cat /sys/class/backlight/intel_backlight/max_brightness
> 29750
> # echo 29750 > /sys/class/backlight/intel_backlight/brightness
>
> However, writing 29749 will turn the display backlight off, and 29748
> appears to be the next valid lower value.

So, writing bit-0 caused a problem, as it seems.


Takashi

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

[Keith Packard]

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

On Sun, 20 Nov 2011 12:22:50 +0100, Takashi Iwai <tiwai@suse.de> wrote:

> So, writing bit-0 caused a problem, as it seems.

Thanks. I've noted that in the patch message.

-- 
keith.packard@intel.com

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

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

[Daniel Mack]

On Mon, Nov 21, 2011 at 7:10 PM, Keith Packard <keithp@keithp.com> wrote:
> On Sun, 20 Nov 2011 12:22:50 +0100, Takashi Iwai <tiwai@suse.de> wrote:
>
>> So, writing bit-0 caused a problem, as it seems.
>
> Thanks. I've noted that in the patch message.

Just curious - is this patch queued somewhere for mainline yet?


Thanks,
Daniel

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

[Keith Packard]

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

On Tue, 22 Nov 2011 17:40:25 +0100, Daniel Mack <zonque@gmail.com> wrote:

> Just curious - is this patch queued somewhere for mainline yet?

I'm waiting for airlied to pull drm-fixes-intel before I push more stuff
there.

-- 
keith.packard@intel.com

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

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

[James]

Hello! I know this is an old thread, but i am having this problem on my macbook
air.  Ideas for a quick fix?

Thanks,

James

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

[Daniel Vetter]

On Thu, Apr 12, 2012 at 01:59:07AM +0000, James wrote:
> Hello! I know this is an old thread, but i am having this problem on my macbook
> air.  Ideas for a quick fix?

Upgrade to kernel 3.2 or newer, that has the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48