More runtime PM fixes

PATCH In review Paulo Zanoni 6 participants 15 messages Mar 7, 2014 – Mar 18, 2014

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Paulo Zanoni

author

2014-03-07 23:06 UTC #0 lore raw

cover More runtime PM fixes

From: Paulo Zanoni <[email protected]>

So after Daniel merged that huge amount of patches that had accumulated, I decided to test runtime PM again and found some regressions. The first two patches are extremely important: they fix the current regressions and put our tree back in the state where it was. Without these, runtime PM won't work at all, and it will completely fill your dmesg if you try to run the PC8 test suite. These two patches are the priority.

Patches 3-5 are fixes that I already had locally, but didn't submit in the last round of patches. Please notice that we even have a bugzilla bug opened for patch 3.

Patch 6 is more for "completeness": it got rejected the last time I sent it, but I still didn't find time to do the proper fix, so I'm just sending it again in case anybody wants it. Please read its commit message for more details. We can survive without this for a few days.

Thanks,
Paulo

Paulo Zanoni (6):
drm/i915: don't schedule force_wake_timer at gen6_read drm/i915: properly disable the VDD when disabling the panel drm/i915: get runtime PM at i915_reg_read_ioctl drm/i915: don't get/put runtime PM at the debugfs forcewake file drm/i915: don't read pp_ctrl_reg if we're suspended drm/i915: get runtime PM at intel_set_mode

 drivers/gpu/drm/i915/i915_debugfs.c  |  2 --
 drivers/gpu/drm/i915/intel_ddi.c     |  1 +
 drivers/gpu/drm/i915/intel_display.c |  5 +++++
 drivers/gpu/drm/i915/intel_dp.c      | 12 ++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_uncore.c  | 20 +++++++++++++-------
 6 files changed, 28 insertions(+), 13 deletions(-)

Signature · 1 line

1.8.5.3

Paulo Zanoni

author

2014-03-07 23:06 UTC #1 lore raw

PATCH 1/6 drm/i915: don't schedule force_wake_timer at gen6_read

From: Paulo Zanoni <[email protected]>

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

Cc Chris Wilson <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_uncore.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

drivers/gpu/drm/i915/intel_uncore.c +5 -4

index 7861d97..c91c0c2 100644
@@ -504,11 +504,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; \
 }

Signature · 1 line

1.8.5.3

Chris Wilson

2014-03-08 09:36 UTC #2 lore raw

Re: [PATCH 1/6] drm/i915: don't schedule force_wake_timer at gen6_read

Quoted 5 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
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.

I'm sure you can think of a trivial way to put things back into balance. -Chris

Signature · 1 line

Chris Wilson, Intel Open Source Technology Centre

Daniel Vetter

2014-03-08 10:27 UTC #3 lore raw

Re: [PATCH 1/6] drm/i915: don't schedule force_wake_timer at gen6_read

Quoted 8 lines · Chris Wilson

Quoted 5 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
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.
I'm sure you can think of a trivial way to put things back into balance.

Yeah, I think a __force_wake_timer which doesn't do the runtime put should be good enough. Chris, can I sign you up for this since Paulo is now on vacation for 2 weeks? No real hurry since we need to stall for QA to hit this anyway - if they still fail to properly run the runtime pm tests then I need to go into full maintainer beserk mode ;-)

Cheers, Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Paulo Zanoni

author

2014-03-07 23:06 UTC #4 lore raw

PATCH 2/6 drm/i915: properly disable the VDD when disabling the panel

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Commit b3064154dfd37deb386b1e459c54e1ca2460b3d5 tried to revert commit dff392dbd258381a6c3164f38420593f2d291e3b, but wasn't complete, which resulted in regressions on Haswell. So this commit should fix b3064154dfd37deb386b1e459c54e1ca2460b3d5 by undoing what it did and providing an actual complete revert of dff392dbd258381a6c3164f38420593f2d291e3b.

Fixes regression introduced by:
commit b3064154dfd37deb386b1e459c54e1ca2460b3d5

Author: Patrik Jakobsson <[email protected]>
Date:   Tue Mar 4 00:42:44 2014 +0100

drm/i915: Don't just say it, actually force edp vdd

Testcase: igt/pm_pc8

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c | 1 +
 drivers/gpu/drm/i915/intel_dp.c  | 9 ++++++---
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +1 -0

index e2665e0..34e8bb3 100644
@@ -1340,6 +1340,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
+		edp_panel_vdd_on(intel_dp);
 		intel_edp_panel_off(intel_dp);
 	}

drivers/gpu/drm/i915/intel_dp.c +6 -3

