* [PATCH 1/7] drm/i915: don't schedule force_wake_timer at gen6_read
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 17:55 ` [PATCH 2/7] drm/i915: get runtime PM at i915_reg_read_ioctl Paulo Zanoni
` (6 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
So far force_wake_timer was only used by gen6_gt_force_wake_put. Since
we always had balanced gen6_gt_force_wake_get/put calls, we could
guarantee balanced calls to intel_runtime_pm_get/put.
Commit 8232644ccf099548710843e97360a3fcd6d28e04, "drm/i915: Convert
the forcewake worker into a timer func" started scheduling the
force_wake_timer at gen6_read, which resulted in an unbalanced
runtime_pm refcount.
So this commit just reverts to the old behavior until we can find a
proper way to used delayed force_wake from the register read/write
macros without leaving the runtime_pm refcounts unbalanced and without
runtime suspending the driver while forcewake is active.
Testcase: igt/pm_pc8/rte
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76544
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c3832d9..c47853b 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -550,11 +550,12 @@ gen6_read##x(struct drm_i915_private *dev_priv, off_t reg, bool trace) { \
NEEDS_FORCE_WAKE((dev_priv), (reg))) { \
dev_priv->uncore.funcs.force_wake_get(dev_priv, \
FORCEWAKE_ALL); \
- dev_priv->uncore.forcewake_count++; \
- mod_timer_pinned(&dev_priv->uncore.force_wake_timer, \
- jiffies + 1); \
+ val = __raw_i915_read##x(dev_priv, reg); \
+ dev_priv->uncore.funcs.force_wake_put(dev_priv, \
+ FORCEWAKE_ALL); \
+ } else { \
+ val = __raw_i915_read##x(dev_priv, reg); \
} \
- val = __raw_i915_read##x(dev_priv, reg); \
REG_READ_FOOTER; \
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/7] drm/i915: get runtime PM at i915_reg_read_ioctl
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
2014-04-01 17:55 ` [PATCH 1/7] drm/i915: don't schedule force_wake_timer at gen6_read Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 17:55 ` [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended Paulo Zanoni
` (5 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
To avoid WARNs when we call it.
Testcase: igt/pm_pc8/reg-read-ioctl
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=75693
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
index c47853b..f729dc7 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -866,7 +866,7 @@ int i915_reg_read_ioctl(struct drm_device *dev,
struct drm_i915_private *dev_priv = dev->dev_private;
struct drm_i915_reg_read *reg = data;
struct register_whitelist const *entry = whitelist;
- int i;
+ int i, ret = 0;
for (i = 0; i < ARRAY_SIZE(whitelist); i++, entry++) {
if (entry->offset == reg->offset &&
@@ -877,6 +877,8 @@ int i915_reg_read_ioctl(struct drm_device *dev,
if (i == ARRAY_SIZE(whitelist))
return -EINVAL;
+ intel_runtime_pm_get(dev_priv);
+
switch (entry->size) {
case 8:
reg->val = I915_READ64(reg->offset);
@@ -892,10 +894,13 @@ int i915_reg_read_ioctl(struct drm_device *dev,
break;
default:
WARN_ON(1);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
- return 0;
+out:
+ intel_runtime_pm_put(dev_priv);
+ return ret;
}
int i915_get_reset_stats_ioctl(struct drm_device *dev,
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
2014-04-01 17:55 ` [PATCH 1/7] drm/i915: don't schedule force_wake_timer at gen6_read Paulo Zanoni
2014-04-01 17:55 ` [PATCH 2/7] drm/i915: get runtime PM at i915_reg_read_ioctl Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 20:37 ` Daniel Vetter
2014-04-01 17:55 ` [PATCH 4/7] drm/i915: get runtime PM at i915_display_info Paulo Zanoni
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
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>
---
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 17:55 ` [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended Paulo Zanoni
@ 2014-04-01 20:37 ` Daniel Vetter
2014-04-01 20:48 ` Paulo Zanoni
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2014-04-01 20:37 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 20:37 ` Daniel Vetter
@ 2014-04-01 20:48 ` Paulo Zanoni
2014-04-01 20:52 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 20:48 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
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
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 20:48 ` Paulo Zanoni
@ 2014-04-01 20:52 ` Daniel Vetter
2014-04-01 21:04 ` Paulo Zanoni
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2014-04-01 20:52 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 20:52 ` Daniel Vetter
@ 2014-04-01 21:04 ` Paulo Zanoni
2014-04-01 21:36 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 21:04 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, 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
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 21:04 ` Paulo Zanoni
@ 2014-04-01 21:36 ` Imre Deak
2014-04-01 21:42 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2014-04-01 21:36 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
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
>
>
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 21:36 ` Imre Deak
@ 2014-04-01 21:42 ` Imre Deak
2014-04-01 22:07 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2014-04-01 21:42 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 21:42 ` Imre Deak
@ 2014-04-01 22:07 ` Daniel Vetter
2014-04-01 22:35 ` Imre Deak
0 siblings, 1 reply; 18+ messages in thread
From: Daniel Vetter @ 2014-04-01 22:07 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel Graphics Development, Paulo Zanoni
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 22:07 ` Daniel Vetter
@ 2014-04-01 22:35 ` Imre Deak
2014-04-02 7:16 ` Daniel Vetter
0 siblings, 1 reply; 18+ messages in thread
From: Imre Deak @ 2014-04-01 22:35 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
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
> >
> >
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended
2014-04-01 22:35 ` Imre Deak
@ 2014-04-02 7:16 ` Daniel Vetter
0 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-02 7:16 UTC (permalink / raw)
To: Imre Deak; +Cc: Intel Graphics Development, Paulo Zanoni
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
> > >
> > >
> >
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/7] drm/i915: get runtime PM at i915_display_info
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
` (2 preceding siblings ...)
2014-04-01 17:55 ` [PATCH 3/7] drm/i915: don't read pp_ctrl_reg if we're suspended Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 17:55 ` [PATCH 5/7] drm/i915: don't read cursor registers on powered down pipes Paulo Zanoni
` (3 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Otherwise we may get some WARNs complaining that we're reading a
register while we're suspended.
Testcase: igt/pm_pc8/debugfs-read
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7522f97..23a6516 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2319,9 +2319,11 @@ static int i915_display_info(struct seq_file *m, void *unused)
{
struct drm_info_node *node = (struct drm_info_node *) m->private;
struct drm_device *dev = node->minor->dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_crtc *crtc;
struct drm_connector *connector;
+ intel_runtime_pm_get(dev_priv);
drm_modeset_lock_all(dev);
seq_printf(m, "CRTC info\n");
seq_printf(m, "---------\n");
@@ -2349,6 +2351,7 @@ static int i915_display_info(struct seq_file *m, void *unused)
intel_connector_info(m, connector);
}
drm_modeset_unlock_all(dev);
+ intel_runtime_pm_put(dev_priv);
return 0;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 5/7] drm/i915: don't read cursor registers on powered down pipes
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
` (3 preceding siblings ...)
2014-04-01 17:55 ` [PATCH 4/7] drm/i915: get runtime PM at i915_display_info Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 17:55 ` [PATCH 6/7] drm/i915: fix WARNs when reading DDI state while suspended Paulo Zanoni
` (2 subsequent siblings)
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
At i915_display_info, don't call cursor_position() for a disabled
CRTC, since the CRTC may be on a powered down pipe, and this will
cause "Unclaimed register before interrupt" error messages.
Testcase: igt/pm_pc8/debugfs-read
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 23a6516..2bf7c42 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2334,14 +2334,15 @@ static int i915_display_info(struct seq_file *m, void *unused)
seq_printf(m, "CRTC %d: pipe: %c, active: %s\n",
crtc->base.base.id, pipe_name(crtc->pipe),
yesno(crtc->active));
- if (crtc->active)
+ if (crtc->active) {
intel_crtc_info(m, crtc);
- active = cursor_position(dev, crtc->pipe, &x, &y);
- seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
- yesno(crtc->cursor_visible),
- x, y, crtc->cursor_addr,
- yesno(active));
+ active = cursor_position(dev, crtc->pipe, &x, &y);
+ seq_printf(m, "\tcursor visible? %s, position (%d, %d), addr 0x%08x, active? %s\n",
+ yesno(crtc->cursor_visible),
+ x, y, crtc->cursor_addr,
+ yesno(active));
+ }
}
seq_printf(m, "\n");
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 6/7] drm/i915: fix WARNs when reading DDI state while suspended
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
` (4 preceding siblings ...)
2014-04-01 17:55 ` [PATCH 5/7] drm/i915: don't read cursor registers on powered down pipes Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 17:55 ` [PATCH 7/7] drm/i915: don't get/put runtime PM at the debugfs forcewake file Paulo Zanoni
2014-04-01 20:45 ` [PATCH 0/7] Runtime PM fixes (resend) Daniel Vetter
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
If runtime PM is enabled and we unset all modes, we will runtime
suspend after __intel_set_mode() , then function
intel_modeset_check_state() will try to read the HW state while it is
suspended and trigger lots of WARNs because it shouldn't be reading
registers.
So on this patch we make intel_ddi_connector_get_hw_state() return
false in case the power domain is disabled, and we also make
intel_display_power_enabled() return false in case the device is
suspended. Notice that we can't just use
intel_display_power_enabled_sw() because while the driver is being
initialized the power domain refcounts are not reflecting the real
state of the hardware.
Just for reference, I have previously published an alternate patch for
this problem, called "drm/i915: get runtime PM at intel_set_mode".
Testcase: igt/pm_pc8
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/intel_ddi.c | 5 +++++
drivers/gpu/drm/i915/intel_pm.c | 3 +++
2 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 070bf2e..0ad4e96 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1108,8 +1108,13 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector)
enum port port = intel_ddi_get_encoder_port(intel_encoder);
enum pipe pipe = 0;
enum transcoder cpu_transcoder;
+ enum intel_display_power_domain power_domain;
uint32_t tmp;
+ power_domain = intel_display_port_power_domain(intel_encoder);
+ if (!intel_display_power_enabled(dev_priv, power_domain))
+ return false;
+
if (!intel_encoder->get_hw_state(intel_encoder, &pipe))
return false;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 38a68ce..299701a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5253,6 +5253,9 @@ bool intel_display_power_enabled(struct drm_i915_private *dev_priv,
bool is_enabled;
int i;
+ if (dev_priv->pm.suspended)
+ return false;
+
power_domains = &dev_priv->power_domains;
is_enabled = true;
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 7/7] drm/i915: don't get/put runtime PM at the debugfs forcewake file
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
` (5 preceding siblings ...)
2014-04-01 17:55 ` [PATCH 6/7] drm/i915: fix WARNs when reading DDI state while suspended Paulo Zanoni
@ 2014-04-01 17:55 ` Paulo Zanoni
2014-04-01 20:45 ` [PATCH 0/7] Runtime PM fixes (resend) Daniel Vetter
7 siblings, 0 replies; 18+ messages in thread
From: Paulo Zanoni @ 2014-04-01 17:55 UTC (permalink / raw)
To: intel-gfx; +Cc: Paulo Zanoni
From: Paulo Zanoni <paulo.r.zanoni@intel.com>
Because gen6_gt_force_wake_{get,put} should already be responsible for
getting/putting runtime PM. If we keep these calls, debugfs will not
be testing the get/put calls of the forcewake functions.
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
drivers/gpu/drm/i915/i915_debugfs.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 2bf7c42..b27b7a5 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3699,7 +3699,6 @@ static int i915_forcewake_open(struct inode *inode, struct file *file)
if (INTEL_INFO(dev)->gen < 6)
return 0;
- intel_runtime_pm_get(dev_priv);
gen6_gt_force_wake_get(dev_priv, FORCEWAKE_ALL);
return 0;
@@ -3714,7 +3713,6 @@ static int i915_forcewake_release(struct inode *inode, struct file *file)
return 0;
gen6_gt_force_wake_put(dev_priv, FORCEWAKE_ALL);
- intel_runtime_pm_put(dev_priv);
return 0;
}
--
1.8.5.3
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 0/7] Runtime PM fixes (resend)
2014-04-01 17:55 [PATCH 0/7] Runtime PM fixes (resend) Paulo Zanoni
` (6 preceding siblings ...)
2014-04-01 17:55 ` [PATCH 7/7] drm/i915: don't get/put runtime PM at the debugfs forcewake file Paulo Zanoni
@ 2014-04-01 20:45 ` Daniel Vetter
7 siblings, 0 replies; 18+ messages in thread
From: Daniel Vetter @ 2014-04-01 20:45 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni
On Tue, Apr 01, 2014 at 02:55:06PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Hi
>
> This is a resend of the patches I have that fix the runtime PM WARNs we get
> while running IGT's pm_pc8 test on Haswell.
>
> Right now, runtime PM is completely broken and these patches should put it back
> to the state where it was before. The issue fixed by the first patch completely
> breaks runtime PM, so we don't have Bugzilla tags for the other issues because
> our QA couldn't even discover them due to the lack of the first patch on our
> trees. I have tested this series locally on my Haswell machine and it works.
Pulled in the entire series for 3.15-fixes, thanks. Please supply the
backtrace for the one case I've replied to so I can add it to the commit
message.
-Daniel
>
> Thanks,
> Paulo
>
> Paulo Zanoni (7):
> drm/i915: don't schedule force_wake_timer at gen6_read
> drm/i915: get runtime PM at i915_reg_read_ioctl
> drm/i915: don't read pp_ctrl_reg if we're suspended
> drm/i915: get runtime PM at i915_display_info
> drm/i915: don't read cursor registers on powered down pipes
> drm/i915: fix WARNs when reading DDI state while suspended
> drm/i915: don't get/put runtime PM at the debugfs forcewake file
>
> drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++--------
> drivers/gpu/drm/i915/intel_ddi.c | 5 +++++
> drivers/gpu/drm/i915/intel_dp.c | 3 ++-
> drivers/gpu/drm/i915/intel_pm.c | 3 +++
> drivers/gpu/drm/i915/intel_uncore.c | 20 +++++++++++++-------
> 5 files changed, 33 insertions(+), 16 deletions(-)
>
> --
> 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
^ permalink raw reply [flat|nested] 18+ messages in thread