Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Apr 02, 2014 at 12:42:50AM +0300, Imre Deak wrote:
> On Wed, 2014-04-02 at 00:36 +0300, Imre Deak wrote:
> > On Tue, 2014-04-01 at 18:04 -0300, Paulo Zanoni wrote:
> > > 2014-04-01 17:52 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > > > On Tue, Apr 01, 2014 at 05:48:15PM -0300, Paulo Zanoni wrote:
> > > >> 2014-04-01 17:37 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> > > >> > On Tue, Apr 01, 2014 at 02:55:09PM -0300, Paulo Zanoni wrote:
> > > >> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >> >>
> > > >> >> ... at edp_have_panel_vdd. Just return false, saying we don't have the
> > > >> >> panel VDD since the device is suspended.
> > > >> >>
> > > >> >> We started getting WARNs about this problem since the patch that
> > > >> >> started checking if we're suspended while reading registers.
> > > >> >>
> > > >> >> Testcase: igt/pm_pc8
> > > >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >> >
> > > >> > Hm, where's an example backtrace for this? I wonder whether we simply need
> > > >> > to extend the range where we hold the runtime pm ref a bit ...
> > > >>
> > > >> There are tons of WARNs, but here is one example:
> > > >>
> > > >> [   63.572201] [drm:hsw_enable_pc8] Enabling package C8+
> > > >> [   63.581831] [drm:i915_runtime_suspend] Device suspended
> > > >> [   63.664798] ------------[ cut here ]------------
> > > >> [   63.664824] WARNING: CPU: 3 PID: 828 at
> > > >> drivers/gpu/drm/i915/intel_uncore.c:47
> > > >> assert_device_not_suspended.isra.7+0x32/0x40 [i915]()
> > > >> [   63.664826] Device suspended
> > > >> [   63.664828] Modules linked in: ccm fuse ip6table_filter ip6_tables
> > > >> ebtable_nat ebtables arc4 ath9k_htc ath9k_common ath9k_hw mac80211 ath
> > > >> cfg80211 iTCO_wdt iTCO_vendor_support x86_pkg_temp_thermal coretemp
> > > >> microcode i2c_i801 e1000e pcspkr serio_raw lpc_ich ptp pps_core mei_me
> > > >> mei mfd_core dm_crypt i915 crc32_pclmul crc32c_intel
> > > >> ghash_clmulni_intel i2c_algo_bit drm_kms_helper drm video
> > > >> [   63.664867] CPU: 3 PID: 828 Comm: kworker/3:3 Not tainted 3.14.0+ #153
> > > >> [   63.664869] Hardware name: Intel Corporation Shark Bay Client
> > > >> platform/WhiteTip Mountain 1, BIOS HSWLPTU1.86C.0133.R00.1309172123
> > > >> 09/17/2013
> > > >> [   63.664887] Workqueue: events edp_panel_vdd_work [i915]
> > > >> [   63.664889]  0000000000000009 ffff88009d745c28 ffffffff8167ec6f
> > > >> ffff88009d745c70
> > > >> [   63.664895]  ffff88009d745c60 ffffffff8106c8ed ffff880036278000
> > > >> 00000000000c7204
> > > >> [   63.664900]  ffff88014f2d3040 ffff880036278070 0000000000000001
> > > >> ffff88009d745cc0
> > > >> [   63.664905] Call Trace:
> > > >> [   63.664911]  [<ffffffff8167ec6f>] dump_stack+0x4d/0x66
> > > >> [   63.664916]  [<ffffffff8106c8ed>] warn_slowpath_common+0x7d/0xa0
> > > >> [   63.664920]  [<ffffffff8106c95c>] warn_slowpath_fmt+0x4c/0x50
> > > >> [   63.664926]  [<ffffffff810bd6be>] ? mark_held_locks+0xae/0x130
> > > >> [   63.664941]  [<ffffffffa00d80d2>]
> > > >> assert_device_not_suspended.isra.7+0x32/0x40 [i915]
> > > >> [   63.664956]  [<ffffffffa00d99d2>] gen6_read32+0x32/0x120 [i915]
> > > >> [   63.664969]  [<ffffffffa00d99a0>] ? gen6_read8+0x120/0x120 [i915]
> > > >> [   63.664985]  [<ffffffffa0106f8f>] edp_have_panel_vdd+0x3f/0x50 [i915]
> > > >> [   63.665000]  [<ffffffffa01074e8>] edp_panel_vdd_off_sync+0x58/0x1c0 [i915]
> > > >> [   63.665004]  [<ffffffff8108a06c>] ? process_one_work+0x18c/0x560
> > > >> [   63.665018]  [<ffffffffa0107684>] edp_panel_vdd_work+0x34/0x50 [i915]
> > > >> [   63.665022]  [<ffffffff8108a0d7>] process_one_work+0x1f7/0x560
> > > >> [   63.665026]  [<ffffffff8108a06c>] ? process_one_work+0x18c/0x560
> > > >> [   63.665031]  [<ffffffff8108ae2b>] worker_thread+0x11b/0x3a0
> > > >
> > > > Ah, that's the async edp worker thread. I guess we need to grab a runtim
> > > > pm ref when we start it and drop it again in the worker instead?
> > > 
> > > IMHO it doesn't make sense to keep the HW awake just so we can read a
> > > register to find out things are disabled, so I prefer the current
> > > solution. If we want a "better" solution, IMHO we should track the
> > > sate of the VDD in software, like intel_dp->has_panel_vdd. This would
> > > avoid many more register reads.

Yeah, we can also try to cancel the work if we go into runtime pm. But
otoh panel vdd is kinda like a power domain of it's own, so we should
apply the usual nesting rules that power domains grab references for the
outer power domains they depend on. Panel vdd without runtime pm probably
isn't a possible comibination, or at least not one that makes sense ;-)
-Daniel

> > 
> > Chiming in, as I was wondering what could cause the inbalance between
> > the edp_panel_vdd_on() and off() calls. One possibility is that
> > intel_dp_probe_oui() calls edp_panel_vdd_on() and then 
> > edp_panel_vdd_off(..., false) which schedules the work and then
> > intel_enable_dp() calls edp_panel_vdd_on() and edp_panel_vdd_off(...,
> > true) and disables VDD synchronously, dropping the RPM ref. Note that in
> > this case we won't get the "eDP VDD not forced on" WARN either.
> > 
> > Whether or not this was the case for you, I think for good measure we
> > should flush any pending vdd_work in _edp_panel_vdd_on() before setting
> > intel_dp->want_panel_vdd = true;
> 
> Actually not flush but cancel the work.
> 
> --Imre
> 
> 

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