All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
@ 2014-08-19  2:07 Scot Doyle
  2014-08-19 14:29 ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Scot Doyle @ 2014-08-19  2:07 UTC (permalink / raw)
  To: intel-gfx; +Cc: Jani Nikula

BIOS or firmware can modify hardware state during suspend/resume,
for example on the Toshiba CB35 or Lenovo T400, so log a debug message
instead of a warning if the backlight is unexpectedly enabled.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80930
Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
---
 drivers/gpu/drm/i915/intel_panel.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
index 59b028f..8e37444 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -801,7 +801,7 @@ static void pch_enable_backlight(struct intel_connector *connector)

 	cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
 	if (cpu_ctl2 & BLM_PWM_ENABLE) {
-		WARN(1, "cpu backlight already enabled\n");
+		DRM_DEBUG_KMS("cpu backlight already enabled\n");
 		cpu_ctl2 &= ~BLM_PWM_ENABLE;
 		I915_WRITE(BLC_PWM_CPU_CTL2, cpu_ctl2);
 	}
@@ -845,7 +845,7 @@ static void i9xx_enable_backlight(struct intel_connector *connector)

 	ctl = I915_READ(BLC_PWM_CTL);
 	if (ctl & BACKLIGHT_DUTY_CYCLE_MASK_PNV) {
-		WARN(1, "backlight already enabled\n");
+		DRM_DEBUG_KMS("backlight already enabled\n");
 		I915_WRITE(BLC_PWM_CTL, 0);
 	}

@@ -876,7 +876,7 @@ static void i965_enable_backlight(struct intel_connector *connector)

 	ctl2 = I915_READ(BLC_PWM_CTL2);
 	if (ctl2 & BLM_PWM_ENABLE) {
-		WARN(1, "backlight already enabled\n");
+		DRM_DEBUG_KMS("backlight already enabled\n");
 		ctl2 &= ~BLM_PWM_ENABLE;
 		I915_WRITE(BLC_PWM_CTL2, ctl2);
 	}
@@ -910,7 +910,7 @@ static void vlv_enable_backlight(struct intel_connector *connector)

 	ctl2 = I915_READ(VLV_BLC_PWM_CTL2(pipe));
 	if (ctl2 & BLM_PWM_ENABLE) {
-		WARN(1, "backlight already enabled\n");
+		DRM_DEBUG_KMS("backlight already enabled\n");
 		ctl2 &= ~BLM_PWM_ENABLE;
 		I915_WRITE(VLV_BLC_PWM_CTL2(pipe), ctl2);
 	}

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-19  2:07 [PATCH] drm/i915: don't warn if backlight unexpectedly enabled Scot Doyle
@ 2014-08-19 14:29 ` Daniel Vetter
  2014-08-21  7:12   ` Scot Doyle
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2014-08-19 14:29 UTC (permalink / raw)
  To: Scot Doyle; +Cc: Jani Nikula, intel-gfx

On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle <lkml14@scotdoyle.com> wrote:
> BIOS or firmware can modify hardware state during suspend/resume,
> for example on the Toshiba CB35 or Lenovo T400, so log a debug message
> instead of a warning if the backlight is unexpectedly enabled.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80930
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>

