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

Imre Deak imre.deak at intel.com
Tue Apr 1 23:36:48 CEST 2014 

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;

--Imre


> 
> >
> > I'll add the backtrace to the commit, thanks.
> > -Daniel
> >
> >> [   63.665035]  [<ffffffff8108ad10>] ? manage_workers.isra.21+0x2a0/0x2a0
> >> [   63.665039]  [<ffffffff810916fc>] kthread+0xfc/0x120
> >> [   63.665043]  [<ffffffff81091600>] ? kthread_create_on_node+0x230/0x230
> >> [   63.665048]  [<ffffffff8169082c>] ret_from_fork+0x7c/0xb0
> >> [   63.665052]  [<ffffffff81091600>] ? kthread_create_on_node+0x230/0x230
> >> [   63.665054] ---[ end trace 1250bcc890af9999 ]---
> >> [   63.665060] [drm:edp_panel_vdd_off_sync] Turning eDP VDD off
> >> [   63.665061] ------------[ cut here ]------------
> >>
> >> >
> >> > Whatever, merged (I can rebase afterwards if you supply the backtrace for
> >> > the commit message, there we should at least have it).
> >> > -Daniel
> >> >> ---
> >> >>  drivers/gpu/drm/i915/intel_dp.c | 3 ++-
> >> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> >>
> >> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> >> index 6f767e5..44471cc 100644
> >> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> >> @@ -314,7 +314,8 @@ static bool edp_have_panel_vdd(struct intel_dp *intel_dp)
> >> >>       struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >> >>
> >> >> -     return (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;
> >> >> +     return !dev_priv->pm.suspended &&
> >> >> +            (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;
> >> >>  }
> >> >>
> >> >>  static void
> >> >> --
> >> >> 1.8.5.3
> >> >>
> >> >> _______________________________________________
> >> >> Intel-gfx mailing list
> >> >> Intel-gfx at lists.freedesktop.org
> >> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >> >
> >> > --
> >> > Daniel Vetter
> >> > Software Engineer, Intel Corporation
> >> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> >>
> >>
> >>
> >> --
> >> Paulo Zanoni
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
>

More information about the Intel-gfx mailing list