[PATCH 0/7] Even more "unclaimed register" fixes

[PATCH 0/7] Even more "unclaimed register" fixes

[Paulo Zanoni]

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

Hi

This is yet another series of patches that prevent our driver from touching
registers that don't exist or are powered off. With this series applied we
shouldn't trigger any "unclaimed register" message when there's a GPU hang or
when i915.disable_power_well=1 and the audio drivers are blacklisted. I'm not
aware of any other "unclaimed register" errors caused by the i915.ko driver.

The first 2 patches on the series are brand new, fixing recent regressions. The
third patch was requested by Daniel Vetter on an email this week. Patches 4-7
are new versions of patches I sent in the last round.

Thanks,
Paulo

Paulo Zanoni (7):
  drm/i915: check the power well inside haswell_get_pipe_config
  drm/i915: use cpu_transcoder for TRANS_DDI_FUNC_CTL
  drm/i915: add intel_display_power_enabled
  drm/i915: add power well and cpu transcoder info to the error state
  drm/i915: clear FPGA_DBG_RM_NOCLAIM when capturing error state
  drm/i915: check the power well on i915_pipe_enabled
  drm/i915: only disable DDI sound if intel_crtc->eld_vld

 drivers/gpu/drm/i915/i915_irq.c      |    4 +++
 drivers/gpu/drm/i915/intel_ddi.c     |   11 +++++---
 drivers/gpu/drm/i915/intel_display.c |   46 ++++++++++++++++++++++++++++------
 drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++-
 drivers/gpu/drm/i915/intel_pm.c      |   23 +++++++++++++----
 5 files changed, 76 insertions(+), 18 deletions(-)

-- 
1.7.10.4

[PATCH 1/7] drm/i915: check the power well inside haswell_get_pipe_config

[Paulo Zanoni]

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

This fixes "unclaimed register" messages when booting with eDP only
and i915.disable_power_well=1.

The error messages were caused by:

  commit 0e8ffe1bf81b0780cc6229cb38664754dffe8776
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Thu Mar 28 10:42:00 2013 +0100
      drm/i915: add hw state readout/checking for pipe_config

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 02faba0..61636de 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5856,9 +5856,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 {
 	struct drm_device *dev = crtc->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
-	tmp = I915_READ(PIPECONF(crtc->config.cpu_transcoder));
+	if (!intel_using_power_well(dev_priv->dev) &&
+	    cpu_transcoder != TRANSCODER_EDP)
+		return false;
+
+	tmp = I915_READ(PIPECONF(cpu_transcoder));
 	if (!(tmp & PIPECONF_ENABLE))
 		return false;
 
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: check the power well inside haswell_get_pipe_config

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This fixes "unclaimed register" messages when booting with eDP only
> and i915.disable_power_well=1.
> 
> The error messages were caused by:
> 
>   commit 0e8ffe1bf81b0780cc6229cb38664754dffe8776
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Thu Mar 28 10:42:00 2013 +0100
>       drm/i915: add hw state readout/checking for pipe_config
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

-- 
Damien

[PATCH 2/7] drm/i915: use cpu_transcoder for TRANS_DDI_FUNC_CTL

[Paulo Zanoni]

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

... inside haswell_get_pipe_config. Because there's one TRANS_DDI_FUNC_CTL
register per CPU transcoder, not per pipe. This solves "unclaimed register"
messages when booting with eDP only and using the i915.disable_power_well=1.

Also fix a comment and remove an useless empty line.

The error messages were caused by:

  commit 88adfff1ad5019f65b9d0b4e1a4ac900fb065183
  Author: Daniel Vetter <daniel.vetter@ffwll.ch>
  Date:   Thu Mar 28 10:42:01 2013 +0100
      drm/i915: hw readout support for ->has_pch_encoders

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 61636de..b86023d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5868,16 +5868,15 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 		return false;
 
 	/*
-	 * aswell has only FDI/PCH transcoder A. It is which is connected to
+	 * Haswell has only FDI/PCH transcoder A. It is which is connected to
 	 * DDI E. So just check whether this pipe is wired to DDI E and whether
 	 * the PCH transcoder is on.
 	 */
-	tmp = I915_READ(TRANS_DDI_FUNC_CTL(crtc->pipe));
+	tmp = I915_READ(TRANS_DDI_FUNC_CTL(cpu_transcoder));
 	if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(PORT_E) &&
 	    I915_READ(TRANSCONF(PIPE_A)) & TRANS_ENABLE)
 		pipe_config->has_pch_encoder = true;
 
-
 	return true;
 }
 
-- 
1.7.10.4

Re: [PATCH 2/7] drm/i915: use cpu_transcoder for TRANS_DDI_FUNC_CTL

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:41PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> ... inside haswell_get_pipe_config. Because there's one TRANS_DDI_FUNC_CTL
> register per CPU transcoder, not per pipe. This solves "unclaimed register"
> messages when booting with eDP only and using the i915.disable_power_well=1.
> 
> Also fix a comment and remove an useless empty line.
> 
> The error messages were caused by:
> 
>   commit 88adfff1ad5019f65b9d0b4e1a4ac900fb065183
>   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>   Date:   Thu Mar 28 10:42:01 2013 +0100
>       drm/i915: hw readout support for ->has_pch_encoders
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

-- 
Damien

Re: [PATCH 2/7] drm/i915: use cpu_transcoder for TRANS_DDI_FUNC_CTL

[Daniel Vetter]

On Fri, Apr 19, 2013 at 06:21:18AM +0100, Damien Lespiau wrote:
> On Thu, Apr 18, 2013 at 04:35:41PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > ... inside haswell_get_pipe_config. Because there's one TRANS_DDI_FUNC_CTL
> > register per CPU transcoder, not per pipe. This solves "unclaimed register"
> > messages when booting with eDP only and using the i915.disable_power_well=1.
> > 
> > Also fix a comment and remove an useless empty line.
> > 
> > The error messages were caused by:
> > 
> >   commit 88adfff1ad5019f65b9d0b4e1a4ac900fb065183
> >   Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> >   Date:   Thu Mar 28 10:42:01 2013 +0100
> >       drm/i915: hw readout support for ->has_pch_encoders
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Up to this patch here merged to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 3/7] drm/i915: add intel_display_power_enabled

[Paulo Zanoni]

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