This should get cleaned up in the modeset state sanitization we do
upon resume, so without someone digging into this bug a bit and coming
up with an explanation for why that fails I'm reluctant to merge this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-19 14:29 ` Daniel Vetter
@ 2014-08-21  7:12   ` Scot Doyle
  2014-08-26 10:45     ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Scot Doyle @ 2014-08-21  7:12 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx


On Tue, 19 Aug 2014, Daniel Vetter wrote:
> On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle <lkml14@scotdoyle.com> wrote:
>> BIOS or firmware can modify hardware state during suspend/resume,
>> for example on the Toshiba CB35 or Lenovo T400, so log a debug message
>> instead of a warning if the backlight is unexpectedly enabled.
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80930
>> Cc: Jani Nikula <jani.nikula@intel.com>
>> Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
>
> This should get cleaned up in the modeset state sanitization we do
> upon resume, so without someone digging into this bug a bit and coming
> up with an explanation for why that fails I'm reluctant to merge this.
> -Daniel

When we enter intel_modeset_setup_hw_state during resume
- BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
- the physical backlight is off
- readout_hw_state says crtcs, encoders and connectors are disabled

The sanitizations done before reaching the force_restore block are
- drm_vblank_off for each crtc
- sarea_priv->pipeA_w=0
- sarea_priv->pipeA_h=0
- ilk_wm_get_hw_state(dev)

Then the warning is logged inside the force_restore block
[11.651495] [<ffffffff8106fc5c>] warn_slowpath_fmt+0x5c/0x80
[11.651508] [<ffffffffa0576d0a>] pch_enable_backlight+0x1ba/0x200
[11.651518] [<ffffffffa0577474>] intel_panel_enable_backlight+0xa4/0x100
[11.651529] [<ffffffffa0568514>] intel_edp_backlight_on+0x54/0x140
[11.651540] [<ffffffffa0560c23>] intel_enable_ddi+0xb3/0x100
[11.651552] [<ffffffffa054b869>] haswell_crtc_enable+0x559/0xe80
[11.651564] [<ffffffffa0545ea4>] __intel_set_mode+0xf94/0x1c00
[11.651575] [<ffffffffa055062b>] intel_modeset_setup_hw_state+0x55b/0xd80
[11.651609] [<ffffffffa04eb3a9>] __i915_drm_thaw+0x159/0x1d0
[11.651617] [<ffffffffa04ebcd8>] i915_resume+0x28/0x50

I see no code in the execution path of intel_modeset_setup_hw_state to 
either note the state of BLC_PWM_CPU_CTL2 or reset it, apart from the 
pch_enable_backlight function. Which would be one explanation :-)

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-21  7:12   ` Scot Doyle
@ 2014-08-26 10:45     ` Daniel Vetter
  2014-08-26 16:15       ` Scot Doyle
  0 siblings, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2014-08-26 10:45 UTC (permalink / raw)
  To: Scot Doyle; +Cc: Jani Nikula, intel-gfx

On Thu, Aug 21, 2014 at 07:12:59AM +0000, Scot Doyle wrote:
> 
> On Tue, 19 Aug 2014, Daniel Vetter wrote:
> >On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle <lkml14@scotdoyle.com> wrote:
> >>BIOS or firmware can modify hardware state during suspend/resume,
> >>for example on the Toshiba CB35 or Lenovo T400, so log a debug message
> >>instead of a warning if the backlight is unexpectedly enabled.
> >>
> >>Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80930
> >>Cc: Jani Nikula <jani.nikula@intel.com>
> >>Signed-off-by: Scot Doyle <lkml14@scotdoyle.com>
> >
> >This should get cleaned up in the modeset state sanitization we do
> >upon resume, so without someone digging into this bug a bit and coming
> >up with an explanation for why that fails I'm reluctant to merge this.
> >-Daniel
> 
> When we enter intel_modeset_setup_hw_state during resume
> - BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
> - the physical backlight is off

Hm, this is actually interesting - we have some other evidence that the
best way to shut off the backlight is actually to just set the pwm duty
cycle to 0. Can you please check that this is the case for your system?

Maybe we just need to extend the check to look for !PWM_ENABLE ||
duty_cycle == 0.

In general if we hit upon a WARN it's not a good idea to just shut it up,
but to dig a bit deeper and figure out why exactly something doesn't work
as we expected it to work.

Thanks, Daniel

