[PATCH 00/15] vlv: add support for RPM

[PATCH 00/15] vlv: add support for RPM

[Imre Deak]

This adds the required helpers for saving/restoring HW context when
going to D3 (and possibly to S0ix afterwards) state on VLV and then
enables RPM.

Since we depend on the RC6 power context mechanism to save some state
this also touches generic RPM parts, so that RPM is only enabled after
the delayed RC6 enabling completes.

For more details on the VLV specific parts see patches 13 and 14.

I also included a related bugfix that Daniel and Paulo were also looking
at (patch 7).

I tested this with igt/kms_flip and pm_pc8 on VLV/DP, they seem to pass
fine, although I'm not sure that pm_pc8 goes through all interesting
paths, I'm planning to check this next.

Imre Deak (15):
  drm/i915: vlv: clean up GTLC wake control/status register macros
  drm/i915: vlv: clear master interrupt flag when disabling interrupts
  drm/i915: vlv: add RC6 residency counters
  drm/i915: fix rc6 status debug print
  drm/i915: take init power domain for sysfs/debugfs entries where
    needed
  drm/i915: get init power domain for gpu error state capture
  drm/i915: vlv: check port power domain instead of only D0 for eDP VDD
    on
  drm/i915: vlv: setup RPS min/max frequencies once during init time
  drm/i915: vlv: factor out vlv_force_gfx_clock
  drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes
  drm/i915: vlv: disable RPM if RC6 is not enabled
  drm/i915: add various missing GTI/Gunit register definitions
  drm/i915: vlv: add gunit s0ix save/restore helpers
  drm/i915: vlv: add runtime PM support
  drm/i915: vlv: enable RPM

 drivers/gpu/drm/i915/i915_debugfs.c |  22 +-
 drivers/gpu/drm/i915/i915_drv.c     | 399 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_drv.h     |  65 +++++-
 drivers/gpu/drm/i915/i915_gem.c     |   5 +-
 drivers/gpu/drm/i915/i915_irq.c     |   6 +
 drivers/gpu/drm/i915/i915_reg.h     |  56 ++++-
 drivers/gpu/drm/i915/i915_sysfs.c   |   4 +
 drivers/gpu/drm/i915/intel_dp.c     |   6 +-
 drivers/gpu/drm/i915/intel_drv.h    |   3 +
 drivers/gpu/drm/i915/intel_pm.c     | 136 ++++++++----
 10 files changed, 647 insertions(+), 55 deletions(-)

-- 
1.8.4

[PATCH 01/15] drm/i915: vlv: clean up GTLC wake control/status register macros

[Imre Deak]

These will be needed by the upcoming VLV RPM helpers.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c |  5 +++--
 drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6370a76..d000d0f 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4458,8 +4458,9 @@ int i915_gem_init(struct drm_device *dev)
 
 	if (IS_VALLEYVIEW(dev)) {
 		/* VLVA0 (potential hack), BIOS isn't actually waking us */
-		I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
-		if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
+		I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_GTLC_ALLOWWAKEREQ);
+		if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
+			      VLV_GTLC_ALLOWWAKEACK), 10))
 			DRM_DEBUG_DRIVER("allow wake ack timed out\n");
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8e60737..0997da3 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4985,9 +4985,15 @@ enum punit_power_well {
 #define  FORCEWAKE_ACK_HSW			0x130044
 #define  FORCEWAKE_ACK				0x130090
 #define  VLV_GTLC_WAKE_CTRL			0x130090
+#define   VLV_GTLC_RENDER_CTX_EXISTS		(1 << 25)
+#define   VLV_GTLC_MEDIA_CTX_EXISTS		(1 << 24)
+#define   VLV_GTLC_ALLOWWAKEREQ			(1 << 0)
+
 #define  VLV_GTLC_PW_STATUS			0x130094
-#define VLV_GTLC_PW_RENDER_STATUS_MASK		0x80
-#define VLV_GTLC_PW_MEDIA_STATUS_MASK		0x20
+#define   VLV_GTLC_ALLOWWAKEACK			(1 << 0)
+#define   VLV_GTLC_ALLOWWAKEERR			(1 << 1)
+#define   VLV_GTLC_PW_MEDIA_STATUS_MASK		(1 << 5)
+#define   VLV_GTLC_PW_RENDER_STATUS_MASK	(1 << 7)
 #define  FORCEWAKE_MT				0xa188 /* multi-threaded */
 #define   FORCEWAKE_KERNEL			0x1
 #define   FORCEWAKE_USER			0x2
-- 
1.8.4

Re: [PATCH 01/15] drm/i915: vlv: clean up GTLC wake control/status register macros

[Rodrigo Vivi]

Well, first of all I couldn't find the regs definitions. I decided to
do this review during fly but I didn't had this regs definitions on
VLV docs I have here with me. Could you please point me to the correct
docs?

Anyway this patch isn't really modifying any behaviour. So I would
tend to let my Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> here
anyway. Considering that the original author of changes plus the first
reviewer already agreed with you.

On Tue, Apr 8, 2014 at 1:57 PM, Imre Deak <imre.deak@intel.com> wrote:
> These will be needed by the upcoming VLV RPM helpers.
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |  5 +++--
>  drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6370a76..d000d0f 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4458,8 +4458,9 @@ int i915_gem_init(struct drm_device *dev)
>
>         if (IS_VALLEYVIEW(dev)) {
>                 /* VLVA0 (potential hack), BIOS isn't actually waking us */
> -               I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
> -               if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
> +               I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_GTLC_ALLOWWAKEREQ);
> +               if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
> +                             VLV_GTLC_ALLOWWAKEACK), 10))
>                         DRM_DEBUG_DRIVER("allow wake ack timed out\n");
>         }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 8e60737..0997da3 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4985,9 +4985,15 @@ enum punit_power_well {
>  #define  FORCEWAKE_ACK_HSW                     0x130044
>  #define  FORCEWAKE_ACK                         0x130090
>  #define  VLV_GTLC_WAKE_CTRL                    0x130090
> +#define   VLV_GTLC_RENDER_CTX_EXISTS           (1 << 25)
> +#define   VLV_GTLC_MEDIA_CTX_EXISTS            (1 << 24)
> +#define   VLV_GTLC_ALLOWWAKEREQ                        (1 << 0)
> +
>  #define  VLV_GTLC_PW_STATUS                    0x130094
> -#define VLV_GTLC_PW_RENDER_STATUS_MASK         0x80
> -#define VLV_GTLC_PW_MEDIA_STATUS_MASK          0x20
> +#define   VLV_GTLC_ALLOWWAKEACK                        (1 << 0)
> +#define   VLV_GTLC_ALLOWWAKEERR                        (1 << 1)
> +#define   VLV_GTLC_PW_MEDIA_STATUS_MASK                (1 << 5)
> +#define   VLV_GTLC_PW_RENDER_STATUS_MASK       (1 << 7)
>  #define  FORCEWAKE_MT                          0xa188 /* multi-threaded */
>  #define   FORCEWAKE_KERNEL                     0x1
>  #define   FORCEWAKE_USER                       0x2
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

Re: [PATCH 01/15] drm/i915: vlv: clean up GTLC wake control/status register macros

[Imre Deak]

On Wed, 2014-04-16 at 18:08 -0300, Rodrigo Vivi wrote:
> Well, first of all I couldn't find the regs definitions. I decided to
> do this review during fly but I didn't had this regs definitions on
> VLV docs I have here with me. Could you please point me to the correct
> docs?

These regs are defined in the Gunit register HAS, I'll send you in
private the details.

> Anyway this patch isn't really modifying any behaviour. So I would
> tend to let my Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com> here
> anyway. Considering that the original author of changes plus the first
> reviewer already agreed with you.

Note that there is already a v2 of this patchset, so please review that
one.

--Imre

> On Tue, Apr 8, 2014 at 1:57 PM, Imre Deak <imre.deak@intel.com> wrote:
> > These will be needed by the upcoming VLV RPM helpers.
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c |  5 +++--
> >  drivers/gpu/drm/i915/i915_reg.h | 10 ++++++++--
> >  2 files changed, 11 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index 6370a76..d000d0f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4458,8 +4458,9 @@ int i915_gem_init(struct drm_device *dev)
> >
> >         if (IS_VALLEYVIEW(dev)) {
> >                 /* VLVA0 (potential hack), BIOS isn't actually waking us */
> > -               I915_WRITE(VLV_GTLC_WAKE_CTRL, 1);
> > -               if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) & 1) == 1, 10))
> > +               I915_WRITE(VLV_GTLC_WAKE_CTRL, VLV_GTLC_ALLOWWAKEREQ);
> > +               if (wait_for((I915_READ(VLV_GTLC_PW_STATUS) &
> > +                             VLV_GTLC_ALLOWWAKEACK), 10))
> >                         DRM_DEBUG_DRIVER("allow wake ack timed out\n");
> >         }
> >
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 8e60737..0997da3 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -4985,9 +4985,15 @@ enum punit_power_well {
> >  #define  FORCEWAKE_ACK_HSW                     0x130044
> >  #define  FORCEWAKE_ACK                         0x130090
> >  #define  VLV_GTLC_WAKE_CTRL                    0x130090
> > +#define   VLV_GTLC_RENDER_CTX_EXISTS           (1 << 25)
> > +#define   VLV_GTLC_MEDIA_CTX_EXISTS            (1 << 24)
> > +#define   VLV_GTLC_ALLOWWAKEREQ                        (1 << 0)
> > +
> >  #define  VLV_GTLC_PW_STATUS                    0x130094
> > -#define VLV_GTLC_PW_RENDER_STATUS_MASK         0x80
> > -#define VLV_GTLC_PW_MEDIA_STATUS_MASK          0x20
> > +#define   VLV_GTLC_ALLOWWAKEACK                        (1 << 0)
> > +#define   VLV_GTLC_ALLOWWAKEERR                        (1 << 1)
> > +#define   VLV_GTLC_PW_MEDIA_STATUS_MASK                (1 << 5)
> > +#define   VLV_GTLC_PW_RENDER_STATUS_MASK       (1 << 7)
> >  #define  FORCEWAKE_MT                          0xa188 /* multi-threaded */
> >  #define   FORCEWAKE_KERNEL                     0x1
> >  #define   FORCEWAKE_USER                       0x2
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

[PATCH 02/15] drm/i915: vlv: clear master interrupt flag when disabling interrupts

[Imre Deak]

Not clearing this flag causes spurious interrupts at least in D3 state,
so before enabling RPM we need to fix this. We were already setting this
flag when enabling interrupts, only clearing it was missing.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 4ca0344..b8f64d7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3305,6 +3305,8 @@ static void valleyview_irq_uninstall(struct drm_device *dev)
 	if (!dev_priv)
 		return;
 
+	I915_WRITE(VLV_MASTER_IER, 0);
+
 	intel_hpd_irq_uninstall(dev_priv);
 
 	for_each_pipe(pipe)
-- 
1.8.4

[PATCH 03/15] drm/i915: vlv: add RC6 residency counters

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 5 +++++
 drivers/gpu/drm/i915/i915_reg.h     | 3 +++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 1e83ae4..02f1b39 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1261,6 +1261,11 @@ static int vlv_drpc_info(struct seq_file *m)
 			(I915_READ(VLV_GTLC_PW_STATUS) &
 				VLV_GTLC_PW_MEDIA_STATUS_MASK) ? "Up" : "Down");
 
+	seq_printf(m, "Render RC6 residency since boot: %u\n",
+		   I915_READ(VLV_GT_RENDER_RC6));
+	seq_printf(m, "Media RC6 residency since boot: %u\n",
+		   I915_READ(VLV_GT_MEDIA_RC6));
+
 	spin_lock_irq(&dev_priv->uncore.lock);
 	fw_rendercount = dev_priv->uncore.fw_rendercount;
 	fw_mediacount = dev_priv->uncore.fw_mediacount;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0997da3..c4fd089 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5125,6 +5125,9 @@ enum punit_power_well {
 #define   VLV_MEDIA_RC6_COUNT_EN		(1<<1)
 #define   VLV_RENDER_RC6_COUNT_EN		(1<<0)
 #define GEN6_GT_GFX_RC6				0x138108
+#define VLV_GT_RENDER_RC6			0x138108
+#define VLV_GT_MEDIA_RC6			0x13810C
+
 #define GEN6_GT_GFX_RC6p			0x13810C
 #define GEN6_GT_GFX_RC6pp			0x138110
 
-- 
1.8.4

[PATCH 04/15] drm/i915: fix rc6 status debug print

[Imre Deak]

The parsing was incorrect for ILK and VLV.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a0527a1..869a4e3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3215,6 +3215,12 @@ static void valleyview_disable_rps(struct drm_device *dev)
 
 static void intel_print_rc6_info(struct drm_device *dev, u32 mode)
 {
+	if (IS_VALLEYVIEW(dev)) {
+		if (mode & (GEN7_RC_CTL_TO_MODE | GEN6_RC_CTL_EI_MODE(1)))
+			mode = GEN6_RC_CTL_RC6_ENABLE;
+		else
+			mode = 0;
+	}
 	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
 		 (mode & GEN6_RC_CTL_RC6_ENABLE) ? "on" : "off",
 		 (mode & GEN6_RC_CTL_RC6p_ENABLE) ? "on" : "off",
@@ -3842,7 +3848,7 @@ static void ironlake_enable_rc6(struct drm_device *dev)
 	I915_WRITE(PWRCTXA, i915_gem_obj_ggtt_offset(dev_priv->ips.pwrctx) | PWRCTX_EN);
 	I915_WRITE(RSTDBYCTL, I915_READ(RSTDBYCTL) & ~RCX_SW_EXIT);
 
-	intel_print_rc6_info(dev, INTEL_RC6_ENABLE);
+	intel_print_rc6_info(dev, GEN6_RC_CTL_RC6_ENABLE);
 }
 
 static unsigned long intel_pxfreq(u32 vidfreq)
-- 
1.8.4

[PATCH 05/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Imre Deak]

For simplicity take the init power domain, which puts the device into D0
and turns on all display power wells.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++--
 drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
 2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 02f1b39..348cf7c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
 	u32 rpmodectl1, rcctl1;
 	unsigned fw_rendercount = 0, fw_mediacount = 0;
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
 	rcctl1 = I915_READ(GEN6_RC_CONTROL);
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	seq_printf(m, "Video Turbo Mode: %s\n",
 		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
 	seq_printf(m, "Turbo enabled: %s\n",
@@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
 		return 0;
 	}
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
 	if (ret)
-		return ret;
+		goto out;
 
 	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
 
@@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
 		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
 
 	mutex_unlock(&dev_priv->dpio_lock);
+out:
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-	return 0;
+	return ret;
 }
 
 static int i915_llc(struct seq_file *m, void *data)
@@ -3305,8 +3313,13 @@ i915_wedged_set(void *data, u64 val)
 {
 	struct drm_device *dev = data;
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	i915_handle_error(dev, val,
 			  "Manually setting wedged to %llu", val);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 9c57029..552c4ed 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (IS_VALLEYVIEW(dev_priv->dev)) {
 		u32 freq;
@@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
 }
 
-- 
1.8.4

[PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Imre Deak]

v2:
- make it actually compile, I managed to send the wrong version as v1
  somehow

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
 drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 02f1b39..ac91a44 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
 	u32 rpmodectl1, rcctl1;
 	unsigned fw_rendercount = 0, fw_mediacount = 0;
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
 	rcctl1 = I915_READ(GEN6_RC_CONTROL);
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	seq_printf(m, "Video Turbo Mode: %s\n",
 		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
 	seq_printf(m, "Turbo enabled: %s\n",
@@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
 		return 0;
 	}
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
 	if (ret)
-		return ret;
+		goto out;
 
 	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
 
@@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
 		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
 
 	mutex_unlock(&dev_priv->dpio_lock);
+out:
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 
-	return 0;
+	return ret;
 }
 
 static int i915_llc(struct seq_file *m, void *data)
@@ -3304,9 +3312,15 @@ static int
 i915_wedged_set(void *data, u64 val)
 {
 	struct drm_device *dev = data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
 
 	i915_handle_error(dev, val,
 			  "Manually setting wedged to %llu", val);
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 9c57029..552c4ed 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (IS_VALLEYVIEW(dev_priv->dev)) {
 		u32 freq;
@@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
 	}
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
+	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
 }
 
-- 
1.8.3.2

Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Daniel Vetter]

On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> v2:
> - make it actually compile, I managed to send the wrong version as v1
>   somehow
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Usually if we just want the device to be out of D3 we just use
intel_runtime_pm_get/put. On a quick look it seems like that's enough for
these places here, too?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
>  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 02f1b39..ac91a44 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
>  	u32 rpmodectl1, rcctl1;
>  	unsigned fw_rendercount = 0, fw_mediacount = 0;
>  
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +
>  	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
>  	rcctl1 = I915_READ(GEN6_RC_CONTROL);
>  
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
>  	seq_printf(m, "Video Turbo Mode: %s\n",
>  		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
>  	seq_printf(m, "Turbo enabled: %s\n",
> @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>  		return 0;
>  	}
>  
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +
>  	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
>  	if (ret)
> -		return ret;
> +		goto out;
>  
>  	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
>  
> @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>  		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
>  
>  	mutex_unlock(&dev_priv->dpio_lock);
> +out:
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static int i915_llc(struct seq_file *m, void *data)
> @@ -3304,9 +3312,15 @@ static int
>  i915_wedged_set(void *data, u64 val)
>  {
>  	struct drm_device *dev = data;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>  
>  	i915_handle_error(dev, val,
>  			  "Manually setting wedged to %llu", val);
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
>  	return 0;
>  }
>  
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 9c57029..552c4ed 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>  
>  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>  
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (IS_VALLEYVIEW(dev_priv->dev)) {
>  		u32 freq;
> @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>  	}
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
>  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
>  }
>  
> -- 
> 1.8.3.2
> 
> _______________________________________________
> 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 v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Paulo Zanoni]

2014-04-09 11:15 GMT-03:00 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
>> v2:
>> - make it actually compile, I managed to send the wrong version as v1
>>   somehow

This version removed the commit message of the first version.

>>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>
> Usually if we just want the device to be out of D3 we just use
> intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> these places here, too?

I agree that using the "init" power domain here is a bad idea. IMHO we
really should keep the init power domain only to the init/resume
paths. Otherwise, if we ever get a bug report saying that INIT get/put
calls are unbalanced, we may waste a lot of time debugging init/resume
while the problem may be in debugfs.

If you just need to resume from runtime PM, just use
intel_runtime_pm_get/put. If you need more specific things, you should
probably get the specific power domains.

> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
>>  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
>>  2 files changed, 20 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 02f1b39..ac91a44 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
>>       u32 rpmodectl1, rcctl1;
>>       unsigned fw_rendercount = 0, fw_mediacount = 0;
>>
>> +     intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
>>       rcctl1 = I915_READ(GEN6_RC_CONTROL);
>>
>> +     intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       seq_printf(m, "Video Turbo Mode: %s\n",
>>                  yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
>>       seq_printf(m, "Turbo enabled: %s\n",
>> @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>>               return 0;
>>       }
>>
>> +     intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
>>       if (ret)
>> -             return ret;
>> +             goto out;
>>
>>       seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
>>
>> @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
>>                  vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
>>
>>       mutex_unlock(&dev_priv->dpio_lock);
>> +out:
>> +     intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>>
>> -     return 0;
>> +     return ret;
>>  }
>>
>>  static int i915_llc(struct seq_file *m, void *data)
>> @@ -3304,9 +3312,15 @@ static int
>>  i915_wedged_set(void *data, u64 val)
>>  {
>>       struct drm_device *dev = data;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>> +
>> +     intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>>
>>       i915_handle_error(dev, val,
>>                         "Manually setting wedged to %llu", val);
>> +
>> +     intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       return 0;
>>  }
>>
>> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
>> index 9c57029..552c4ed 100644
>> --- a/drivers/gpu/drm/i915/i915_sysfs.c
>> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
>> @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>>
>>       flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>>
>> +     intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       mutex_lock(&dev_priv->rps.hw_lock);
>>       if (IS_VALLEYVIEW(dev_priv->dev)) {
>>               u32 freq;
>> @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
>>       }
>>       mutex_unlock(&dev_priv->rps.hw_lock);
>>
>> +     intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
>> +
>>       return snprintf(buf, PAGE_SIZE, "%d\n", ret);
>>  }
>>
>> --
>> 1.8.3.2
>>
>> _______________________________________________
>> 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Imre Deak]

On Wed, 2014-04-09 at 16:15 +0200, Daniel Vetter wrote:
> On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> > v2:
> > - make it actually compile, I managed to send the wrong version as v1
> >   somehow
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> Usually if we just want the device to be out of D3 we just use
> intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> these places here, too?

For drpc yes, but for the others we need the DPIO power wells up too. I
got for all VLV case the power well ref for simplicity, I can change it
to an RPM ref for drpc, if that's ok.

--Imre

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
> >  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index 02f1b39..ac91a44 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
> >  	u32 rpmodectl1, rcctl1;
> >  	unsigned fw_rendercount = 0, fw_mediacount = 0;
> >  
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> >  	rcctl1 = I915_READ(GEN6_RC_CONTROL);
> >  
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	seq_printf(m, "Video Turbo Mode: %s\n",
> >  		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
> >  	seq_printf(m, "Turbo enabled: %s\n",
> > @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> >  		return 0;
> >  	}
> >  
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
> >  	if (ret)
> > -		return ret;
> > +		goto out;
> >  
> >  	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
> >  
> > @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> >  		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
> >  
> >  	mutex_unlock(&dev_priv->dpio_lock);
> > +out:
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >  
> > -	return 0;
> > +	return ret;
> >  }
> >  
> >  static int i915_llc(struct seq_file *m, void *data)
> > @@ -3304,9 +3312,15 @@ static int
> >  i915_wedged_set(void *data, u64 val)
> >  {
> >  	struct drm_device *dev = data;
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> >  
> >  	i915_handle_error(dev, val,
> >  			  "Manually setting wedged to %llu", val);
> > +
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > index 9c57029..552c4ed 100644
> > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> >  
> >  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> >  
> > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  	if (IS_VALLEYVIEW(dev_priv->dev)) {
> >  		u32 freq;
> > @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> >  	}
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> >  
> > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > +
> >  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> >  }
> >  
> > -- 
> > 1.8.3.2
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Ville Syrjälä]

On Wed, Apr 09, 2014 at 05:21:47PM +0300, Imre Deak wrote:
> On Wed, 2014-04-09 at 16:15 +0200, Daniel Vetter wrote:
> > On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> > > v2:
> > > - make it actually compile, I managed to send the wrong version as v1
> > >   somehow
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > 
> > Usually if we just want the device to be out of D3 we just use
> > intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> > these places here, too?
> 
> For drpc yes, but for the others we need the DPIO power wells up too. I
> got for all VLV case the power well ref for simplicity, I can change it
> to an RPM ref for drpc, if that's ok.

The dpio_info file would need DPIO wells up. However I think we should
just kill that file. It just dumps a small subset of the registers w/o
decoding anything, and we can get more extensive dumps w/ igt, so I
don't see the point of this file.

The error handler should already be able to deal with the wells being
down no?

The gt_cutr_freq file just talks to the punit so no need for any power
wells either AFAICS.

> 
> --Imre
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
> > >  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
> > >  2 files changed, 20 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > index 02f1b39..ac91a44 100644
> > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
> > >  	u32 rpmodectl1, rcctl1;
> > >  	unsigned fw_rendercount = 0, fw_mediacount = 0;
> > >  
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> > >  	rcctl1 = I915_READ(GEN6_RC_CONTROL);
> > >  
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	seq_printf(m, "Video Turbo Mode: %s\n",
> > >  		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
> > >  	seq_printf(m, "Turbo enabled: %s\n",
> > > @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > >  		return 0;
> > >  	}
> > >  
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
> > >  	if (ret)
> > > -		return ret;
> > > +		goto out;
> > >  
> > >  	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
> > >  
> > > @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > >  		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
> > >  
> > >  	mutex_unlock(&dev_priv->dpio_lock);
> > > +out:
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > >  
> > > -	return 0;
> > > +	return ret;
> > >  }
> > >  
> > >  static int i915_llc(struct seq_file *m, void *data)
> > > @@ -3304,9 +3312,15 @@ static int
> > >  i915_wedged_set(void *data, u64 val)
> > >  {
> > >  	struct drm_device *dev = data;
> > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > >  
> > >  	i915_handle_error(dev, val,
> > >  			  "Manually setting wedged to %llu", val);
> > > +
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	return 0;
> > >  }
> > >  
> > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > > index 9c57029..552c4ed 100644
> > > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > > @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > >  
> > >  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > >  
> > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  	if (IS_VALLEYVIEW(dev_priv->dev)) {
> > >  		u32 freq;
> > > @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > >  	}
> > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > >  
> > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > +
> > >  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > >  }
> > >  
> > > -- 
> > > 1.8.3.2
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Imre Deak]

On Wed, 2014-04-09 at 19:06 +0300, Ville Syrjälä wrote:
> On Wed, Apr 09, 2014 at 05:21:47PM +0300, Imre Deak wrote:
> > On Wed, 2014-04-09 at 16:15 +0200, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> > > > v2:
> > > > - make it actually compile, I managed to send the wrong version as v1
> > > >   somehow
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Usually if we just want the device to be out of D3 we just use
> > > intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> > > these places here, too?
> > 
> > For drpc yes, but for the others we need the DPIO power wells up too. I
> > got for all VLV case the power well ref for simplicity, I can change it
> > to an RPM ref for drpc, if that's ok.
> 
> The dpio_info file would need DPIO wells up. However I think we should
> just kill that file. It just dumps a small subset of the registers w/o
> decoding anything, and we can get more extensive dumps w/ igt, so I
> don't see the point of this file.

Agreed, there are at least the intel_dpio_* tools and other similar ones
for reading registers in other address spaces, so this file looks a bit
arbitrary. I can remove it if no one objects.

> The error handler should already be able to deal with the wells being
> down no?

i915_handle_error can be called from IRQ context, so it can't get power
domain refs. On an interrupt error detection path this isn't a problem
since there the HW is in D0, but here that's not guaranteed.

I took the separate power domain refs in the error _capture_ function
since that's a deferred work. There again D0 is not guaranteed, for
example we could reset the HW, schedule the capture work, drop the last
RPM ref and only then start running the capture function.

> The gt_cutr_freq file just talks to the punit so no need for any power
> wells either AFAICS.

Hm right, this one needs only D0 as drpc. I'll change this too.

--Imre 

> 
> > 
> > --Imre
> > 
> > > -Daniel
> > > 
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_debugfs.c | 18 ++++++++++++++++--
> > > >  drivers/gpu/drm/i915/i915_sysfs.c   |  4 ++++
> > > >  2 files changed, 20 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > index 02f1b39..ac91a44 100644
> > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > > > @@ -1239,9 +1239,13 @@ static int vlv_drpc_info(struct seq_file *m)
> > > >  	u32 rpmodectl1, rcctl1;
> > > >  	unsigned fw_rendercount = 0, fw_mediacount = 0;
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	rpmodectl1 = I915_READ(GEN6_RP_CONTROL);
> > > >  	rcctl1 = I915_READ(GEN6_RC_CONTROL);
> > > >  
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	seq_printf(m, "Video Turbo Mode: %s\n",
> > > >  		   yesno(rpmodectl1 & GEN6_RP_MEDIA_TURBO));
> > > >  	seq_printf(m, "Turbo enabled: %s\n",
> > > > @@ -1916,9 +1920,11 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > > >  		return 0;
> > > >  	}
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	ret = mutex_lock_interruptible(&dev_priv->dpio_lock);
> > > >  	if (ret)
> > > > -		return ret;
> > > > +		goto out;
> > > >  
> > > >  	seq_printf(m, "DPIO_CTL: 0x%08x\n", I915_READ(DPIO_CTL));
> > > >  
> > > > @@ -1946,8 +1952,10 @@ static int i915_dpio_info(struct seq_file *m, void *data)
> > > >  		   vlv_dpio_read(dev_priv, PIPE_A, VLV_CMN_DW0));
> > > >  
> > > >  	mutex_unlock(&dev_priv->dpio_lock);
> > > > +out:
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > >  
> > > > -	return 0;
> > > > +	return ret;
> > > >  }
> > > >  
> > > >  static int i915_llc(struct seq_file *m, void *data)
> > > > @@ -3304,9 +3312,15 @@ static int
> > > >  i915_wedged_set(void *data, u64 val)
> > > >  {
> > > >  	struct drm_device *dev = data;
> > > > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > >  
> > > >  	i915_handle_error(dev, val,
> > > >  			  "Manually setting wedged to %llu", val);
> > > > +
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	return 0;
> > > >  }
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> > > > index 9c57029..552c4ed 100644
> > > > --- a/drivers/gpu/drm/i915/i915_sysfs.c
> > > > +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> > > > @@ -263,6 +263,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > > >  
> > > >  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
> > > >  
> > > > +	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > > >  	if (IS_VALLEYVIEW(dev_priv->dev)) {
> > > >  		u32 freq;
> > > > @@ -273,6 +275,8 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> > > >  	}
> > > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > > >  
> > > > +	intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> > > > +
> > > >  	return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> > > >  }
> > > >  
> > > > -- 
> > > > 1.8.3.2
> > > > 
> > > > _______________________________________________
> > > > Intel-gfx mailing list
> > > > Intel-gfx@lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > > 
> > 
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 


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

Re: [PATCH v2 5/15] drm/i915: take init power domain for sysfs/debugfs entries where needed

[Daniel Vetter]

On Wed, Apr 09, 2014 at 07:06:14PM +0300, Ville Syrjälä wrote:
> On Wed, Apr 09, 2014 at 05:21:47PM +0300, Imre Deak wrote:
> > On Wed, 2014-04-09 at 16:15 +0200, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 10:34:41PM +0300, Imre Deak wrote:
> > > > v2:
> > > > - make it actually compile, I managed to send the wrong version as v1
> > > >   somehow
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > 
> > > Usually if we just want the device to be out of D3 we just use
> > > intel_runtime_pm_get/put. On a quick look it seems like that's enough for
> > > these places here, too?
> > 
> > For drpc yes, but for the others we need the DPIO power wells up too. I
> > got for all VLV case the power well ref for simplicity, I can change it
> > to an RPM ref for drpc, if that's ok.
> 
> The dpio_info file would need DPIO wells up. However I think we should
> just kill that file. It just dumps a small subset of the registers w/o
> decoding anything, and we can get more extensive dumps w/ igt, so I
> don't see the point of this file.
> 
> The error handler should already be able to deal with the wells being
> down no?
> 
> The gt_cutr_freq file just talks to the punit so no need for any power
> wells either AFAICS.

I'm ok with this plan of ripping out useless debugfs files. And if we need
a specific set of display power domains then imo we should grab the
specific list, not just INIT, i.e. all of them.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 06/15] drm/i915: get init power domain for gpu error state capture

[Imre Deak]

Since the state capture happens from a deferred work, we may drop the
last power domain/RPM reference since the error got triggered (from an
interrupt handler for example). I hit this by writing to the i915_wedged
debugfs file.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index b8f64d7..a586c16 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2174,6 +2174,8 @@ static void i915_error_work_func(struct work_struct *work)
 		kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE,
 				   reset_event);
 
+		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
+
 		/*
 		 * All state reset _must_ be completed before we update the
 		 * reset counter, for otherwise waiters might miss the reset
@@ -2184,6 +2186,8 @@ static void i915_error_work_func(struct work_struct *work)
 
 		intel_display_handle_reset(dev);
 
+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
+
 		if (ret == 0) {
 			/*
 			 * After all the gem state is reset, increment the reset
-- 
1.8.4

Re: [PATCH 06/15] drm/i915: get init power domain for gpu error state capture

[Daniel Vetter]

On Tue, Apr 08, 2014 at 07:57:47PM +0300, Imre Deak wrote:
> Since the state capture happens from a deferred work, we may drop the
> last power domain/RPM reference since the error got triggered (from an
> interrupt handler for example). I hit this by writing to the i915_wedged
> debugfs file.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>

This looks fishy - as long as the gpu is busy we should keep a runtime pm
ref around. Also same comment as before, grabbing a display power domain
for this seems wrong.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b8f64d7..a586c16 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2174,6 +2174,8 @@ static void i915_error_work_func(struct work_struct *work)
>  		kobject_uevent_env(&dev->primary->kdev->kobj, KOBJ_CHANGE,
>  				   reset_event);
>  
> +		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> +
>  		/*
>  		 * All state reset _must_ be completed before we update the
>  		 * reset counter, for otherwise waiters might miss the reset
> @@ -2184,6 +2186,8 @@ static void i915_error_work_func(struct work_struct *work)
>  
>  		intel_display_handle_reset(dev);
>  
> +		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> +
>  		if (ret == 0) {
>  			/*
>  			 * After all the gem state is reset, increment the reset
> -- 
> 1.8.4
> 
> _______________________________________________
> 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

[PATCH 07/15] drm/i915: vlv: check port power domain instead of only D0 for eDP VDD on

[Imre Deak]

Some platforms need additional power domains to be on in addition to the
device D0 state to access the panel registers.

Suggested by Daniel.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e48d47c..c7a52d1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -313,8 +313,12 @@ 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;
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct intel_encoder *intel_encoder = &intel_dig_port->base;
+	enum intel_display_power_domain power_domain;
 
-	return !dev_priv->pm.suspended &&
+	power_domain = intel_display_port_power_domain(intel_encoder);
+	return intel_display_power_enabled(dev_priv, power_domain) &&
 	       (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;
 }
 
-- 
1.8.4

Re: [PATCH 07/15] drm/i915: vlv: check port power domain instead of only D0 for eDP VDD on

[Paulo Zanoni]

2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> Some platforms need additional power domains to be on in addition to the
> device D0 state to access the panel registers.
>
> Suggested by Daniel.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e48d47c..c7a52d1 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -313,8 +313,12 @@ 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;
> +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +       struct intel_encoder *intel_encoder = &intel_dig_port->base;
> +       enum intel_display_power_domain power_domain;
>
> -       return !dev_priv->pm.suspended &&
> +       power_domain = intel_display_port_power_domain(intel_encoder);
> +       return intel_display_power_enabled(dev_priv, power_domain) &&
>                (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;

This is an interesting problem. The patch looks like the correct fix
for VLV, but it creates the problem that it will enable unneeded power
wells on HSW/BDW. My crystal ball tells me we're gonna have many
instances of this type of "problem" in the future. In this case,
shouldn't we create a new power domain, e.g., POWER_DOMAIN_EDP_VDD,
that grabs the appropriate power wells on VLV, but just gets runtime
PM on HSW+?

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



-- 
Paulo Zanoni

Re: [PATCH 07/15] drm/i915: vlv: check port power domain instead of only D0 for eDP VDD on

[Imre Deak]

On Wed, 2014-04-09 at 11:32 -0300, Paulo Zanoni wrote:
> 2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > Some platforms need additional power domains to be on in addition to the
> > device D0 state to access the panel registers.
> >
> > Suggested by Daniel.
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=76987
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index e48d47c..c7a52d1 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -313,8 +313,12 @@ 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;
> > +       struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > +       struct intel_encoder *intel_encoder = &intel_dig_port->base;
> > +       enum intel_display_power_domain power_domain;
> >
> > -       return !dev_priv->pm.suspended &&
> > +       power_domain = intel_display_port_power_domain(intel_encoder);
> > +       return intel_display_power_enabled(dev_priv, power_domain) &&
> >                (I915_READ(_pp_ctrl_reg(intel_dp)) & EDP_FORCE_VDD) != 0;
> 
> This is an interesting problem. The patch looks like the correct fix
> for VLV, but it creates the problem that it will enable unneeded power
> wells on HSW/BDW.

I guess you mean port A here. In that case power_domain is
POWER_DOMAIN_PORT_DDI_A_4_LANES, which will map both for HSW and BDW to
the always-on power well, hence we only get an RPM ref for them?

> My crystal ball tells me we're gonna have many
> instances of this type of "problem" in the future. In this case,
> shouldn't we create a new power domain, e.g., POWER_DOMAIN_EDP_VDD,
> that grabs the appropriate power wells on VLV, but just gets runtime
> PM on HSW+?

Yea, well it's always a possibility that the current granularity is not
enough for a new platform and then we have no choice than to extend the
set of power domains. This is what happened now for the AUX stuff on
SKL, we need to add the new AUX domains. We can also decide that the
power saving doesn't justify the complexity and just grab the coarser
domain for a given functionality.

--Imre

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

[PATCH 08/15] drm/i915: vlv: setup RPS min/max frequencies once during init time

[Imre Deak]

When enabling RPM on VLV, GT power save enabling becomes relatively
frequent, so optimize it a bit.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 66 +++++++++++++++++++++++++----------------
 1 file changed, 41 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 869a4e3..e0b8982 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3633,6 +3633,45 @@ static void valleyview_cleanup_pctx(struct drm_device *dev)
 	dev_priv->vlv_pctx = NULL;
 }
 
+static void valleyview_init_gt_powersave(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	valleyview_setup_pctx(dev);
+
+	mutex_lock(&dev_priv->rps.hw_lock);
+
+	dev_priv->rps.max_freq = valleyview_rps_max_freq(dev_priv);
+	dev_priv->rps.rp0_freq = dev_priv->rps.max_freq;
+	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.max_freq),
+			 dev_priv->rps.max_freq);
+
+	dev_priv->rps.efficient_freq = valleyview_rps_rpe_freq(dev_priv);
+	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
+			 dev_priv->rps.efficient_freq);
+
+	dev_priv->rps.min_freq = valleyview_rps_min_freq(dev_priv);
+	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
+			 vlv_gpu_freq(dev_priv, dev_priv->rps.min_freq),
+			 dev_priv->rps.min_freq);
+
+	/* Preserve min/max settings in case of re-init */
+	if (dev_priv->rps.max_freq_softlimit == 0)
+		dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
+
+	if (dev_priv->rps.min_freq_softlimit == 0)
+		dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
+
+	mutex_unlock(&dev_priv->rps.hw_lock);
+}
+
+static void valleyview_cleanup_gt_powersave(struct drm_device *dev)
+{
+	valleyview_cleanup_pctx(dev);
+}
+
 static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3699,29 +3738,6 @@ static void valleyview_enable_rps(struct drm_device *dev)
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.cur_freq),
 			 dev_priv->rps.cur_freq);
 
-	dev_priv->rps.max_freq = valleyview_rps_max_freq(dev_priv);
-	dev_priv->rps.rp0_freq  = dev_priv->rps.max_freq;
-	DRM_DEBUG_DRIVER("max GPU freq: %d MHz (%u)\n",
-			 vlv_gpu_freq(dev_priv, dev_priv->rps.max_freq),
-			 dev_priv->rps.max_freq);
-
-	dev_priv->rps.efficient_freq = valleyview_rps_rpe_freq(dev_priv);
-	DRM_DEBUG_DRIVER("RPe GPU freq: %d MHz (%u)\n",
-			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
-			 dev_priv->rps.efficient_freq);
-
-	dev_priv->rps.min_freq = valleyview_rps_min_freq(dev_priv);
-	DRM_DEBUG_DRIVER("min GPU freq: %d MHz (%u)\n",
-			 vlv_gpu_freq(dev_priv, dev_priv->rps.min_freq),
-			 dev_priv->rps.min_freq);
-
-	/* Preserve min/max settings in case of re-init */
-	if (dev_priv->rps.max_freq_softlimit == 0)
-		dev_priv->rps.max_freq_softlimit = dev_priv->rps.max_freq;
-
-	if (dev_priv->rps.min_freq_softlimit == 0)
-		dev_priv->rps.min_freq_softlimit = dev_priv->rps.min_freq;
-
 	DRM_DEBUG_DRIVER("setting GPU freq to %d MHz (%u)\n",
 			 vlv_gpu_freq(dev_priv, dev_priv->rps.efficient_freq),
 			 dev_priv->rps.efficient_freq);
@@ -4463,13 +4479,13 @@ static void intel_init_emon(struct drm_device *dev)
 void intel_init_gt_powersave(struct drm_device *dev)
 {
 	if (IS_VALLEYVIEW(dev))
-		valleyview_setup_pctx(dev);
+		valleyview_init_gt_powersave(dev);
 }
 
 void intel_cleanup_gt_powersave(struct drm_device *dev)
 {
 	if (IS_VALLEYVIEW(dev))
-		valleyview_cleanup_pctx(dev);
+		valleyview_cleanup_gt_powersave(dev);
 }
 
 void intel_disable_gt_powersave(struct drm_device *dev)
-- 
1.8.4

[PATCH 09/15] drm/i915: vlv: factor out vlv_force_gfx_clock

[Imre Deak]

This will be needed by the VLV RPM helpers too.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 32 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c | 16 ++--------------
 3 files changed, 35 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5d8250f..8dee34c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -920,6 +920,38 @@ static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
 	hsw_disable_pc8(dev_priv);
 }
 
+int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
+{
+	u32 val;
+	int err;
+
+#define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG) & VLV_GFX_CLK_STATUS_BIT)
+	/* Wait for a previous force on/off to settle */
+	if (force_on) {
+		err = wait_for(!COND, 5);
+		if (err) {
+			DRM_ERROR("timeout waiting for GFX clock force-off\n");
+			return err;
+		}
+	}
+
+	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
+	val &= ~VLV_GFX_CLK_FORCE_ON_BIT;
+	if (force_on)
+		val |= VLV_GFX_CLK_FORCE_ON_BIT;
+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG, val);
+
+	if (!force_on)
+		return 0;
+
+	err = wait_for(COND, 5);
+	if (err)
+		DRM_ERROR("timeout waiting for GFX clock force-on\n");
+
+	return err;
+#undef COND
+}
+
 static int intel_runtime_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 55addaa..0800429 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1952,6 +1952,7 @@ extern unsigned long i915_chipset_val(struct drm_i915_private *dev_priv);
 extern unsigned long i915_mch_val(struct drm_i915_private *dev_priv);
 extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
+int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
 extern void intel_console_resume(struct work_struct *work);
 
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e0b8982..5728238 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3094,16 +3094,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 	/* Mask turbo interrupt so that they will not come in between */
 	I915_WRITE(GEN6_PMINTRMSK, 0xffffffff);
 
-	/* Bring up the Gfx clock */
-	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
-		I915_READ(VLV_GTLC_SURVIVABILITY_REG) |
-				VLV_GFX_CLK_FORCE_ON_BIT);
-
-	if (wait_for(((VLV_GFX_CLK_STATUS_BIT &
-		I915_READ(VLV_GTLC_SURVIVABILITY_REG)) != 0), 5)) {
-			DRM_ERROR("GFX_CLK_ON request timed out\n");
-		return;
-	}
+	vlv_force_gfx_clock(dev_priv, true);
 
 	dev_priv->rps.cur_freq = dev_priv->rps.min_freq_softlimit;
 
@@ -3114,10 +3105,7 @@ static void vlv_set_rps_idle(struct drm_i915_private *dev_priv)
 				& GENFREQSTATUS) == 0, 5))
 		DRM_ERROR("timed out waiting for Punit\n");
 
-	/* Release the Gfx clock */
-	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,
-		I915_READ(VLV_GTLC_SURVIVABILITY_REG) &
-				~VLV_GFX_CLK_FORCE_ON_BIT);
+	vlv_force_gfx_clock(dev_priv, false);
 
 	I915_WRITE(GEN6_PMINTRMSK,
 		   gen6_rps_pm_mask(dev_priv, dev_priv->rps.cur_freq));
-- 
1.8.4

[PATCH 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

[Imre Deak]

At least on some platforms we depend on RC6 being enabled for RPM, so
disable RPM until the delayed RC6 enabling completes. For simplicity
don't differentiate between platforms, those that don't have this
dependency will enable RC6 only rarely during driver init and system
resume.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
 3 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 8dee34c..0948228 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
 		 * of re-init after reset. */
 		if (INTEL_INFO(dev)->gen > 5) {
 			mutex_lock(&dev->struct_mutex);
-			intel_enable_gt_powersave(dev);
+			intel_reset_gt_powersave(dev);
 			mutex_unlock(&dev->struct_mutex);
 		}
 
@@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
+	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
+		return -ENODEV;
+
 	WARN_ON(!HAS_RUNTIME_PM(dev));
 	assert_force_wake_inactive(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 42762b7..7f4e873 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
 void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
+void intel_reset_gt_powersave(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
 void gen6_update_ring_freq(struct drm_device *dev);
 void gen6_rps_idle(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 5728238..c082936 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
 	}
 	dev_priv->rps.enabled = true;
 	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	intel_runtime_pm_put(dev_priv);
 }
 
 void intel_enable_gt_powersave(struct drm_device *dev)
@@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
 		 * PCU communication is slow and this doesn't need to be
 		 * done at any specific time, so do this out of our fast path
 		 * to make resume and init faster.
+		 *
+		 * On platforms like Valleyview we depend on the HW RC6 power
+		 * context save/restore mechanism when entering D3 through
+		 * runtime PM suspend. So disable RPM until RPS/RC6 is
+		 * properly setup.
 		 */
+		if (!cancel_delayed_work(&dev_priv->rps.delayed_resume_work))
+			intel_runtime_pm_get_noresume(dev_priv);
 		schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
 				      round_jiffies_up_relative(HZ));
 	}
 }
 
+void intel_reset_gt_powersave(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	dev_priv->rps.enabled = false;
+	intel_enable_gt_powersave(dev);
+}
+
 static void ibx_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.8.4

Re: [PATCH 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

[Daniel Vetter]

On Tue, Apr 08, 2014 at 07:57:51PM +0300, Imre Deak wrote:
> At least on some platforms we depend on RC6 being enabled for RPM, so
> disable RPM until the delayed RC6 enabling completes. For simplicity
> don't differentiate between platforms, those that don't have this
> dependency will enable RC6 only rarely during driver init and system
> resume.
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
>  3 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 8dee34c..0948228 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
>  		 * of re-init after reset. */
>  		if (INTEL_INFO(dev)->gen > 5) {
>  			mutex_lock(&dev->struct_mutex);
> -			intel_enable_gt_powersave(dev);
> +			intel_reset_gt_powersave(dev);
>  			mutex_unlock(&dev->struct_mutex);
>  		}
>  
> @@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> +	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> +		return -ENODEV;
> +
>  	WARN_ON(!HAS_RUNTIME_PM(dev));
>  	assert_force_wake_inactive(dev_priv);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 42762b7..7f4e873 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
>  void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
> +void intel_reset_gt_powersave(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
>  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 5728238..c082936 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
>  	}
>  	dev_priv->rps.enabled = true;
>  	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	intel_runtime_pm_put(dev_priv);
>  }
>  
>  void intel_enable_gt_powersave(struct drm_device *dev)
> @@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
>  		 * PCU communication is slow and this doesn't need to be
>  		 * done at any specific time, so do this out of our fast path
>  		 * to make resume and init faster.
> +		 *
> +		 * On platforms like Valleyview we depend on the HW RC6 power
> +		 * context save/restore mechanism when entering D3 through
> +		 * runtime PM suspend. So disable RPM until RPS/RC6 is
> +		 * properly setup.
>  		 */

I'm pretty sure that depency also exists on other platforms since I did
wonder quite a bit how the current runtime pm stuff resurrects the render
state ;-)

Imo the right approach here is to grab a runtime pm reference in the
function that launches the async work and drop it again when that work has
completed. We already temporarily disable runtime pm in driver load and
suspend/resume code, so that should cover us.
-Daniel


> +		if (!cancel_delayed_work(&dev_priv->rps.delayed_resume_work))
> +			intel_runtime_pm_get_noresume(dev_priv);
>  		schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
>  				      round_jiffies_up_relative(HZ));
>  	}
>  }
>  
> +void intel_reset_gt_powersave(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	dev_priv->rps.enabled = false;
> +	intel_enable_gt_powersave(dev);
> +}
> +
>  static void ibx_init_clock_gating(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 1.8.4
> 
> _______________________________________________
> 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 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

[Imre Deak]

On Wed, 2014-04-09 at 16:19 +0200, Daniel Vetter wrote:
> On Tue, Apr 08, 2014 at 07:57:51PM +0300, Imre Deak wrote:
> > At least on some platforms we depend on RC6 being enabled for RPM, so
> > disable RPM until the delayed RC6 enabling completes. For simplicity
> > don't differentiate between platforms, those that don't have this
> > dependency will enable RC6 only rarely during driver init and system
> > resume.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
> >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> >  drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
> >  3 files changed, 22 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 8dee34c..0948228 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
> >  		 * of re-init after reset. */
> >  		if (INTEL_INFO(dev)->gen > 5) {
> >  			mutex_lock(&dev->struct_mutex);
> > -			intel_enable_gt_powersave(dev);
> > +			intel_reset_gt_powersave(dev);
> >  			mutex_unlock(&dev->struct_mutex);
> >  		}
> >  
> > @@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
> >  	struct drm_device *dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  
> > +	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> > +		return -ENODEV;
> > +
> >  	WARN_ON(!HAS_RUNTIME_PM(dev));
> >  	assert_force_wake_inactive(dev_priv);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 42762b7..7f4e873 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
> >  void intel_cleanup_gt_powersave(struct drm_device *dev);
> >  void intel_enable_gt_powersave(struct drm_device *dev);
> >  void intel_disable_gt_powersave(struct drm_device *dev);
> > +void intel_reset_gt_powersave(struct drm_device *dev);
> >  void ironlake_teardown_rc6(struct drm_device *dev);
> >  void gen6_update_ring_freq(struct drm_device *dev);
> >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 5728238..c082936 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> >  	}
> >  	dev_priv->rps.enabled = true;
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +	intel_runtime_pm_put(dev_priv);
> >  }
> >  
> >  void intel_enable_gt_powersave(struct drm_device *dev)
> > @@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> >  		 * PCU communication is slow and this doesn't need to be
> >  		 * done at any specific time, so do this out of our fast path
> >  		 * to make resume and init faster.
> > +		 *
> > +		 * On platforms like Valleyview we depend on the HW RC6 power
> > +		 * context save/restore mechanism when entering D3 through
> > +		 * runtime PM suspend. So disable RPM until RPS/RC6 is
> > +		 * properly setup.
> >  		 */
> 
> I'm pretty sure that depency also exists on other platforms since I did
> wonder quite a bit how the current runtime pm stuff resurrects the render
> state ;-)

Yes, that's possible, I haven't checked this for other platforms, but
they should be covered too after this patch. Well, we'll still need to
re-enable RPS for those during RPM resume. Perhaps that can be done in
the context of the refactoring work you asked for 14/15.

> Imo the right approach here is to grab a runtime pm reference in the
> function that launches the async work and drop it again when that work has
> completed.

Do you mean something different than what this patch does? I get here
the RPM reference when rps.delayed_resume_work is scheduled and drop it
when the work is completed. Note that after we enable RPM for VLV we
have to do this same sequence when re-enabling RPS from runtime resume,
hence the _noresume version.

> We already temporarily disable runtime pm in driver load and
> suspend/resume code, so that should cover us.

As stated above, we need the same re-enabling sequence during runtime
resume.

--Imre 


> -Daniel
> 
> 
> > +		if (!cancel_delayed_work(&dev_priv->rps.delayed_resume_work))
> > +			intel_runtime_pm_get_noresume(dev_priv);
> >  		schedule_delayed_work(&dev_priv->rps.delayed_resume_work,
> >  				      round_jiffies_up_relative(HZ));
> >  	}
> >  }
> >  
> > +void intel_reset_gt_powersave(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	dev_priv->rps.enabled = false;
> > +	intel_enable_gt_powersave(dev);
> > +}
> > +
> >  static void ibx_init_clock_gating(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

[Daniel Vetter]

On Wed, Apr 09, 2014 at 05:38:07PM +0300, Imre Deak wrote:
> On Wed, 2014-04-09 at 16:19 +0200, Daniel Vetter wrote:
> > On Tue, Apr 08, 2014 at 07:57:51PM +0300, Imre Deak wrote:
> > > At least on some platforms we depend on RC6 being enabled for RPM, so
> > > disable RPM until the delayed RC6 enabling completes. For simplicity
> > > don't differentiate between platforms, those that don't have this
> > > dependency will enable RC6 only rarely during driver init and system
> > > resume.
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
> > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > >  drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
> > >  3 files changed, 22 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 8dee34c..0948228 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
> > >  		 * of re-init after reset. */
> > >  		if (INTEL_INFO(dev)->gen > 5) {
> > >  			mutex_lock(&dev->struct_mutex);
> > > -			intel_enable_gt_powersave(dev);
> > > +			intel_reset_gt_powersave(dev);
> > >  			mutex_unlock(&dev->struct_mutex);
> > >  		}
> > >  
> > > @@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
> > >  	struct drm_device *dev = pci_get_drvdata(pdev);
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > >  
> > > +	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> > > +		return -ENODEV;
> > > +
> > >  	WARN_ON(!HAS_RUNTIME_PM(dev));
> > >  	assert_force_wake_inactive(dev_priv);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 42762b7..7f4e873 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
> > >  void intel_cleanup_gt_powersave(struct drm_device *dev);
> > >  void intel_enable_gt_powersave(struct drm_device *dev);
> > >  void intel_disable_gt_powersave(struct drm_device *dev);
> > > +void intel_reset_gt_powersave(struct drm_device *dev);
> > >  void ironlake_teardown_rc6(struct drm_device *dev);
> > >  void gen6_update_ring_freq(struct drm_device *dev);
> > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > index 5728238..c082936 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> > >  	}
> > >  	dev_priv->rps.enabled = true;
> > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > > +
> > > +	intel_runtime_pm_put(dev_priv);
> > >  }
> > >  
> > >  void intel_enable_gt_powersave(struct drm_device *dev)
> > > @@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> > >  		 * PCU communication is slow and this doesn't need to be
> > >  		 * done at any specific time, so do this out of our fast path
> > >  		 * to make resume and init faster.
> > > +		 *
> > > +		 * On platforms like Valleyview we depend on the HW RC6 power
> > > +		 * context save/restore mechanism when entering D3 through
> > > +		 * runtime PM suspend. So disable RPM until RPS/RC6 is
> > > +		 * properly setup.
> > >  		 */
> > 
> > I'm pretty sure that depency also exists on other platforms since I did
> > wonder quite a bit how the current runtime pm stuff resurrects the render
> > state ;-)
> 
> Yes, that's possible, I haven't checked this for other platforms, but
> they should be covered too after this patch. Well, we'll still need to
> re-enable RPS for those during RPM resume. Perhaps that can be done in
> the context of the refactoring work you asked for 14/15.
> 
> > Imo the right approach here is to grab a runtime pm reference in the
> > function that launches the async work and drop it again when that work has
> > completed.
> 
> Do you mean something different than what this patch does? I get here
> the RPM reference when rps.delayed_resume_work is scheduled and drop it
> when the work is completed. Note that after we enable RPM for VLV we
> have to do this same sequence when re-enabling RPS from runtime resume,
> hence the _noresume version.

Ah, just failed to read the patch, somehow was getting confused ... Please
extend this comment to explain why we need the _noresume variant, that
sounds like a really crucial detail.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/15] drm/i915: disable runtime PM until delayed RPS/RC6 enabling completes

[Daniel Vetter]

On Wed, Apr 09, 2014 at 06:38:30PM +0200, Daniel Vetter wrote:
> On Wed, Apr 09, 2014 at 05:38:07PM +0300, Imre Deak wrote:
> > On Wed, 2014-04-09 at 16:19 +0200, Daniel Vetter wrote:
> > > On Tue, Apr 08, 2014 at 07:57:51PM +0300, Imre Deak wrote:
> > > > At least on some platforms we depend on RC6 being enabled for RPM, so
> > > > disable RPM until the delayed RC6 enabling completes. For simplicity
> > > > don't differentiate between platforms, those that don't have this
> > > > dependency will enable RC6 only rarely during driver init and system
> > > > resume.
> > > > 
> > > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/i915_drv.c  |  5 ++++-
> > > >  drivers/gpu/drm/i915/intel_drv.h |  1 +
> > > >  drivers/gpu/drm/i915/intel_pm.c  | 17 +++++++++++++++++
> > > >  3 files changed, 22 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index 8dee34c..0948228 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -755,7 +755,7 @@ int i915_reset(struct drm_device *dev)
> > > >  		 * of re-init after reset. */
> > > >  		if (INTEL_INFO(dev)->gen > 5) {
> > > >  			mutex_lock(&dev->struct_mutex);
> > > > -			intel_enable_gt_powersave(dev);
> > > > +			intel_reset_gt_powersave(dev);
> > > >  			mutex_unlock(&dev->struct_mutex);
> > > >  		}
> > > >  
> > > > @@ -958,6 +958,9 @@ static int intel_runtime_suspend(struct device *device)
> > > >  	struct drm_device *dev = pci_get_drvdata(pdev);
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > >  
> > > > +	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> > > > +		return -ENODEV;
> > > > +
> > > >  	WARN_ON(!HAS_RUNTIME_PM(dev));
> > > >  	assert_force_wake_inactive(dev_priv);
> > > >  
> > > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > > index 42762b7..7f4e873 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -901,6 +901,7 @@ void intel_init_gt_powersave(struct drm_device *dev);
> > > >  void intel_cleanup_gt_powersave(struct drm_device *dev);
> > > >  void intel_enable_gt_powersave(struct drm_device *dev);
> > > >  void intel_disable_gt_powersave(struct drm_device *dev);
> > > > +void intel_reset_gt_powersave(struct drm_device *dev);
> > > >  void ironlake_teardown_rc6(struct drm_device *dev);
> > > >  void gen6_update_ring_freq(struct drm_device *dev);
> > > >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > > > index 5728238..c082936 100644
> > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > @@ -4519,6 +4519,8 @@ static void intel_gen6_powersave_work(struct work_struct *work)
> > > >  	}
> > > >  	dev_priv->rps.enabled = true;
> > > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > > > +
> > > > +	intel_runtime_pm_put(dev_priv);
> > > >  }
> > > >  
> > > >  void intel_enable_gt_powersave(struct drm_device *dev)
> > > > @@ -4534,12 +4536,27 @@ void intel_enable_gt_powersave(struct drm_device *dev)
> > > >  		 * PCU communication is slow and this doesn't need to be
> > > >  		 * done at any specific time, so do this out of our fast path
> > > >  		 * to make resume and init faster.
> > > > +		 *
> > > > +		 * On platforms like Valleyview we depend on the HW RC6 power
> > > > +		 * context save/restore mechanism when entering D3 through
> > > > +		 * runtime PM suspend. So disable RPM until RPS/RC6 is
> > > > +		 * properly setup.
> > > >  		 */
> > > 
> > > I'm pretty sure that depency also exists on other platforms since I did
> > > wonder quite a bit how the current runtime pm stuff resurrects the render
> > > state ;-)
> > 
> > Yes, that's possible, I haven't checked this for other platforms, but
> > they should be covered too after this patch. Well, we'll still need to
> > re-enable RPS for those during RPM resume. Perhaps that can be done in
> > the context of the refactoring work you asked for 14/15.
> > 
> > > Imo the right approach here is to grab a runtime pm reference in the
> > > function that launches the async work and drop it again when that work has
> > > completed.
> > 
> > Do you mean something different than what this patch does? I get here
> > the RPM reference when rps.delayed_resume_work is scheduled and drop it
> > when the work is completed. Note that after we enable RPM for VLV we
> > have to do this same sequence when re-enabling RPS from runtime resume,
> > hence the _noresume version.
> 
> Ah, just failed to read the patch, somehow was getting confused ... Please
> extend this comment to explain why we need the _noresume variant, that
> sounds like a really crucial detail.

Aside: Collaborating on this test with Mika would be good since he's
working on the initial render state stuff for hw contexts/rp6 on bdw.
Closely related topics imo. Adding Mika.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 11/15] drm/i915: vlv: disable RPM if RC6 is not enabled

[Imre Deak]

On VLV we depend on RC6 saving the GT render and media HW context before
going to D3 state, so disable RPM if RC6 is not enabled.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 drivers/gpu/drm/i915/intel_pm.c  | 29 ++++++++++++++++++++++++++++-
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7f4e873..b4fcd14 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -902,6 +902,7 @@ void intel_cleanup_gt_powersave(struct drm_device *dev);
 void intel_enable_gt_powersave(struct drm_device *dev);
 void intel_disable_gt_powersave(struct drm_device *dev);
 void intel_reset_gt_powersave(struct drm_device *dev);
+bool valleyview_rc6_enabled(struct drm_device *dev);
 void ironlake_teardown_rc6(struct drm_device *dev);
 void gen6_update_ring_freq(struct drm_device *dev);
 void gen6_rps_idle(struct drm_i915_private *dev_priv);
@@ -909,6 +910,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv);
 void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
 void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
+void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
 void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
 void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index c082936..15e6b38 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3660,6 +3660,11 @@ static void valleyview_cleanup_gt_powersave(struct drm_device *dev)
 	valleyview_cleanup_pctx(dev);
 }
 
+bool valleyview_rc6_enabled(struct drm_device *dev)
+{
+	return intel_enable_rc6(dev) & INTEL_RC6_ENABLE;
+}
+
 static void valleyview_enable_rps(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3709,7 +3714,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
 		   _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
 				      VLV_MEDIA_RC6_COUNT_EN |
 				      VLV_RENDER_RC6_COUNT_EN));
-	if (intel_enable_rc6(dev) & INTEL_RC6_ENABLE)
+	if (valleyview_rc6_enabled(dev))
 		rc6_mode = GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL;
 
 	intel_print_rc6_info(dev, rc6_mode);
@@ -6010,6 +6015,18 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
 	WARN(dev_priv->pm.suspended, "Device still suspended.\n");
 }
 
+void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	struct device *device = &dev->pdev->dev;
+
+	if (!HAS_RUNTIME_PM(dev))
+		return;
+
+	WARN(dev_priv->pm.suspended, "Can't get nosync-ref while suspended.\n");
+	pm_runtime_get_noresume(device);
+}
+
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
@@ -6032,6 +6049,13 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
 
 	pm_runtime_set_active(device);
 
+	/*
+	 * On Valleyview we depend on the HW RC6 power context save/restore
+	 * mechanism for RPM, so leave RPM disabled if RC6 is disabled.
+	 */
+	if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
+		return;
+
 	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
@@ -6047,6 +6071,9 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
 	if (!HAS_RUNTIME_PM(dev))
 		return;
 
+	if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
+		return;
+
 	/* Make sure we're not suspended first. */
 	pm_runtime_get_sync(device);
 	pm_runtime_disable(device);
-- 
1.8.4

Re: [PATCH 11/15] drm/i915: vlv: disable RPM if RC6 is not enabled

[Paulo Zanoni]

2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> On VLV we depend on RC6 saving the GT render and media HW context before
> going to D3 state, so disable RPM if RC6 is not enabled.

Maybe this problem affects other platforms too, and we just didn't
notice because RC6 is always enabled on them. Did we test this on the
other platforms?

Do we have a way to disable RC6 at runtime? If yes, then we could
probably try to write some test at pm_pc8.c.

>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c  | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 30 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7f4e873..b4fcd14 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -902,6 +902,7 @@ void intel_cleanup_gt_powersave(struct drm_device *dev);
>  void intel_enable_gt_powersave(struct drm_device *dev);
>  void intel_disable_gt_powersave(struct drm_device *dev);
>  void intel_reset_gt_powersave(struct drm_device *dev);
> +bool valleyview_rc6_enabled(struct drm_device *dev);
>  void ironlake_teardown_rc6(struct drm_device *dev);
>  void gen6_update_ring_freq(struct drm_device *dev);
>  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> @@ -909,6 +910,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
>  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
>  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
>  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index c082936..15e6b38 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3660,6 +3660,11 @@ static void valleyview_cleanup_gt_powersave(struct drm_device *dev)
>         valleyview_cleanup_pctx(dev);
>  }
>
> +bool valleyview_rc6_enabled(struct drm_device *dev)
> +{
> +       return intel_enable_rc6(dev) & INTEL_RC6_ENABLE;
> +}
> +
>  static void valleyview_enable_rps(struct drm_device *dev)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -3709,7 +3714,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
>                    _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
>                                       VLV_MEDIA_RC6_COUNT_EN |
>                                       VLV_RENDER_RC6_COUNT_EN));
> -       if (intel_enable_rc6(dev) & INTEL_RC6_ENABLE)
> +       if (valleyview_rc6_enabled(dev))
>                 rc6_mode = GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL;
>
>         intel_print_rc6_info(dev, rc6_mode);
> @@ -6010,6 +6015,18 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
>         WARN(dev_priv->pm.suspended, "Device still suspended.\n");
>  }
>
> +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       struct device *device = &dev->pdev->dev;
> +
> +       if (!HAS_RUNTIME_PM(dev))
> +               return;
> +
> +       WARN(dev_priv->pm.suspended, "Can't get nosync-ref while suspended.\n");
> +       pm_runtime_get_noresume(device);
> +}
> +
>  void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
> @@ -6032,6 +6049,13 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
>
>         pm_runtime_set_active(device);
>
> +       /*
> +        * On Valleyview we depend on the HW RC6 power context save/restore
> +        * mechanism for RPM, so leave RPM disabled if RC6 is disabled.
> +        */
> +       if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
> +               return;
> +
>         pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
>         pm_runtime_mark_last_busy(device);
>         pm_runtime_use_autosuspend(device);
> @@ -6047,6 +6071,9 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
>         if (!HAS_RUNTIME_PM(dev))
>                 return;
>
> +       if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
> +               return;
> +
>         /* Make sure we're not suspended first. */
>         pm_runtime_get_sync(device);
>         pm_runtime_disable(device);
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 11/15] drm/i915: vlv: disable RPM if RC6 is not enabled

[Imre Deak]

On Wed, 2014-04-09 at 11:36 -0300, Paulo Zanoni wrote:
> 2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > On VLV we depend on RC6 saving the GT render and media HW context before
> > going to D3 state, so disable RPM if RC6 is not enabled.
> 
> Maybe this problem affects other platforms too, and we just didn't
> notice because RC6 is always enabled on them. Did we test this on the
> other platforms?

No, tbh I wanted to submit this as early as possible. I think now with
patch 10/15 RPM should be disabled for all GEN6+ platforms. 

> Do we have a way to disable RC6 at runtime? If yes, then we could
> probably try to write some test at pm_pc8.c.

It's a module option, but I can look into adding a test for it.

--Imre

> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h |  2 ++
> >  drivers/gpu/drm/i915/intel_pm.c  | 29 ++++++++++++++++++++++++++++-
> >  2 files changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7f4e873..b4fcd14 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -902,6 +902,7 @@ void intel_cleanup_gt_powersave(struct drm_device *dev);
> >  void intel_enable_gt_powersave(struct drm_device *dev);
> >  void intel_disable_gt_powersave(struct drm_device *dev);
> >  void intel_reset_gt_powersave(struct drm_device *dev);
> > +bool valleyview_rc6_enabled(struct drm_device *dev);
> >  void ironlake_teardown_rc6(struct drm_device *dev);
> >  void gen6_update_ring_freq(struct drm_device *dev);
> >  void gen6_rps_idle(struct drm_i915_private *dev_priv);
> > @@ -909,6 +910,7 @@ void gen6_rps_boost(struct drm_i915_private *dev_priv);
> >  void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
> >  void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
> >  void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
> > +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
> >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
> >  void intel_init_runtime_pm(struct drm_i915_private *dev_priv);
> >  void intel_fini_runtime_pm(struct drm_i915_private *dev_priv);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index c082936..15e6b38 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -3660,6 +3660,11 @@ static void valleyview_cleanup_gt_powersave(struct drm_device *dev)
> >         valleyview_cleanup_pctx(dev);
> >  }
> >
> > +bool valleyview_rc6_enabled(struct drm_device *dev)
> > +{
> > +       return intel_enable_rc6(dev) & INTEL_RC6_ENABLE;
> > +}
> > +
> >  static void valleyview_enable_rps(struct drm_device *dev)
> >  {
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > @@ -3709,7 +3714,7 @@ static void valleyview_enable_rps(struct drm_device *dev)
> >                    _MASKED_BIT_ENABLE(VLV_COUNT_RANGE_HIGH |
> >                                       VLV_MEDIA_RC6_COUNT_EN |
> >                                       VLV_RENDER_RC6_COUNT_EN));
> > -       if (intel_enable_rc6(dev) & INTEL_RC6_ENABLE)
> > +       if (valleyview_rc6_enabled(dev))
> >                 rc6_mode = GEN7_RC_CTL_TO_MODE | VLV_RC_CTL_CTX_RST_PARALLEL;
> >
> >         intel_print_rc6_info(dev, rc6_mode);
> > @@ -6010,6 +6015,18 @@ void intel_runtime_pm_get(struct drm_i915_private *dev_priv)
> >         WARN(dev_priv->pm.suspended, "Device still suspended.\n");
> >  }
> >
> > +void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv)
> > +{
> > +       struct drm_device *dev = dev_priv->dev;
> > +       struct device *device = &dev->pdev->dev;
> > +
> > +       if (!HAS_RUNTIME_PM(dev))
> > +               return;
> > +
> > +       WARN(dev_priv->pm.suspended, "Can't get nosync-ref while suspended.\n");
> > +       pm_runtime_get_noresume(device);
> > +}
> > +
> >  void intel_runtime_pm_put(struct drm_i915_private *dev_priv)
> >  {
> >         struct drm_device *dev = dev_priv->dev;
> > @@ -6032,6 +6049,13 @@ void intel_init_runtime_pm(struct drm_i915_private *dev_priv)
> >
> >         pm_runtime_set_active(device);
> >
> > +       /*
> > +        * On Valleyview we depend on the HW RC6 power context save/restore
> > +        * mechanism for RPM, so leave RPM disabled if RC6 is disabled.
> > +        */
> > +       if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
> > +               return;
> > +
> >         pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
> >         pm_runtime_mark_last_busy(device);
> >         pm_runtime_use_autosuspend(device);
> > @@ -6047,6 +6071,9 @@ void intel_fini_runtime_pm(struct drm_i915_private *dev_priv)
> >         if (!HAS_RUNTIME_PM(dev))
> >                 return;
> >
> > +       if (IS_VALLEYVIEW(dev) && !valleyview_rc6_enabled(dev))
> > +               return;
> > +
> >         /* Make sure we're not suspended first. */
> >         pm_runtime_get_sync(device);
> >         pm_runtime_disable(device);
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

Re: [PATCH 11/15] drm/i915: vlv: disable RPM if RC6 is not enabled

[Daniel Vetter]

On Wed, Apr 09, 2014 at 05:59:30PM +0300, Imre Deak wrote:
> On Wed, 2014-04-09 at 11:36 -0300, Paulo Zanoni wrote:
> > 2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > > On VLV we depend on RC6 saving the GT render and media HW context before
> > > going to D3 state, so disable RPM if RC6 is not enabled.
> > 
> > Maybe this problem affects other platforms too, and we just didn't
> > notice because RC6 is always enabled on them. Did we test this on the
> > other platforms?
> 
> No, tbh I wanted to submit this as early as possible. I think now with
> patch 10/15 RPM should be disabled for all GEN6+ platforms. 
> 
> > Do we have a way to disable RC6 at runtime? If yes, then we could
> > probably try to write some test at pm_pc8.c.
> 
> It's a module option, but I can look into adding a test for it.

Doing a test for this is fairly tricky and since disabling rc6 isn't a
supported option really not that useful imo. What might be genuinely
useful though is checking that the entire render state gets restored
correctly by doing
- rendercpy batch
- make sure we go into runtime suspend
- re-emit the rendercpy 3D_PRIM command _without_ all the state setup

This obviously needs hw contexts to work. If we somehow fail to restore
some of the render state then this should nicely blow up all over the
place. Of course it's a crude check, but better than just doing a blit or
something which essentially just checks that the rings and ptes still
work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 12/15] drm/i915: add various missing GTI/Gunit register definitions

[Imre Deak]

Needed by the VLV S0ix context save/restore helpers.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 43 ++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c4fd089..4796c3c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -92,6 +92,9 @@
 #define   GEN6_MBC_SNPCR_LOW	(2<<21)
 #define   GEN6_MBC_SNPCR_MIN	(3<<21) /* only 1/16th of the cache is shared */
 
+#define GEN7_G3DCTL		0x9024
+#define GEN7_GSCKGCTL		0x9028
+
 #define GEN6_MBCTL		0x0907c
 #define   GEN6_MBCTL_ENABLE_BOOT_FETCH	(1 << 4)
 #define   GEN6_MBCTL_CTX_FETCH_NEEDED	(1 << 3)
@@ -775,9 +778,20 @@ enum punit_power_well {
 #define RING_MAX_IDLE(base)	((base)+0x54)
 #define RING_HWS_PGA(base)	((base)+0x80)
 #define RING_HWS_PGA_GEN6(base)	((base)+0x2080)
-#define ARB_MODE		0x04030
+
+#define GEN7_WR_WATERMARK	0x4028
+#define GEN7_GFX_PRIO_CTRL	0x402C
+#define ARB_MODE		0x4030
 #define   ARB_MODE_SWIZZLE_SNB	(1<<4)
 #define   ARB_MODE_SWIZZLE_IVB	(1<<5)
+#define GEN7_GFX_PEND_TLB0	0x4034
+#define GEN7_GFX_PEND_TLB1	0x4038
+/* L3, CVS, ZTLB, RCC, CASC LRA min, max values */
+#define GEN7_LRA_LIMITS_BASE	0x403C
+#define GEN7_LRA_LIMITS_REG_NUM	13
+#define GEN7_MEDIA_MAX_REQ_COUNT	0x4070
+#define GEN7_GFX_MAX_REQ_COUNT		0x4074
+
 #define GAMTARBMODE		0x04a08
 #define   ARB_MODE_BWGTLB_DISABLE (1<<9)
 #define   ARB_MODE_SWIZZLE_BDW	(1<<1)
@@ -812,6 +826,9 @@ enum punit_power_well {
 #define   RING_WAIT_I8XX	(1<<0) /* gen2, PRBx_HEAD */
 #define   RING_WAIT		(1<<11) /* gen3+, PRBx_CTL */
 #define   RING_WAIT_SEMAPHORE	(1<<10) /* gen6+ */
+
+#define GEN7_TLB_RD_ADDR	0x4700
+
 #if 0
 #define PRB0_TAIL	0x02030
 #define PRB0_HEAD	0x02034
@@ -938,6 +955,8 @@ enum punit_power_well {
 
 #define VLV_DISPLAY_BASE 0x180000
 
+#define VLV_GU_CTL0	(VLV_DISPLAY_BASE + 0x2030)
+#define VLV_GU_CTL1	(VLV_DISPLAY_BASE + 0x2034)
 #define SCPD0		0x0209c /* 915+ only */
 #define IER		0x020a0
 #define IIR		0x020a4
@@ -945,6 +964,7 @@ enum punit_power_well {
 #define ISR		0x020ac
 #define VLV_GUNIT_CLOCK_GATE	(VLV_DISPLAY_BASE + 0x2060)
 #define   GCFG_DIS		(1<<8)
+#define VLV_GUNIT_CLOCK_GATE2	(VLV_DISPLAY_BASE + 0x2064)
 #define VLV_IIR_RW	(VLV_DISPLAY_BASE + 0x2084)
 #define VLV_IER		(VLV_DISPLAY_BASE + 0x20a0)
 #define VLV_IIR		(VLV_DISPLAY_BASE + 0x20a4)
@@ -4977,6 +4997,8 @@ enum punit_power_well {
 
 #define  EDP_LINK_TRAIN_VOL_EMP_MASK_IVB	(0x3f<<22)
 
+#define  VLV_PMWGICZ				0x1300a4
+
 #define  FORCEWAKE				0xA18C
 #define  FORCEWAKE_VLV				0x1300b0
 #define  FORCEWAKE_ACK_VLV			0x1300b4
@@ -5000,6 +5022,7 @@ enum punit_power_well {
 #define  FORCEWAKE_MT_ACK			0x130040
 #define  ECOBUS					0xa180
 #define    FORCEWAKE_MT_ENABLE			(1<<5)
+#define  VLV_SPAREG2H				0xA194
 
 #define  GTFIFODBG				0x120000
 #define    GT_FIFO_SBDROPERR			(1<<6)
@@ -5029,12 +5052,24 @@ enum punit_power_well {
 # define GEN6_RCPBUNIT_CLOCK_GATE_DISABLE		(1 << 12)
 # define GEN6_RCCUNIT_CLOCK_GATE_DISABLE		(1 << 11)
 
+#define GEN7_UCGCTL3				0x9408
+
 #define GEN7_UCGCTL4				0x940c
 #define  GEN7_L3BANK2X_CLOCK_GATE_DISABLE	(1<<25)
 
+#define GEN7_RCGCTL1				0x9410
+#define GEN7_RCGCTL2				0x9414
+#define GEN7_RSTCTL				0x9420
+
 #define GEN8_UCGCTL6				0x9430
 #define   GEN8_SDEUNIT_CLOCK_GATE_DISABLE	(1<<14)
 
+#define GEN7_GFXPAUSE				0xA000
+#define GEN7_RPDEUHWTC				0xA080
+#define GEN7_RPDEUC				0xA084
+
+#define VLV_PWRDWNUPCTL				0xA294
+
 #define GEN6_RPNSWREQ				0xA008
 #define   GEN6_TURBO_DISABLE			(1<<31)
 #define   GEN6_FREQUENCY(x)			((x)<<25)
@@ -5087,6 +5122,7 @@ enum punit_power_well {
 #define GEN6_RP_UP_EI				0xA068
 #define GEN6_RP_DOWN_EI				0xA06C
 #define GEN6_RP_IDLE_HYSTERSIS			0xA070
+#define GEN7_RPDEUCSW				0xA088
 #define GEN6_RC_STATE				0xA094
 #define GEN6_RC1_WAKE_RATE_LIMIT		0xA098
 #define GEN6_RC6_WAKE_RATE_LIMIT		0xA09C
@@ -5094,9 +5130,11 @@ enum punit_power_well {
 #define GEN6_RC_EVALUATION_INTERVAL		0xA0A8
 #define GEN6_RC_IDLE_HYSTERSIS			0xA0AC
 #define GEN6_RC_SLEEP				0xA0B0
+#define VLV_RCUBMABDTMR				0xA0B0
 #define GEN6_RC1e_THRESHOLD			0xA0B4
 #define GEN6_RC6_THRESHOLD			0xA0B8
 #define GEN6_RC6p_THRESHOLD			0xA0BC
+#define VLV_RCEDATA				0xA0BC
 #define GEN6_RC6pp_THRESHOLD			0xA0C0
 #define GEN6_PMINTRMSK				0xA168
 
@@ -5115,6 +5153,9 @@ enum punit_power_well {
 						 GEN6_PM_RP_DOWN_THRESHOLD | \
 						 GEN6_PM_RP_DOWN_TIMEOUT)
 
+#define GEN7_GT_SCRATCH_BASE			0x4F100
+#define GEN7_GT_SCRATCH_REG_NUM			8
+
 #define VLV_GTLC_SURVIVABILITY_REG              0x130098
 #define VLV_GFX_CLK_STATUS_BIT			(1<<3)
 #define VLV_GFX_CLK_FORCE_ON_BIT		(1<<2)
-- 
1.8.4

[PATCH 13/15] drm/i915: vlv: add gunit s0ix save/restore helpers

[Imre Deak]

Needed by the next patch adding RPM suspend/resume support to VLV.

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 192 ++++++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h |  62 +++++++++++++
 2 files changed, 254 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0948228..9f9d0db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -920,6 +920,198 @@ static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
 	hsw_disable_pc8(dev_priv);
 }
 
+/*
+ * Save all Gunit registers that may be lost after a D3 and a subsequent
+ * S0i[R123] transition. The list of registers needing a save/restore is
+ * defined in the VLV2_S0IXRegs document. This documents marks all Gunit
+ * registers in the following way:
+ * - Driver: saved/restored by the driver
+ * - Punit : saved/restored by the Punit firmware
+ * - No, w/o marking: no need to save/restore, since the register is R/O or
+ *                    used internally by the HW in a way that doesn't depend
+ *                    keeping the content across a suspend/resume.
+ * - Debug : used for debugging
+ *
+ * We save/restore all registers marked with 'Driver', with the following
+ * exceptions:
+ * - Registers out of use, including also registers marked with 'Debug'.
+ *   These have no effect on the driver's operation, so we don't save/restore
+ *   them to reduce the overhead.
+ * - Registers that are fully setup by an initialization function called from
+ *   the resume path. For example many clock gating and RPS/RC6 registers.
+ * - Registers that provide the right functionality with their reset defaults.
+ *
+ * TODO: Except for registers that based on the above 3 criteria can be safely
+ * ignored, we save/restore all others, practically treating the HW context as
+ * a black-box for the driver. Further investigation is needed to reduce the
+ * saved/restored registers even further, by following the same 3 criteria.
+ */
+static void vlv_save_gunit_s0ix_state(struct drm_i915_private *dev_priv)
+{
+	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
+	int i;
+
+	/* GAM 0x4000-0x4770 */
+	s->wr_watermark		= I915_READ(GEN7_WR_WATERMARK);
+	s->gfx_prio_ctrl	= I915_READ(GEN7_GFX_PRIO_CTRL);
+	s->arb_mode		= I915_READ(ARB_MODE);
+	s->gfx_pend_tlb0	= I915_READ(GEN7_GFX_PEND_TLB0);
+	s->gfx_pend_tlb1	= I915_READ(GEN7_GFX_PEND_TLB1);
+
+	for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++)
+		s->lra_limits[i] = I915_READ(GEN7_LRA_LIMITS_BASE + i * 4);
+
+	s->media_max_req_count	= I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
+	s->gfx_max_req_count	= I915_READ(GEN7_MEDIA_MAX_REQ_COUNT);
+
+	s->render_hwsp		= I915_READ(RENDER_HWS_PGA_GEN7);
+	s->ecochk		= I915_READ(GAM_ECOCHK);
+	s->bsd_hwsp		= I915_READ(BSD_HWS_PGA_GEN7);
+	s->blt_hwsp		= I915_READ(BLT_HWS_PGA_GEN7);
+
+	s->tlb_rd_addr		= I915_READ(GEN7_TLB_RD_ADDR);
+
+	/* MBC 0x9024-0x91D0, 0x8500 */
+	s->g3dctl		= I915_READ(GEN7_G3DCTL);
+	s->gsckgctl		= I915_READ(GEN7_GSCKGCTL);
+	s->mbctl		= I915_READ(GEN6_MBCTL);
+
+	/* GCP 0x9400-0x9424, 0x8100-0x810C */
+	s->ucgctl1		= I915_READ(GEN6_UCGCTL1);
+	s->ucgctl3		= I915_READ(GEN7_UCGCTL3);
+	s->rcgctl1		= I915_READ(GEN7_RCGCTL1);
+	s->rcgctl2		= I915_READ(GEN7_RCGCTL2);
+	s->rstctl		= I915_READ(GEN7_RSTCTL);
+	s->misccpctl		= I915_READ(GEN7_MISCCPCTL);
+
+	/* GPM 0xA000-0xAA84, 0x8000-0x80FC */
+	s->gfxpause		= I915_READ(GEN7_GFXPAUSE);
+	s->rpdeuhwtc		= I915_READ(GEN7_RPDEUHWTC);
+	s->rpdeuc		= I915_READ(GEN7_RPDEUC);
+	s->ecobus		= I915_READ(ECOBUS);
+	s->pwrdwnupctl		= I915_READ(VLV_PWRDWNUPCTL);
+	s->rp_down_timeout	= I915_READ(GEN6_RP_DOWN_TIMEOUT);
+	s->rp_deucsw		= I915_READ(GEN7_RPDEUCSW);
+	s->rcubmabdtmr		= I915_READ(VLV_RCUBMABDTMR);
+	s->rcedata		= I915_READ(VLV_RCEDATA);
+	s->spare2gh		= I915_READ(VLV_SPAREG2H);
+
+	/* Display CZ domain, 0x4400C-0x4402C, 0x4F000-0x4F11F */
+	s->gt_imr		= I915_READ(GTIMR);
+	s->gt_ier		= I915_READ(GTIER);
+	s->pm_imr		= I915_READ(GEN6_PMIIR);
+	s->pm_ier		= I915_READ(GEN6_PMIER);
+
+	for (i = 0; i < ARRAY_SIZE(s->gt_scratch); i++)
+		s->gt_scratch[i] = I915_READ(GEN7_GT_SCRATCH_BASE + i * 4);
+
+	/* GT SA CZ domain, 0x100000-0x138124 */
+	s->tilectl		= I915_READ(TILECTL);
+	s->gt_fifoctl		= I915_READ(GTFIFOCTL);
+	s->gtlc_wake_ctrl	= I915_READ(VLV_GTLC_WAKE_CTRL);
+	s->gtlc_survive		= I915_READ(VLV_GTLC_SURVIVABILITY_REG);
+	s->pmwgicz		= I915_READ(VLV_PMWGICZ);
+
+	/* Gunit-Display CZ domain, 0x182028-0x1821CF */
+	s->gu_ctl0		= I915_READ(VLV_GU_CTL0);
+	s->gu_ctl1		= I915_READ(VLV_GU_CTL1);
+	s->clock_gate_dis2	= I915_READ(VLV_GUNIT_CLOCK_GATE2);
+
+	/*
+	 * Not saving any of:
+	 * DFT,		0x9800-0x9EC0
+	 * SARB,	0xB000-0xB1FC
+	 * GAC,		0x5208-0x524C, 0x14000-0x14C000
+	 * PCI CFG
+	 */
+}
+
+static void vlv_restore_gunit_s0ix_state(struct drm_i915_private *dev_priv)
+{
+	struct vlv_s0ix_state *s = &dev_priv->vlv_s0ix_state;
+	u32 val;
+	int i;
+
+	/* GAM 0x4000-0x4770 */
+	I915_WRITE(GEN7_WR_WATERMARK,	s->wr_watermark);
+	I915_WRITE(GEN7_GFX_PRIO_CTRL,	s->gfx_prio_ctrl);
+	I915_WRITE(ARB_MODE,		s->arb_mode | (0xffff << 16));
+	I915_WRITE(GEN7_GFX_PEND_TLB0,	s->gfx_pend_tlb0);
+	I915_WRITE(GEN7_GFX_PEND_TLB1,	s->gfx_pend_tlb1);
+
+	for (i = 0; i < ARRAY_SIZE(s->lra_limits); i++)
+		I915_WRITE(GEN7_LRA_LIMITS_BASE + i * 4, s->lra_limits[i]);
+
+	I915_WRITE(GEN7_MEDIA_MAX_REQ_COUNT, s->media_max_req_count);
+	I915_WRITE(GEN7_MEDIA_MAX_REQ_COUNT, s->gfx_max_req_count);
+
+	I915_WRITE(RENDER_HWS_PGA_GEN7,	s->render_hwsp);
+	I915_WRITE(GAM_ECOCHK,		s->ecochk);
+	I915_WRITE(BSD_HWS_PGA_GEN7,	s->bsd_hwsp);
+	I915_WRITE(BLT_HWS_PGA_GEN7,	s->blt_hwsp);
+
+	I915_WRITE(GEN7_TLB_RD_ADDR,	s->tlb_rd_addr);
+
+	/* MBC 0x9024-0x91D0, 0x8500 */
+	I915_WRITE(GEN7_G3DCTL,		s->g3dctl);
+	I915_WRITE(GEN7_GSCKGCTL,	s->gsckgctl);
+	I915_WRITE(GEN6_MBCTL,		s->mbctl);
+
+	/* GCP 0x9400-0x9424, 0x8100-0x810C */
+	I915_WRITE(GEN6_UCGCTL1,	s->ucgctl1);
+	I915_WRITE(GEN7_UCGCTL3,	s->ucgctl3);
+	I915_WRITE(GEN7_RCGCTL1,	s->rcgctl1);
+	I915_WRITE(GEN7_RCGCTL2,	s->rcgctl2);
+	I915_WRITE(GEN7_RSTCTL,		s->rstctl);
+	I915_WRITE(GEN7_MISCCPCTL,	s->misccpctl);
+
+	/* GPM 0xA000-0xAA84, 0x8000-0x80FC */
+	I915_WRITE(GEN7_GFXPAUSE,	s->gfxpause);
+	I915_WRITE(GEN7_RPDEUHWTC,	s->rpdeuhwtc);
+	I915_WRITE(GEN7_RPDEUC,		s->rpdeuc);
+	I915_WRITE(ECOBUS,		s->ecobus);
+	I915_WRITE(VLV_PWRDWNUPCTL,	s->pwrdwnupctl);
+	I915_WRITE(GEN6_RP_DOWN_TIMEOUT,s->rp_down_timeout);
+	I915_WRITE(GEN7_RPDEUCSW,	s->rp_deucsw);
+	I915_WRITE(VLV_RCUBMABDTMR,	s->rcubmabdtmr);
+	I915_WRITE(VLV_RCEDATA,		s->rcedata);
+	I915_WRITE(VLV_SPAREG2H,	s->spare2gh);
+
+	/* Display CZ domain, 0x4400C-0x4402C, 0x4F000-0x4F11F */
+	I915_WRITE(GTIMR,		s->gt_imr);
+	I915_WRITE(GTIER,		s->gt_ier);
+	I915_WRITE(GEN6_PMIIR,		s->pm_imr);
+	I915_WRITE(GEN6_PMIER,		s->pm_ier);
+
+	for (i = 0; i < ARRAY_SIZE(s->gt_scratch); i++)
+		I915_WRITE(GEN7_GT_SCRATCH_BASE + i * 4, s->gt_scratch[i]);
+
+	/* GT SA CZ domain, 0x100000-0x138124 */
+	I915_WRITE(TILECTL,			s->tilectl);
+	I915_WRITE(GTFIFOCTL,			s->gt_fifoctl);
+	/*
+	 * Preserve the GT allow wake and GFX force clock bit, they are not
+	 * be restored, as they are used to control the s0ix suspend/resume
+	 * sequence by the caller.
+	 */
+	val = I915_READ(VLV_GTLC_WAKE_CTRL);
+	val &= VLV_GTLC_ALLOWWAKEREQ;
+	val |= s->gtlc_wake_ctrl & ~VLV_GTLC_ALLOWWAKEREQ;
+	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
+
+	val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
+	val &= VLV_GFX_CLK_FORCE_ON_BIT;
+	val |= s->gtlc_survive & ~VLV_GFX_CLK_FORCE_ON_BIT;
+	I915_WRITE(VLV_GTLC_SURVIVABILITY_REG,	val);
+
+	I915_WRITE(VLV_PMWGICZ,			s->pmwgicz);
+
+	/* Gunit-Display CZ domain, 0x182028-0x1821CF */
+	I915_WRITE(VLV_GU_CTL0,			s->gu_ctl0);
+	I915_WRITE(VLV_GU_CTL1,			s->gu_ctl1);
+	I915_WRITE(VLV_GUNIT_CLOCK_GATE2,	s->clock_gate_dis2);
+}
+
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 {
 	u32 val;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 0800429..d94e10a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -816,6 +816,67 @@ struct i915_suspend_saved_registers {
 	u32 savePCH_PORT_HOTPLUG;
 };
 
+struct vlv_s0ix_state {
+	/* GAM */
+	u32 wr_watermark;
+	u32 gfx_prio_ctrl;
+	u32 arb_mode;
+	u32 gfx_pend_tlb0;
+	u32 gfx_pend_tlb1;
+	u32 lra_limits[GEN7_LRA_LIMITS_REG_NUM];
+	u32 media_max_req_count;
+	u32 gfx_max_req_count;
+	u32 render_hwsp;
+	u32 ecochk;
+	u32 bsd_hwsp;
+	u32 blt_hwsp;
+	u32 tlb_rd_addr;
+
+	/* MBC */
+	u32 g3dctl;
+	u32 gsckgctl;
+	u32 mbctl;
+
+	/* GCP */
+	u32 ucgctl1;
+	u32 ucgctl3;
+	u32 rcgctl1;
+	u32 rcgctl2;
+	u32 rstctl;
+	u32 misccpctl;
+
+	/* GPM */
+	u32 gfxpause;
+	u32 rpdeuhwtc;
+	u32 rpdeuc;
+	u32 ecobus;
+	u32 pwrdwnupctl;
+	u32 rp_down_timeout;
+	u32 rp_deucsw;
+	u32 rcubmabdtmr;
+	u32 rcedata;
+	u32 spare2gh;
+
+	/* Display 1 CZ domain */
+	u32 gt_imr;
+	u32 gt_ier;
+	u32 pm_imr;
+	u32 pm_ier;
+	u32 gt_scratch[GEN7_GT_SCRATCH_REG_NUM];
+
+	/* GT SA CZ domain */
+	u32 tilectl;
+	u32 gt_fifoctl;
+	u32 gtlc_wake_ctrl;
+	u32 gtlc_survive;
+	u32 pmwgicz;
+
+	/* Display 2 CZ domain */
+	u32 gu_ctl0;
+	u32 gu_ctl1;
+	u32 clock_gate_dis2;
+};
+
 struct intel_gen6_power_mgmt {
 	/* work and pm_iir are protected by dev_priv->irq_lock */
 	struct work_struct work;
@@ -1432,6 +1493,7 @@ struct drm_i915_private {
 
 	u32 suspend_count;
 	struct i915_suspend_saved_registers regfile;
+	struct vlv_s0ix_state vlv_s0ix_state;
 
 	struct {
 		/*
-- 
1.8.4

[PATCH 14/15] drm/i915: vlv: add runtime PM support

[Imre Deak]

Add runtime PM support for VLV, but leave it disabled. The next patch
enables it.

The suspend/resume sequence used is based on [1] and [2]. In practice we
depend on the GT RC6 mechanism to save the HW context depending on the
render and media power wells. By the time we run the runtime suspend
callback the display side is also off and the HW context for that is
managed by the display power domain framework.

Besides the above there are Gunit registers that depend on a system-wide
power well. This power well goes off once the device enters any of the
S0i[R123] states. To handle this scenario, save/restore these Gunit
registers. Note that this is not the complete register set dictated by
[2], to remove some overhead registers that are known not to be used are
ignored. Also some registers are fully setup by initialization functions
called during resume, these are not saved either. The list of registers
can be further reduced, see the TODO note in the code.

[1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
[2] VLV2_S0IXRegs

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 166 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9f9d0db..16ca37f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
 	return i915_drm_freeze(drm_dev);
 }
 
-static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
+static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
 	intel_runtime_pm_disable_interrupts(dev);
+
+	return 0;
 }
 
-static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
+static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	hsw_enable_pc8(dev_priv);
+
+	return 0;
 }
 
-static void snb_runtime_resume(struct drm_i915_private *dev_priv)
+static int snb_runtime_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = dev_priv->dev;
 
@@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
 	mutex_lock(&dev_priv->rps.hw_lock);
 	gen6_update_ring_freq(dev);
 	mutex_unlock(&dev_priv->rps.hw_lock);
+
+	return 0;
 }
 
-static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
+static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
 {
 	hsw_disable_pc8(dev_priv);
+
+	return 0;
 }
 
 /*
@@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
 #undef COND
 }
 
+static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
+{
+	u32 val;
+	int err = 0;
+
+	val = I915_READ(VLV_GTLC_WAKE_CTRL);
+	val &= ~VLV_GTLC_ALLOWWAKEREQ;
+	if (allow)
+		val |= VLV_GTLC_ALLOWWAKEREQ;
+	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
+	POSTING_READ(VLV_GTLC_WAKE_CTRL);
+
+#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
+	      allow)
+	err = wait_for(COND, 1);
+	if (err)
+		DRM_ERROR("timeout disabling GT waking\n");
+	return err;
+#undef COND
+}
+
+static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
+				 bool wait_for_on)
+{
+	u32 mask;
+	u32 val;
+	int err;
+
+	mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
+	val = wait_for_on ? mask : 0;
+#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
+	if (COND)
+		return 0;
+
+	DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
+			wait_for_on ? "on" : "off",
+			I915_READ(VLV_GTLC_PW_STATUS));
+
+	/*
+	 * RC6 transitioning can be delayed up to 2 msec (see
+	 * valleyview_enable_rps), use 3 msec for safety.
+	 */
+	err = wait_for(COND, 3);
+	if (err)
+		DRM_ERROR("timeout waiting for GT wells to go %s\n",
+			  wait_for_on ? "on" : "off");
+
+	return err;
+#undef COND
+}
+
+static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
+{
+	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
+		return;
+
+	DRM_ERROR("GT register access while GT waking disabled\n");
+	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
+}
+
+static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	u32 mask;
+	int err;
+
+	if (WARN_ON(!valleyview_rc6_enabled(dev)))
+		return -ENODEV;
+
+	intel_runtime_pm_disable_interrupts(dev);
+	cancel_work_sync(&dev_priv->rps.work);
+
+	/*
+	 * Bspec defines the following GT well on flags as debug only, so
+	 * don't treat them as hard failures.
+	 */
+	(void)vlv_wait_for_gt_wells(dev_priv, false);
+
+	mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
+	WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
+
+	vlv_check_no_gt_access(dev_priv);
+
+	err = vlv_force_gfx_clock(dev_priv, true);
+	if (err)
+		goto err1;
+
+	err = vlv_allow_gt_wake(dev_priv, false);
+	if (err)
+		goto err2;
+	vlv_save_gunit_s0ix_state(dev_priv);
+
+	err = vlv_force_gfx_clock(dev_priv, false);
+	if (err)
+		goto err2;
+
+	return 0;
+
+err2:
+	/* For safety always re-enable waking and disable gfx clock forcing */
+	vlv_allow_gt_wake(dev_priv, true);
+err1:
+	vlv_force_gfx_clock(dev_priv, false);
+	intel_runtime_pm_restore_interrupts(dev);
+
+	return err;
+}
+
+static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = dev_priv->dev;
+	int err;
+	int ret;
+
+	/*
+	 * If any of the steps fail just try to continue, that's the best we
+	 * can do at this point. Return the first error code (which will also
+	 * leave RPM permanentyl disabled).
+	 */
+	ret = vlv_force_gfx_clock(dev_priv, true);
+
+	vlv_restore_gunit_s0ix_state(dev_priv);
+
+	err = vlv_allow_gt_wake(dev_priv, true);
+	if (!ret)
+		ret = err;
+
+	err = vlv_force_gfx_clock(dev_priv, false);
+	if (!ret)
+		ret = err;
+
+	vlv_check_no_gt_access(dev_priv);
+
+	intel_init_clock_gating(dev);
+	intel_reset_gt_powersave(dev);
+	i915_gem_init_swizzling(dev);
+	i915_gem_restore_fences(dev);
+
+	intel_runtime_pm_restore_interrupts(dev);
+
+	return ret;
+}
+
 static int intel_runtime_suspend(struct device *device)
 {
 	struct pci_dev *pdev = to_pci_dev(device);
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret = 0;
 
 	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
 		return -ENODEV;
@@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
 		snb_runtime_suspend(dev_priv);
 	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_runtime_suspend(dev_priv);
+	else if (IS_VALLEYVIEW(dev))
+		ret = vlv_runtime_suspend(dev_priv);
 	else
 		WARN_ON(1);
 
+	if (ret) {
+		DRM_ERROR("Runtime suspend failed, disabling it\n");
+
+		return ret;
+	}
+
 	i915_gem_release_all_mmaps(dev_priv);
 
 	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
@@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
 		snb_runtime_resume(dev_priv);
 	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
 		hsw_runtime_resume(dev_priv);
+	else if (IS_VALLEYVIEW(dev))
+		vlv_runtime_resume(dev_priv);
 	else
 		WARN_ON(1);
 
-- 
1.8.4

Re: [PATCH 14/15] drm/i915: vlv: add runtime PM support

[Daniel Vetter]

On Tue, Apr 08, 2014 at 07:57:55PM +0300, Imre Deak wrote:
> Add runtime PM support for VLV, but leave it disabled. The next patch
> enables it.
> 
> The suspend/resume sequence used is based on [1] and [2]. In practice we
> depend on the GT RC6 mechanism to save the HW context depending on the
> render and media power wells. By the time we run the runtime suspend
> callback the display side is also off and the HW context for that is
> managed by the display power domain framework.
> 
> Besides the above there are Gunit registers that depend on a system-wide
> power well. This power well goes off once the device enters any of the
> S0i[R123] states. To handle this scenario, save/restore these Gunit
> registers. Note that this is not the complete register set dictated by
> [2], to remove some overhead registers that are known not to be used are
> ignored. Also some registers are fully setup by initialization functions
> called during resume, these are not saved either. The list of registers
> can be further reduced, see the TODO note in the code.
> 
> [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> [2] VLV2_S0IXRegs
> 
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 166 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f9d0db..16ca37f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
>  	return i915_drm_freeze(drm_dev);
>  }
>  
> -static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
>  	intel_runtime_pm_disable_interrupts(dev);
> +
> +	return 0;
>  }
>  
> -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>  	hsw_enable_pc8(dev_priv);
> +
> +	return 0;
>  }
>  
> -static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> +static int snb_runtime_resume(struct drm_i915_private *dev_priv)
>  {
>  	struct drm_device *dev = dev_priv->dev;
>  
> @@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	gen6_update_ring_freq(dev);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +	return 0;
>  }
>  
> -static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
>  {
>  	hsw_disable_pc8(dev_priv);
> +
> +	return 0;
>  }
>  
>  /*
> @@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  #undef COND
>  }
>  
> +static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> +{
> +	u32 val;
> +	int err = 0;
> +
> +	val = I915_READ(VLV_GTLC_WAKE_CTRL);
> +	val &= ~VLV_GTLC_ALLOWWAKEREQ;
> +	if (allow)
> +		val |= VLV_GTLC_ALLOWWAKEREQ;
> +	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
> +	POSTING_READ(VLV_GTLC_WAKE_CTRL);
> +
> +#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
> +	      allow)
> +	err = wait_for(COND, 1);
> +	if (err)
> +		DRM_ERROR("timeout disabling GT waking\n");
> +	return err;
> +#undef COND
> +}
> +
> +static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +				 bool wait_for_on)
> +{
> +	u32 mask;
> +	u32 val;
> +	int err;
> +
> +	mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
> +	val = wait_for_on ? mask : 0;
> +#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> +	if (COND)
> +		return 0;
> +
> +	DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> +			wait_for_on ? "on" : "off",
> +			I915_READ(VLV_GTLC_PW_STATUS));
> +
> +	/*
> +	 * RC6 transitioning can be delayed up to 2 msec (see
> +	 * valleyview_enable_rps), use 3 msec for safety.
> +	 */
> +	err = wait_for(COND, 3);
> +	if (err)
> +		DRM_ERROR("timeout waiting for GT wells to go %s\n",
> +			  wait_for_on ? "on" : "off");
> +
> +	return err;
> +#undef COND
> +}
> +
> +static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> +{
> +	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
> +		return;
> +
> +	DRM_ERROR("GT register access while GT waking disabled\n");
> +	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> +}
> +
> +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	u32 mask;
> +	int err;
> +
> +	if (WARN_ON(!valleyview_rc6_enabled(dev)))
> +		return -ENODEV;
> +
> +	intel_runtime_pm_disable_interrupts(dev);
> +	cancel_work_sync(&dev_priv->rps.work);
> +
> +	/*
> +	 * Bspec defines the following GT well on flags as debug only, so
> +	 * don't treat them as hard failures.
> +	 */
> +	(void)vlv_wait_for_gt_wells(dev_priv, false);
> +
> +	mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> +	WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> +
> +	vlv_check_no_gt_access(dev_priv);
> +
> +	err = vlv_force_gfx_clock(dev_priv, true);
> +	if (err)
> +		goto err1;
> +
> +	err = vlv_allow_gt_wake(dev_priv, false);
> +	if (err)
> +		goto err2;
> +	vlv_save_gunit_s0ix_state(dev_priv);
> +
> +	err = vlv_force_gfx_clock(dev_priv, false);
> +	if (err)
> +		goto err2;
> +
> +	return 0;
> +
> +err2:
> +	/* For safety always re-enable waking and disable gfx clock forcing */
> +	vlv_allow_gt_wake(dev_priv, true);
> +err1:
> +	vlv_force_gfx_clock(dev_priv, false);
> +	intel_runtime_pm_restore_interrupts(dev);
> +
> +	return err;
> +}
> +
> +static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = dev_priv->dev;
> +	int err;
> +	int ret;
> +
> +	/*
> +	 * If any of the steps fail just try to continue, that's the best we
> +	 * can do at this point. Return the first error code (which will also
> +	 * leave RPM permanentyl disabled).
> +	 */
> +	ret = vlv_force_gfx_clock(dev_priv, true);
> +
> +	vlv_restore_gunit_s0ix_state(dev_priv);
> +
> +	err = vlv_allow_gt_wake(dev_priv, true);
> +	if (!ret)
> +		ret = err;
> +
> +	err = vlv_force_gfx_clock(dev_priv, false);
> +	if (!ret)
> +		ret = err;
> +
> +	vlv_check_no_gt_access(dev_priv);
> +
> +	intel_init_clock_gating(dev);
> +	intel_reset_gt_powersave(dev);
> +	i915_gem_init_swizzling(dev);
> +	i915_gem_restore_fences(dev);
> +
> +	intel_runtime_pm_restore_interrupts(dev);
> +
> +	return ret;
> +}
> +
>  static int intel_runtime_suspend(struct device *device)
>  {
>  	struct pci_dev *pdev = to_pci_dev(device);
>  	struct drm_device *dev = pci_get_drvdata(pdev);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret = 0;
>  
>  	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
>  		return -ENODEV;
> @@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
>  		snb_runtime_suspend(dev_priv);
>  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_runtime_suspend(dev_priv);
> +	else if (IS_VALLEYVIEW(dev))
> +		ret = vlv_runtime_suspend(dev_priv);
>  	else
>  		WARN_ON(1);
>  
> +	if (ret) {
> +		DRM_ERROR("Runtime suspend failed, disabling it\n");
> +
> +		return ret;
> +	}
> +
>  	i915_gem_release_all_mmaps(dev_priv);
>  
>  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> @@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
>  		snb_runtime_resume(dev_priv);
>  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>  		hsw_runtime_resume(dev_priv);
> +	else if (IS_VALLEYVIEW(dev))
> +		vlv_runtime_resume(dev_priv);

Golden rule of refactoring: The 3rd guy gets to cleanup the mess. Imo
it's time to refactor the common parts form these platform functions out
and move them into generic code, and only call down into platform code as
needed. If we don't do that we'll have completely hell due to slight
differences in ordering between platforms.

Also I think we should try to share as much code as possible with the
other setup/teardowns paths, i.e. driver load/unload, system
suspend/resume and gpu reset.

Thanks, Daniel

>  	else
>  		WARN_ON(1);
>  
> -- 
> 1.8.4
> 
> _______________________________________________
> 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 14/15] drm/i915: vlv: add runtime PM support

[Imre Deak]

On Wed, 2014-04-09 at 16:22 +0200, Daniel Vetter wrote:
> On Tue, Apr 08, 2014 at 07:57:55PM +0300, Imre Deak wrote:
> > Add runtime PM support for VLV, but leave it disabled. The next patch
> > enables it.
> > 
> > The suspend/resume sequence used is based on [1] and [2]. In practice we
> > depend on the GT RC6 mechanism to save the HW context depending on the
> > render and media power wells. By the time we run the runtime suspend
> > callback the display side is also off and the HW context for that is
> > managed by the display power domain framework.
> > 
> > Besides the above there are Gunit registers that depend on a system-wide
> > power well. This power well goes off once the device enters any of the
> > S0i[R123] states. To handle this scenario, save/restore these Gunit
> > registers. Note that this is not the complete register set dictated by
> > [2], to remove some overhead registers that are known not to be used are
> > ignored. Also some registers are fully setup by initialization functions
> > called during resume, these are not saved either. The list of registers
> > can be further reduced, see the TODO note in the code.
> > 
> > [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> > [2] VLV2_S0IXRegs
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 166 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9f9d0db..16ca37f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
> >  	return i915_drm_freeze(drm_dev);
> >  }
> >  
> > -static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  
> >  	intel_runtime_pm_disable_interrupts(dev);
> > +
> > +	return 0;
> >  }
> >  
> > -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	hsw_enable_pc8(dev_priv);
> > +
> > +	return 0;
> >  }
> >  
> > -static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> >  {
> >  	struct drm_device *dev = dev_priv->dev;
> >  
> > @@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  	gen6_update_ring_freq(dev);
> >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +	return 0;
> >  }
> >  
> > -static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> >  {
> >  	hsw_disable_pc8(dev_priv);
> > +
> > +	return 0;
> >  }
> >  
> >  /*
> > @@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> >  #undef COND
> >  }
> >  
> > +static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> > +{
> > +	u32 val;
> > +	int err = 0;
> > +
> > +	val = I915_READ(VLV_GTLC_WAKE_CTRL);
> > +	val &= ~VLV_GTLC_ALLOWWAKEREQ;
> > +	if (allow)
> > +		val |= VLV_GTLC_ALLOWWAKEREQ;
> > +	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
> > +	POSTING_READ(VLV_GTLC_WAKE_CTRL);
> > +
> > +#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
> > +	      allow)
> > +	err = wait_for(COND, 1);
> > +	if (err)
> > +		DRM_ERROR("timeout disabling GT waking\n");
> > +	return err;
> > +#undef COND
> > +}
> > +
> > +static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> > +				 bool wait_for_on)
> > +{
> > +	u32 mask;
> > +	u32 val;
> > +	int err;
> > +
> > +	mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
> > +	val = wait_for_on ? mask : 0;
> > +#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> > +	if (COND)
> > +		return 0;
> > +
> > +	DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> > +			wait_for_on ? "on" : "off",
> > +			I915_READ(VLV_GTLC_PW_STATUS));
> > +
> > +	/*
> > +	 * RC6 transitioning can be delayed up to 2 msec (see
> > +	 * valleyview_enable_rps), use 3 msec for safety.
> > +	 */
> > +	err = wait_for(COND, 3);
> > +	if (err)
> > +		DRM_ERROR("timeout waiting for GT wells to go %s\n",
> > +			  wait_for_on ? "on" : "off");
> > +
> > +	return err;
> > +#undef COND
> > +}
> > +
> > +static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> > +{
> > +	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
> > +		return;
> > +
> > +	DRM_ERROR("GT register access while GT waking disabled\n");
> > +	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> > +}
> > +
> > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	u32 mask;
> > +	int err;
> > +
> > +	if (WARN_ON(!valleyview_rc6_enabled(dev)))
> > +		return -ENODEV;
> > +
> > +	intel_runtime_pm_disable_interrupts(dev);
> > +	cancel_work_sync(&dev_priv->rps.work);
> > +
> > +	/*
> > +	 * Bspec defines the following GT well on flags as debug only, so
> > +	 * don't treat them as hard failures.
> > +	 */
> > +	(void)vlv_wait_for_gt_wells(dev_priv, false);
> > +
> > +	mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> > +	WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> > +
> > +	vlv_check_no_gt_access(dev_priv);
> > +
> > +	err = vlv_force_gfx_clock(dev_priv, true);
> > +	if (err)
> > +		goto err1;
> > +
> > +	err = vlv_allow_gt_wake(dev_priv, false);
> > +	if (err)
> > +		goto err2;
> > +	vlv_save_gunit_s0ix_state(dev_priv);
> > +
> > +	err = vlv_force_gfx_clock(dev_priv, false);
> > +	if (err)
> > +		goto err2;
> > +
> > +	return 0;
> > +
> > +err2:
> > +	/* For safety always re-enable waking and disable gfx clock forcing */
> > +	vlv_allow_gt_wake(dev_priv, true);
> > +err1:
> > +	vlv_force_gfx_clock(dev_priv, false);
> > +	intel_runtime_pm_restore_interrupts(dev);
> > +
> > +	return err;
> > +}
> > +
> > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +{
> > +	struct drm_device *dev = dev_priv->dev;
> > +	int err;
> > +	int ret;
> > +
> > +	/*
> > +	 * If any of the steps fail just try to continue, that's the best we
> > +	 * can do at this point. Return the first error code (which will also
> > +	 * leave RPM permanentyl disabled).
> > +	 */
> > +	ret = vlv_force_gfx_clock(dev_priv, true);
> > +
> > +	vlv_restore_gunit_s0ix_state(dev_priv);
> > +
> > +	err = vlv_allow_gt_wake(dev_priv, true);
> > +	if (!ret)
> > +		ret = err;
> > +
> > +	err = vlv_force_gfx_clock(dev_priv, false);
> > +	if (!ret)
> > +		ret = err;
> > +
> > +	vlv_check_no_gt_access(dev_priv);
> > +
> > +	intel_init_clock_gating(dev);
> > +	intel_reset_gt_powersave(dev);
> > +	i915_gem_init_swizzling(dev);
> > +	i915_gem_restore_fences(dev);
> > +
> > +	intel_runtime_pm_restore_interrupts(dev);
> > +
> > +	return ret;
> > +}
> > +
> >  static int intel_runtime_suspend(struct device *device)
> >  {
> >  	struct pci_dev *pdev = to_pci_dev(device);
> >  	struct drm_device *dev = pci_get_drvdata(pdev);
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret = 0;
> >  
> >  	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> >  		return -ENODEV;
> > @@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
> >  		snb_runtime_suspend(dev_priv);
> >  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		hsw_runtime_suspend(dev_priv);
> > +	else if (IS_VALLEYVIEW(dev))
> > +		ret = vlv_runtime_suspend(dev_priv);
> >  	else
> >  		WARN_ON(1);
> >  
> > +	if (ret) {
> > +		DRM_ERROR("Runtime suspend failed, disabling it\n");
> > +
> > +		return ret;
> > +	}
> > +
> >  	i915_gem_release_all_mmaps(dev_priv);
> >  
> >  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > @@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
> >  		snb_runtime_resume(dev_priv);
> >  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >  		hsw_runtime_resume(dev_priv);
> > +	else if (IS_VALLEYVIEW(dev))
> > +		vlv_runtime_resume(dev_priv);
> 
> Golden rule of refactoring: The 3rd guy gets to cleanup the mess. Imo
> it's time to refactor the common parts form these platform functions out
> and move them into generic code, and only call down into platform code as
> needed. If we don't do that we'll have completely hell due to slight
> differences in ordering between platforms.

Ok, the common parts basically boil down to

intel_runtime_pm_disable_interrupts(); / _enable_interrupts();

and I think we also need to add

cancel_work_sync(&dev_priv->rps.work);

to all platforms. I can do this.

> Also I think we should try to share as much code as possible with the
> other setup/teardowns paths, i.e. driver load/unload, system
> suspend/resume and gpu reset.

I agree, but this needs much more refactoring first on the other parts
you mention above before we can unify things. That's mainly because on
the RPM path we can call only low level functions (since an RPM callback
can be called basically from anywhere in the driver) whereas the
handlers you mention do a high level initialization. So for VLV RPM
resume we call for example 

intel_init_clock_gating();
i915_gem_init_swizzling();
i915_gem_restore_fences();

but for system resume we simply do a full

intel_modeset_init_hw();

which includes the above low level steps interleaved with quite a lot of
other init steps. We may also need to rethink locking before sharing
those parts.  

So I'm happy to do this refactoring, but I'd suggest doing it as a
follow-up.

--Imre

> Thanks, Daniel
> 
> >  	else
> >  		WARN_ON(1);
> >  
> > -- 
> > 1.8.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

Re: [PATCH 14/15] drm/i915: vlv: add runtime PM support

[Daniel Vetter]

On Wed, Apr 09, 2014 at 06:43:10PM +0300, Imre Deak wrote:
> On Wed, 2014-04-09 at 16:22 +0200, Daniel Vetter wrote:
> > On Tue, Apr 08, 2014 at 07:57:55PM +0300, Imre Deak wrote:
> > > Add runtime PM support for VLV, but leave it disabled. The next patch
> > > enables it.
> > > 
> > > The suspend/resume sequence used is based on [1] and [2]. In practice we
> > > depend on the GT RC6 mechanism to save the HW context depending on the
> > > render and media power wells. By the time we run the runtime suspend
> > > callback the display side is also off and the HW context for that is
> > > managed by the display power domain framework.
> > > 
> > > Besides the above there are Gunit registers that depend on a system-wide
> > > power well. This power well goes off once the device enters any of the
> > > S0i[R123] states. To handle this scenario, save/restore these Gunit
> > > registers. Note that this is not the complete register set dictated by
> > > [2], to remove some overhead registers that are known not to be used are
> > > ignored. Also some registers are fully setup by initialization functions
> > > called during resume, these are not saved either. The list of registers
> > > can be further reduced, see the TODO note in the code.
> > > 
> > > [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> > > [2] VLV2_S0IXRegs
> > > 
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 166 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > index 9f9d0db..16ca37f 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > @@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
> > >  	return i915_drm_freeze(drm_dev);
> > >  }
> > >  
> > > -static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> > > +static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
> > >  {
> > >  	struct drm_device *dev = dev_priv->dev;
> > >  
> > >  	intel_runtime_pm_disable_interrupts(dev);
> > > +
> > > +	return 0;
> > >  }
> > >  
> > > -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > > +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > >  {
> > >  	hsw_enable_pc8(dev_priv);
> > > +
> > > +	return 0;
> > >  }
> > >  
> > > -static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> > > +static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> > >  {
> > >  	struct drm_device *dev = dev_priv->dev;
> > >  
> > > @@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> > >  	mutex_lock(&dev_priv->rps.hw_lock);
> > >  	gen6_update_ring_freq(dev);
> > >  	mutex_unlock(&dev_priv->rps.hw_lock);
> > > +
> > > +	return 0;
> > >  }
> > >  
> > > -static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > > +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > >  {
> > >  	hsw_disable_pc8(dev_priv);
> > > +
> > > +	return 0;
> > >  }
> > >  
> > >  /*
> > > @@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> > >  #undef COND
> > >  }
> > >  
> > > +static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> > > +{
> > > +	u32 val;
> > > +	int err = 0;
> > > +
> > > +	val = I915_READ(VLV_GTLC_WAKE_CTRL);
> > > +	val &= ~VLV_GTLC_ALLOWWAKEREQ;
> > > +	if (allow)
> > > +		val |= VLV_GTLC_ALLOWWAKEREQ;
> > > +	I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
> > > +	POSTING_READ(VLV_GTLC_WAKE_CTRL);
> > > +
> > > +#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
> > > +	      allow)
> > > +	err = wait_for(COND, 1);
> > > +	if (err)
> > > +		DRM_ERROR("timeout disabling GT waking\n");
> > > +	return err;
> > > +#undef COND
> > > +}
> > > +
> > > +static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> > > +				 bool wait_for_on)
> > > +{
> > > +	u32 mask;
> > > +	u32 val;
> > > +	int err;
> > > +
> > > +	mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
> > > +	val = wait_for_on ? mask : 0;
> > > +#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> > > +	if (COND)
> > > +		return 0;
> > > +
> > > +	DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> > > +			wait_for_on ? "on" : "off",
> > > +			I915_READ(VLV_GTLC_PW_STATUS));
> > > +
> > > +	/*
> > > +	 * RC6 transitioning can be delayed up to 2 msec (see
> > > +	 * valleyview_enable_rps), use 3 msec for safety.
> > > +	 */
> > > +	err = wait_for(COND, 3);
> > > +	if (err)
> > > +		DRM_ERROR("timeout waiting for GT wells to go %s\n",
> > > +			  wait_for_on ? "on" : "off");
> > > +
> > > +	return err;
> > > +#undef COND
> > > +}
> > > +
> > > +static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> > > +{
> > > +	if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
> > > +		return;
> > > +
> > > +	DRM_ERROR("GT register access while GT waking disabled\n");
> > > +	I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> > > +}
> > > +
> > > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct drm_device *dev = dev_priv->dev;
> > > +	u32 mask;
> > > +	int err;
> > > +
> > > +	if (WARN_ON(!valleyview_rc6_enabled(dev)))
> > > +		return -ENODEV;
> > > +
> > > +	intel_runtime_pm_disable_interrupts(dev);
> > > +	cancel_work_sync(&dev_priv->rps.work);
> > > +
> > > +	/*
> > > +	 * Bspec defines the following GT well on flags as debug only, so
> > > +	 * don't treat them as hard failures.
> > > +	 */
> > > +	(void)vlv_wait_for_gt_wells(dev_priv, false);
> > > +
> > > +	mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> > > +	WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> > > +
> > > +	vlv_check_no_gt_access(dev_priv);
> > > +
> > > +	err = vlv_force_gfx_clock(dev_priv, true);
> > > +	if (err)
> > > +		goto err1;
> > > +
> > > +	err = vlv_allow_gt_wake(dev_priv, false);
> > > +	if (err)
> > > +		goto err2;
> > > +	vlv_save_gunit_s0ix_state(dev_priv);
> > > +
> > > +	err = vlv_force_gfx_clock(dev_priv, false);
> > > +	if (err)
> > > +		goto err2;
> > > +
> > > +	return 0;
> > > +
> > > +err2:
> > > +	/* For safety always re-enable waking and disable gfx clock forcing */
> > > +	vlv_allow_gt_wake(dev_priv, true);
> > > +err1:
> > > +	vlv_force_gfx_clock(dev_priv, false);
> > > +	intel_runtime_pm_restore_interrupts(dev);
> > > +
> > > +	return err;
> > > +}
> > > +
> > > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > > +{
> > > +	struct drm_device *dev = dev_priv->dev;
> > > +	int err;
> > > +	int ret;
> > > +
> > > +	/*
> > > +	 * If any of the steps fail just try to continue, that's the best we
> > > +	 * can do at this point. Return the first error code (which will also
> > > +	 * leave RPM permanentyl disabled).
> > > +	 */
> > > +	ret = vlv_force_gfx_clock(dev_priv, true);
> > > +
> > > +	vlv_restore_gunit_s0ix_state(dev_priv);
> > > +
> > > +	err = vlv_allow_gt_wake(dev_priv, true);
> > > +	if (!ret)
> > > +		ret = err;
> > > +
> > > +	err = vlv_force_gfx_clock(dev_priv, false);
> > > +	if (!ret)
> > > +		ret = err;
> > > +
> > > +	vlv_check_no_gt_access(dev_priv);
> > > +
> > > +	intel_init_clock_gating(dev);
> > > +	intel_reset_gt_powersave(dev);
> > > +	i915_gem_init_swizzling(dev);
> > > +	i915_gem_restore_fences(dev);
> > > +
> > > +	intel_runtime_pm_restore_interrupts(dev);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int intel_runtime_suspend(struct device *device)
> > >  {
> > >  	struct pci_dev *pdev = to_pci_dev(device);
> > >  	struct drm_device *dev = pci_get_drvdata(pdev);
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int ret = 0;
> > >  
> > >  	if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> > >  		return -ENODEV;
> > > @@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
> > >  		snb_runtime_suspend(dev_priv);
> > >  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > >  		hsw_runtime_suspend(dev_priv);
> > > +	else if (IS_VALLEYVIEW(dev))
> > > +		ret = vlv_runtime_suspend(dev_priv);
> > >  	else
> > >  		WARN_ON(1);
> > >  
> > > +	if (ret) {
> > > +		DRM_ERROR("Runtime suspend failed, disabling it\n");
> > > +
> > > +		return ret;
> > > +	}
> > > +
> > >  	i915_gem_release_all_mmaps(dev_priv);
> > >  
> > >  	del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > > @@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
> > >  		snb_runtime_resume(dev_priv);
> > >  	else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> > >  		hsw_runtime_resume(dev_priv);
> > > +	else if (IS_VALLEYVIEW(dev))
> > > +		vlv_runtime_resume(dev_priv);
> > 
> > Golden rule of refactoring: The 3rd guy gets to cleanup the mess. Imo
> > it's time to refactor the common parts form these platform functions out
> > and move them into generic code, and only call down into platform code as
> > needed. If we don't do that we'll have completely hell due to slight
> > differences in ordering between platforms.
> 
> Ok, the common parts basically boil down to
> 
> intel_runtime_pm_disable_interrupts(); / _enable_interrupts();
> 
> and I think we also need to add
> 
> cancel_work_sync(&dev_priv->rps.work);
> 
> to all platforms. I can do this.
> 
> > Also I think we should try to share as much code as possible with the
> > other setup/teardowns paths, i.e. driver load/unload, system
> > suspend/resume and gpu reset.
> 
> I agree, but this needs much more refactoring first on the other parts
> you mention above before we can unify things. That's mainly because on
> the RPM path we can call only low level functions (since an RPM callback
> can be called basically from anywhere in the driver) whereas the
> handlers you mention do a high level initialization. So for VLV RPM
> resume we call for example 
> 
> intel_init_clock_gating();
> i915_gem_init_swizzling();
> i915_gem_restore_fences();
> 
> but for system resume we simply do a full
> 
> intel_modeset_init_hw();
> 
> which includes the above low level steps interleaved with quite a lot of
> other init steps. We may also need to rethink locking before sharing
> those parts.  
> 
> So I'm happy to do this refactoring, but I'd suggest doing it as a
> follow-up.

Yeah I agree that the full thing needs more work and maybe 1-2 more
platforms so that we have a clearer picture. But imo it's time to start,
and the above few things you've mentioned look like a good first stab at
the problem.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 14/15] drm/i915: vlv: add runtime PM support

[Paulo Zanoni]

2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> Add runtime PM support for VLV, but leave it disabled. The next patch
> enables it.
>
> The suspend/resume sequence used is based on [1] and [2]. In practice we
> depend on the GT RC6 mechanism to save the HW context depending on the
> render and media power wells. By the time we run the runtime suspend
> callback the display side is also off and the HW context for that is
> managed by the display power domain framework.
>
> Besides the above there are Gunit registers that depend on a system-wide
> power well. This power well goes off once the device enters any of the
> S0i[R123] states. To handle this scenario, save/restore these Gunit
> registers. Note that this is not the complete register set dictated by
> [2], to remove some overhead registers that are known not to be used are
> ignored. Also some registers are fully setup by initialization functions
> called during resume, these are not saved either. The list of registers
> can be further reduced, see the TODO note in the code.
>
> [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> [2] VLV2_S0IXRegs
>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 166 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9f9d0db..16ca37f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
>         return i915_drm_freeze(drm_dev);
>  }
>
> -static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
>
>         intel_runtime_pm_disable_interrupts(dev);
> +
> +       return 0;
>  }
>
> -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
>  {
>         hsw_enable_pc8(dev_priv);
> +
> +       return 0;
>  }
>
> -static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> +static int snb_runtime_resume(struct drm_i915_private *dev_priv)
>  {
>         struct drm_device *dev = dev_priv->dev;
>
> @@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
>         mutex_lock(&dev_priv->rps.hw_lock);
>         gen6_update_ring_freq(dev);
>         mutex_unlock(&dev_priv->rps.hw_lock);
> +
> +       return 0;
>  }
>
> -static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
>  {
>         hsw_disable_pc8(dev_priv);
> +
> +       return 0;
>  }
>
>  /*
> @@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
>  #undef COND
>  }
>
> +static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> +{
> +       u32 val;
> +       int err = 0;
> +
> +       val = I915_READ(VLV_GTLC_WAKE_CTRL);
> +       val &= ~VLV_GTLC_ALLOWWAKEREQ;
> +       if (allow)
> +               val |= VLV_GTLC_ALLOWWAKEREQ;
> +       I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
> +       POSTING_READ(VLV_GTLC_WAKE_CTRL);
> +
> +#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
> +             allow)
> +       err = wait_for(COND, 1);
> +       if (err)
> +               DRM_ERROR("timeout disabling GT waking\n");
> +       return err;
> +#undef COND
> +}
> +
> +static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> +                                bool wait_for_on)
> +{
> +       u32 mask;
> +       u32 val;
> +       int err;
> +
> +       mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
> +       val = wait_for_on ? mask : 0;
> +#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> +       if (COND)
> +               return 0;
> +
> +       DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> +                       wait_for_on ? "on" : "off",
> +                       I915_READ(VLV_GTLC_PW_STATUS));
> +
> +       /*
> +        * RC6 transitioning can be delayed up to 2 msec (see
> +        * valleyview_enable_rps), use 3 msec for safety.
> +        */
> +       err = wait_for(COND, 3);
> +       if (err)
> +               DRM_ERROR("timeout waiting for GT wells to go %s\n",
> +                         wait_for_on ? "on" : "off");
> +
> +       return err;
> +#undef COND
> +}
> +
> +static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> +{
> +       if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
> +               return;
> +
> +       DRM_ERROR("GT register access while GT waking disabled\n");
> +       I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> +}
> +
> +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       u32 mask;
> +       int err;
> +
> +       if (WARN_ON(!valleyview_rc6_enabled(dev)))
> +               return -ENODEV;
> +
> +       intel_runtime_pm_disable_interrupts(dev);
> +       cancel_work_sync(&dev_priv->rps.work);
> +
> +       /*
> +        * Bspec defines the following GT well on flags as debug only, so
> +        * don't treat them as hard failures.
> +        */
> +       (void)vlv_wait_for_gt_wells(dev_priv, false);
> +
> +       mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> +       WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> +
> +       vlv_check_no_gt_access(dev_priv);
> +
> +       err = vlv_force_gfx_clock(dev_priv, true);
> +       if (err)
> +               goto err1;
> +
> +       err = vlv_allow_gt_wake(dev_priv, false);
> +       if (err)
> +               goto err2;
> +       vlv_save_gunit_s0ix_state(dev_priv);
> +
> +       err = vlv_force_gfx_clock(dev_priv, false);
> +       if (err)
> +               goto err2;
> +
> +       return 0;
> +
> +err2:
> +       /* For safety always re-enable waking and disable gfx clock forcing */
> +       vlv_allow_gt_wake(dev_priv, true);
> +err1:
> +       vlv_force_gfx_clock(dev_priv, false);
> +       intel_runtime_pm_restore_interrupts(dev);
> +
> +       return err;
> +}
> +
> +static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> +{
> +       struct drm_device *dev = dev_priv->dev;
> +       int err;
> +       int ret;
> +
> +       /*
> +        * If any of the steps fail just try to continue, that's the best we
> +        * can do at this point. Return the first error code (which will also
> +        * leave RPM permanentyl disabled).
> +        */
> +       ret = vlv_force_gfx_clock(dev_priv, true);
> +
> +       vlv_restore_gunit_s0ix_state(dev_priv);
> +
> +       err = vlv_allow_gt_wake(dev_priv, true);
> +       if (!ret)
> +               ret = err;
> +
> +       err = vlv_force_gfx_clock(dev_priv, false);
> +       if (!ret)
> +               ret = err;
> +
> +       vlv_check_no_gt_access(dev_priv);
> +
> +       intel_init_clock_gating(dev);
> +       intel_reset_gt_powersave(dev);
> +       i915_gem_init_swizzling(dev);
> +       i915_gem_restore_fences(dev);
> +
> +       intel_runtime_pm_restore_interrupts(dev);
> +
> +       return ret;
> +}
> +
>  static int intel_runtime_suspend(struct device *device)
>  {
>         struct pci_dev *pdev = to_pci_dev(device);
>         struct drm_device *dev = pci_get_drvdata(pdev);
>         struct drm_i915_private *dev_priv = dev->dev_private;
> +       int ret = 0;
>
>         if (WARN_ON_ONCE(!dev_priv->rps.enabled))
>                 return -ENODEV;
> @@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
>                 snb_runtime_suspend(dev_priv);
>         else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>                 hsw_runtime_suspend(dev_priv);

You make these guys return an error code, but you don't check for
them. Even if they always return 0, I think it's good to add the check
now, because in the future someone may write a patch adding a non-zero
return and forget to add code to check for the error.

> +       else if (IS_VALLEYVIEW(dev))
> +               ret = vlv_runtime_suspend(dev_priv);
>         else
>                 WARN_ON(1);
>
> +       if (ret) {
> +               DRM_ERROR("Runtime suspend failed, disabling it\n");
> +
> +               return ret;
> +       }
> +
>         i915_gem_release_all_mmaps(dev_priv);
>
>         del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> @@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
>                 snb_runtime_resume(dev_priv);
>         else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
>                 hsw_runtime_resume(dev_priv);
> +       else if (IS_VALLEYVIEW(dev))
> +               vlv_runtime_resume(dev_priv);

Same here.

>         else
>                 WARN_ON(1);
>
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 14/15] drm/i915: vlv: add runtime PM support

[Imre Deak]

On Wed, 2014-04-09 at 13:40 -0300, Paulo Zanoni wrote:
> 2014-04-08 13:57 GMT-03:00 Imre Deak <imre.deak@intel.com>:
> > Add runtime PM support for VLV, but leave it disabled. The next patch
> > enables it.
> >
> > The suspend/resume sequence used is based on [1] and [2]. In practice we
> > depend on the GT RC6 mechanism to save the HW context depending on the
> > render and media power wells. By the time we run the runtime suspend
> > callback the display side is also off and the HW context for that is
> > managed by the display power domain framework.
> >
> > Besides the above there are Gunit registers that depend on a system-wide
> > power well. This power well goes off once the device enters any of the
> > S0i[R123] states. To handle this scenario, save/restore these Gunit
> > registers. Note that this is not the complete register set dictated by
> > [2], to remove some overhead registers that are known not to be used are
> > ignored. Also some registers are fully setup by initialization functions
> > called during resume, these are not saved either. The list of registers
> > can be further reduced, see the TODO note in the code.
> >
> > [1] VLV_gfx_clocking_PM_reset_y12w21d3 / "Driver D3 entry/exit"
> > [2] VLV2_S0IXRegs
> >
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c | 170 +++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 166 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 9f9d0db..16ca37f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -891,19 +891,23 @@ static int i915_pm_poweroff(struct device *dev)
> >         return i915_drm_freeze(drm_dev);
> >  }
> >
> > -static void snb_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int snb_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >         struct drm_device *dev = dev_priv->dev;
> >
> >         intel_runtime_pm_disable_interrupts(dev);
> > +
> > +       return 0;
> >  }
> >
> > -static void hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> > +static int hsw_runtime_suspend(struct drm_i915_private *dev_priv)
> >  {
> >         hsw_enable_pc8(dev_priv);
> > +
> > +       return 0;
> >  }
> >
> > -static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int snb_runtime_resume(struct drm_i915_private *dev_priv)
> >  {
> >         struct drm_device *dev = dev_priv->dev;
> >
> > @@ -913,11 +917,15 @@ static void snb_runtime_resume(struct drm_i915_private *dev_priv)
> >         mutex_lock(&dev_priv->rps.hw_lock);
> >         gen6_update_ring_freq(dev);
> >         mutex_unlock(&dev_priv->rps.hw_lock);
> > +
> > +       return 0;
> >  }
> >
> > -static void hsw_runtime_resume(struct drm_i915_private *dev_priv)
> > +static int hsw_runtime_resume(struct drm_i915_private *dev_priv)
> >  {
> >         hsw_disable_pc8(dev_priv);
> > +
> > +       return 0;
> >  }
> >
> >  /*
> > @@ -1144,11 +1152,155 @@ int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool force_on)
> >  #undef COND
> >  }
> >
> > +static int vlv_allow_gt_wake(struct drm_i915_private *dev_priv, bool allow)
> > +{
> > +       u32 val;
> > +       int err = 0;
> > +
> > +       val = I915_READ(VLV_GTLC_WAKE_CTRL);
> > +       val &= ~VLV_GTLC_ALLOWWAKEREQ;
> > +       if (allow)
> > +               val |= VLV_GTLC_ALLOWWAKEREQ;
> > +       I915_WRITE(VLV_GTLC_WAKE_CTRL, val);
> > +       POSTING_READ(VLV_GTLC_WAKE_CTRL);
> > +
> > +#define COND (!!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEACK) == \
> > +             allow)
> > +       err = wait_for(COND, 1);
> > +       if (err)
> > +               DRM_ERROR("timeout disabling GT waking\n");
> > +       return err;
> > +#undef COND
> > +}
> > +
> > +static int vlv_wait_for_gt_wells(struct drm_i915_private *dev_priv,
> > +                                bool wait_for_on)
> > +{
> > +       u32 mask;
> > +       u32 val;
> > +       int err;
> > +
> > +       mask = VLV_GTLC_PW_MEDIA_STATUS_MASK | VLV_GTLC_PW_RENDER_STATUS_MASK;
> > +       val = wait_for_on ? mask : 0;
> > +#define COND ((I915_READ(VLV_GTLC_PW_STATUS) & mask) == val)
> > +       if (COND)
> > +               return 0;
> > +
> > +       DRM_DEBUG_KMS("waiting for GT wells to go %s (%08x)\n",
> > +                       wait_for_on ? "on" : "off",
> > +                       I915_READ(VLV_GTLC_PW_STATUS));
> > +
> > +       /*
> > +        * RC6 transitioning can be delayed up to 2 msec (see
> > +        * valleyview_enable_rps), use 3 msec for safety.
> > +        */
> > +       err = wait_for(COND, 3);
> > +       if (err)
> > +               DRM_ERROR("timeout waiting for GT wells to go %s\n",
> > +                         wait_for_on ? "on" : "off");
> > +
> > +       return err;
> > +#undef COND
> > +}
> > +
> > +static void vlv_check_no_gt_access(struct drm_i915_private *dev_priv)
> > +{
> > +       if (!(I915_READ(VLV_GTLC_PW_STATUS) & VLV_GTLC_ALLOWWAKEERR))
> > +               return;
> > +
> > +       DRM_ERROR("GT register access while GT waking disabled\n");
> > +       I915_WRITE(VLV_GTLC_PW_STATUS, VLV_GTLC_ALLOWWAKEERR);
> > +}
> > +
> > +static int vlv_runtime_suspend(struct drm_i915_private *dev_priv)
> > +{
> > +       struct drm_device *dev = dev_priv->dev;
> > +       u32 mask;
> > +       int err;
> > +
> > +       if (WARN_ON(!valleyview_rc6_enabled(dev)))
> > +               return -ENODEV;
> > +
> > +       intel_runtime_pm_disable_interrupts(dev);
> > +       cancel_work_sync(&dev_priv->rps.work);
> > +
> > +       /*
> > +        * Bspec defines the following GT well on flags as debug only, so
> > +        * don't treat them as hard failures.
> > +        */
> > +       (void)vlv_wait_for_gt_wells(dev_priv, false);
> > +
> > +       mask = VLV_GTLC_RENDER_CTX_EXISTS | VLV_GTLC_MEDIA_CTX_EXISTS;
> > +       WARN_ON((I915_READ(VLV_GTLC_WAKE_CTRL) & mask) != mask);
> > +
> > +       vlv_check_no_gt_access(dev_priv);
> > +
> > +       err = vlv_force_gfx_clock(dev_priv, true);
> > +       if (err)
> > +               goto err1;
> > +
> > +       err = vlv_allow_gt_wake(dev_priv, false);
> > +       if (err)
> > +               goto err2;
> > +       vlv_save_gunit_s0ix_state(dev_priv);
> > +
> > +       err = vlv_force_gfx_clock(dev_priv, false);
> > +       if (err)
> > +               goto err2;
> > +
> > +       return 0;
> > +
> > +err2:
> > +       /* For safety always re-enable waking and disable gfx clock forcing */
> > +       vlv_allow_gt_wake(dev_priv, true);
> > +err1:
> > +       vlv_force_gfx_clock(dev_priv, false);
> > +       intel_runtime_pm_restore_interrupts(dev);
> > +
> > +       return err;
> > +}
> > +
> > +static int vlv_runtime_resume(struct drm_i915_private *dev_priv)
> > +{
> > +       struct drm_device *dev = dev_priv->dev;
> > +       int err;
> > +       int ret;
> > +
> > +       /*
> > +        * If any of the steps fail just try to continue, that's the best we
> > +        * can do at this point. Return the first error code (which will also
> > +        * leave RPM permanentyl disabled).
> > +        */
> > +       ret = vlv_force_gfx_clock(dev_priv, true);
> > +
> > +       vlv_restore_gunit_s0ix_state(dev_priv);
> > +
> > +       err = vlv_allow_gt_wake(dev_priv, true);
> > +       if (!ret)
> > +               ret = err;
> > +
> > +       err = vlv_force_gfx_clock(dev_priv, false);
> > +       if (!ret)
> > +               ret = err;
> > +
> > +       vlv_check_no_gt_access(dev_priv);
> > +
> > +       intel_init_clock_gating(dev);
> > +       intel_reset_gt_powersave(dev);
> > +       i915_gem_init_swizzling(dev);
> > +       i915_gem_restore_fences(dev);
> > +
> > +       intel_runtime_pm_restore_interrupts(dev);
> > +
> > +       return ret;
> > +}
> > +
> >  static int intel_runtime_suspend(struct device *device)
> >  {
> >         struct pci_dev *pdev = to_pci_dev(device);
> >         struct drm_device *dev = pci_get_drvdata(pdev);
> >         struct drm_i915_private *dev_priv = dev->dev_private;
> > +       int ret = 0;
> >
> >         if (WARN_ON_ONCE(!dev_priv->rps.enabled))
> >                 return -ENODEV;
> > @@ -1162,9 +1314,17 @@ static int intel_runtime_suspend(struct device *device)
> >                 snb_runtime_suspend(dev_priv);
> >         else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >                 hsw_runtime_suspend(dev_priv);
> 
> You make these guys return an error code, but you don't check for
> them. Even if they always return 0, I think it's good to add the check
> now, because in the future someone may write a patch adding a non-zero
> return and forget to add code to check for the error.

Yep, I missed that.

I actually depend on these propagating the error from the VLV callbacks,
since those can fail.

BTW, right now I'm leaving RPM permanently disabled in case of an error,
but I think we should try to reenable it.. Perhaps after a reset. I
wasn't sure what's the right policy here, but in any case this can be
added later as a follow-up if needed.

--Imre

> 
> > +       else if (IS_VALLEYVIEW(dev))
> > +               ret = vlv_runtime_suspend(dev_priv);
> >         else
> >                 WARN_ON(1);
> >
> > +       if (ret) {
> > +               DRM_ERROR("Runtime suspend failed, disabling it\n");
> > +
> > +               return ret;
> > +       }
> > +
> >         i915_gem_release_all_mmaps(dev_priv);
> >
> >         del_timer_sync(&dev_priv->gpu_error.hangcheck_timer);
> > @@ -1200,6 +1360,8 @@ static int intel_runtime_resume(struct device *device)
> >                 snb_runtime_resume(dev_priv);
> >         else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
> >                 hsw_runtime_resume(dev_priv);
> > +       else if (IS_VALLEYVIEW(dev))
> > +               vlv_runtime_resume(dev_priv);
> 
> Same here.
> 
> >         else
> >                 WARN_ON(1);
> >
> > --
> > 1.8.4
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 

[PATCH 15/15] drm/i915: vlv: enable RPM

[Imre Deak]

Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d94e10a..84d4298 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1928,7 +1928,7 @@ struct drm_i915_cmd_table {
 #define HAS_FPGA_DBG_UNCLAIMED(dev)	(INTEL_INFO(dev)->has_fpga_dbg)
 #define HAS_PSR(dev)		(IS_HASWELL(dev) || IS_BROADWELL(dev))
 #define HAS_RUNTIME_PM(dev)	(IS_GEN6(dev) || IS_HASWELL(dev) || \
-				 IS_BROADWELL(dev))
+				 IS_BROADWELL(dev) || IS_VALLEYVIEW(dev))
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
-- 
1.8.4