[PATCH 0/4] SNB runtime PM fixes

[PATCH 0/4] SNB runtime PM fixes

[Paulo Zanoni]

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

Hi

When I originally wrote and tested SNB runtime PM it was working without any
errors, then I sent the patches to the mailing list and we ended up discarding
one of the patches that was needed. We ended up replacing that patch with a few
patches that only plugged the HSW holes, leaving SNB runtime PM broken since
then. Since I never got any bug report about this, I assumed SNB runtime PM was
working and didn't test things again... Then a long time later we got the bug
reports, and now we have fixes :)

Also notice that patch 1 was also sent as part of the "Runtime PM on DPMS on
HSW" series, so it's duplicated.

Thanks,
Paulo

Paulo Zanoni (4):
  drm/i915: add POWER_DOMAIN_PLLS
  drm/i915: check the power domains in ironlake_get_pipe_config()
  drm/i915: check the power domains in intel_lvds_get_hw_state()
  drm/i915: don't read LVDS regs at compute_config time

 drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_lvds.c    | 18 ++++++++++++++----
 drivers/gpu/drm/i915/intel_pm.c      |  1 +
 5 files changed, 32 insertions(+), 4 deletions(-)

-- 
2.0.0

[PATCH 1/4] drm/i915: add POWER_DOMAIN_PLLS

[Paulo Zanoni]

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

And get/put it when needed. The special thing about this commit is
that it will now return false in ibx_pch_dpll_get_hw_state() in case
the power domain is not enabled. This will fix some WARNs we have when
we run pm_rpm on SNB.

Testcase: igt/pm_rpm
Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=80463
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
 drivers/gpu/drm/i915/i915_drv.h      |  1 +
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_pm.c      |  1 +
 4 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8da9985..18d4f9e 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2134,6 +2134,8 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
 		return "VGA";
 	case POWER_DOMAIN_AUDIO:
 		return "AUDIO";
+	case POWER_DOMAIN_PLLS:
+		return "PLLS";
 	case POWER_DOMAIN_INIT:
 		return "INIT";
 	default:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ac06c0f..1cc1b8f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -129,6 +129,7 @@ enum intel_display_power_domain {
 	POWER_DOMAIN_PORT_OTHER,
 	POWER_DOMAIN_VGA,
 	POWER_DOMAIN_AUDIO,
+	POWER_DOMAIN_PLLS,
 	POWER_DOMAIN_INIT,
 
 	POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c12a5da..3d69097 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1837,6 +1837,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
 	}
 	WARN_ON(pll->on);
 
+	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
+
 	DRM_DEBUG_KMS("enabling %s\n", pll->name);
 	pll->enable(dev_priv, pll);
 	pll->on = true;
@@ -1873,6 +1875,8 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
 	DRM_DEBUG_KMS("disabling %s\n", pll->name);
 	pll->disable(dev_priv, pll);
 	pll->on = false;
+
+	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
 }
 
 static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
@@ -11280,6 +11284,9 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
 {
 	uint32_t val;
 
+	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PLLS))
+		return false;
+
 	val = I915_READ(PCH_DPLL(pll->id));
 	hw_state->dpll = val;
 	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
@@ -12845,6 +12852,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 
 		DRM_DEBUG_KMS("%s hw state readout: refcount %i, on %i\n",
 			      pll->name, pll->refcount, pll->on);
+
+		if (pll->refcount)
+			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 31ae2b4..cf4c521 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -6310,6 +6310,7 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
 	BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) |		\
 	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |		\
 	BIT(POWER_DOMAIN_PORT_CRT) |			\
+	BIT(POWER_DOMAIN_PLLS) |			\
 	BIT(POWER_DOMAIN_INIT))
 #define HSW_DISPLAY_POWER_DOMAINS (				\
 	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	\
-- 
2.0.0

Re: [PATCH 1/4] drm/i915: add POWER_DOMAIN_PLLS

[Damien Lespiau]

On Fri, Jul 04, 2014 at 01:38:33PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> And get/put it when needed. The special thing about this commit is
> that it will now return false in ibx_pch_dpll_get_hw_state() in case
> the power domain is not enabled. This will fix some WARNs we have when
> we run pm_rpm on SNB.
> 
> Testcase: igt/pm_rpm
> Bugzilla:https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h      |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
>  drivers/gpu/drm/i915/intel_pm.c      |  1 +
>  4 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 8da9985..18d4f9e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2134,6 +2134,8 @@ static const char *power_domain_str(enum intel_display_power_domain domain)
>  		return "VGA";
>  	case POWER_DOMAIN_AUDIO:
>  		return "AUDIO";
> +	case POWER_DOMAIN_PLLS:
> +		return "PLLS";
>  	case POWER_DOMAIN_INIT:
>  		return "INIT";
>  	default:
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ac06c0f..1cc1b8f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -129,6 +129,7 @@ enum intel_display_power_domain {
>  	POWER_DOMAIN_PORT_OTHER,
>  	POWER_DOMAIN_VGA,
>  	POWER_DOMAIN_AUDIO,
> +	POWER_DOMAIN_PLLS,
>  	POWER_DOMAIN_INIT,
>  
>  	POWER_DOMAIN_NUM,
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index c12a5da..3d69097 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1837,6 +1837,8 @@ static void intel_enable_shared_dpll(struct intel_crtc *crtc)
>  	}
>  	WARN_ON(pll->on);
>  
> +	intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
> +
>  	DRM_DEBUG_KMS("enabling %s\n", pll->name);
>  	pll->enable(dev_priv, pll);
>  	pll->on = true;
> @@ -1873,6 +1875,8 @@ static void intel_disable_shared_dpll(struct intel_crtc *crtc)
>  	DRM_DEBUG_KMS("disabling %s\n", pll->name);
>  	pll->disable(dev_priv, pll);
>  	pll->on = false;
> +
> +	intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS);
>  }
>  
>  static void ironlake_enable_pch_transcoder(struct drm_i915_private *dev_priv,
> @@ -11280,6 +11284,9 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv,
>  {
>  	uint32_t val;
>  
> +	if (!intel_display_power_enabled(dev_priv, POWER_DOMAIN_PLLS))
> +		return false;
> +
>  	val = I915_READ(PCH_DPLL(pll->id));
>  	hw_state->dpll = val;
>  	hw_state->fp0 = I915_READ(PCH_FP0(pll->id));
> @@ -12845,6 +12852,9 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  
>  		DRM_DEBUG_KMS("%s hw state readout: refcount %i, on %i\n",
>  			      pll->name, pll->refcount, pll->on);
> +
> +		if (pll->refcount)
> +			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	}
>  
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 31ae2b4..cf4c521 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -6310,6 +6310,7 @@ EXPORT_SYMBOL_GPL(i915_release_power_well);
>  	BIT(POWER_DOMAIN_PORT_DDI_D_2_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_DDI_D_4_LANES) |		\
>  	BIT(POWER_DOMAIN_PORT_CRT) |			\
> +	BIT(POWER_DOMAIN_PLLS) |			\
>  	BIT(POWER_DOMAIN_INIT))
>  #define HSW_DISPLAY_POWER_DOMAINS (				\
>  	(POWER_DOMAIN_MASK & ~HSW_ALWAYS_ON_POWER_DOMAINS) |	\
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/4] drm/i915: check the power domains in ironlake_get_pipe_config()

[Paulo Zanoni]

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

Just like we already do in haswell_get_pipe_config(). This should
prevent some WARNs when we run pm_rpm on SNB.

Testcase: igt/pm_rpm
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3d69097..0e6217a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7261,6 +7261,10 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
+	if (!intel_display_power_enabled(dev_priv,
+					 POWER_DOMAIN_PIPE(crtc->pipe)))
+		return false;
+
 	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
 	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
 
-- 
2.0.0

Re: [PATCH 2/4] drm/i915: check the power domains in ironlake_get_pipe_config()

[Damien Lespiau]

On Fri, Jul 04, 2014 at 01:38:34PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like we already do in haswell_get_pipe_config(). This should
> prevent some WARNs when we run pm_rpm on SNB.
> 
> Testcase: igt/pm_rpm
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3d69097..0e6217a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -7261,6 +7261,10 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t tmp;
>  
> +	if (!intel_display_power_enabled(dev_priv,
> +					 POWER_DOMAIN_PIPE(crtc->pipe)))
> +		return false;
> +
>  	pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe;
>  	pipe_config->shared_dpll = DPLL_ID_PRIVATE;
>  
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: check the power domains in intel_lvds_get_hw_state()

[Paulo Zanoni]

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

Just like we do for the other encoders. This should fix some WARNs
when running pm_rpm on SNB.

Testcase: igt/pm_rpm
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 4d29a83..8aa7973 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -71,8 +71,13 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
+	enum intel_display_power_domain power_domain;
 	u32 tmp;
 
+	power_domain = intel_display_port_power_domain(encoder);
+	if (!intel_display_power_enabled(dev_priv, power_domain))
+		return false;
+
 	tmp = I915_READ(lvds_encoder->reg);
 
 	if (!(tmp & LVDS_PORT_EN))
-- 
2.0.0

Re: [PATCH 3/4] drm/i915: check the power domains in intel_lvds_get_hw_state()

[Damien Lespiau]

On Fri, Jul 04, 2014 at 01:38:35PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Just like we do for the other encoders. This should fix some WARNs
> when running pm_rpm on SNB.
> 
> Testcase: igt/pm_rpm
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 4d29a83..8aa7973 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -71,8 +71,13 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>  	struct drm_device *dev = encoder->base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> +	enum intel_display_power_domain power_domain;
>  	u32 tmp;
>  
> +	power_domain = intel_display_port_power_domain(encoder);
> +	if (!intel_display_power_enabled(dev_priv, power_domain))
> +		return false;
> +
>  	tmp = I915_READ(lvds_encoder->reg);
>  
>  	if (!(tmp & LVDS_PORT_EN))
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: don't read LVDS regs at compute_config time

[Paulo Zanoni]

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

We may reach this point while the machine is still runtime suspended,
so we'll hit a WARN. The other encoders also don't touch registers at
this point, so instead of waking the machine up, write some code to
keep the register always at the same state, including after we runtime
suspend/resume.

Testcase: igt/pm_rpm
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 8aa7973..c511287 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -51,6 +51,7 @@ struct intel_lvds_encoder {
 
 	bool is_dual_link;
 	u32 reg;
+	u32 a3_power;
 
 	struct intel_lvds_connector *attached_connector;
 };
@@ -170,8 +171,11 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder)
 
 	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
 	 * appropriately here, but we need to look more thoroughly into how
-	 * panels behave in the two modes.
+	 * panels behave in the two modes. For now, let's just maintain the
+	 * value we got from the BIOS.
 	 */
+	 temp &= ~LVDS_A3_POWER_MASK;
+	 temp |= lvds_encoder->a3_power;
 
 	/* Set the dithering flag on LVDS as needed, note that there is no
 	 * special lvds dither control bit on pch-split platforms, dithering is
@@ -269,7 +273,6 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 				      struct intel_crtc_config *pipe_config)
 {
 	struct drm_device *dev = intel_encoder->base.dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_lvds_encoder *lvds_encoder =
 		to_lvds_encoder(&intel_encoder->base);
 	struct intel_connector *intel_connector =
@@ -284,8 +287,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 		return false;
 	}
 
-	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
-	    LVDS_A3_POWER_UP)
+	if (lvds_encoder->a3_power == LVDS_A3_POWER_UP)
 		lvds_bpp = 8*3;
 	else
 		lvds_bpp = 6*3;
@@ -1086,6 +1088,9 @@ out:
 	DRM_DEBUG_KMS("detected %s-link lvds configuration\n",
 		      lvds_encoder->is_dual_link ? "dual" : "single");
 
+	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
+				 LVDS_A3_POWER_MASK;
+
 	/*
 	 * Unlock registers and just
 	 * leave them unlocked
-- 
2.0.0

Re: [PATCH 4/4] drm/i915: don't read LVDS regs at compute_config time

[Damien Lespiau]

On Fri, Jul 04, 2014 at 01:38:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We may reach this point while the machine is still runtime suspended,
> so we'll hit a WARN. The other encoders also don't touch registers at
> this point, so instead of waking the machine up, write some code to
> keep the register always at the same state, including after we runtime
> suspend/resume.
> 
> Testcase: igt/pm_rpm
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I would store lvds_bpp directly in the encoder structure, have the logic
around A3 power in the _init() function and don't bother re-writing the
A3,B3 control. But anyway, this looks correct to me.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 8aa7973..c511287 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -51,6 +51,7 @@ struct intel_lvds_encoder {
>  
>  	bool is_dual_link;
>  	u32 reg;
> +	u32 a3_power;
>  
>  	struct intel_lvds_connector *attached_connector;
>  };
> @@ -170,8 +171,11 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder)
>  
>  	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
>  	 * appropriately here, but we need to look more thoroughly into how
> -	 * panels behave in the two modes.
> +	 * panels behave in the two modes. For now, let's just maintain the
> +	 * value we got from the BIOS.
>  	 */
> +	 temp &= ~LVDS_A3_POWER_MASK;
> +	 temp |= lvds_encoder->a3_power;


I guess we actually don't need this as we'll be just preserving what we
read earlier.

>  
>  	/* Set the dithering flag on LVDS as needed, note that there is no
>  	 * special lvds dither control bit on pch-split platforms, dithering is
> @@ -269,7 +273,6 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  				      struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_device *dev = intel_encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_lvds_encoder *lvds_encoder =
>  		to_lvds_encoder(&intel_encoder->base);
>  	struct intel_connector *intel_connector =
> @@ -284,8 +287,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		return false;
>  	}
>  
> -	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
> -	    LVDS_A3_POWER_UP)
> +	if (lvds_encoder->a3_power == LVDS_A3_POWER_UP)
>  		lvds_bpp = 8*3;
>  	else
>  		lvds_bpp = 6*3;
> @@ -1086,6 +1088,9 @@ out:
>  	DRM_DEBUG_KMS("detected %s-link lvds configuration\n",
>  		      lvds_encoder->is_dual_link ? "dual" : "single");
>  
> +	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
> +				 LVDS_A3_POWER_MASK;
> +
>  	/*
>  	 * Unlock registers and just
>  	 * leave them unlocked
> -- 
> 2.0.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: don't read LVDS regs at compute_config time

[Daniel Vetter]

On Fri, Jul 04, 2014 at 01:38:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We may reach this point while the machine is still runtime suspended,
> so we'll hit a WARN. The other encoders also don't touch registers at
> this point, so instead of waking the machine up, write some code to
> keep the register always at the same state, including after we runtime
> suspend/resume.

Excellent catch - we really don't want to touch the hw at all in the
compute stage: Once we have atomic modeset we run this code for the
test-only mode when userspace tries to figure out what's possible. Waking
up the hardware here is really not what we want to do.

> Testcase: igt/pm_rpm
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=80463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Entire series pulled into dinq, thanks.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 8aa7973..c511287 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -51,6 +51,7 @@ struct intel_lvds_encoder {
>  
>  	bool is_dual_link;
>  	u32 reg;
> +	u32 a3_power;
>  
>  	struct intel_lvds_connector *attached_connector;
>  };
> @@ -170,8 +171,11 @@ static void intel_pre_enable_lvds(struct intel_encoder *encoder)
>  
>  	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
>  	 * appropriately here, but we need to look more thoroughly into how
> -	 * panels behave in the two modes.
> +	 * panels behave in the two modes. For now, let's just maintain the
> +	 * value we got from the BIOS.
>  	 */
> +	 temp &= ~LVDS_A3_POWER_MASK;
> +	 temp |= lvds_encoder->a3_power;
>  
>  	/* Set the dithering flag on LVDS as needed, note that there is no
>  	 * special lvds dither control bit on pch-split platforms, dithering is
> @@ -269,7 +273,6 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  				      struct intel_crtc_config *pipe_config)
>  {
>  	struct drm_device *dev = intel_encoder->base.dev;
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_lvds_encoder *lvds_encoder =
>  		to_lvds_encoder(&intel_encoder->base);
>  	struct intel_connector *intel_connector =
> @@ -284,8 +287,7 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		return false;
>  	}
>  
> -	if ((I915_READ(lvds_encoder->reg) & LVDS_A3_POWER_MASK) ==
> -	    LVDS_A3_POWER_UP)
> +	if (lvds_encoder->a3_power == LVDS_A3_POWER_UP)
>  		lvds_bpp = 8*3;
>  	else
>  		lvds_bpp = 6*3;
> @@ -1086,6 +1088,9 @@ out:
>  	DRM_DEBUG_KMS("detected %s-link lvds configuration\n",
>  		      lvds_encoder->is_dual_link ? "dual" : "single");
>  
> +	lvds_encoder->a3_power = I915_READ(lvds_encoder->reg) &
> +				 LVDS_A3_POWER_MASK;
> +
>  	/*
>  	 * Unlock registers and just
>  	 * leave them unlocked
> -- 
> 2.0.0
> 
> _______________________________________________
> 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