> - readout_hw_state says crtcs, encoders and connectors are disabled
> 
> The sanitizations done before reaching the force_restore block are
> - drm_vblank_off for each crtc
> - sarea_priv->pipeA_w=0
> - sarea_priv->pipeA_h=0
> - ilk_wm_get_hw_state(dev)
> 
> Then the warning is logged inside the force_restore block
> [11.651495] [<ffffffff8106fc5c>] warn_slowpath_fmt+0x5c/0x80
> [11.651508] [<ffffffffa0576d0a>] pch_enable_backlight+0x1ba/0x200
> [11.651518] [<ffffffffa0577474>] intel_panel_enable_backlight+0xa4/0x100
> [11.651529] [<ffffffffa0568514>] intel_edp_backlight_on+0x54/0x140
> [11.651540] [<ffffffffa0560c23>] intel_enable_ddi+0xb3/0x100
> [11.651552] [<ffffffffa054b869>] haswell_crtc_enable+0x559/0xe80
> [11.651564] [<ffffffffa0545ea4>] __intel_set_mode+0xf94/0x1c00
> [11.651575] [<ffffffffa055062b>] intel_modeset_setup_hw_state+0x55b/0xd80
> [11.651609] [<ffffffffa04eb3a9>] __i915_drm_thaw+0x159/0x1d0
> [11.651617] [<ffffffffa04ebcd8>] i915_resume+0x28/0x50
> 
> I see no code in the execution path of intel_modeset_setup_hw_state to
> either note the state of BLC_PWM_CPU_CTL2 or reset it, apart from the
> pch_enable_backlight function. Which would be one explanation :-)

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-26 10:45     ` Daniel Vetter
@ 2014-08-26 16:15       ` Scot Doyle
  2014-08-26 17:33         ` Daniel Vetter
  0 siblings, 1 reply; 9+ messages in thread
From: Scot Doyle @ 2014-08-26 16:15 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx

On Tue, 26 Aug 2014, Daniel Vetter wrote:
> On Thu, Aug 21, 2014 at 07:12:59AM +0000, Scot Doyle wrote:
>> When we enter intel_modeset_setup_hw_state during resume
>> - BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
>> - the physical backlight is off
>
> Hm, this is actually interesting - we have some other evidence that the
> best way to shut off the backlight is actually to just set the pwm duty
> cycle to 0. Can you please check that this is the case for your system?

/sys/class/backlight/intel_backlight/brightness
0 -> backlight not visible
1 -> backlight visible
937 -> max backlight

Setting /sys/class/backlight/intel_backlight/brightness to 0 updates 
BLC_PWM_CPU_CTL, but BLC_PWM_CPU_CTL2 remains 0xe0000000.


> Maybe we just need to extend the check to look for !PWM_ENABLE ||
> duty_cycle == 0.

The following measurements hold true no matter the duty cycle before 
suspend:

When entering hsw_enable_pc8 during suspend
- the physical backlight is off
- BLC_PWM_CPU_CTL == 0x3a900000 (BACKLIGHT_DUTY_CYCLE_MASK == ffff)
- BLC_PWM_CPU_CTL2 == 0x60000000 (BLM_PWM_ENABLE)

When exiting hsw_disable_pc8 during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL == 0x200
- BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)

When entering pch_enable_backlight during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL == 0x200
- BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE)

When exiting pch_enable_backlight during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL == duty cycle prior to suspend
- BLC_PWM_CPU_CTL2 == 0xe0000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)


So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x80000000 ?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-26 16:15       ` Scot Doyle
@ 2014-08-26 17:33         ` Daniel Vetter
  2014-08-26 17:34           ` Daniel Vetter
  2014-08-27 11:01           ` Jani Nikula
  0 siblings, 2 replies; 9+ messages in thread
From: Daniel Vetter @ 2014-08-26 17:33 UTC (permalink / raw)
  To: Scot Doyle; +Cc: Jani Nikula, intel-gfx

On Tue, Aug 26, 2014 at 04:15:53PM +0000, Scot Doyle wrote:
> On Tue, 26 Aug 2014, Daniel Vetter wrote:
> > On Thu, Aug 21, 2014 at 07:12:59AM +0000, Scot Doyle wrote:
> >> When we enter intel_modeset_setup_hw_state during resume
> >> - BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
> >> - the physical backlight is off
> >
> > Hm, this is actually interesting - we have some other evidence that the
> > best way to shut off the backlight is actually to just set the pwm duty
> > cycle to 0. Can you please check that this is the case for your system?
> 
> /sys/class/backlight/intel_backlight/brightness
> 0 -> backlight not visible
> 1 -> backlight visible
> 937 -> max backlight
> 
> Setting /sys/class/backlight/intel_backlight/brightness to 0 updates 
> BLC_PWM_CPU_CTL, but BLC_PWM_CPU_CTL2 remains 0xe0000000.
> 
> 
> > Maybe we just need to extend the check to look for !PWM_ENABLE ||
> > duty_cycle == 0.
> 
> The following measurements hold true no matter the duty cycle before 
> suspend:
> 
> When entering hsw_enable_pc8 during suspend
> - the physical backlight is off
> - BLC_PWM_CPU_CTL == 0x3a900000 (BACKLIGHT_DUTY_CYCLE_MASK == ffff)
> - BLC_PWM_CPU_CTL2 == 0x60000000 (BLM_PWM_ENABLE)
> 
> When exiting hsw_disable_pc8 during resume
> - the physical backlight is off
> - BLC_PWM_CPU_CTL == 0x200
> - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
> 
> When entering pch_enable_backlight during resume
> - the physical backlight is off
> - BLC_PWM_CPU_CTL == 0x200
> - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE)
> 
> When exiting pch_enable_backlight during resume
> - the physical backlight is off
> - BLC_PWM_CPU_CTL == duty cycle prior to suspend
> - BLC_PWM_CPU_CTL2 == 0xe0000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
> 
> 
> So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x80000000 ?

Indeed the bios seems to just but gunk into that register. And if we add
in all the knobs there's piles of them (you have semi-duplicated backlight
registers on hsw on the PCH), so I guess it doesn't make sense to combine
them all and warn if something goes awry, at least not in a -fixes patch.
So Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on your original
patch.

Jani can decide whether he wants to save this WARN_ON (imo it's useful to
have such sanity-checks) in -next by taking all the various bits and duty
cycles into account. But maybe just on the latest platforms, that still
should give is good coverage, but with a lot less fuss.

Thanks for tracking this all down.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-26 17:33         ` Daniel Vetter
@ 2014-08-26 17:34           ` Daniel Vetter
  2014-08-26 19:36             ` Scot Doyle
  2014-08-27 11:01           ` Jani Nikula
  1 sibling, 1 reply; 9+ messages in thread
From: Daniel Vetter @ 2014-08-26 17:34 UTC (permalink / raw)
  To: Scot Doyle; +Cc: Jani Nikula, intel-gfx

On Tue, Aug 26, 2014 at 07:33:25PM +0200, Daniel Vetter wrote:
> On Tue, Aug 26, 2014 at 04:15:53PM +0000, Scot Doyle wrote:
> > On Tue, 26 Aug 2014, Daniel Vetter wrote:
> > > On Thu, Aug 21, 2014 at 07:12:59AM +0000, Scot Doyle wrote:
> > >> When we enter intel_modeset_setup_hw_state during resume
> > >> - BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
> > >> - the physical backlight is off
> > >
> > > Hm, this is actually interesting - we have some other evidence that the
> > > best way to shut off the backlight is actually to just set the pwm duty
> > > cycle to 0. Can you please check that this is the case for your system?
> > 
> > /sys/class/backlight/intel_backlight/brightness
> > 0 -> backlight not visible
> > 1 -> backlight visible
> > 937 -> max backlight
> > 
> > Setting /sys/class/backlight/intel_backlight/brightness to 0 updates 
> > BLC_PWM_CPU_CTL, but BLC_PWM_CPU_CTL2 remains 0xe0000000.
> > 
> > 
> > > Maybe we just need to extend the check to look for !PWM_ENABLE ||
> > > duty_cycle == 0.
> > 
> > The following measurements hold true no matter the duty cycle before 
> > suspend:
> > 
> > When entering hsw_enable_pc8 during suspend
> > - the physical backlight is off
> > - BLC_PWM_CPU_CTL == 0x3a900000 (BACKLIGHT_DUTY_CYCLE_MASK == ffff)
> > - BLC_PWM_CPU_CTL2 == 0x60000000 (BLM_PWM_ENABLE)
> > 
> > When exiting hsw_disable_pc8 during resume
> > - the physical backlight is off
> > - BLC_PWM_CPU_CTL == 0x200
> > - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
> > 
> > When entering pch_enable_backlight during resume
> > - the physical backlight is off
> > - BLC_PWM_CPU_CTL == 0x200
> > - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE)
> > 
> > When exiting pch_enable_backlight during resume
> > - the physical backlight is off
> > - BLC_PWM_CPU_CTL == duty cycle prior to suspend
> > - BLC_PWM_CPU_CTL2 == 0xe0000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
> > 
> > 
> > So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x80000000 ?
> 
> Indeed the bios seems to just but gunk into that register. And if we add
> in all the knobs there's piles of them (you have semi-duplicated backlight
> registers on hsw on the PCH), so I guess it doesn't make sense to combine
> them all and warn if something goes awry, at least not in a -fixes patch.
> So Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on your original
> patch.
> 
> Jani can decide whether he wants to save this WARN_ON (imo it's useful to
> have such sanity-checks) in -next by taking all the various bits and duty
> cycles into account. But maybe just on the latest platforms, that still
> should give is good coverage, but with a lot less fuss.

Out of curiosity: What are the PCH copies of the backlight registers doing
after resume?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-26 17:34           ` Daniel Vetter
@ 2014-08-26 19:36             ` Scot Doyle
  0 siblings, 0 replies; 9+ messages in thread
From: Scot Doyle @ 2014-08-26 19:36 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Jani Nikula, intel-gfx

On Tue, 26 Aug 2014, Daniel Vetter wrote:
> On Tue, Aug 26, 2014 at 07:33:25PM +0200, Daniel Vetter wrote:
>> Indeed the bios seems to just but gunk into that register. And if we add
>> in all the knobs there's piles of them (you have semi-duplicated backlight
>> registers on hsw on the PCH), so I guess it doesn't make sense to combine
>> them all and warn if something goes awry, at least not in a -fixes patch.
>> So Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on your original
>> patch.
>>
>> Jani can decide whether he wants to save this WARN_ON (imo it's useful to
>> have such sanity-checks) in -next by taking all the various bits and duty
>> cycles into account. But maybe just on the latest platforms, that still
>> should give is good coverage, but with a lot less fuss.
>
> Out of curiosity: What are the PCH copies of the backlight registers doing
> after resume?

Thanks Daniel, here's the info:

When entering hsw_enable_pc8 during suspend
- the physical backlight is off
- BLC_PWM_CPU_CTL  == 0x 3a90000 (BACKLIGHT_DUTY_CYCLE_MASK == ffff)
- BLC_PWM_CPU_CTL2 == 0x60000000
- BLC_PWM_PCH_CTL  == 0x       0
- BLC_PWM_PCH_CTL2 == 0x 3a90000

When exiting hsw_disable_pc8 during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL  == 0x     200
- BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE)
- BLC_PWM_PCH_CTL  == 0x80000000
- BLC_PWM_PCH_CTL2 == 0x 4000000

When entering pch_enable_backlight during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL  == 0x     200
- BLC_PWM_CPU_CTL2 == 0x80000000
- BLC_PWM_PCH_CTL  == 0x80000000
- BLC_PWM_PCH_CTL2 == 0x 4000000

When exiting pch_enable_backlight during resume
- the physical backlight is off
- BLC_PWM_CPU_CTL  == duty cycle prior to suspend
- BLC_PWM_CPU_CTL2 == 0xe0000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
- BLC_PWM_PCH_CTL  == 0x80000000
- BLC_PWM_PCH_CTL2 == 0x 3a90000

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
  2014-08-26 17:33         ` Daniel Vetter
  2014-08-26 17:34           ` Daniel Vetter
@ 2014-08-27 11:01           ` Jani Nikula
  1 sibling, 0 replies; 9+ messages in thread