index 22e1bdd..e936f36 100644
@@ -91,7 +91,6 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
 }
 
 static void intel_dp_link_down(struct intel_dp *intel_dp);
-static void edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 
 static int

@@ -1162,7 +1161,7 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 	return control;
 }
 
-static void edp_panel_vdd_on(struct intel_dp *intel_dp)
+void edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -1338,11 +1337,16 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 
 	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
 
+	intel_dp->want_panel_vdd = false;
+
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
 
 	intel_dp->last_power_cycle = jiffies;
 	wait_panel_off(intel_dp);
+
+	/* We got a reference when we enabled the VDD. */
+	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -1880,7 +1884,6 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 	intel_edp_backlight_off(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	intel_edp_panel_off(intel_dp);
-	edp_panel_vdd_off(intel_dp, true);
 
 	/* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
 	if (!(port == PORT_A || IS_VALLEYVIEW(dev)))

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 9c70905..805d207 100644
@@ -762,6 +762,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
+void edp_panel_vdd_on(struct intel_dp *intel_dp);
 
 
 /* intel_dsi.c */

Signature · 1 line

1.8.5.3

Jani Nikula

maintainer

2014-03-14 12:07 UTC #5 lore raw

Re: [PATCH 2/6] drm/i915: properly disable the VDD when disabling the panel

Quoted 9 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Commit b3064154dfd37deb386b1e459c54e1ca2460b3d5 tried to revert commit dff392dbd258381a6c3164f38420593f2d291e3b, but wasn't complete, which resulted in regressions on Haswell. So this commit should fix b3064154dfd37deb386b1e459c54e1ca2460b3d5 by undoing what it did and providing an actual complete revert of dff392dbd258381a6c3164f38420593f2d291e3b.

Quoted 5 lines

Fixes regression introduced by:
commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
Author: Patrik Jakobsson <[email protected]>
Date:   Tue Mar 4 00:42:44 2014 +0100
drm/i915: Don't just say it, actually force edp vdd

Patrik, would you mind acking/testing this patch, as it blames your commit for regressing?

Thanks,
Jani.

Quoted 86 lines

Testcase: igt/pm_pc8

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c | 1 +
 drivers/gpu/drm/i915/intel_dp.c  | 9 ++++++---
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 3 files changed, 8 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +1 -0

index e2665e0..34e8bb3 100644
@@ -1340,6 +1340,7 @@ static void intel_ddi_post_disable(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
+		edp_panel_vdd_on(intel_dp);
 		intel_edp_panel_off(intel_dp);
 	}

drivers/gpu/drm/i915/intel_dp.c +6 -3

index 22e1bdd..e936f36 100644
@@ -91,7 +91,6 @@ static struct intel_dp *intel_attached_dp(struct drm_connector *connector)
 }
 
 static void intel_dp_link_down(struct intel_dp *intel_dp);
-static void edp_panel_vdd_on(struct intel_dp *intel_dp);
 static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 
 static int

@@ -1162,7 +1161,7 @@ static  u32 ironlake_get_pp_control(struct intel_dp *intel_dp)
 	return control;
 }
 
-static void edp_panel_vdd_on(struct intel_dp *intel_dp)
+void edp_panel_vdd_on(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;

@@ -1338,11 +1337,16 @@ void intel_edp_panel_off(struct intel_dp *intel_dp)
 
 	pp_ctrl_reg = _pp_ctrl_reg(intel_dp);
 
+	intel_dp->want_panel_vdd = false;
+
 	I915_WRITE(pp_ctrl_reg, pp);
 	POSTING_READ(pp_ctrl_reg);
 
 	intel_dp->last_power_cycle = jiffies;
 	wait_panel_off(intel_dp);
+
+	/* We got a reference when we enabled the VDD. */
+	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_edp_backlight_on(struct intel_dp *intel_dp)

@@ -1880,7 +1884,6 @@ static void intel_disable_dp(struct intel_encoder *encoder)
 	intel_edp_backlight_off(intel_dp);
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_OFF);
 	intel_edp_panel_off(intel_dp);
-	edp_panel_vdd_off(intel_dp, true);
 
 	/* cpu edp my only be disable _after_ the cpu pipe/plane is disabled. */
 	if (!(port == PORT_A || IS_VALLEYVIEW(dev)))

drivers/gpu/drm/i915/intel_drv.h +1 -0

index 9c70905..805d207 100644
@@ -762,6 +762,7 @@ void intel_edp_panel_off(struct intel_dp *intel_dp);
 void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
+void edp_panel_vdd_on(struct intel_dp *intel_dp);
 
 
 /* intel_dsi.c */

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Daniel Vetter

