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

[Imre Deak]

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 at 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 at ffwll.ch>:
> > >> > On Tue, Apr 01, 2014 at 02:55:09PM -0300, Paulo Zanoni wrote:
> > >> >> From: Paulo Zanoni <paulo.r.zanoni at 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 at 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.
> 
> 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