From: Jani Nikula @ 2014-08-27 11:01 UTC (permalink / raw)
  To: Daniel Vetter, Scot Doyle; +Cc: intel-gfx

On Tue, 26 Aug 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Aug 26, 2014 at 04:15:53PM +0000, Scot Doyle wrote:
>> On Tue, 26 Aug 2014, Daniel Vetter wrote:
>> > On Thu, Aug 21, 2014 at 07:12:59AM +0000, Scot Doyle wrote:
>> >> When we enter intel_modeset_setup_hw_state during resume
>> >> - BLC_PWM_CPU_CTL2 == BLM_PWM_ENABLE
>> >> - the physical backlight is off
>> >
>> > Hm, this is actually interesting - we have some other evidence that the
>> > best way to shut off the backlight is actually to just set the pwm duty
>> > cycle to 0. Can you please check that this is the case for your system?
>> 
>> /sys/class/backlight/intel_backlight/brightness
>> 0 -> backlight not visible
>> 1 -> backlight visible
>> 937 -> max backlight
>> 
>> Setting /sys/class/backlight/intel_backlight/brightness to 0 updates 
>> BLC_PWM_CPU_CTL, but BLC_PWM_CPU_CTL2 remains 0xe0000000.
>> 
>> 
>> > Maybe we just need to extend the check to look for !PWM_ENABLE ||
>> > duty_cycle == 0.
>> 
>> The following measurements hold true no matter the duty cycle before 
>> suspend:
>> 
>> When entering hsw_enable_pc8 during suspend
>> - the physical backlight is off
>> - BLC_PWM_CPU_CTL == 0x3a900000 (BACKLIGHT_DUTY_CYCLE_MASK == ffff)
>> - BLC_PWM_CPU_CTL2 == 0x60000000 (BLM_PWM_ENABLE)
>> 
>> When exiting hsw_disable_pc8 during resume
>> - the physical backlight is off
>> - BLC_PWM_CPU_CTL == 0x200
>> - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
>> 
>> When entering pch_enable_backlight during resume
>> - the physical backlight is off
>> - BLC_PWM_CPU_CTL == 0x200
>> - BLC_PWM_CPU_CTL2 == 0x80000000 (BLM_PWM_ENABLE)
>> 
>> When exiting pch_enable_backlight during resume
>> - the physical backlight is off
>> - BLC_PWM_CPU_CTL == duty cycle prior to suspend
>> - BLC_PWM_CPU_CTL2 == 0xe0000000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP)
>> 
>> 
>> So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x80000000 ?
>
> Indeed the bios seems to just but gunk into that register. And if we add
> in all the knobs there's piles of them (you have semi-duplicated backlight
> registers on hsw on the PCH), so I guess it doesn't make sense to combine
> them all and warn if something goes awry, at least not in a -fixes patch.
> So Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> on your original
> patch.

Pushed to drm-intel-fixes, thanks for the patch and review.

BR,
Jani.


>
> Jani can decide whether he wants to save this WARN_ON (imo it's useful to
> have such sanity-checks) in -next by taking all the various bits and duty
> cycles into account. But maybe just on the latest platforms, that still
> should give is good coverage, but with a lot less fuss.
>
> Thanks for tracking this all down.
>
> Cheers, Daniel
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2014-08-27 11:01 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-19  2:07 [PATCH] drm/i915: don't warn if backlight unexpectedly enabled Scot Doyle
2014-08-19 14:29 ` Daniel Vetter
2014-08-21  7:12   ` Scot Doyle
2014-08-26 10:45     ` Daniel Vetter
2014-08-26 16:15       ` Scot Doyle
2014-08-26 17:33         ` Daniel Vetter
2014-08-26 17:34           ` Daniel Vetter
2014-08-26 19:36             ` Scot Doyle
2014-08-27 11:01           ` Jani Nikula

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.