properly enable the blc controller on the right pipe
diff mbox

Message ID 50088F7D.1020008@osadl.org
State New, archived
Headers show

Commit Message

Carsten Emde July 19, 2012, 10:51 p.m. UTC
On 07/19/2012 04:40 PM, Daniel Vetter wrote:
> On Thu, Jul 19, 2012 at 4:00 PM, Carsten Emde<C.Emde@osadl.org>  wrote:
>> On 06/11/2012 10:51 AM, Daniel Vetter wrote:
>>> I've updated my backlight-confusion branch with new patches for gen4
>>> (it doesn't seem to work for these machines yet). Can you please test
>>> the updated branch?
>>
>> I have added an additional test rack (#6) to our QA farm (osadl.org/QA) and
>> populated this rack solely with notebooks that are equipped with Intel
>> graphics board:
>>
>> Slot #1: Lenovo T61/7661W4G, Intel GMA X3100
>> Slot #2: IBM T601951-A47, Intel GMA 950
>> Slot #4: Acer Homa/Extensa 5230E-901G16N, Intel GMA 4500MHD
>> Slot #5: Acer Morar/TravelMate 2410, Intel GMA 900
>> Slot #6: Dell Inspirion 1300/0RJ272, Intel GMA 900
>> Slot #7: Acer Aspire 5734Z, Intel GMA 4500M
>>
>> With the exception of slot #4 that runs a 3.2-based kernel, all other
>> notebooks run a 3.4-based kernel. The notebook in Slot #7 is the one with
>> the backlight headache. Unfortunately :-; no other notebook had any
>> backlight problem when running a vanilla kernel, even the two new Acers are
>> behaving well.
>>
>> In the meantime, however, I have received a report that the Acer 5732Z has
>> the same problem. I am sure that the reporter will be willing to help with
>> testing. Looks like the backlight problem is very specific to Acer Aspire
>> 573xZ. But I agree that a generic solution always is better than a quirk.
>>
>> I've checked out the backlight-confusion branch of your git tree and tested
>> it with and without the Acer quirk. I can report that the backlight works
>> correctly with either version. I will now contact the owner of the Acer
>> Aspire 5732Z and ask him to test the backlight-confusion branch without the
>> Acer quirk on his notebook as well.
>>
>> Do you wish me to do any other test?
>
> Well, the backlight-confusion branch had a bug and outright disabled
> the intel backlight :( drm-intel-next-queued should have fixes for all
> the backlight goofus I've accidentally created, so testing feedback on
> that branch would be highly appreciated.
This branch is now broken again. The original drm-intel-next-queued 
branch works well (it still has the quirk), but after I reverted the 
Acer quirk, the screen remains dark.

I double-checked it and used exactly the same patch to revert the quirk 
in the two trees: The backlight-confusion branch works, and the 
drm-intel-next-queued branch does not.

I then used drivers/gpu/drm/i915/intel_panel.c from backlight-confusion 
in drm-intel-next-queued and - it worked again. When testing the two 
differences separately it turned out this patch did the magic:


Hope this helps,

	-Carsten.

Comments

Daniel Vetter July 20, 2012, 8:10 a.m. UTC | #1
On Fri, Jul 20, 2012 at 12:51:41AM +0200, Carsten Emde wrote:
> On 07/19/2012 04:40 PM, Daniel Vetter wrote:
> >Well, the backlight-confusion branch had a bug and outright disabled
> >the intel backlight :( drm-intel-next-queued should have fixes for all
> >the backlight goofus I've accidentally created, so testing feedback on
> >that branch would be highly appreciated.
> This branch is now broken again. The original drm-intel-next-queued
> branch works well (it still has the quirk), but after I reverted the
> Acer quirk, the screen remains dark.
> 
> I double-checked it and used exactly the same patch to revert the
> quirk in the two trees: The backlight-confusion branch works, and
> the drm-intel-next-queued branch does not.
> 
> I then used drivers/gpu/drm/i915/intel_panel.c from
> backlight-confusion in drm-intel-next-queued and - it worked again.
> When testing the two differences separately it turned out this patch
> did the magic:

Yeah, I guess your system only worked whith the old code because it
accidentally disabled the entire backlight stuff. Now that that doesn't
happen any more, it's as broken as ever.

I'll look into creating another patch, afaict from digging through git
history, this backlight combination mode is totally bogus. But if we kill
it, a few machines will resume with dimmed backlight, because we don't
restore the lbpc register properly.

Thanks for testing, Daniel

> 
> Index: drm-intel/drivers/gpu/drm/i915/intel_panel.c
> ===================================================================
> --- drm-intel.orig/drivers/gpu/drm/i915/intel_panel.c
> +++ drm-intel/drivers/gpu/drm/i915/intel_panel.c
> @@ -121,6 +121,8 @@ static int is_backlight_combination_mode
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> +	return 0;
> +
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> 
> Finally, only the first condition is obviously harmful - this patch
> also works well:
> 
> Index: drm-intel/drivers/gpu/drm/i915/intel_panel.c
> ===================================================================
> --- drm-intel.orig/drivers/gpu/drm/i915/intel_panel.c
> +++ drm-intel/drivers/gpu/drm/i915/intel_panel.c
> @@ -121,8 +121,10 @@ static int is_backlight_combination_mode
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> +#if 0
>  	if (INTEL_INFO(dev)->gen >= 4)
>  		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
> +#endif
> 
>  	if (IS_GEN2(dev))
>  		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;
> 
> Hope this helps,
> 
> 	-Carsten.
Carsten Emde July 24, 2012, 7:30 a.m. UTC | #2
On 07/20/2012 10:10 AM, Daniel Vetter wrote:
> On Fri, Jul 20, 2012 at 12:51:41AM +0200, Carsten Emde wrote:
>> On 07/19/2012 04:40 PM, Daniel Vetter wrote:
>>> Well, the backlight-confusion branch had a bug and outright disabled
>>> the intel backlight :( drm-intel-next-queued should have fixes for all
>>> the backlight goofus I've accidentally created, so testing feedback on
>>> that branch would be highly appreciated.
>> This branch is now broken again. The original drm-intel-next-queued
>> branch works well (it still has the quirk), but after I reverted the
>> Acer quirk, the screen remains dark.
>>
>> I double-checked it and used exactly the same patch to revert the
>> quirk in the two trees: The backlight-confusion branch works, and
>> the drm-intel-next-queued branch does not.
>>
>> I then used drivers/gpu/drm/i915/intel_panel.c from
>> backlight-confusion in drm-intel-next-queued and - it worked again.
>> When testing the two differences separately it turned out this patch
>> did the magic:
>
> Yeah, I guess your system only worked whith the old code because it
> accidentally disabled the entire backlight stuff. Now that that doesn't
> happen any more, it's as broken as ever.
>
> I'll look into creating another patch, afaict from digging through git
> history, this backlight combination mode is totally bogus. But if we kill
> it, a few machines will resume with dimmed backlight, because we don't
> restore the lbpc register properly.
After all that hassle and bearing in mind that we found only two 
notebooks so far, Acer Aspire 5732Z and 5734Z, that suffer from this 
backlight inversion problem, shouldn't we simply return to the quirk 
solution? I would quickly add the data of the other Acer to the 
backlight quirks, send a rebased patch, and we are all set.

	-Carsten.

Patch
diff mbox

Index: drm-intel/drivers/gpu/drm/i915/intel_panel.c
===================================================================
--- drm-intel.orig/drivers/gpu/drm/i915/intel_panel.c
+++ drm-intel/drivers/gpu/drm/i915/intel_panel.c
@@ -121,6 +121,8 @@  static int is_backlight_combination_mode
  {
  	struct drm_i915_private *dev_priv = dev->dev_private;

+	return 0;
+
  	if (INTEL_INFO(dev)->gen >= 4)
  		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;

Finally, only the first condition is obviously harmful - this patch also 
works well:

Index: drm-intel/drivers/gpu/drm/i915/intel_panel.c
===================================================================
--- drm-intel.orig/drivers/gpu/drm/i915/intel_panel.c
+++ drm-intel/drivers/gpu/drm/i915/intel_panel.c
@@ -121,8 +121,10 @@  static int is_backlight_combination_mode
  {
  	struct drm_i915_private *dev_priv = dev->dev_private;

+#if 0
  	if (INTEL_INFO(dev)->gen >= 4)
  		return I915_READ(BLC_PWM_CTL2) & BLM_COMBINATION_MODE;
+#endif

  	if (IS_GEN2(dev))
  		return I915_READ(BLC_PWM_CTL) & BLM_LEGACY_MODE;