Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, 26 Aug 2014, Daniel Vetter dan...@ffwll.ch wrote: On Tue, Aug 26, 2014 at 04:15:53PM +, Scot Doyle wrote: On Tue, 26 Aug 2014, Daniel Vetter wrote: On Thu, Aug 21, 2014 at 07:12:59AM +, 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 0xe000. 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 == 0x3a90 (BACKLIGHT_DUTY_CYCLE_MASK == ) - BLC_PWM_CPU_CTL2 == 0x6000 (BLM_PWM_ENABLE) When exiting hsw_disable_pc8 during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x200 - BLC_PWM_CPU_CTL2 == 0x8000 (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 == 0x8000 (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 == 0xe000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP) So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x8000 ? 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.vet...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Thu, Aug 21, 2014 at 07:12:59AM +, Scot Doyle wrote: On Tue, 19 Aug 2014, Daniel Vetter wrote: On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle lkm...@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.nik...@intel.com Signed-off-by: Scot Doyle lkm...@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] [8106fc5c] warn_slowpath_fmt+0x5c/0x80 [11.651508] [a0576d0a] pch_enable_backlight+0x1ba/0x200 [11.651518] [a0577474] intel_panel_enable_backlight+0xa4/0x100 [11.651529] [a0568514] intel_edp_backlight_on+0x54/0x140 [11.651540] [a0560c23] intel_enable_ddi+0xb3/0x100 [11.651552] [a054b869] haswell_crtc_enable+0x559/0xe80 [11.651564] [a0545ea4] __intel_set_mode+0xf94/0x1c00 [11.651575] [a055062b] intel_modeset_setup_hw_state+0x55b/0xd80 [11.651609] [a04eb3a9] __i915_drm_thaw+0x159/0x1d0 [11.651617] [a04ebcd8] 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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, 26 Aug 2014, Daniel Vetter wrote: On Thu, Aug 21, 2014 at 07:12:59AM +, 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 0xe000. 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 == 0x3a90 (BACKLIGHT_DUTY_CYCLE_MASK == ) - BLC_PWM_CPU_CTL2 == 0x6000 (BLM_PWM_ENABLE) When exiting hsw_disable_pc8 during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x200 - BLC_PWM_CPU_CTL2 == 0x8000 (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 == 0x8000 (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 == 0xe000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP) So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x8000 ? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, Aug 26, 2014 at 04:15:53PM +, Scot Doyle wrote: On Tue, 26 Aug 2014, Daniel Vetter wrote: On Thu, Aug 21, 2014 at 07:12:59AM +, 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 0xe000. 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 == 0x3a90 (BACKLIGHT_DUTY_CYCLE_MASK == ) - BLC_PWM_CPU_CTL2 == 0x6000 (BLM_PWM_ENABLE) When exiting hsw_disable_pc8 during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x200 - BLC_PWM_CPU_CTL2 == 0x8000 (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 == 0x8000 (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 == 0xe000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP) So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x8000 ? 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.vet...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, Aug 26, 2014 at 07:33:25PM +0200, Daniel Vetter wrote: On Tue, Aug 26, 2014 at 04:15:53PM +, Scot Doyle wrote: On Tue, 26 Aug 2014, Daniel Vetter wrote: On Thu, Aug 21, 2014 at 07:12:59AM +, 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 0xe000. 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 == 0x3a90 (BACKLIGHT_DUTY_CYCLE_MASK == ) - BLC_PWM_CPU_CTL2 == 0x6000 (BLM_PWM_ENABLE) When exiting hsw_disable_pc8 during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x200 - BLC_PWM_CPU_CTL2 == 0x8000 (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 == 0x8000 (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 == 0xe000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP) So the BIOS is setting BLC_PWM_CPU_CTL=0x200 and BLC_PWM_CPU_CTL2=0x8000 ? 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.vet...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
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.vet...@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 3a9 (BACKLIGHT_DUTY_CYCLE_MASK == ) - BLC_PWM_CPU_CTL2 == 0x6000 - BLC_PWM_PCH_CTL == 0x 0 - BLC_PWM_PCH_CTL2 == 0x 3a9 When exiting hsw_disable_pc8 during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x 200 - BLC_PWM_CPU_CTL2 == 0x8000 (BLM_PWM_ENABLE) - BLC_PWM_PCH_CTL == 0x8000 - BLC_PWM_PCH_CTL2 == 0x 400 When entering pch_enable_backlight during resume - the physical backlight is off - BLC_PWM_CPU_CTL == 0x 200 - BLC_PWM_CPU_CTL2 == 0x8000 - BLC_PWM_PCH_CTL == 0x8000 - BLC_PWM_PCH_CTL2 == 0x 400 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 == 0xe000 (BLM_PWM_ENABLE | BLM_TRANSCODER_EDP) - BLC_PWM_PCH_CTL == 0x8000 - BLC_PWM_PCH_CTL2 == 0x 3a9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, 19 Aug 2014, Daniel Vetter wrote: On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle lkm...@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.nik...@intel.com Signed-off-by: Scot Doyle lkm...@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] [8106fc5c] warn_slowpath_fmt+0x5c/0x80 [11.651508] [a0576d0a] pch_enable_backlight+0x1ba/0x200 [11.651518] [a0577474] intel_panel_enable_backlight+0xa4/0x100 [11.651529] [a0568514] intel_edp_backlight_on+0x54/0x140 [11.651540] [a0560c23] intel_enable_ddi+0xb3/0x100 [11.651552] [a054b869] haswell_crtc_enable+0x559/0xe80 [11.651564] [a0545ea4] __intel_set_mode+0xf94/0x1c00 [11.651575] [a055062b] intel_modeset_setup_hw_state+0x55b/0xd80 [11.651609] [a04eb3a9] __i915_drm_thaw+0x159/0x1d0 [11.651617] [a04ebcd8] 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 :-) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't warn if backlight unexpectedly enabled
On Tue, Aug 19, 2014 at 4:07 AM, Scot Doyle lkm...@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.nik...@intel.com Signed-off-by: Scot Doyle lkm...@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 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