2014-03-14 13:57 UTC #6 lore raw

Re: [PATCH 2/6] drm/i915: properly disable the VDD when disabling the panel

Quoted 8 lines · Jani Nikula

Quoted 5 lines
Fixes regression introduced by:
commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
Author: Patrik Jakobsson <[email protected]>
Date:   Tue Mar 4 00:42:44 2014 +0100
drm/i915: Don't just say it, actually force edp vdd
Patrik, would you mind acking/testing this patch, as it blames your commit for regressing?

Testing will be hard I guess since it does the equivalent change Patrik has done to the pre-hsw dp code to the ddi hsw+ code. But a review would be nice - I've signed up Damien for this but he seems to be sick :(
-Daniel

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Patrik Jakobsson

Tested-by

2014-03-14 19:00 UTC #7 lore raw

Re: [PATCH 2/6] drm/i915: properly disable the VDD when disabling the panel

Quoted 8 lines · Daniel Vetter

Quoted 6 lines · Jani Nikula
Quoted 5 lines
Fixes regression introduced by:
commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
Author: Patrik Jakobsson <[email protected]>
Date:   Tue Mar 4 00:42:44 2014 +0100
drm/i915: Don't just say it, actually force edp vdd

Quoted 8 lines

Quoted 2 lines
Patrik, would you mind acking/testing this patch, as it blames your commit for regressing?
Testing will be hard I guess since it does the equivalent change Patrik has done to the pre-hsw dp code to the ddi hsw+ code. But a review would be nice - I've signed up Damien for this but he seems to be sick :(
-Daniel

Yes, as expected, this patch applied on top of -nightly is working fine. It's the EDP_FORCE_VDD bit that is required for my Macbook Air 6,x.

Is the proper fix for -fixes to completely revert:
dff392dbd258381a6c3164f38420593f2d291e3b

Tested-by Patrik Jakobsson <[email protected]>

Thanks
Patrik

Jani Nikula

maintainer

2014-03-18 11:27 UTC #8 lore raw

Re: [PATCH 2/6] drm/i915: properly disable the VDD when disabling the panel

Quoted 18 lines · Patrik Jakobsson

Quoted 8 lines · Daniel Vetter
Quoted 6 lines · Jani Nikula
Quoted 5 lines
Fixes regression introduced by:
commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
Author: Patrik Jakobsson <[email protected]>
Date:   Tue Mar 4 00:42:44 2014 +0100
drm/i915: Don't just say it, actually force edp vdd
Quoted 8 lines
Quoted 2 lines
Patrik, would you mind acking/testing this patch, as it blames your commit for regressing?
Testing will be hard I guess since it does the equivalent change Patrik has done to the pre-hsw dp code to the ddi hsw+ code. But a review would be nice - I've signed up Damien for this but he seems to be sick :(
-Daniel

Quoted 5 lines

Yes, as expected, this patch applied on top of -nightly is working fine. It's the EDP_FORCE_VDD bit that is required for my Macbook Air 6,x.
Is the proper fix for -fixes to completely revert:
dff392dbd258381a6c3164f38420593f2d291e3b

Hi Patrik, I've done just that. Giving the drm-intel-fixes branch a spin would be great, if you don't mind me asking.

BR,
Jani.

Quoted 5 lines

Tested-by Patrik Jakobsson <[email protected]>
Thanks
Patrik

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Paulo Zanoni

author

2014-03-07 23:06 UTC #9 lore raw

PATCH 3/6 drm/i915: get runtime PM at i915_reg_read_ioctl

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_uncore.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_uncore.c +8 -3

index c91c0c2..aed78f6 100644
@@ -822,7 +822,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 &&

@@ -833,6 +833,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);

@@ -848,10 +850,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,

Signature · 1 line

1.8.5.3

Paulo Zanoni

author

2014-03-07 23:06 UTC #10 lore raw

PATCH 4/6 drm/i915: don't get/put runtime PM at the debugfs forcewake file

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

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 <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 --
 1 file changed, 2 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +0 -2

index a90d31c..6ee529e 100644
@@ -3623,7 +3623,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;

@@ -3638,7 +3637,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;
 }

Signature · 1 line

1.8.5.3

Paulo Zanoni

author

2014-03-07 23:06 UTC #11 lore raw

PATCH 5/6 drm/i915: don't read pp_ctrl_reg if we're suspended

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

... 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 <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/gpu/drm/i915/intel_dp.c +2 -1

index e936f36..0d051ef 100644
@@ -313,7 +313,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

Signature · 1 line

1.8.5.3

