[Intel-gfx] [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
Paulo Zanoni
przanoni at gmail.com
Tue Apr 1 23:04:28 CEST 2014
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.
>
> 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
--
Paulo Zanoni
More information about the Intel-gfx
mailing list