| Message ID | alpine.LNX.2.11.1408190201560.2901@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 :-)
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 :-)
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 ?
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
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
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
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
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); }
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(-)