Paulo Zanoni

author

2014-03-07 23:06 UTC #12 lore raw

PATCH 6/6 drm/i915: get runtime PM at intel_set_mode

In reply to Paulo Zanoni

From: Paulo Zanoni <[email protected]>

Otherwise, when we run intel_modeset_check_state we may already be runtime suspended, and our state checking code will read registers while the device is suspended. This can only happen if your autosuspend_delay_ms is low (not the default 10s) and i915.pc8_timeout is set to zero.

NOTE: The correct way to fix this problem would be to properly get/put runtime PM at the get_config/state functions. I still didn't do this, so I'll keep this patch on the series because it fixes the remaining WARNs we get while running the pm_pc8 test suite. It's fine if we don't merge this. The problem

v2: - Add the note above.

Reviewed-by Jesse Barnes <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/intel_display.c +5 -0

index 0868afb..dc2b377 100644
@@ -9785,13 +9785,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
 			  int x, int y, struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	intel_runtime_pm_get(dev_priv);
+
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }

Signature · 1 line

1.8.5.3

Daniel Vetter

2014-03-08 10:04 UTC #13 lore raw

Re: [PATCH 6/6] drm/i915: get runtime PM at intel_set_mode

Quoted 8 lines · Paulo Zanoni

From: Paulo Zanoni <[email protected]>
Otherwise, when we run intel_modeset_check_state we may already be runtime suspended, and our state checking code will read registers while the device is suspended. This can only happen if your autosuspend_delay_ms is low (not the default 10s) and i915.pc8_timeout is set to zero.

Quoted 5 lines

NOTE: The correct way to fix this problem would be to properly get/put runtime PM at the get_config/state functions. I still didn't do this, so I'll keep this patch on the series because it fixes the remaining WARNs we get while running the pm_pc8 test suite. It's fine if we don't merge this. The problem

Imre has added a bunch more power well enabling checks ... do we still have gaps in our coverage?

If it's not a terrible lot of them I'd prefer we properly close those instead of this little hack here. So still not too happy to merge this ;-) -Daniel

Quoted 39 lines

v2: - Add the note above.

Reviewed-by Jesse Barnes <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

drivers/gpu/drm/i915/intel_display.c +5 -0

index 0868afb..dc2b377 100644
@@ -9785,13 +9785,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
 			  int x, int y, struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	intel_runtime_pm_get(dev_priv);
+
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Signature · 3 lines

Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Imre Deak

2014-03-10 16:18 UTC #14 lore raw

Re: [PATCH 6/6] drm/i915: get runtime PM at intel_set_mode

Quoted 9 lines · Daniel Vetter

Quoted 8 lines · Paulo Zanoni
From: Paulo Zanoni <[email protected]>
Otherwise, when we run intel_modeset_check_state we may already be runtime suspended, and our state checking code will read registers while the device is suspended. This can only happen if your autosuspend_delay_ms is low (not the default 10s) and i915.pc8_timeout is set to zero.

Quoted 8 lines

Quoted 5 lines
NOTE: The correct way to fix this problem would be to properly get/put runtime PM at the get_config/state functions. I still didn't do this, so I'll keep this patch on the series because it fixes the remaining WARNs we get while running the pm_pc8 test suite. It's fine if we don't merge this. The problem
Imre has added a bunch more power well enabling checks ... do we still have gaps in our coverage?

We still need Paulo's "[PATCH 03/16] drm/i915: get/put runtime PM when we get/put a power domain", but after that we don't need the extra intel_runtime_pm_get/put calls here.

Quoted 36 lines

If it's not a terrible lot of them I'd prefer we properly close those instead of this little hack here. So still not too happy to merge this ;-) -Daniel
Quoted 32 lines
v2: - Add the note above.
Reviewed-by Jesse Barnes <[email protected]>
Signed-off-by Paulo Zanoni <[email protected]>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)
drivers/gpu/drm/i915/intel_display.c +5 -0
index 0868afb..dc2b377 100644
@@ -9785,13 +9785,18 @@ static int intel_set_mode(struct drm_crtc *crtc,
 			  struct drm_display_mode *mode,
 			  int x, int y, struct drm_framebuffer *fb)
 {
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	int ret;
 
+	intel_runtime_pm_get(dev_priv);
+
 	ret = __intel_set_mode(crtc, mode, x, y, fb);
 
 	if (ret == 0)
 		intel_modeset_check_state(crtc->dev);
 
+	intel_runtime_pm_put(dev_priv);
 	return ret;
 }
 

Quoted 8 lines

Quoted 7 lines

Signature · 6 lines

1.8.5.3

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx