| Message ID | 1396374913-2030-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 ... 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@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
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 [ 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@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
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? 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@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
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. > > 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@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
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. 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@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 > > >
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. > > 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
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 > >
On Wed, 2014-04-02 at 00:07 +0200, Daniel Vetter wrote: > 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 ;-) Well this nesting is already provided, since we get the port power domain in edp_panel_vdd_on() and through that an RPM ref. > -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 > > > > >
On Wed, Apr 02, 2014 at 01:35:22AM +0300, Imre Deak wrote: > On Wed, 2014-04-02 at 00:07 +0200, Daniel Vetter wrote: > > 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 ;-) > > Well this nesting is already provided, since we get the port power > domain in edp_panel_vdd_on() and through that an RPM ref. Hm right ... I guess it would be better then to replace the dev_priv->pm.suspended check with a check for the right power domain. And a comment explaining what's going on. The current code very much looks like duct-tape. Paulo, can you please look into that? -Daniel > > > -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 > > > > > > > > > >
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