This should replace intel_using_power_well. The idea is that we're
adding the requested power domain as an argument, so this might enable
the code to look less platform-specific and also allows us to easily
add new domains in case we need.

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
 drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
 3 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b86023d..b492eaa 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 		state = true;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP) {
+	if (cpu_transcoder != TRANSCODER_EDP &&
+	    !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
 		cur_state = false;
 	} else {
 		reg = PIPECONF(cpu_transcoder);
@@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
+	enum intel_display_power_domain domain;
 
 	if (!intel_crtc->active)
 		return;
@@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	/* XXX: Once we have proper panel fitter state tracking implemented with
 	 * hardware state read/check support we should switch to only disable
 	 * the panel fitter when we know it's used. */
-	if (intel_using_power_well(dev)) {
+	if (pipe == PIPE_A)
+		domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
+	else
+		domain = POWER_DOMAIN_OTHER;
+
+	if (intel_display_power_enabled(dev, domain)) {
 		I915_WRITE(PF_CTL(pipe), 0);
 		I915_WRITE(PF_WIN_SZ(pipe), 0);
 	}
@@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP)
+	if (cpu_transcoder != TRANSCODER_EDP &&
+	    !intel_display_power_enabled(dev, POWER_DOMAIN_OTHER))
 		return false;
 
 	tmp = I915_READ(PIPECONF(cpu_transcoder));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b5b6d19..04b3f84 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -465,6 +465,13 @@ struct intel_fbc_work {
 	int interval;
 };
 
+enum intel_display_power_domain {
+	POWER_DOMAIN_PIPE_A,
+	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
+	POWER_DOMAIN_TRANSCODER_EDP,
+	POWER_DOMAIN_OTHER,
+};
+
 int intel_pch_rawclk(struct drm_device *dev);
 
 int intel_connector_update_modes(struct drm_connector *connector,
@@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern bool intel_using_power_well(struct drm_device *dev);
+extern bool intel_display_power_enabled(struct drm_device *dev,
+					enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2557926..1c472d7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_using_power_well(struct drm_device *dev)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool power_well_enabled;
 
-	if (IS_HASWELL(dev))
-		return I915_READ(HSW_PWR_WELL_DRIVER) ==
-		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
-	else
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
+			     (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
 		return true;
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_OTHER:
+		return power_well_enabled;
+	default:
+		BUG();
+	}
 }
 
 void intel_set_power_well(struct drm_device *dev, bool enable)
-- 
1.7.10.4

Re: [PATCH 3/7] drm/i915: add intel_display_power_enabled

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This should replace intel_using_power_well. The idea is that we're
> adding the requested power domain as an argument, so this might enable
> the code to look less platform-specific and also allows us to easily
> add new domains in case we need.
> 
> Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
>  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
>  3 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b86023d..b492eaa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>  		state = true;
>  
> -	if (!intel_using_power_well(dev_priv->dev) &&
> -	    cpu_transcoder != TRANSCODER_EDP) {
> +	if (cpu_transcoder != TRANSCODER_EDP &&
> +	    !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {

I'm wondering if we could simplify the look of this and try to avoid
using POWER_DOMAIN_OTHER, for instance having a:

if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
	return false;

playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
the cpu_transcoder in them.

>  		cur_state = false;
>  	} else {
>  		reg = PIPECONF(cpu_transcoder);
> @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	int pipe = intel_crtc->pipe;
>  	int plane = intel_crtc->plane;
>  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> +	enum intel_display_power_domain domain;
>  
>  	if (!intel_crtc->active)
>  		return;
> @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	/* XXX: Once we have proper panel fitter state tracking implemented with
>  	 * hardware state read/check support we should switch to only disable
>  	 * the panel fitter when we know it's used. */
> -	if (intel_using_power_well(dev)) {
> +	if (pipe == PIPE_A)
> +		domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
> +	else
> +		domain = POWER_DOMAIN_OTHER;
> +

Of course, if the above is found useful, same thing for here for the
panel fitter function.

> +	if (intel_display_power_enabled(dev, domain)) {
>  		I915_WRITE(PF_CTL(pipe), 0);
>  		I915_WRITE(PF_WIN_SZ(pipe), 0);
>  	}
> @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>  	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>  	uint32_t tmp;
>  

[...]

> +enum intel_display_power_domain {
> +	POWER_DOMAIN_PIPE_A,
> +	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
> +	POWER_DOMAIN_TRANSCODER_EDP,
> +	POWER_DOMAIN_OTHER,
> +};
> +
>  int intel_pch_rawclk(struct drm_device *dev);
>  
>  int intel_connector_update_modes(struct drm_connector *connector,
> @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
>  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>  extern void intel_gpu_ips_teardown(void);
>  
> -extern bool intel_using_power_well(struct drm_device *dev);
> +extern bool intel_display_power_enabled(struct drm_device *dev,
> +					enum intel_display_power_domain domain);
>  extern void intel_init_power_well(struct drm_device *dev);
>  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>  extern void intel_enable_gt_powersave(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 2557926..1c472d7 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
>   * enable it, so check if it's enabled and also check if we've requested it to
>   * be enabled.
>   */
> -bool intel_using_power_well(struct drm_device *dev)
> +bool intel_display_power_enabled(struct drm_device *dev,
> +				 enum intel_display_power_domain domain)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	bool power_well_enabled;
>  
> -	if (IS_HASWELL(dev))
> -		return I915_READ(HSW_PWR_WELL_DRIVER) ==
> -		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> -	else
> +	if (!HAS_POWER_WELL(dev))
> +		return true;
> +
> +	power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
> +			     (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> +
> +	switch (domain) {
> +	case POWER_DOMAIN_PIPE_A:
> +	case POWER_DOMAIN_TRANSCODER_EDP:
>  		return true;
> +	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> +	case POWER_DOMAIN_OTHER:
> +		return power_well_enabled;
> +	default:
> +		BUG();
> +	}
>  }
  
-- 
Damien

Re: [PATCH 3/7] drm/i915: add intel_display_power_enabled

[Daniel Vetter]

On Fri, Apr 19, 2013 at 06:44:47AM +0100, Damien Lespiau wrote:
> On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This should replace intel_using_power_well. The idea is that we're
> > adding the requested power domain as an argument, so this might enable
> > the code to look less platform-specific and also allows us to easily
> > add new domains in case we need.
> > 
> > Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
> >  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
> >  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
> >  3 files changed, 38 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index b86023d..b492eaa 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >  	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >  		state = true;
> >  
> > -	if (!intel_using_power_well(dev_priv->dev) &&
> > -	    cpu_transcoder != TRANSCODER_EDP) {
> > +	if (cpu_transcoder != TRANSCODER_EDP &&
> > +	    !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
> 
> I'm wondering if we could simplify the look of this and try to avoid
> using POWER_DOMAIN_OTHER, for instance having a:
> 
> if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> 	return false;
> 
> playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
> the cpu_transcoder in them.

Well I have my own bikeshed - imo POWER_DOMAIN_TRANSCODER_EDP should be
called just POWER_DOMAIN_EDP since it also includes the eDP port. So the
transcoder part in the name is a bit misleading. And I suspect that we
still miss some DDI A vs everything else checks in the hw state readout
code ...

My fear with using more fine-grained domains than what we currently (and
for the next few generations of hw, hopefully) need is that we spill check
complexity into code where it's simply not relevant. So I think that the
code clarity we gain by pushing the "is this thing in one of the special
domains" into intel_display_power_enabled we'll waste again with
inconsistency in checking - it's not relevant for most things after all.
-Daniel

> 
> >  		cur_state = false;
> >  	} else {
> >  		reg = PIPECONF(cpu_transcoder);
> > @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >  	int pipe = intel_crtc->pipe;
> >  	int plane = intel_crtc->plane;
> >  	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
> > +	enum intel_display_power_domain domain;
> >  
> >  	if (!intel_crtc->active)
> >  		return;
> > @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >  	/* XXX: Once we have proper panel fitter state tracking implemented with
> >  	 * hardware state read/check support we should switch to only disable
> >  	 * the panel fitter when we know it's used. */
> > -	if (intel_using_power_well(dev)) {
> > +	if (pipe == PIPE_A)
> > +		domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
> > +	else
> > +		domain = POWER_DOMAIN_OTHER;
> > +
> 
> Of course, if the above is found useful, same thing for here for the
> panel fitter function.
> 
> > +	if (intel_display_power_enabled(dev, domain)) {
> >  		I915_WRITE(PF_CTL(pipe), 0);
> >  		I915_WRITE(PF_WIN_SZ(pipe), 0);
> >  	}
> > @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
> >  	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
> >  	uint32_t tmp;
> >  
> 
> [...]
> 
> > +enum intel_display_power_domain {
> > +	POWER_DOMAIN_PIPE_A,
> > +	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
> > +	POWER_DOMAIN_TRANSCODER_EDP,
> > +	POWER_DOMAIN_OTHER,
> > +};
> > +
> >  int intel_pch_rawclk(struct drm_device *dev);
> >  
> >  int intel_connector_update_modes(struct drm_connector *connector,
> > @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
> >  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
> >  extern void intel_gpu_ips_teardown(void);
> >  
> > -extern bool intel_using_power_well(struct drm_device *dev);
> > +extern bool intel_display_power_enabled(struct drm_device *dev,
> > +					enum intel_display_power_domain domain);
> >  extern void intel_init_power_well(struct drm_device *dev);
> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index 2557926..1c472d7 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
> >   * enable it, so check if it's enabled and also check if we've requested it to
> >   * be enabled.
> >   */
> > -bool intel_using_power_well(struct drm_device *dev)
> > +bool intel_display_power_enabled(struct drm_device *dev,
> > +				 enum intel_display_power_domain domain)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	bool power_well_enabled;
> >  
> > -	if (IS_HASWELL(dev))
> > -		return I915_READ(HSW_PWR_WELL_DRIVER) ==
> > -		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> > -	else
> > +	if (!HAS_POWER_WELL(dev))
> > +		return true;
> > +
> > +	power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
> > +			     (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> > +
> > +	switch (domain) {
> > +	case POWER_DOMAIN_PIPE_A:
> > +	case POWER_DOMAIN_TRANSCODER_EDP:
> >  		return true;
> > +	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
> > +	case POWER_DOMAIN_OTHER:
> > +		return power_well_enabled;
> > +	default:
> > +		BUG();
> > +	}
> >  }
>   
> -- 
> Damien
> _______________________________________________
> 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 3/7] drm/i915: add intel_display_power_enabled

[Paulo Zanoni]

Hi

2013/4/19 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Apr 19, 2013 at 06:44:47AM +0100, Damien Lespiau wrote:
>> On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > This should replace intel_using_power_well. The idea is that we're
>> > adding the requested power domain as an argument, so this might enable
>> > the code to look less platform-specific and also allows us to easily
>> > add new domains in case we need.
>> >
>> > Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
>> >  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
>> >  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
>> >  3 files changed, 38 insertions(+), 11 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index b86023d..b492eaa 100644
>> > --- a/drivers/gpu/drm/i915/intel_display.c
>> > +++ b/drivers/gpu/drm/i915/intel_display.c
>> > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
>> >     if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>> >             state = true;
>> >
>> > -   if (!intel_using_power_well(dev_priv->dev) &&
>> > -       cpu_transcoder != TRANSCODER_EDP) {
>> > +   if (cpu_transcoder != TRANSCODER_EDP &&
>> > +       !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
>>
>> I'm wondering if we could simplify the look of this and try to avoid
>> using POWER_DOMAIN_OTHER, for instance having a:
>>
>> if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
>>       return false;
>>
>> playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
>> the cpu_transcoder in them.
>
> Well I have my own bikeshed - imo POWER_DOMAIN_TRANSCODER_EDP should be
> called just POWER_DOMAIN_EDP since it also includes the eDP port. So the
> transcoder part in the name is a bit misleading. And I suspect that we
> still miss some DDI A vs everything else checks in the hw state readout
> code ...

I think POWER_DOMAIN_EDP can be misleading. We can have "PIPE_B +
TRANSCODER_EDP" configured (who can guarantee what the xrandr apps
really do?), and we can also have the "eDP on port D" which will never
use TRANSCODER_EDP, it will use PIPE_X + CPU_TRANSCODER_X.

>
> My fear with using more fine-grained domains than what we currently (and
> for the next few generations of hw, hopefully) need is that we spill check
> complexity into code where it's simply not relevant. So I think that the
> code clarity we gain by pushing the "is this thing in one of the special
> domains" into intel_display_power_enabled we'll waste again with
> inconsistency in checking - it's not relevant for most things after all.
> -Daniel
>
>>
>> >             cur_state = false;
>> >     } else {
>> >             reg = PIPECONF(cpu_transcoder);
>> > @@ -3584,6 +3584,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >     int pipe = intel_crtc->pipe;
>> >     int plane = intel_crtc->plane;
>> >     enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
>> > +   enum intel_display_power_domain domain;
>> >
>> >     if (!intel_crtc->active)
>> >             return;
>> > @@ -3607,7 +3608,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>> >     /* XXX: Once we have proper panel fitter state tracking implemented with
>> >      * hardware state read/check support we should switch to only disable
>> >      * the panel fitter when we know it's used. */
>> > -   if (intel_using_power_well(dev)) {
>> > +   if (pipe == PIPE_A)
>> > +           domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
>> > +   else
>> > +           domain = POWER_DOMAIN_OTHER;
>> > +
>>
>> Of course, if the above is found useful, same thing for here for the
>> panel fitter function.
>>
>> > +   if (intel_display_power_enabled(dev, domain)) {
>> >             I915_WRITE(PF_CTL(pipe), 0);
>> >             I915_WRITE(PF_WIN_SZ(pipe), 0);
>> >     }
>> > @@ -5859,8 +5865,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>> >     enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
>> >     uint32_t tmp;
>> >
>>
>> [...]
>>
>> > +enum intel_display_power_domain {
>> > +   POWER_DOMAIN_PIPE_A,
>> > +   POWER_DOMAIN_PIPE_A_PANEL_FITTER,
>> > +   POWER_DOMAIN_TRANSCODER_EDP,
>> > +   POWER_DOMAIN_OTHER,
>> > +};
>> > +
>> >  int intel_pch_rawclk(struct drm_device *dev);
>> >
>> >  int intel_connector_update_modes(struct drm_connector *connector,
>> > @@ -700,7 +707,8 @@ extern void intel_update_fbc(struct drm_device *dev);
>> >  extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
>> >  extern void intel_gpu_ips_teardown(void);
>> >
>> > -extern bool intel_using_power_well(struct drm_device *dev);
>> > +extern bool intel_display_power_enabled(struct drm_device *dev,
>> > +                                   enum intel_display_power_domain domain);
>> >  extern void intel_init_power_well(struct drm_device *dev);
>> >  extern void intel_set_power_well(struct drm_device *dev, bool enable);
>> >  extern void intel_enable_gt_powersave(struct drm_device *dev);
>> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> > index 2557926..1c472d7 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -4287,15 +4287,28 @@ void intel_init_clock_gating(struct drm_device *dev)
>> >   * enable it, so check if it's enabled and also check if we've requested it to
>> >   * be enabled.
>> >   */
>> > -bool intel_using_power_well(struct drm_device *dev)
>> > +bool intel_display_power_enabled(struct drm_device *dev,
>> > +                            enum intel_display_power_domain domain)
>> >  {
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > +   bool power_well_enabled;
>> >
>> > -   if (IS_HASWELL(dev))
>> > -           return I915_READ(HSW_PWR_WELL_DRIVER) ==
>> > -                  (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
>> > -   else
>> > +   if (!HAS_POWER_WELL(dev))
>> > +           return true;
>> > +
>> > +   power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
>> > +                        (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
>> > +
>> > +   switch (domain) {
>> > +   case POWER_DOMAIN_PIPE_A:
>> > +   case POWER_DOMAIN_TRANSCODER_EDP:
>> >             return true;
>> > +   case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
>> > +   case POWER_DOMAIN_OTHER:
>> > +           return power_well_enabled;
>> > +   default:
>> > +           BUG();
>> > +   }
>> >  }
>>
>> --
>> Damien
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



--
Paulo Zanoni

Re: [PATCH 3/7] drm/i915: add intel_display_power_enabled

[Daniel Vetter]

On Fri, Apr 19, 2013 at 12:28:30PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/4/19 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Apr 19, 2013 at 06:44:47AM +0100, Damien Lespiau wrote:
> >> On Thu, Apr 18, 2013 at 04:35:42PM -0300, Paulo Zanoni wrote:
> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > This should replace intel_using_power_well. The idea is that we're
> >> > adding the requested power domain as an argument, so this might enable
> >> > the code to look less platform-specific and also allows us to easily
> >> > add new domains in case we need.
> >> >
> >> > Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
> >> >  drivers/gpu/drm/i915/intel_drv.h     |   10 +++++++++-
> >> >  drivers/gpu/drm/i915/intel_pm.c      |   23 ++++++++++++++++++-----
> >> >  3 files changed, 38 insertions(+), 11 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> > index b86023d..b492eaa 100644
> >> > --- a/drivers/gpu/drm/i915/intel_display.c
> >> > +++ b/drivers/gpu/drm/i915/intel_display.c
> >> > @@ -1227,8 +1227,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
> >> >     if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
> >> >             state = true;
> >> >
> >> > -   if (!intel_using_power_well(dev_priv->dev) &&
> >> > -       cpu_transcoder != TRANSCODER_EDP) {
> >> > +   if (cpu_transcoder != TRANSCODER_EDP &&
> >> > +       !intel_display_power_enabled(dev_priv->dev, POWER_DOMAIN_OTHER)) {
> >>
> >> I'm wondering if we could simplify the look of this and try to avoid
> >> using POWER_DOMAIN_OTHER, for instance having a:
> >>
> >> if (!intel_display_power_enabled(dev, POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> >>       return false;
> >>
> >> playing a bit with the POWER_DOMAIN_TRANSCODER_* enums to encode the
> >> the cpu_transcoder in them.
> >
> > Well I have my own bikeshed - imo POWER_DOMAIN_TRANSCODER_EDP should be
> > called just POWER_DOMAIN_EDP since it also includes the eDP port. So the
> > transcoder part in the name is a bit misleading. And I suspect that we
> > still miss some DDI A vs everything else checks in the hw state readout
> > code ...
> 
> I think POWER_DOMAIN_EDP can be misleading. We can have "PIPE_B +
> TRANSCODER_EDP" configured (who can guarantee what the xrandr apps
> really do?), and we can also have the "eDP on port D" which will never
> use TRANSCODER_EDP, it will use PIPE_X + CPU_TRANSCODER_X.

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

[PATCH 3/7] drm/i915: add intel_display_power_enabled

[Paulo Zanoni]

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

This should replace intel_using_power_well. The idea is that we're
adding the requested power domain as an argument, so this might enable
the code to look less platform-specific and also allows us to easily
add new domains in case we need.

v2: Add more domains to enum intel_display_power_domain

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   14 ++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
 drivers/gpu/drm/i915/intel_pm.c      |   27 ++++++++++++++++++++++-----
 4 files changed, 49 insertions(+), 11 deletions(-)

I hope this reflects the discussions both via email and IRC.

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index bd2d7f1..c79622a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -88,6 +88,20 @@ enum port {
 };
 #define port_name(p) ((p) + 'A')
 
+enum intel_display_power_domain {
+	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
+	POWER_DOMAIN_PIPE_A,
+	POWER_DOMAIN_PIPE_B,
+	POWER_DOMAIN_PIPE_C,
+	POWER_DOMAIN_TRANSCODER_A,
+	POWER_DOMAIN_TRANSCODER_B,
+	POWER_DOMAIN_TRANSCODER_C,
+	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+};
+
+#define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
+#define POWER_DOMAIN_TRANSCODER(tran) ((tran) + POWER_DOMAIN_TRANSCODER_A)
+
 enum hpd_pin {
 	HPD_NONE = 0,
 	HPD_PORT_A = HPD_NONE, /* PORT_A is internal */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c90605..bf80942 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1215,8 +1215,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 		state = true;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP) {
+	if (!intel_display_power_enabled(dev_priv->dev,
+				POWER_DOMAIN_TRANSCODER(cpu_transcoder))) {
 		cur_state = false;
 	} else {
 		reg = PIPECONF(cpu_transcoder);
@@ -3597,6 +3597,7 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	enum transcoder cpu_transcoder = intel_crtc->config.cpu_transcoder;
+	enum intel_display_power_domain domain;
 
 	if (!intel_crtc->active)
 		return;
@@ -3622,7 +3623,12 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	/* XXX: Once we have proper panel fitter state tracking implemented with
 	 * hardware state read/check support we should switch to only disable
 	 * the panel fitter when we know it's used. */
-	if (intel_using_power_well(dev)) {
+	if (pipe == PIPE_A)
+		domain = POWER_DOMAIN_PIPE_A_PANEL_FITTER;
+	else
+		domain = POWER_DOMAIN_PIPE(pipe);
+
+	if (intel_display_power_enabled(dev, domain)) {
 		I915_WRITE(PF_CTL(pipe), 0);
 		I915_WRITE(PF_WIN_SZ(pipe), 0);
 	}
@@ -5981,8 +5987,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP)
+	if (!intel_display_power_enabled(dev,
+			POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
 		return false;
 
 	tmp = I915_READ(PIPECONF(cpu_transcoder));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c20201d..d82cef8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -721,7 +721,8 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern bool intel_using_power_well(struct drm_device *dev);
+extern bool intel_display_power_enabled(struct drm_device *dev,
+					enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 2557926..4e20ba7 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4287,15 +4287,32 @@ void intel_init_clock_gating(struct drm_device *dev)
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_using_power_well(struct drm_device *dev)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool power_well_enabled;
 
-	if (IS_HASWELL(dev))
-		return I915_READ(HSW_PWR_WELL_DRIVER) ==
-		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
-	else
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	power_well_enabled = I915_READ(HSW_PWR_WELL_DRIVER) ==
+			     (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
 		return true;
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
+		return power_well_enabled;
+	default:
+		BUG();
+	}
 }
 
 void intel_set_power_well(struct drm_device *dev, bool enable)
-- 
1.7.10.4

Re: [PATCH 3/7] drm/i915: add intel_display_power_enabled

[Daniel Vetter]

On Fri, Apr 19, 2013 at 11:54 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> This should replace intel_using_power_well. The idea is that we're
> adding the requested power domain as an argument, so this might enable
> the code to look less platform-specific and also allows us to easily
> add new domains in case we need.
>
> v2: Add more domains to enum intel_display_power_domain
>
> Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |   14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   16 +++++++++++-----
>  drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
>  drivers/gpu/drm/i915/intel_pm.c      |   27 ++++++++++++++++++++++-----
>  4 files changed, 49 insertions(+), 11 deletions(-)
>
> I hope this reflects the discussions both via email and IRC.
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index bd2d7f1..c79622a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -88,6 +88,20 @@ enum port {
>  };
>  #define port_name(p) ((p) + 'A')
>
> +enum intel_display_power_domain {
> +       POWER_DOMAIN_PIPE_A_PANEL_FITTER,
> +       POWER_DOMAIN_PIPE_A,
> +       POWER_DOMAIN_PIPE_B,
> +       POWER_DOMAIN_PIPE_C,
> +       POWER_DOMAIN_TRANSCODER_A,
> +       POWER_DOMAIN_TRANSCODER_B,
> +       POWER_DOMAIN_TRANSCODER_C,
> +       POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
> +};
> +
> +#define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
> +#define POWER_DOMAIN_TRANSCODER(tran) ((tran) + POWER_DOMAIN_TRANSCODER_A)

I think if we go this way we should have a macro (and different
domains) for pfit A/B/C, too. Since now we have imo an ugly middle
ground ....
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/5] drm/i915: add intel_display_power_enabled

[Paulo Zanoni]

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

This should replace intel_using_power_well. The idea is that we're
adding the requested power domain as an argument, so this might enable
the code to look less platform-specific and also allows us to easily
add new domains in case we need.

v2: Add more domains to enum intel_display_power_domain
v3: Even more domains requested

Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |   18 ++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++-----
 drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
 drivers/gpu/drm/i915/intel_pm.c      |   24 ++++++++++++++++++++----
 4 files changed, 46 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3ac71db..5160e1e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -88,6 +88,24 @@ enum port {
 };
 #define port_name(p) ((p) + 'A')
 
+enum intel_display_power_domain {
+	POWER_DOMAIN_PIPE_A,
+	POWER_DOMAIN_PIPE_B,
+	POWER_DOMAIN_PIPE_C,
+	POWER_DOMAIN_PIPE_A_PANEL_FITTER,
+	POWER_DOMAIN_PIPE_B_PANEL_FITTER,
+	POWER_DOMAIN_PIPE_C_PANEL_FITTER,
+	POWER_DOMAIN_TRANSCODER_A,
+	POWER_DOMAIN_TRANSCODER_B,
+	POWER_DOMAIN_TRANSCODER_C,
+	POWER_DOMAIN_TRANSCODER_EDP = POWER_DOMAIN_TRANSCODER_A + 0xF,
+};
+
+#define POWER_DOMAIN_PIPE(pipe) ((pipe) + POWER_DOMAIN_PIPE_A)
+#define POWER_DOMAIN_PIPE_PANEL_FITTER(pipe) \
+		((pipe) + POWER_DOMAIN_PIPE_A_PANEL_FITTER)
+#define POWER_DOMAIN_TRANSCODER(tran) ((tran) + POWER_DOMAIN_TRANSCODER_A)
+
 enum hpd_pin {
 	HPD_NONE = 0,
 	HPD_PORT_A = HPD_NONE, /* PORT_A is internal */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6504337..71a9770 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1110,8 +1110,8 @@ void assert_pipe(struct drm_i915_private *dev_priv,
 	if (pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 		state = true;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP) {
+	if (!intel_display_power_enabled(dev_priv->dev,
+				POWER_DOMAIN_TRANSCODER(cpu_transcoder))) {
 		cur_state = false;
 	} else {
 		reg = PIPECONF(cpu_transcoder);
@@ -3523,7 +3523,8 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	/* XXX: Once we have proper panel fitter state tracking implemented with
 	 * hardware state read/check support we should switch to only disable
 	 * the panel fitter when we know it's used. */
-	if (intel_using_power_well(dev)) {
+	if (intel_display_power_enabled(dev,
+					POWER_DOMAIN_PIPE_PANEL_FITTER(pipe))) {
 		I915_WRITE(PF_CTL(pipe), 0);
 		I915_WRITE(PF_WIN_SZ(pipe), 0);
 	}
@@ -6023,8 +6024,8 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
 	enum transcoder cpu_transcoder = crtc->config.cpu_transcoder;
 	uint32_t tmp;
 
-	if (!intel_using_power_well(dev_priv->dev) &&
-	    cpu_transcoder != TRANSCODER_EDP)
+	if (!intel_display_power_enabled(dev,
+			POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
 		return false;
 
 	tmp = I915_READ(PIPECONF(cpu_transcoder));
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dfcf546..ae73631 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -759,7 +759,8 @@ extern void intel_update_fbc(struct drm_device *dev);
 extern void intel_gpu_ips_init(struct drm_i915_private *dev_priv);
 extern void intel_gpu_ips_teardown(void);
 
-extern bool intel_using_power_well(struct drm_device *dev);
+extern bool intel_display_power_enabled(struct drm_device *dev,
+					enum intel_display_power_domain domain);
 extern void intel_init_power_well(struct drm_device *dev);
 extern void intel_set_power_well(struct drm_device *dev, bool enable);
 extern void intel_enable_gt_powersave(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0f4b46e..664c59f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4333,15 +4333,31 @@ void intel_init_clock_gating(struct drm_device *dev)
  * enable it, so check if it's enabled and also check if we've requested it to
  * be enabled.
  */
-bool intel_using_power_well(struct drm_device *dev)
+bool intel_display_power_enabled(struct drm_device *dev,
+				 enum intel_display_power_domain domain)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_HASWELL(dev))
+	if (!HAS_POWER_WELL(dev))
+		return true;
+
+	switch (domain) {
+	case POWER_DOMAIN_PIPE_A:
+	case POWER_DOMAIN_TRANSCODER_EDP:
+		return true;
+	case POWER_DOMAIN_PIPE_B:
+	case POWER_DOMAIN_PIPE_C:
+	case POWER_DOMAIN_PIPE_A_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_B_PANEL_FITTER:
+	case POWER_DOMAIN_PIPE_C_PANEL_FITTER:
+	case POWER_DOMAIN_TRANSCODER_A:
+	case POWER_DOMAIN_TRANSCODER_B:
+	case POWER_DOMAIN_TRANSCODER_C:
 		return I915_READ(HSW_PWR_WELL_DRIVER) ==
 		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
-	else
-		return true;
+	default:
+		BUG();
+	}
 }
 
 void intel_set_power_well(struct drm_device *dev, bool enable)
-- 
1.7.10.4

[PATCH 2/5] drm/i915: add power well and cpu transcoder info to the error state

[Paulo Zanoni]

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

We need to dump these registers if we want to properly interpret the
others.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 71a9770..190b787 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9740,6 +9740,9 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 #include <linux/seq_file.h>
 
 struct intel_display_error_state {
+
+	u32 power_well_driver;
+
 	struct intel_cursor_error_state {
 		u32 control;
 		u32 position;
@@ -9748,6 +9751,7 @@ struct intel_display_error_state {
 	} cursor[I915_MAX_PIPES];
 
 	struct intel_pipe_error_state {
+		enum transcoder cpu_transcoder;
 		u32 conf;
 		u32 source;
 
@@ -9782,8 +9786,12 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (error == NULL)
 		return NULL;
 
+	if (HAS_POWER_WELL(dev))
+		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
+
 	for_each_pipe(i) {
 		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
+		error->pipe[i].cpu_transcoder = cpu_transcoder;
 
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
 			error->cursor[i].control = I915_READ(CURCNTR(i));
@@ -9829,8 +9837,13 @@ intel_display_print_error_state(struct seq_file *m,
 	int i;
 
 	seq_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
+	if (HAS_POWER_WELL(dev))
+		seq_printf(m, "PWR_WELL_CTL2: %08x\n",
+			   error->power_well_driver);
 	for_each_pipe(i) {
 		seq_printf(m, "Pipe [%d]:\n", i);
+		seq_printf(m, "  CPU transcoder: %c\n",
+			   transcoder_name(error->pipe[i].cpu_transcoder));
 		seq_printf(m, "  CONF: %08x\n", error->pipe[i].conf);
 		seq_printf(m, "  SRC: %08x\n", error->pipe[i].source);
 		seq_printf(m, "  HTOTAL: %08x\n", error->pipe[i].htotal);
-- 
1.7.10.4

[PATCH 3/5] drm/i915: clear FPGA_DBG_RM_NOCLAIM when capturing error state

[Paulo Zanoni]

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

In the error state function we read the registers without checking if
the power well is on, so after doing this we have to clear the
FPGA_DBG_RM_NOCLAIM bit to prevent the next I915_WRITE from detecting
it and printing an error message.

The first version of this patch was checking for the power well state
and then avoiding reading registers that were off, but the reviewers
requested to just read the registers any way and then later clear the
FPGA_DBG_RM_NOCLAIM bit.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 190b787..fcb1367 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9826,6 +9826,13 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
 	}
 
+	/* In the code above we read the registers without checking if the power
+	 * well was on, so here we have to clear the FPGA_DBG_RM_NOCLAIM bit to
+	 * prevent the next I915_WRITE from detecting it and printing an error
+	 * message. */
+	if (HAS_POWER_WELL(dev))
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
 	return error;
 }
 
-- 
1.7.10.4

[PATCH 4/5] drm/i915: check the power well on i915_pipe_enabled

[Paulo Zanoni]

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

This fixes "unclaimed register" messages when the power well is
disabled and there's a GPU hang.

v2: Use the new intel_display_power_enabled().
v3: Use the new domains for intel_display_power_enabled().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 03a31be..161101f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -384,6 +384,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 
+	if (!intel_display_power_enabled(dev,
+		POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
+		return false;
+
 	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
 }
 
-- 
1.7.10.4

Re: [PATCH 4/5] drm/i915: check the power well on i915_pipe_enabled

[Damien Lespiau]

On Fri, May 03, 2013 at 12:15:39PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This fixes "unclaimed register" messages when the power well is
> disabled and there's a GPU hang.
> 
> v2: Use the new intel_display_power_enabled().
> v3: Use the new domains for intel_display_power_enabled().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

-- 
Damien

Re: [PATCH 4/5] drm/i915: check the power well on i915_pipe_enabled

[Daniel Vetter]

On Fri, May 03, 2013 at 12:15:39PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This fixes "unclaimed register" messages when the power well is
> disabled and there's a GPU hang.

How does this blow up in a gpu hang? Afaics pipe_enabled is mostly called
in the vblank code, so this should blow up any time we call a vblank
function on a disabled pipe ...

Also yet another reason to hate the vblank code, the locking is horrible
in here.
-Daniel

> 
> v2: Use the new intel_display_power_enabled().
> v3: Use the new domains for intel_display_power_enabled().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 03a31be..161101f 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -384,6 +384,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
>  
> +	if (!intel_display_power_enabled(dev,
> +		POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> +		return false;
> +
>  	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
>  }
>  
> -- 
> 1.7.10.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 4/5] drm/i915: check the power well on i915_pipe_enabled

[Paulo Zanoni]

2013/5/7 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, May 03, 2013 at 12:15:39PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> This fixes "unclaimed register" messages when the power well is
>> disabled and there's a GPU hang.
>
> How does this blow up in a gpu hang? Afaics pipe_enabled is mostly called
> in the vblank code, so this should blow up any time we call a vblank
> function on a disabled pipe ...

If I add patches to also detect unclaimed "reads" instead of just
writes and some dump_stack() calls, then call ZZ_hangman:

[   89.025325] [drm:i915_error_work_func], resetting chip
[   89.029470] [drm] Simulated gpu hang, resetting stop_rings
[   89.029623] [drm:hsw_unclaimed_reg_check] *ERROR* Unclaimed 71008
[   89.032526] Pid: 63, comm: kworker/u:3 Not tainted
3.9.0-rc5.1305071105+ #721
[   89.032529] Call Trace:
[   89.032547]  [<ffffffffa00a550a>] hsw_unclaimed_reg_check+0x5b/0x5f
[i915]
[   89.032558]  [<ffffffffa00a5d35>] i915_read32+0xd0/0xe0 [i915]
[   89.032569]  [<ffffffffa00a9704>] i915_pipe_enabled+0x59/0x63
[i915]
[   89.032580]  [<ffffffffa00ab488>] gm45_get_vblank_counter+0x1a/0x5b
[i915]
[   89.032594]  [<ffffffffa0018324>] drm_irq_uninstall+0xbe/0x169
[drm]
[   89.032604]  [<ffffffffa00a6609>] i915_reset+0x140/0x16a [i915]
[   89.032615]  [<ffffffffa00ab030>] i915_error_work_func+0xc1/0x159
[i915]
[   89.032620]  [<ffffffff810498a7>] process_one_work+0x17c/0x2b8
[   89.032624]  [<ffffffff8104a216>] worker_thread+0x135/0x1e4
[   89.032627]  [<ffffffff8104a0e1>] ? manage_workers+0x248/0x248
[   89.032631]  [<ffffffff8104e5a1>] kthread+0x8d/0x95
[   89.032635]  [<ffffffff8104e514>] ? __kthread_parkme+0x65/0x65
[   89.032639]  [<ffffffff814e5b5c>] ret_from_fork+0x7c/0xb0
[   89.032643]  [<ffffffff8104e514>] ? __kthread_parkme+0x65/0x65
[   89.032645] [drm:gm45_get_vblank_counter], trying to get vblank
count for disabled pipe B
[   89.032651] [drm:hsw_unclaimed_reg_check] *ERROR* Unclaimed 72008
[   89.035460] Pid: 63, comm: kworker/u:3 Not tainted
3.9.0-rc5.1305071105+ #721
[   89.035462] Call Trace:
[   89.035473]  [<ffffffffa00a550a>] hsw_unclaimed_reg_check+0x5b/0x5f
[i915]
[   89.035483]  [<ffffffffa00a5d35>] i915_read32+0xd0/0xe0 [i915]
[   89.035493]  [<ffffffffa00a9704>] i915_pipe_enabled+0x59/0x63
[i915]
[   89.035503]  [<ffffffffa00ab488>] gm45_get_vblank_counter+0x1a/0x5b
[i915]
[   89.035514]  [<ffffffffa0018324>] drm_irq_uninstall+0xbe/0x169
[drm]
[   89.035524]  [<ffffffffa00a6609>] i915_reset+0x140/0x16a [i915]
[   89.035535]  [<ffffffffa00ab030>] i915_error_work_func+0xc1/0x159
[i915]
[   89.035539]  [<ffffffff810498a7>] process_one_work+0x17c/0x2b8
[   89.035542]  [<ffffffff8104a216>] worker_thread+0x135/0x1e4
[   89.035545]  [<ffffffff8104a0e1>] ? manage_workers+0x248/0x248
[   89.035549]  [<ffffffff8104e5a1>] kthread+0x8d/0x95
[   89.035553]  [<ffffffff8104e514>] ? __kthread_parkme+0x65/0x65
[   89.035556]  [<ffffffff814e5b5c>] ret_from_fork+0x7c/0xb0
[   89.035560]  [<ffffffff8104e514>] ? __kthread_parkme+0x65/0x65
[   89.035562] [drm:gm45_get_vblank_counter], trying to get vblank
count for disabled pipe C


>
> Also yet another reason to hate the vblank code, the locking is horrible
> in here.
> -Daniel
>
>>
>> v2: Use the new intel_display_power_enabled().
>> v3: Use the new domains for intel_display_power_enabled().
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 03a31be..161101f 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -384,6 +384,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>>       enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>>                                                                     pipe);
>>
>> +     if (!intel_display_power_enabled(dev,
>> +             POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
>> +             return false;
>> +
>>       return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
>>  }
>>
>> --
>> 1.7.10.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



-- 
Paulo Zanoni

Re: [PATCH 4/5] drm/i915: check the power well on i915_pipe_enabled

[Daniel Vetter]

On Tue, May 7, 2013 at 4:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/5/7 Daniel Vetter <daniel@ffwll.ch>:
>> On Fri, May 03, 2013 at 12:15:39PM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>>
>>> This fixes "unclaimed register" messages when the power well is
>>> disabled and there's a GPU hang.
>>
>> How does this blow up in a gpu hang? Afaics pipe_enabled is mostly called
>> in the vblank code, so this should blow up any time we call a vblank
>> function on a disabled pipe ...
>
> If I add patches to also detect unclaimed "reads" instead of just
> writes and some dump_stack() calls, then call ZZ_hangman:

Yeah, reading the gpu reset code I've stumbled over the irq
install/unistall stuff. I still wondered why you've mentioned this and
not the vblank stuff itself. Does that nowhere show up when running
i-g-t? I've somewhat expected that kms_flip would hit it. If not I
need to add another wishlist subtest ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 5/5] drm/i915: only disable DDI sound if intel_crtc->eld_vld

[Paulo Zanoni]

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

We already have the same check on intel_enable_ddi. This patch
prevents "unclaimed register" messages when the power well is
disabled.

V2: Reset intel_crtc->eld_vld to false after the mode_set function.
V3: Add both "type != INTEL_OUTPUT_EDP" requested.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |   11 +++++++----
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 3ff4de6..21fb852 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1300,7 +1300,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 		ironlake_edp_backlight_on(intel_dp);
 	}
 
-	if (intel_crtc->eld_vld) {
+	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
 		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
 		tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
 		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
@@ -1318,9 +1318,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
-	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-	tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
-	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
+		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
+			 (pipe * 4));
+		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	}
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fcb1367..7aa2295 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3855,8 +3855,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
 
-	intel_crtc->eld_vld = false;
 	dev_priv->display.crtc_disable(crtc);
+	intel_crtc->eld_vld = false;
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
-- 
1.7.10.4

Re: [PATCH 5/5] drm/i915: only disable DDI sound if intel_crtc->eld_vld

[Daniel Vetter]

On Fri, May 03, 2013 at 12:15:40PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We already have the same check on intel_enable_ddi. This patch
> prevents "unclaimed register" messages when the power well is
> disabled.
> 
> V2: Reset intel_crtc->eld_vld to false after the mode_set function.
> V3: Add both "type != INTEL_OUTPUT_EDP" requested.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Meh, intel_crtc->eld_vld is a horrible piece of state tracking. I'll jot
down a "add has_audio to pipe_config" task for me.

Anyway, we can polish this turd later on, all patches merged to dinq.

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |   11 +++++++----
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 3ff4de6..21fb852 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1300,7 +1300,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
>  		ironlake_edp_backlight_on(intel_dp);
>  	}
>  
> -	if (intel_crtc->eld_vld) {
> +	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
>  		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
>  		tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
>  		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> @@ -1318,9 +1318,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t tmp;
>  
> -	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> -	tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
> -	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> +	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
> +		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
> +		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
> +			 (pipe * 4));
> +		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
> +	}
>  
>  	if (type == INTEL_OUTPUT_EDP) {
>  		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fcb1367..7aa2295 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3855,8 +3855,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
>  	/* crtc should still be enabled when we disable it. */
>  	WARN_ON(!crtc->enabled);
>  
> -	intel_crtc->eld_vld = false;
>  	dev_priv->display.crtc_disable(crtc);
> +	intel_crtc->eld_vld = false;
>  	intel_crtc_update_sarea(crtc, false);
>  	dev_priv->display.off(crtc);
>  
> -- 
> 1.7.10.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 1/5] drm/i915: add intel_display_power_enabled

[Damien Lespiau]

On Fri, May 03, 2013 at 12:15:36PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This should replace intel_using_power_well. The idea is that we're
> adding the requested power domain as an argument, so this might enable
> the code to look less platform-specific and also allows us to easily
> add new domains in case we need.
> 
> v2: Add more domains to enum intel_display_power_domain
> v3: Even more domains requested
> 
> Requested-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

[PATCH 4/7] drm/i915: add power well and cpu transcoder info to the error state

[Paulo Zanoni]

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

We need to dump these registers if we want to properly interpret the
others.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b492eaa..215f76c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9499,6 +9499,9 @@ int intel_modeset_vga_set_state(struct drm_device *dev, bool state)
 #include <linux/seq_file.h>
 
 struct intel_display_error_state {
+
+	u32 power_well_driver;
+
 	struct intel_cursor_error_state {
 		u32 control;
 		u32 position;
@@ -9507,6 +9510,7 @@ struct intel_display_error_state {
 	} cursor[I915_MAX_PIPES];
 
 	struct intel_pipe_error_state {
+		enum transcoder cpu_transcoder;
 		u32 conf;
 		u32 source;
 
@@ -9541,8 +9545,12 @@ intel_display_capture_error_state(struct drm_device *dev)
 	if (error == NULL)
 		return NULL;
 
+	if (HAS_POWER_WELL(dev))
+		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
+
 	for_each_pipe(i) {
 		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
+		error->pipe[i].cpu_transcoder = cpu_transcoder;
 
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
 			error->cursor[i].control = I915_READ(CURCNTR(i));
@@ -9588,8 +9596,13 @@ intel_display_print_error_state(struct seq_file *m,
 	int i;
 
 	seq_printf(m, "Num Pipes: %d\n", INTEL_INFO(dev)->num_pipes);
+	if (HAS_POWER_WELL(dev))
+		seq_printf(m, "PWR_WELL_CTL2: %08x\n",
+			   error->power_well_driver);
 	for_each_pipe(i) {
 		seq_printf(m, "Pipe [%d]:\n", i);
+		seq_printf(m, "  CPU transcoder: %c\n",
+			   transcoder_name(error->pipe[i].cpu_transcoder));
 		seq_printf(m, "  CONF: %08x\n", error->pipe[i].conf);
 		seq_printf(m, "  SRC: %08x\n", error->pipe[i].source);
 		seq_printf(m, "  HTOTAL: %08x\n", error->pipe[i].htotal);
-- 
1.7.10.4

Re: [PATCH 4/7] drm/i915: add power well and cpu transcoder info to the error state

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:43PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We need to dump these registers if we want to properly interpret the
> others.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

-- 
Damien

[PATCH 5/7] drm/i915: clear FPGA_DBG_RM_NOCLAIM when capturing error state

[Paulo Zanoni]

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

In the error state function we read the registers without checking if
the power well is on, so after doing this we have to clear the
FPGA_DBG_RM_NOCLAIM bit to prevent the next I915_WRITE from detecting
it and printing an error message.

The first version of this patch was checking for the power well state
and then avoiding reading registers that were off, but the reviewers
requested to just read the registers any way and then later clear the
FPGA_DBG_RM_NOCLAIM bit.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 215f76c..43de5ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9585,6 +9585,13 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
 	}
 
+	/* In the code above we read the registers without checking if the power
+	 * well was on, so here we have to clear the FPGA_DBG_RM_NOCLAIM bit to
+	 * prevent the next I915_WRITE from detecting it and printing an error
+	 * message. */
+	if (HAS_POWER_WELL(dev))
+		I915_WRITE_NOTRACE(FPGA_DBG, FPGA_DBG_RM_NOCLAIM);
+
 	return error;
 }
 
-- 
1.7.10.4

Re: [PATCH 5/7] drm/i915: clear FPGA_DBG_RM_NOCLAIM when capturing error state

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:44PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> In the error state function we read the registers without checking if
> the power well is on, so after doing this we have to clear the
> FPGA_DBG_RM_NOCLAIM bit to prevent the next I915_WRITE from detecting
> it and printing an error message.
> 
> The first version of this patch was checking for the power well state
> and then avoiding reading registers that were off, but the reviewers
> requested to just read the registers any way and then later clear the
> FPGA_DBG_RM_NOCLAIM bit.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

-- 
Damien

[PATCH 6/7] drm/i915: check the power well on i915_pipe_enabled

[Paulo Zanoni]

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

This fixes "unclaimed register" messages when the power well is
disabled and there's a GPU hang.

v2: Use the new intel_display_power_enabled().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 932e7f8..fda2998 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -184,6 +184,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 
+	if (cpu_transcoder != TRANSCODER_EDP &&
+	    !intel_display_power_enabled(dev, POWER_DOMAIN_OTHER))
+		return false;
+
 	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
 }
 
-- 
1.7.10.4

Re: [PATCH 6/7] drm/i915: check the power well on i915_pipe_enabled

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:45PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This fixes "unclaimed register" messages when the power well is
> disabled and there's a GPU hang.
> 
> v2: Use the new intel_display_power_enabled().
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 932e7f8..fda2998 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -184,6 +184,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
>  
> +	if (cpu_transcoder != TRANSCODER_EDP &&
> +	    !intel_display_power_enabled(dev, POWER_DOMAIN_OTHER))
> +		return false;
> +

Same remark as in patch 3, I'd rather have a simplified logic asking
explicitely if the transcoder we're checking is powered.

-- 
Damien

[PATCH 6/7] drm/i915: check the power well on i915_pipe_enabled

[Paulo Zanoni]

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

This fixes "unclaimed register" messages when the power well is
disabled and there's a GPU hang.

v2: Use the new intel_display_power_enabled().
v3: Use the new domains for intel_display_power_enabled().

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@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 dae8953..124b2eb 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -391,6 +391,10 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 
+	if (!intel_display_power_enabled(dev,
+		POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
+		return false;
+
 	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
 }
 
-- 
1.7.10.4

[PATCH 7/7] drm/i915: only disable DDI sound if intel_crtc->eld_vld

[Paulo Zanoni]

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

We already have the same check on intel_enable_ddi. This patch
prevents "unclaimed register" messages when the power well is
disabled.

V2: Reset intel_crtc->eld_vld to false after the mode_set function.
V3: Add both "type != INTEL_OUTPUT_EDP" requested.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c     |   11 +++++++----
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index e14fe5f..441f27e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1329,7 +1329,7 @@ static void intel_enable_ddi(struct intel_encoder *intel_encoder)
 		ironlake_edp_backlight_on(intel_dp);
 	}
 
-	if (intel_crtc->eld_vld) {
+	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
 		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
 		tmp |= ((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
 		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
@@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
-	tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
-	tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) << (pipe * 4));
-	I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	if (intel_crtc->eld_vld && type != INTEL_OUTPUT_EDP) {
+		tmp = I915_READ(HSW_AUD_PIN_ELD_CP_VLD);
+		tmp &= ~((AUDIO_OUTPUT_ENABLE_A | AUDIO_ELD_VALID_A) <<
+			 (pipe * 4));
+		I915_WRITE(HSW_AUD_PIN_ELD_CP_VLD, tmp);
+	}
 
 	if (type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 43de5ef..10963f7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3856,8 +3856,8 @@ static void intel_crtc_disable(struct drm_crtc *crtc)
 	/* crtc should still be enabled when we disable it. */
 	WARN_ON(!crtc->enabled);
 
-	intel_crtc->eld_vld = false;
 	dev_priv->display.crtc_disable(crtc);
+	intel_crtc->eld_vld = false;
 	intel_crtc_update_sarea(crtc, false);
 	dev_priv->display.off(crtc);
 
-- 
1.7.10.4

Re: [PATCH 7/7] drm/i915: only disable DDI sound if intel_crtc->eld_vld

[Damien Lespiau]

On Thu, Apr 18, 2013 at 04:35:46PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We already have the same check on intel_enable_ddi. This patch
> prevents "unclaimed register" messages when the power well is
> disabled.
> 
> V2: Reset intel_crtc->eld_vld to false after the mode_set function.
> V3: Add both "type != INTEL_OUTPUT_EDP" requested.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

(having read back the original comments from Daniel and Ville)

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

-- 
Damien