[PATCH 0/6] More runtime PM fixes

[PATCH 0/6] More runtime PM fixes

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hi

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(-)

-- 
1.8.5.3

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

[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
Cc: Chris Wilson <chris@chris-wilson.co.uk>
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 7861d97..c91c0c2 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -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; \
 }
 
-- 
1.8.5.3

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

[Chris Wilson]

On Fri, Mar 07, 2014 at 08:05:19PM -0300, Paulo Zanoni wrote:
> 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.

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

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Daniel Vetter]

On Sat, Mar 08, 2014 at 09:36:01AM +0000, Chris Wilson wrote:
> On Fri, Mar 07, 2014 at 08:05:19PM -0300, Paulo Zanoni wrote:
> > 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.
> 
> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <patrik.r.jakobsson@gmail.com>
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 <paulo.r.zanoni@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index e2665e0..34e8bb3 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -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);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 22e1bdd..e936f36 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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)))
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 9c70905..805d207 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 */
-- 
1.8.5.3

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

[Jani Nikula]

On Sat, 08 Mar 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> 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 <patrik.r.jakobsson@gmail.com>
> 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.


>
> Testcase: igt/pm_pc8
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  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(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index e2665e0..34e8bb3 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -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);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 22e1bdd..e936f36 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -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)))
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 9c70905..805d207 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 */
> -- 
> 1.8.5.3
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

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

[Daniel Vetter]

On Fri, Mar 14, 2014 at 1:07 PM, Jani Nikula
<jani.nikula@linux.intel.com> wrote:
>> Fixes regression introduced by:
>> commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
>> Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>> 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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Patrik Jakobsson]

On Fri, Mar 14, 2014 at 2:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Mar 14, 2014 at 1:07 PM, Jani Nikula
> <jani.nikula@linux.intel.com> wrote:
>>> Fixes regression introduced by:
>>> commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
>>> Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>>> 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

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 <patrik.r.jakobsson@gmail.com>

Thanks
Patrik

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

[Jani Nikula]

On Fri, 14 Mar 2014, Patrik Jakobsson <patrik.r.jakobsson@gmail.com> wrote:
> On Fri, Mar 14, 2014 at 2:57 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Mar 14, 2014 at 1:07 PM, Jani Nikula
>> <jani.nikula@linux.intel.com> wrote:
>>>> Fixes regression introduced by:
>>>> commit b3064154dfd37deb386b1e459c54e1ca2460b3d5
>>>> Author: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>>>> 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
>
> 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.

>
> Tested-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
>
> Thanks
> Patrik

-- 
Jani Nikula, Intel Open Source Technology Center

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

[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 c91c0c2..aed78f6 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -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,
-- 
1.8.5.3

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

[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 a90d31c..6ee529e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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;
 }
-- 
1.8.5.3

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

[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 e936f36..0d051ef 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -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
-- 
1.8.5.3

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

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <jbarnes@virtuousgeek.org>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0868afb..dc2b377 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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;
 }
 
-- 
1.8.5.3

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

[Daniel Vetter]

On Fri, Mar 07, 2014 at 08:05:24PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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

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

> 
> v2: - Add the note above.
> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0868afb..dc2b377 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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;
>  }
>  
> -- 
> 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

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

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 2544 bytes --]

On Sat, 2014-03-08 at 11:04 +0100, Daniel Vetter wrote:
> On Fri, Mar 07, 2014 at 08:05:24PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > 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
> 
> 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.

> 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
> 
> > 
> > v2: - Add the note above.
> > 
> > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 0868afb..dc2b377 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -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;
> >  }
> >  
> > -- 
> > 1.8.5.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx