[PATCH 00/15] More "unclaimed register" fixes

[PATCH 00/15] More "unclaimed register" fixes

[Paulo Zanoni]

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

Hi

This series fixes a few more "unclaimed register" problems. Some of them are
only visible if you're only using eDP (i.e., the power well is down) and most of
them are only visible if you get a GPU hang. The good thing is that I only get
GPU hangs when I run ZZ_hangman :)

Now when I boot a machine with only eDP the only "unclaimed register" message I
see is right after the audio driver loads. And then if I run ZZ_hangmen I won't
see "unclaimed register" messages.

Big series but small patches, most of them trivial.

Thanks,
Paulo

Paulo Zanoni (15):
  drm/i915: only disable DDI sound if intel_crtc->eld_vld
  drm/i915: disable sound first on intel_disable_ddi
  drm/i915: add intel_power_well_is_down
  drm/i915: don't touch the PF regs if the power well is down
  drm/i915: capture the correct cursor registers on IVB
  drm/i915: there's no DSPSIZE register on gen4+
  drm/i915: there's no DSPADDR register on Haswell
  drm/i915: remove DSPPOS register
  drm/i915: there's no PIPESTAT on Gen5+
  drm/i915: check the power well when capturing error state
  drm/i915: add HAS_POWER_WELL
  drm/i915: reorganize intel_lvds_supported
  drm/i915: don't save/restore PCH_LVDS on LPT
  drm/i915: check the power well on i915_pipe_enabled
  drm/i915: add missing space in error message

 drivers/gpu/drm/i915/i915_drv.h      |    3 +-
 drivers/gpu/drm/i915/i915_irq.c      |   10 +++--
 drivers/gpu/drm/i915/i915_reg.h      |    3 --
 drivers/gpu/drm/i915/i915_suspend.c  |    7 +--
 drivers/gpu/drm/i915/i915_ums.c      |    4 --
 drivers/gpu/drm/i915/intel_ddi.c     |   11 +++--
 drivers/gpu/drm/i915/intel_display.c |   80 +++++++++++++++++++++++-----------
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_lvds.c    |   10 ++---
 drivers/gpu/drm/i915/intel_pm.c      |   20 ++++++++-
 10 files changed, 98 insertions(+), 51 deletions(-)

-- 
1.7.10.4

[PATCH 01/15] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 56bb7cb..cd2f519 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 		ironlake_edp_backlight_off(intel_dp);
 	}
 
-	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) {
+		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);
+	}
 }
 
 int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
-- 
1.7.10.4

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

[Ville Syrjälä]

On Wed, Mar 06, 2013 at 08:03:08PM -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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 56bb7cb..cd2f519 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  		ironlake_edp_backlight_off(intel_dp);
>  	}
>  
> -	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) {
> +		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);
> +	}

We set eld_vld=false before disabling the crtc in intel_crtc_disable().
I think you need to rearrange that so that we clear eld_vld only
after ->crtc_disable has been called.

>  }
>  
>  int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

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

[Daniel Vetter]

On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -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.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >  		ironlake_edp_backlight_off(intel_dp);
> >  	}
> >  
> > -	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) {
> > +		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);
> > +	}
> 
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.

Just looked a bit through the code here and ->eld_vld is another case of
where the ddi encoder needs to know something which only the connector
really knows currently. Since in our dp/hdmi/sdvo code we just check
<connector>->has_audio in the respective modeset function.

And it's not really the only place where the apparently common ddi
functions are just if ladders gropping around in connector details.

I guess we need to eventually clean this mess up, once things have settled
a bit, since I fear the duplication it icky little bugs this fragmentation
of state keeping might (or probably will) cause.

Ideas welcome ;-)

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

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

[Daniel Vetter]

On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:08PM -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.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >  1 file changed, 6 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 56bb7cb..cd2f519 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >  		ironlake_edp_backlight_off(intel_dp);
> >  	}
> >  
> > -	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) {
> > +		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);
> > +	}
> 
> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> I think you need to rearrange that so that we clear eld_vld only
> after ->crtc_disable has been called.

I've forgotten to drop my bikeshed on the patch itself:

This looks a bit fishy since currently we assume that disabling something
just works (especially clearing a few registers). And I don't really
understand how we can hit unclaimed register issues since the pipe should
be enabled when we call this function here ...

So either transcoder eDP doesn't have audio, in which case I think it'd be
better to check for that here (plus ensure that we yell at callers for
integrated eDP in e.g. hsw_write_eld). Or it _does_ have audio, but the
audio stuff is in the power well. In which case we need to add a check.

Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
don't want to fire up the work machine ;-)

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

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

[Paulo Zanoni]

Hi

2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
>> On Wed, Mar 06, 2013 at 08:03:08PM -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.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
>> >  1 file changed, 6 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> > index 56bb7cb..cd2f519 100644
>> > --- a/drivers/gpu/drm/i915/intel_ddi.c
>> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>> >             ironlake_edp_backlight_off(intel_dp);
>> >     }
>> >
>> > -   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) {
>> > +           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);
>> > +   }
>>
>> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
>> I think you need to rearrange that so that we clear eld_vld only
>> after ->crtc_disable has been called.
>
> I've forgotten to drop my bikeshed on the patch itself:
>
> This looks a bit fishy since currently we assume that disabling something
> just works (especially clearing a few registers). And I don't really
> understand how we can hit unclaimed register issues since the pipe should
> be enabled when we call this function here ...

The audio registers are on the power well. My test case is: eDP panel
without sound. We don't hit unclaimed registers at the ->enable
function because it's protected by eld_vld (which is false, because we
don't have sound), but then at the disable path we just
unconditionally touch the registers which are on the power down well,
so "unclaimed register".

>
> So either transcoder eDP doesn't have audio, in which case I think it'd be
> better to check for that here (plus ensure that we yell at callers for
> integrated eDP in e.g. hsw_write_eld).

The eld_vld is the "check for audio" which you're asking for.

> Or it _does_ have audio, but the
> audio stuff is in the power well. In which case we need to add a check.

We need to patch haswell_modeset_global_resources to enable the power
well in case eDP has sound, but this is an unrelated bug with a
different patch. It's on my TODO list too.

Still, Ville's comment is valid, so I need to resend.

>
> Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
> don't want to fire up the work machine ;-)
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

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

[Daniel Vetter]

On Wed, Mar 20, 2013 at 07:03:25PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> > On Thu, Mar 07, 2013 at 11:31:23AM +0200, Ville Syrjälä wrote:
> >> On Wed, Mar 06, 2013 at 08:03:08PM -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.
> >> >
> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_ddi.c |    9 ++++++---
> >> >  1 file changed, 6 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> > index 56bb7cb..cd2f519 100644
> >> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> > @@ -1347,9 +1347,12 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
> >> >             ironlake_edp_backlight_off(intel_dp);
> >> >     }
> >> >
> >> > -   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) {
> >> > +           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);
> >> > +   }
> >>
> >> We set eld_vld=false before disabling the crtc in intel_crtc_disable().
> >> I think you need to rearrange that so that we clear eld_vld only
> >> after ->crtc_disable has been called.
> >
> > I've forgotten to drop my bikeshed on the patch itself:
> >
> > This looks a bit fishy since currently we assume that disabling something
> > just works (especially clearing a few registers). And I don't really
> > understand how we can hit unclaimed register issues since the pipe should
> > be enabled when we call this function here ...
> 
> The audio registers are on the power well. My test case is: eDP panel
> without sound. We don't hit unclaimed registers at the ->enable
> function because it's protected by eld_vld (which is false, because we
> don't have sound), but then at the disable path we just
> unconditionally touch the registers which are on the power down well,
> so "unclaimed register".
> 
> >
> > So either transcoder eDP doesn't have audio, in which case I think it'd be
> > better to check for that here (plus ensure that we yell at callers for
> > integrated eDP in e.g. hsw_write_eld).
> 
> The eld_vld is the "check for audio" which you're asking for.

Ok, now I've been slightly less lazy and quickly checked the docs. If I'm
reading the diagrams and audio connections correctly, then there's _no_
audio support at all for the eDP transcoder. Which means we'd need to
protect both the enable and disable side in trancoder != TRANS_EDP checks.

That's what I've meant with proper check. The eld_vld thing is a bit
ad-hoc and imo due to our ddi encoder/connector split: Only the connector
has the edid and so knows whether the sync claims to support audio, but we
need that information in the ddi encoder functions.

But since our current ddi code is full of such little control inversions
where the ddi code has to dig out some piece of information about the
current connector from somewhere, I don't care and postpone this to a time
when hsw support has really settled.
-Daniel

> 
> > Or it _does_ have audio, but the
> > audio stuff is in the power well. In which case we need to add a check.
> 
> We need to patch haswell_modeset_global_resources to enable the power
> well in case eDP has sound, but this is an unrelated bug with a
> different patch. It's on my TODO list too.
> 
> Still, Ville's comment is valid, so I need to resend.
> 
> >
> > Yes, I'm too lazy to check the docs myself, but tbh it's still w/e here so
> > don't want to fire up the work machine ;-)
> >
> > Cheers, Daniel
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Paulo Zanoni

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

[PATCH 2/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.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index bfcc58f..cb0d0f7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1341,9 +1341,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) {
+		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 ec26a85..188e31f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3822,8 +3822,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 2/7] drm/i915: only disable DDI sound if intel_crtc->eld_vld

[Ville Syrjälä]

On Fri, Mar 22, 2013 at 02:11:43PM -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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c     |    9 ++++++---
>  drivers/gpu/drm/i915/intel_display.c |    2 +-
>  2 files changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index bfcc58f..cb0d0f7 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1341,9 +1341,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) {
> +		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 ec26a85..188e31f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3822,8 +3822,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

-- 
Ville Syrjälä
Intel OTC

[PATCH 02/15] drm/i915: disable sound first on intel_disable_ddi

[Paulo Zanoni]

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

Our mode set sequence documentation says audio must be disabled before
the backlight.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cd2f519..c4c0e7e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1341,18 +1341,18 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t tmp;
 
-	if (type == INTEL_OUTPUT_EDP) {
-		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
-
-		ironlake_edp_backlight_off(intel_dp);
-	}
-
 	if (intel_crtc->eld_vld) {
 		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);
+
+		ironlake_edp_backlight_off(intel_dp);
+	}
 }
 
 int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
-- 
1.7.10.4

Re: [PATCH 02/15] drm/i915: disable sound first on intel_disable_ddi

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:09PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Our mode set sequence documentation says audio must be disabled before
> the backlight.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

At least what I see it says, Audio must be disabled, "first." So I'd
recommend modifying the commit message to not mention backlight
specifically

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/intel_ddi.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cd2f519..c4c0e7e 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1341,18 +1341,18 @@ static void intel_disable_ddi(struct intel_encoder *intel_encoder)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint32_t tmp;
>  
> -	if (type == INTEL_OUTPUT_EDP) {
> -		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> -
> -		ironlake_edp_backlight_off(intel_dp);
> -	}
> -
>  	if (intel_crtc->eld_vld) {
>  		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);
> +
> +		ironlake_edp_backlight_off(intel_dp);
> +	}
>  }
>  
>  int intel_ddi_get_cdclk_freq(struct drm_i915_private *dev_priv)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 02/15] drm/i915: disable sound first on intel_disable_ddi

[Daniel Vetter]

On Fri, Mar 15, 2013 at 11:24:03AM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:09PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Our mode set sequence documentation says audio must be disabled before
> > the backlight.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> At least what I see it says, Audio must be disabled, "first." So I'd
> recommend modifying the commit message to not mention backlight
> specifically
> 
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next with Ben's bikeshed, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/15] drm/i915: add intel_power_well_is_down

[Paulo Zanoni]

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

It returns true if we're not supposed to touch the registers on the
power down well.

For now there's just one caller, but I'm going to add more.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    4 ++--
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
-	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
+	if (intel_power_well_is_down(dev_priv->dev) &&
+	    cpu_transcoder != TRANSCODER_EDP) {
 		cur_state = false;
 	} else {
 		reg = PIPECONF(cpu_transcoder);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 010e998..28c4789 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -671,6 +671,7 @@ 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_power_well_is_down(struct drm_device *dev);
 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 5479363..90562bc 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
+/**
+ * Returns true if we're not supposed to touch the registers on the power down
+ * well. Notice that we don't check whether the power well is actually off, we
+ * just check if our driver told the hardware that it doesn't need the power
+ * well enabled.
+ */
+bool intel_power_well_is_down(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_HASWELL(dev))
+		return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
+	else
+		return false;
+}
+
 void intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.10.4

Re: [PATCH 03/15] drm/i915: add intel_power_well_is_down

[Daniel Vetter]

On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It returns true if we're not supposed to touch the registers on the
> power down well.
> 
> For now there's just one caller, but I'm going to add more.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
> -	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
> +	if (intel_power_well_is_down(dev_priv->dev) &&

The name here feels a bit too generic given that we already have on hsw
different display c states with different requirements and different
pieces of hw not working.

Can't thinkg of anything better than intel_display_power_well_is_down
though ...
-Daniel

> +	    cpu_transcoder != TRANSCODER_EDP) {
>  		cur_state = false;
>  	} else {
>  		reg = PIPECONF(cpu_transcoder);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 010e998..28c4789 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -671,6 +671,7 @@ 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_power_well_is_down(struct drm_device *dev);
>  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 5479363..90562bc 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> +/**
> + * Returns true if we're not supposed to touch the registers on the power down
> + * well. Notice that we don't check whether the power well is actually off, we
> + * just check if our driver told the hardware that it doesn't need the power
> + * well enabled.
> + */
> +bool intel_power_well_is_down(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (IS_HASWELL(dev))
> +		return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
> +	else
> +		return false;
> +}
> +
>  void intel_set_power_well(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 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 03/15] drm/i915: add intel_power_well_is_down

[Daniel Vetter]

On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote:
> On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > It returns true if we're not supposed to touch the registers on the
> > power down well.
> > 
> > For now there's just one caller, but I'm going to add more.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    4 ++--
> >  drivers/gpu/drm/i915/intel_drv.h     |    1 +
> >  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
> >  3 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
> > -	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
> > +	if (intel_power_well_is_down(dev_priv->dev) &&
> 
> The name here feels a bit too generic given that we already have on hsw
> different display c states with different requirements and different
> pieces of hw not working.
> 
> Can't thinkg of anything better than intel_display_power_well_is_down
> though ...

Also maybe make the function assure that the power well is on i.e.
power_well_enabled to avoid double-negation logic in a bunch of places.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 03/15] drm/i915: add intel_power_well_is_down

[Ben Widawsky]

On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote:
> On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > It returns true if we're not supposed to touch the registers on the
> > power down well.
> > 
> > For now there's just one caller, but I'm going to add more.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    4 ++--
> >  drivers/gpu/drm/i915/intel_drv.h     |    1 +
> >  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
> >  3 files changed, 19 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
> > -	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
> > +	if (intel_power_well_is_down(dev_priv->dev) &&
> 
> The name here feels a bit too generic given that we already have on hsw
> different display c states with different requirements and different
> pieces of hw not working.
> 
> Can't thinkg of anything better than intel_display_power_well_is_down
> though ...
> -Daniel
> 
> > +	    cpu_transcoder != TRANSCODER_EDP) {
> >  		cur_state = false;
> >  	} else {
> >  		reg = PIPECONF(cpu_transcoder);
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 010e998..28c4789 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -671,6 +671,7 @@ 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_power_well_is_down(struct drm_device *dev);
> >  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 5479363..90562bc 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev)
> >  	dev_priv->display.init_clock_gating(dev);
> >  }
> >  
> > +/**
> > + * Returns true if we're not supposed to touch the registers on the power down
> > + * well. Notice that we don't check whether the power well is actually off, we
> > + * just check if our driver told the hardware that it doesn't need the power
> > + * well enabled.
> > + */

I agree with Denial that the name sucks because your comment clearly
contradicts what the function is actually called. Can't think of
anything better either.

In the bikeshed realm, I think it makes more sense to do the IS_HASWELL
check in your pipe assertion, but I'll assume that you have a good usage
as you mention later.

Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> > +bool intel_power_well_is_down(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	if (IS_HASWELL(dev))
> > +		return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
> > +	else
> > +		return false;
> > +}
> > +
> >  void intel_set_power_well(struct drm_device *dev, bool enable)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > -- 
> > 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 03/15] drm/i915: add intel_power_well_is_down

[Paulo Zanoni]

Hi

2013/3/15 Ben Widawsky <ben@bwidawsk.net>:
> On Thu, Mar 07, 2013 at 12:26:23AM +0100, Daniel Vetter wrote:
>> On Wed, Mar 06, 2013 at 08:03:10PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > It returns true if we're not supposed to touch the registers on the
>> > power down well.
>> >
>> > For now there's just one caller, but I'm going to add more.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>> >  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>> >  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
>> >  3 files changed, 19 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> > index 502cb28..bd27336 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
>> > -       !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
>> > +   if (intel_power_well_is_down(dev_priv->dev) &&
>>
>> The name here feels a bit too generic given that we already have on hsw
>> different display c states with different requirements and different
>> pieces of hw not working.
>>
>> Can't thinkg of anything better than intel_display_power_well_is_down
>> though ...
>> -Daniel
>>
>> > +       cpu_transcoder != TRANSCODER_EDP) {
>> >             cur_state = false;
>> >     } else {
>> >             reg = PIPECONF(cpu_transcoder);
>> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> > index 010e998..28c4789 100644
>> > --- a/drivers/gpu/drm/i915/intel_drv.h
>> > +++ b/drivers/gpu/drm/i915/intel_drv.h
>> > @@ -671,6 +671,7 @@ 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_power_well_is_down(struct drm_device *dev);
>> >  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 5479363..90562bc 100644
>> > --- a/drivers/gpu/drm/i915/intel_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_pm.c
>> > @@ -4070,6 +4070,22 @@ void intel_init_clock_gating(struct drm_device *dev)
>> >     dev_priv->display.init_clock_gating(dev);
>> >  }
>> >
>> > +/**
>> > + * Returns true if we're not supposed to touch the registers on the power down
>> > + * well. Notice that we don't check whether the power well is actually off, we
>> > + * just check if our driver told the hardware that it doesn't need the power
>> > + * well enabled.
>> > + */
>
> I agree with Denial that the name sucks because your comment clearly
> contradicts what the function is actually called. Can't think of
> anything better either.

intel_using_power_well?

>
> In the bikeshed realm, I think it makes more sense to do the IS_HASWELL
> check in your pipe assertion, but I'll assume that you have a good usage
> as you mention later.

I want to make all the power well functions work correctly on Gens
that don't have a power well. We already introduced bugs related to
this in the past, so let's be defensive.

>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
>
>> > +bool intel_power_well_is_down(struct drm_device *dev)
>> > +{
>> > +   struct drm_i915_private *dev_priv = dev->dev_private;
>> > +
>> > +   if (IS_HASWELL(dev))
>> > +           return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
>> > +   else
>> > +           return false;
>> > +}
>> > +
>> >  void intel_set_power_well(struct drm_device *dev, bool enable)
>> >  {
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> > --
>> > 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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ben Widawsky, Intel Open Source Technology Center



-- 
Paulo Zanoni

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

[Paulo Zanoni]

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

It returns true if we've requested to turn the power well on and it's
really on. It also returns true for all the previous gens.

For now there's just one caller, but I'm going to add more.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    4 ++--
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 188e31f..be70f2d 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
-	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
+	if (!intel_using_power_well(dev_priv->dev) &&
+	    cpu_transcoder != TRANSCODER_EDP) {
 		cur_state = false;
 	} else {
 		reg = PIPECONF(cpu_transcoder);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index e6f84d0..40733d9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -671,6 +671,7 @@ 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 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 2de6da6..13404f6 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4086,6 +4086,22 @@ void intel_init_clock_gating(struct drm_device *dev)
 	dev_priv->display.init_clock_gating(dev);
 }
 
+/**
+ * We should only use the power well if we explicitly asked the hardware to
+ * 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)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (IS_HASWELL(dev))
+		return I915_READ(HSW_PWR_WELL_DRIVER) ==
+		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
+	else
+		return true;
+}
+
 void intel_set_power_well(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-- 
1.7.10.4

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

[Daniel Vetter]

Hi Paulo,

On Fri, Mar 22, 2013 at 02:14:13PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> It returns true if we've requested to turn the power well on and it's
> really on. It also returns true for all the previous gens.
> 
> For now there's just one caller, but I'm going to add more.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Yeah, I've merged this but just stumbled over it again while rebasing the
-internal tree. And I'm still unhappy with the name a bit, since
power_well is a bit generic. I know it's what bspec uses, but still I'd
like to have some notion in it that this is about display stuff

The other thing which always irked me is that sprinkling power wells
checks all over the place just feels ugly. What we actually want to check
is whether the display hw is powered on, which feels much less
platform-specific.

So what about a s/intel_using_power_well/intel_display_power_enabled? It's
not perfect since the actual piece of hw we care about is still platform
specific, so I'd suggest to throw an enum on top:

enum intel_display_power_domains {
	POWER_DOMAIN_EDP,
	POWER_DOMAIN_EDP_PFIT,
	POWER_DOMAIN_OTHER
};

bool intel_display_power_enabled(struct drm_device *dev,
				 enum intel_display_power_domain domain);

We could easily add new domains for e.g. the pc8 stuff with a
POWER_DOMAIN_CONNECTOR_AUX or so if we need to work around more unclaimed
register warnings.

With that piece of infrastructure I think I'll stop being grumpy about
power wells checks and unclaimed register fixup patches and just merge
them all.

Comments?

Cheers, Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |    4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_pm.c      |   16 ++++++++++++++++
>  3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 188e31f..be70f2d 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 (IS_HASWELL(dev_priv->dev) && cpu_transcoder != TRANSCODER_EDP &&
> -	    !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE)) {
> +	if (!intel_using_power_well(dev_priv->dev) &&
> +	    cpu_transcoder != TRANSCODER_EDP) {
>  		cur_state = false;
>  	} else {
>  		reg = PIPECONF(cpu_transcoder);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index e6f84d0..40733d9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -671,6 +671,7 @@ 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 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 2de6da6..13404f6 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4086,6 +4086,22 @@ void intel_init_clock_gating(struct drm_device *dev)
>  	dev_priv->display.init_clock_gating(dev);
>  }
>  
> +/**
> + * We should only use the power well if we explicitly asked the hardware to
> + * 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)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (IS_HASWELL(dev))
> +		return I915_READ(HSW_PWR_WELL_DRIVER) ==
> +		       (HSW_PWR_WELL_ENABLE | HSW_PWR_WELL_STATE);
> +	else
> +		return true;
> +}
> +
>  void intel_set_power_well(struct drm_device *dev, bool enable)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -- 
> 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 3/7] drm/i915: add intel_using_power_well

[Daniel Vetter]

On Wed, Apr 17, 2013 at 11:04:23AM +0200, Daniel Vetter wrote:
> Hi Paulo,
>
> On Fri, Mar 22, 2013 at 02:14:13PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > It returns true if we've requested to turn the power well on and it's
> > really on. It also returns true for all the previous gens.
> >
> > For now there's just one caller, but I'm going to add more.
> >
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Yeah, I've merged this but just stumbled over it again while rebasing the
> -internal tree. And I'm still unhappy with the name a bit, since
> power_well is a bit generic. I know it's what bspec uses, but still I'd
> like to have some notion in it that this is about display stuff
>
> The other thing which always irked me is that sprinkling power wells
> checks all over the place just feels ugly. What we actually want to check
> is whether the display hw is powered on, which feels much less
> platform-specific.
>
> So what about a s/intel_using_power_well/intel_display_power_enabled? It's
> not perfect since the actual piece of hw we care about is still platform
> specific, so I'd suggest to throw an enum on top:
>
> enum intel_display_power_domains {
> POWER_DOMAIN_EDP,
> POWER_DOMAIN_EDP_PFIT,
> POWER_DOMAIN_OTHER
> };
>
> bool intel_display_power_enabled(struct drm_device *dev,
> enum intel_display_power_domain domain);
>
> We could easily add new domains for e.g. the pc8 stuff with a
> POWER_DOMAIN_CONNECTOR_AUX or so if we need to work around more unclaimed
> register warnings.
>
> With that piece of infrastructure I think I'll stop being grumpy about
> power wells checks and unclaimed register fixup patches and just merge
> them all.

Also, that function should probably use HAS_POWER_WELL instead of the
manual IS_HASWELL check.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 04/15] drm/i915: don't touch the PF regs if the power well is down

[Paulo Zanoni]

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

This solves some "unclaimed register" messages when booting the
machine with eDP attached.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bd27336..9a9f6d7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3584,8 +3584,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
 	/* Disable PF */
-	I915_WRITE(PF_CTL(pipe), 0);
-	I915_WRITE(PF_WIN_SZ(pipe), 0);
+	if (!intel_power_well_is_down(dev)) {
+		I915_WRITE(PF_CTL(pipe), 0);
+		I915_WRITE(PF_WIN_SZ(pipe), 0);
+	}
 
 	intel_ddi_disable_pipe_clock(intel_crtc);
 
-- 
1.7.10.4

Re: [PATCH 04/15] drm/i915: don't touch the PF regs if the power well is down

[Daniel Vetter]

On Wed, Mar 06, 2013 at 08:03:11PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This solves some "unclaimed register" messages when booting the
> machine with eDP attached.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bd27336..9a9f6d7 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3584,8 +3584,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
>  
>  	/* Disable PF */
> -	I915_WRITE(PF_CTL(pipe), 0);
> -	I915_WRITE(PF_WIN_SZ(pipe), 0);
> +	if (!intel_power_well_is_down(dev)) {
> +		I915_WRITE(PF_CTL(pipe), 0);
> +		I915_WRITE(PF_WIN_SZ(pipe), 0);
> +	}

I'd vote for a /* XXX: Once we have proper pfit state tracking implemented
with hw state read/check support we should switch to only disable the pfit
when we know it's used */

The idea is that the power well code here irks me a bit, after all if
something is on and we want to disable it, it can't be also off due to the
power well being down ;-)
-Daniel

>  
>  	intel_ddi_disable_pipe_clock(intel_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 04/15] drm/i915: don't touch the PF regs if the power well is down

[Ben Widawsky]

On Thu, Mar 07, 2013 at 12:28:32AM +0100, Daniel Vetter wrote:
> On Wed, Mar 06, 2013 at 08:03:11PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This solves some "unclaimed register" messages when booting the
> > machine with eDP attached.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index bd27336..9a9f6d7 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3584,8 +3584,10 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
> >  	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
> >  
> >  	/* Disable PF */
> > -	I915_WRITE(PF_CTL(pipe), 0);
> > -	I915_WRITE(PF_WIN_SZ(pipe), 0);
> > +	if (!intel_power_well_is_down(dev)) {
> > +		I915_WRITE(PF_CTL(pipe), 0);
> > +		I915_WRITE(PF_WIN_SZ(pipe), 0);
> > +	}
> 
> I'd vote for a /* XXX: Once we have proper pfit state tracking implemented
> with hw state read/check support we should switch to only disable the pfit
> when we know it's used */
> 
> The idea is that the power well code here irks me a bit, after all if
> something is on and we want to disable it, it can't be also off due to the
> power well being down ;-)
> -Daniel


Daniel, your second paragraph doesn't make sense to me. I agree with the
first paragraph, but I like that this fixes a real error message in the
meantime. As best I can tell, there is nothing wrong with the code on
the existing d-i-n.

Paulo: P.S. This doesn't help my previous bikeshed about not needing an
IS_HASWELL check in intel_power_well_is_down() ;-).


> 
> >  
> >  	intel_ddi_disable_pipe_clock(intel_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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 4/7] drm/i915: don't touch the PF regs if the power well is down

[Paulo Zanoni]

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

This solves some "unclaimed register" messages when booting the
machine with eDP attached.

V2: Rebase and add the comment requested by Daniel.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index be70f2d..36e0445 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3594,9 +3594,13 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_ddi_disable_transcoder_func(dev_priv, cpu_transcoder);
 
-	/* Disable PF */
-	I915_WRITE(PF_CTL(pipe), 0);
-	I915_WRITE(PF_WIN_SZ(pipe), 0);
+	/* 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)) {
+		I915_WRITE(PF_CTL(pipe), 0);
+		I915_WRITE(PF_WIN_SZ(pipe), 0);
+	}
 
 	intel_ddi_disable_pipe_clock(intel_crtc);
 
-- 
1.7.10.4

Re: [PATCH 4/7] drm/i915: don't touch the PF regs if the power well is down

[Daniel Vetter]

On Fri, Mar 22, 2013 at 02:16:38PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This solves some "unclaimed register" messages when booting the
> machine with eDP attached.
> 
> V2: Rebase and add the comment requested by Daniel.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Merged this one and the previous prep patch to dinq, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 05/15] drm/i915: capture the correct cursor registers on IVB

[Paulo Zanoni]

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

This solves some "unclaimed register" messages when there's a GPU hang
on Haswell.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a9f6d7..789a95a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9336,9 +9336,15 @@ intel_display_capture_error_state(struct drm_device *dev)
 	for_each_pipe(i) {
 		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
 
-		error->cursor[i].control = I915_READ(CURCNTR(i));
-		error->cursor[i].position = I915_READ(CURPOS(i));
-		error->cursor[i].base = I915_READ(CURBASE(i));
+		if (INTEL_INFO(dev)->gen <= 6) {
+			error->cursor[i].control = I915_READ(CURCNTR(i));
+			error->cursor[i].position = I915_READ(CURPOS(i));
+			error->cursor[i].base = I915_READ(CURBASE(i));
+		} else {
+			error->cursor[i].control = I915_READ(CURCNTR_IVB(i));
+			error->cursor[i].position = I915_READ(CURPOS_IVB(i));
+			error->cursor[i].base = I915_READ(CURBASE_IVB(i));
+		}
 
 		error->plane[i].control = I915_READ(DSPCNTR(i));
 		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
-- 
1.7.10.4

Re: [PATCH 05/15] drm/i915: capture the correct cursor registers on IVB

[Ville Syrjälä]

On Wed, Mar 06, 2013 at 08:03:12PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This solves some "unclaimed register" messages when there's a GPU hang
> on Haswell.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9a9f6d7..789a95a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9336,9 +9336,15 @@ intel_display_capture_error_state(struct drm_device *dev)
>  	for_each_pipe(i) {
>  		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
>  
> -		error->cursor[i].control = I915_READ(CURCNTR(i));
> -		error->cursor[i].position = I915_READ(CURPOS(i));
> -		error->cursor[i].base = I915_READ(CURBASE(i));
> +		if (INTEL_INFO(dev)->gen <= 6) {
> +			error->cursor[i].control = I915_READ(CURCNTR(i));
> +			error->cursor[i].position = I915_READ(CURPOS(i));
> +			error->cursor[i].base = I915_READ(CURBASE(i));
> +		} else {
> +			error->cursor[i].control = I915_READ(CURCNTR_IVB(i));
> +			error->cursor[i].position = I915_READ(CURPOS_IVB(i));
> +			error->cursor[i].base = I915_READ(CURBASE_IVB(i));
> +		}

Needs a VLV check.

>  
>  		error->plane[i].control = I915_READ(DSPCNTR(i));
>  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 05/15] drm/i915: capture the correct cursor registers on IVB

[Ben Widawsky]

On Thu, Mar 07, 2013 at 11:34:08AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:12PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This solves some "unclaimed register" messages when there's a GPU hang
> > on Haswell.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> >  1 file changed, 9 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 9a9f6d7..789a95a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9336,9 +9336,15 @@ intel_display_capture_error_state(struct drm_device *dev)
> >  	for_each_pipe(i) {
> >  		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> >  
> > -		error->cursor[i].control = I915_READ(CURCNTR(i));
> > -		error->cursor[i].position = I915_READ(CURPOS(i));
> > -		error->cursor[i].base = I915_READ(CURBASE(i));
> > +		if (INTEL_INFO(dev)->gen <= 6) {
> > +			error->cursor[i].control = I915_READ(CURCNTR(i));
> > +			error->cursor[i].position = I915_READ(CURPOS(i));
> > +			error->cursor[i].base = I915_READ(CURBASE(i));
> > +		} else {
> > +			error->cursor[i].control = I915_READ(CURCNTR_IVB(i));
> > +			error->cursor[i].position = I915_READ(CURPOS_IVB(i));
> > +			error->cursor[i].base = I915_READ(CURBASE_IVB(i));
> > +		}
> 
> Needs a VLV check.

Has anyone ever used this to actually debug an issue?

Ville's right, I suppose (I'm too lazy to find VLV docs). The non-VLV
part of the patch is:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> >  
> >  		error->plane[i].control = I915_READ(DSPCNTR(i));
> >  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 05/15] drm/i915: capture the correct cursor registers on IVB

[Daniel Vetter]

On Fri, Mar 15, 2013 at 11:45:47AM -0700, Ben Widawsky wrote:
> On Thu, Mar 07, 2013 at 11:34:08AM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 06, 2013 at 08:03:12PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > This solves some "unclaimed register" messages when there's a GPU hang
> > > on Haswell.
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
> > >  1 file changed, 9 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 9a9f6d7..789a95a 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9336,9 +9336,15 @@ intel_display_capture_error_state(struct drm_device *dev)
> > >  	for_each_pipe(i) {
> > >  		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> > >  
> > > -		error->cursor[i].control = I915_READ(CURCNTR(i));
> > > -		error->cursor[i].position = I915_READ(CURPOS(i));
> > > -		error->cursor[i].base = I915_READ(CURBASE(i));
> > > +		if (INTEL_INFO(dev)->gen <= 6) {
> > > +			error->cursor[i].control = I915_READ(CURCNTR(i));
> > > +			error->cursor[i].position = I915_READ(CURPOS(i));
> > > +			error->cursor[i].base = I915_READ(CURBASE(i));
> > > +		} else {
> > > +			error->cursor[i].control = I915_READ(CURCNTR_IVB(i));
> > > +			error->cursor[i].position = I915_READ(CURPOS_IVB(i));
> > > +			error->cursor[i].base = I915_READ(CURBASE_IVB(i));
> > > +		}
> > 
> > Needs a VLV check.
> 
> Has anyone ever used this to actually debug an issue?
> 
> Ville's right, I suppose (I'm too lazy to find VLV docs). The non-VLV
> part of the patch is:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next with the IS_VLV check added, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+

[Paulo Zanoni]

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

So don't read it when capturing the error state. This solves some
"unclaimed register" messages on Haswell when we hang the GPU.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 789a95a..56cca6e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9348,7 +9348,8 @@ intel_display_capture_error_state(struct drm_device *dev)
 
 		error->plane[i].control = I915_READ(DSPCNTR(i));
 		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
-		error->plane[i].size = I915_READ(DSPSIZE(i));
+		if (INTEL_INFO(dev)->gen <= 3)
+			error->plane[i].size = I915_READ(DSPSIZE(i));
 		error->plane[i].pos = I915_READ(DSPPOS(i));
 		error->plane[i].addr = I915_READ(DSPADDR(i));
 		if (INTEL_INFO(dev)->gen >= 4) {
@@ -9392,7 +9393,8 @@ intel_display_print_error_state(struct seq_file *m,
 		seq_printf(m, "Plane [%d]:\n", i);
 		seq_printf(m, "  CNTR: %08x\n", error->plane[i].control);
 		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
-		seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
+		if (INTEL_INFO(dev)->gen <= 3)
+			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
 		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
 		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
 		if (INTEL_INFO(dev)->gen >= 4) {
-- 
1.7.10.4

Re: [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+

[Ville Syrjälä]

On Wed, Mar 06, 2013 at 08:03:13PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So don't read it when capturing the error state. This solves some
> "unclaimed register" messages on Haswell when we hang the GPU.

You're sure about gen4? I haven't really checked but my impression is
that gen4 is more like gen3, and gen4.5 more like gen5 as far as the
display is concerned (eg. gen4 would have the old video overlay, and
4.5 would have video sprites). Can anyone confirm?

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 789a95a..56cca6e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9348,7 +9348,8 @@ intel_display_capture_error_state(struct drm_device *dev)
>  
>  		error->plane[i].control = I915_READ(DSPCNTR(i));
>  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
> -		error->plane[i].size = I915_READ(DSPSIZE(i));
> +		if (INTEL_INFO(dev)->gen <= 3)
> +			error->plane[i].size = I915_READ(DSPSIZE(i));
>  		error->plane[i].pos = I915_READ(DSPPOS(i));
>  		error->plane[i].addr = I915_READ(DSPADDR(i));
>  		if (INTEL_INFO(dev)->gen >= 4) {
> @@ -9392,7 +9393,8 @@ intel_display_print_error_state(struct seq_file *m,
>  		seq_printf(m, "Plane [%d]:\n", i);
>  		seq_printf(m, "  CNTR: %08x\n", error->plane[i].control);
>  		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
> -		seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> +		if (INTEL_INFO(dev)->gen <= 3)
> +			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
>  		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
>  		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
>  		if (INTEL_INFO(dev)->gen >= 4) {
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+

[Ben Widawsky]

On Thu, Mar 07, 2013 at 11:38:24AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:13PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > So don't read it when capturing the error state. This solves some
> > "unclaimed register" messages on Haswell when we hang the GPU.
> 
> You're sure about gen4? I haven't really checked but my impression is
> that gen4 is more like gen3, and gen4.5 more like gen5 as far as the
> display is concerned (eg. gen4 would have the old video overlay, and
> 4.5 would have video sprites). Can anyone confirm?

This register isn't anywhere in modern docs, what's up?

> 
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 789a95a..56cca6e 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -9348,7 +9348,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> >  
> >  		error->plane[i].control = I915_READ(DSPCNTR(i));
> >  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
> > -		error->plane[i].size = I915_READ(DSPSIZE(i));
> > +		if (INTEL_INFO(dev)->gen <= 3)
> > +			error->plane[i].size = I915_READ(DSPSIZE(i));
> >  		error->plane[i].pos = I915_READ(DSPPOS(i));
> >  		error->plane[i].addr = I915_READ(DSPADDR(i));
> >  		if (INTEL_INFO(dev)->gen >= 4) {
> > @@ -9392,7 +9393,8 @@ intel_display_print_error_state(struct seq_file *m,
> >  		seq_printf(m, "Plane [%d]:\n", i);
> >  		seq_printf(m, "  CNTR: %08x\n", error->plane[i].control);
> >  		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
> > -		seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> > +		if (INTEL_INFO(dev)->gen <= 3)
> > +			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> >  		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
> >  		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
> >  		if (INTEL_INFO(dev)->gen >= 4) {
> > -- 
> > 1.7.10.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+

[Ben Widawsky]

On Fri, Mar 15, 2013 at 12:04:11PM -0700, Ben Widawsky wrote:
> On Thu, Mar 07, 2013 at 11:38:24AM +0200, Ville Syrjälä wrote:
> > On Wed, Mar 06, 2013 at 08:03:13PM -0300, Paulo Zanoni wrote:
> > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > 
> > > So don't read it when capturing the error state. This solves some
> > > "unclaimed register" messages on Haswell when we hang the GPU.
> > 
> > You're sure about gen4? I haven't really checked but my impression is
> > that gen4 is more like gen3, and gen4.5 more like gen5 as far as the
> > display is concerned (eg. gen4 would have the old video overlay, and
> > 4.5 would have video sprites). Can anyone confirm?
> 
> This register isn't anywhere in modern docs, what's up?

Ooops, inverted the logic in my head. If you make the check for gen <=6
which I can lazily check with modern docs:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> 
> > 
> > > 
> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 789a95a..56cca6e 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -9348,7 +9348,8 @@ intel_display_capture_error_state(struct drm_device *dev)
> > >  
> > >  		error->plane[i].control = I915_READ(DSPCNTR(i));
> > >  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
> > > -		error->plane[i].size = I915_READ(DSPSIZE(i));
> > > +		if (INTEL_INFO(dev)->gen <= 3)
> > > +			error->plane[i].size = I915_READ(DSPSIZE(i));
> > >  		error->plane[i].pos = I915_READ(DSPPOS(i));
> > >  		error->plane[i].addr = I915_READ(DSPADDR(i));
> > >  		if (INTEL_INFO(dev)->gen >= 4) {
> > > @@ -9392,7 +9393,8 @@ intel_display_print_error_state(struct seq_file *m,
> > >  		seq_printf(m, "Plane [%d]:\n", i);
> > >  		seq_printf(m, "  CNTR: %08x\n", error->plane[i].control);
> > >  		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
> > > -		seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> > > +		if (INTEL_INFO(dev)->gen <= 3)
> > > +			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> > >  		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
> > >  		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
> > >  		if (INTEL_INFO(dev)->gen >= 4) {
> > > -- 
> > > 1.7.10.4
> > > 
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 06/15] drm/i915: there's no DSPSIZE register on gen4+

[Daniel Vetter]

On Fri, Mar 15, 2013 at 12:08:10PM -0700, Ben Widawsky wrote:
> On Fri, Mar 15, 2013 at 12:04:11PM -0700, Ben Widawsky wrote:
> > On Thu, Mar 07, 2013 at 11:38:24AM +0200, Ville Syrjälä wrote:
> > > On Wed, Mar 06, 2013 at 08:03:13PM -0300, Paulo Zanoni wrote:
> > > > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > 
> > > > So don't read it when capturing the error state. This solves some
> > > > "unclaimed register" messages on Haswell when we hang the GPU.
> > > 
> > > You're sure about gen4? I haven't really checked but my impression is
> > > that gen4 is more like gen3, and gen4.5 more like gen5 as far as the
> > > display is concerned (eg. gen4 would have the old video overlay, and
> > > 4.5 would have video sprites). Can anyone confirm?
> > 
> > This register isn't anywhere in modern docs, what's up?
> 
> Ooops, inverted the logic in my head. If you make the check for gen <=6
> which I can lazily check with modern docs:
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

I've also checked vanilla gen4 docs and that offset is already marked as
reserved, so we're good here.

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

[PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell

[Paulo Zanoni]

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

So don't read it when we hang the GPU. This solves "unclaimed
register" messages.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 56cca6e..0451056 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9351,7 +9351,8 @@ intel_display_capture_error_state(struct drm_device *dev)
 		if (INTEL_INFO(dev)->gen <= 3)
 			error->plane[i].size = I915_READ(DSPSIZE(i));
 		error->plane[i].pos = I915_READ(DSPPOS(i));
-		error->plane[i].addr = I915_READ(DSPADDR(i));
+		if (!IS_HASWELL(dev))
+			error->plane[i].addr = I915_READ(DSPADDR(i));
 		if (INTEL_INFO(dev)->gen >= 4) {
 			error->plane[i].surface = I915_READ(DSPSURF(i));
 			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
@@ -9396,7 +9397,8 @@ intel_display_print_error_state(struct seq_file *m,
 		if (INTEL_INFO(dev)->gen <= 3)
 			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
 		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
-		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
+		if (!IS_HASWELL(dev))
+			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
 		if (INTEL_INFO(dev)->gen >= 4) {
 			seq_printf(m, "  SURF: %08x\n", error->plane[i].surface);
 			seq_printf(m, "  TILEOFF: %08x\n", error->plane[i].tile_offset);
-- 
1.7.10.4

Re: [PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:14PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So don't read it when we hang the GPU. This solves "unclaimed
> register" messages.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
It would be nice if you could make this a bit more future proof, but
looks correct to me:
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 56cca6e..0451056 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9351,7 +9351,8 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		if (INTEL_INFO(dev)->gen <= 3)
>  			error->plane[i].size = I915_READ(DSPSIZE(i));
>  		error->plane[i].pos = I915_READ(DSPPOS(i));
> -		error->plane[i].addr = I915_READ(DSPADDR(i));
> +		if (!IS_HASWELL(dev))
> +			error->plane[i].addr = I915_READ(DSPADDR(i));
>  		if (INTEL_INFO(dev)->gen >= 4) {
>  			error->plane[i].surface = I915_READ(DSPSURF(i));
>  			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
> @@ -9396,7 +9397,8 @@ intel_display_print_error_state(struct seq_file *m,
>  		if (INTEL_INFO(dev)->gen <= 3)
>  			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
>  		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
> -		seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
> +		if (!IS_HASWELL(dev))
> +			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
>  		if (INTEL_INFO(dev)->gen >= 4) {
>  			seq_printf(m, "  SURF: %08x\n", error->plane[i].surface);
>  			seq_printf(m, "  TILEOFF: %08x\n", error->plane[i].tile_offset);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell

[Daniel Vetter]

On Fri, Mar 15, 2013 at 12:10:02PM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:14PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > So don't read it when we hang the GPU. This solves "unclaimed
> > register" messages.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> It would be nice if you could make this a bit more future proof, but
> looks correct to me:

Done.

> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell

[Paulo Zanoni]

Hi

2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> On Fri, Mar 15, 2013 at 12:10:02PM -0700, Ben Widawsky wrote:
>> On Wed, Mar 06, 2013 at 08:03:14PM -0300, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> >
>> > So don't read it when we hang the GPU. This solves "unclaimed
>> > register" messages.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> It would be nice if you could make this a bit more future proof, but
>> looks correct to me:
>
> Done.

"Done" for intel_display_capture_error_state but not for
intel_display_print_error_state. Do you want to fix this or do you
want me to send a patch on top of dinq?

Thanks for the reviews!
Paulo

>
>> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Queued for -next, thanks for the patch.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 07/15] drm/i915: there's no DSPADDR register on Haswell

[Daniel Vetter]

On Wed, Mar 20, 2013 at 03:01:34PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> > On Fri, Mar 15, 2013 at 12:10:02PM -0700, Ben Widawsky wrote:
> >> On Wed, Mar 06, 2013 at 08:03:14PM -0300, Paulo Zanoni wrote:
> >> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> >
> >> > So don't read it when we hang the GPU. This solves "unclaimed
> >> > register" messages.
> >> >
> >> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> It would be nice if you could make this a bit more future proof, but
> >> looks correct to me:
> >
> > Done.
> 
> "Done" for intel_display_capture_error_state but not for
> intel_display_print_error_state. Do you want to fix this or do you
> want me to send a patch on top of dinq?

Oh, I've failed again ;-) Yes, please submit a patch since due to the
backmerge it'll be a mess to rebase this one out.

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

[PATCH 5/7] drm/i915: fix DSPADDR Gen check

[Paulo Zanoni]

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

The first version of commit "drm/i915: there's no DSPADDR register on
Haswell" added 2 "!IS_HASWELL" checks. When reviewing the patch, Ben
suggested to make these checks more future-proof, so when Daniel
applied the patch he fixed the first check but not the second. This
commit makes the second check also "future-proof".

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 36e0445..98ac3bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9416,7 +9416,7 @@ intel_display_print_error_state(struct seq_file *m,
 		if (INTEL_INFO(dev)->gen <= 3)
 			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
 		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
-		if (!IS_HASWELL(dev))
+		if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
 			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
 		if (INTEL_INFO(dev)->gen >= 4) {
 			seq_printf(m, "  SURF: %08x\n", error->plane[i].surface);
-- 
1.7.10.4

Re: [PATCH 5/7] drm/i915: fix DSPADDR Gen check

[Daniel Vetter]

On Fri, Mar 22, 2013 at 02:19:21PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The first version of commit "drm/i915: there's no DSPADDR register on
> Haswell" added 2 "!IS_HASWELL" checks. When reviewing the patch, Ben
> suggested to make these checks more future-proof, so when Daniel
> applied the patch he fixed the first check but not the second. This
> commit makes the second check also "future-proof".
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

[PATCH 08/15] drm/i915: remove DSPPOS register

[Paulo Zanoni]

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

I couldn't find any evidence that this register exists on Gen2+. On
Gen 2/3/4 documents this register is listed as reserved and read-only.
On the newer Gens this register is not even documented.

Also all we do with this register is:
  - Write 0 to it on i9xx_crtc_mode_set
  - Save/restore its value on the UMS code
  - Read it on intel_display_capture_error_state

This commit fixes "unclaimed register" messages when there's a GPU
hang on Haswell.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |    2 --
 drivers/gpu/drm/i915/i915_reg.h      |    3 ---
 drivers/gpu/drm/i915/i915_ums.c      |    4 ----
 drivers/gpu/drm/i915/intel_display.c |    4 ----
 4 files changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ca6b215..9bf15e5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -513,7 +513,6 @@ struct i915_suspend_saved_registers {
 	u32 savePIPEASTAT;
 	u32 saveDSPASTRIDE;
 	u32 saveDSPASIZE;
-	u32 saveDSPAPOS;
 	u32 saveDSPAADDR;
 	u32 saveDSPASURF;
 	u32 saveDSPATILEOFF;
@@ -544,7 +543,6 @@ struct i915_suspend_saved_registers {
 	u32 savePIPEBSTAT;
 	u32 saveDSPBSTRIDE;
 	u32 saveDSPBSIZE;
-	u32 saveDSPBPOS;
 	u32 saveDSPBADDR;
 	u32 saveDSPBSURF;
 	u32 saveDSPBTILEOFF;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 4cf3ece..26e4b86 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3051,7 +3051,6 @@
 #define   DISPPLANE_TILED			(1<<10)
 #define _DSPAADDR		(dev_priv->info->display_mmio_offset + 0x70184)
 #define _DSPASTRIDE		(dev_priv->info->display_mmio_offset + 0x70188)
-#define _DSPAPOS		(dev_priv->info->display_mmio_offset + 0x7018C) /* reserved */
 #define _DSPASIZE		(dev_priv->info->display_mmio_offset + 0x70190)
 #define _DSPASURF		(dev_priv->info->display_mmio_offset + 0x7019C) /* 965+ only */
 #define _DSPATILEOFF		(dev_priv->info->display_mmio_offset + 0x701A4) /* 965+ only */
@@ -3061,7 +3060,6 @@
 #define DSPCNTR(plane) _PIPE(plane, _DSPACNTR, _DSPBCNTR)
 #define DSPADDR(plane) _PIPE(plane, _DSPAADDR, _DSPBADDR)
 #define DSPSTRIDE(plane) _PIPE(plane, _DSPASTRIDE, _DSPBSTRIDE)
-#define DSPPOS(plane) _PIPE(plane, _DSPAPOS, _DSPBPOS)
 #define DSPSIZE(plane) _PIPE(plane, _DSPASIZE, _DSPBSIZE)
 #define DSPSURF(plane) _PIPE(plane, _DSPASURF, _DSPBSURF)
 #define DSPTILEOFF(plane) _PIPE(plane, _DSPATILEOFF, _DSPBTILEOFF)
@@ -3109,7 +3107,6 @@
 #define   DISPPLANE_SPRITE_ABOVE_OVERLAY	(1)
 #define _DSPBADDR		(dev_priv->info->display_mmio_offset + 0x71184)
 #define _DSPBSTRIDE		(dev_priv->info->display_mmio_offset + 0x71188)
-#define _DSPBPOS		(dev_priv->info->display_mmio_offset + 0x7118C)
 #define _DSPBSIZE		(dev_priv->info->display_mmio_offset + 0x71190)
 #define _DSPBSURF		(dev_priv->info->display_mmio_offset + 0x7119C)
 #define _DSPBTILEOFF		(dev_priv->info->display_mmio_offset + 0x711A4)
diff --git a/drivers/gpu/drm/i915/i915_ums.c b/drivers/gpu/drm/i915/i915_ums.c
index 985a097..0e21925 100644
--- a/drivers/gpu/drm/i915/i915_ums.c
+++ b/drivers/gpu/drm/i915/i915_ums.c
@@ -160,7 +160,6 @@ void i915_save_display_reg(struct drm_device *dev)
 	dev_priv->regfile.saveDSPACNTR = I915_READ(_DSPACNTR);
 	dev_priv->regfile.saveDSPASTRIDE = I915_READ(_DSPASTRIDE);
 	dev_priv->regfile.saveDSPASIZE = I915_READ(_DSPASIZE);
-	dev_priv->regfile.saveDSPAPOS = I915_READ(_DSPAPOS);
 	dev_priv->regfile.saveDSPAADDR = I915_READ(_DSPAADDR);
 	if (INTEL_INFO(dev)->gen >= 4) {
 		dev_priv->regfile.saveDSPASURF = I915_READ(_DSPASURF);
@@ -217,7 +216,6 @@ void i915_save_display_reg(struct drm_device *dev)
 	dev_priv->regfile.saveDSPBCNTR = I915_READ(_DSPBCNTR);
 	dev_priv->regfile.saveDSPBSTRIDE = I915_READ(_DSPBSTRIDE);
 	dev_priv->regfile.saveDSPBSIZE = I915_READ(_DSPBSIZE);
-	dev_priv->regfile.saveDSPBPOS = I915_READ(_DSPBPOS);
 	dev_priv->regfile.saveDSPBADDR = I915_READ(_DSPBADDR);
 	if (INTEL_INFO(dev)->gen >= 4) {
 		dev_priv->regfile.saveDSPBSURF = I915_READ(_DSPBSURF);
@@ -390,7 +388,6 @@ void i915_restore_display_reg(struct drm_device *dev)
 
 	/* Restore plane info */
 	I915_WRITE(_DSPASIZE, dev_priv->regfile.saveDSPASIZE);
-	I915_WRITE(_DSPAPOS, dev_priv->regfile.saveDSPAPOS);
 	I915_WRITE(_PIPEASRC, dev_priv->regfile.savePIPEASRC);
 	I915_WRITE(_DSPAADDR, dev_priv->regfile.saveDSPAADDR);
 	I915_WRITE(_DSPASTRIDE, dev_priv->regfile.saveDSPASTRIDE);
@@ -459,7 +456,6 @@ void i915_restore_display_reg(struct drm_device *dev)
 
 	/* Restore plane info */
 	I915_WRITE(_DSPBSIZE, dev_priv->regfile.saveDSPBSIZE);
-	I915_WRITE(_DSPBPOS, dev_priv->regfile.saveDSPBPOS);
 	I915_WRITE(_PIPEBSRC, dev_priv->regfile.savePIPEBSRC);
 	I915_WRITE(_DSPBADDR, dev_priv->regfile.saveDSPBADDR);
 	I915_WRITE(_DSPBSTRIDE, dev_priv->regfile.saveDSPBSTRIDE);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0451056..b319cd3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4781,7 +4781,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	I915_WRITE(DSPSIZE(plane),
 		   ((mode->vdisplay - 1) << 16) |
 		   (mode->hdisplay - 1));
-	I915_WRITE(DSPPOS(plane), 0);
 
 	I915_WRITE(PIPECONF(pipe), pipeconf);
 	POSTING_READ(PIPECONF(pipe));
@@ -9314,7 +9313,6 @@ struct intel_display_error_state {
 		u32 control;
 		u32 stride;
 		u32 size;
-		u32 pos;
 		u32 addr;
 		u32 surface;
 		u32 tile_offset;
@@ -9350,7 +9348,6 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
 		if (INTEL_INFO(dev)->gen <= 3)
 			error->plane[i].size = I915_READ(DSPSIZE(i));
-		error->plane[i].pos = I915_READ(DSPPOS(i));
 		if (!IS_HASWELL(dev))
 			error->plane[i].addr = I915_READ(DSPADDR(i));
 		if (INTEL_INFO(dev)->gen >= 4) {
@@ -9396,7 +9393,6 @@ intel_display_print_error_state(struct seq_file *m,
 		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
 		if (INTEL_INFO(dev)->gen <= 3)
 			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
-		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
 		if (!IS_HASWELL(dev))
 			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
 		if (INTEL_INFO(dev)->gen >= 4) {
-- 
1.7.10.4

Re: [PATCH 08/15] drm/i915: remove DSPPOS register

[Ville Syrjälä]

On Wed, Mar 06, 2013 at 08:03:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I couldn't find any evidence that this register exists on Gen2+. On
> Gen 2/3/4 documents this register is listed as reserved and read-only.
> On the newer Gens this register is not even documented.

DSPPOS goes hand in hand with DSPSIZE. IIRC DSPA never had the
windowing capability, but DSPB and DSPC on older gens had it.

> Also all we do with this register is:
>   - Write 0 to it on i9xx_crtc_mode_set
>   - Save/restore its value on the UMS code
>   - Read it on intel_display_capture_error_state
> 
> This commit fixes "unclaimed register" messages when there's a GPU
> hang on Haswell.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |    2 --
>  drivers/gpu/drm/i915/i915_reg.h      |    3 ---
>  drivers/gpu/drm/i915/i915_ums.c      |    4 ----
>  drivers/gpu/drm/i915/intel_display.c |    4 ----
>  4 files changed, 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ca6b215..9bf15e5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -513,7 +513,6 @@ struct i915_suspend_saved_registers {
>  	u32 savePIPEASTAT;
>  	u32 saveDSPASTRIDE;
>  	u32 saveDSPASIZE;
> -	u32 saveDSPAPOS;
>  	u32 saveDSPAADDR;
>  	u32 saveDSPASURF;
>  	u32 saveDSPATILEOFF;
> @@ -544,7 +543,6 @@ struct i915_suspend_saved_registers {
>  	u32 savePIPEBSTAT;
>  	u32 saveDSPBSTRIDE;
>  	u32 saveDSPBSIZE;
> -	u32 saveDSPBPOS;
>  	u32 saveDSPBADDR;
>  	u32 saveDSPBSURF;
>  	u32 saveDSPBTILEOFF;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 4cf3ece..26e4b86 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3051,7 +3051,6 @@
>  #define   DISPPLANE_TILED			(1<<10)
>  #define _DSPAADDR		(dev_priv->info->display_mmio_offset + 0x70184)
>  #define _DSPASTRIDE		(dev_priv->info->display_mmio_offset + 0x70188)
> -#define _DSPAPOS		(dev_priv->info->display_mmio_offset + 0x7018C) /* reserved */
>  #define _DSPASIZE		(dev_priv->info->display_mmio_offset + 0x70190)
>  #define _DSPASURF		(dev_priv->info->display_mmio_offset + 0x7019C) /* 965+ only */
>  #define _DSPATILEOFF		(dev_priv->info->display_mmio_offset + 0x701A4) /* 965+ only */
> @@ -3061,7 +3060,6 @@
>  #define DSPCNTR(plane) _PIPE(plane, _DSPACNTR, _DSPBCNTR)
>  #define DSPADDR(plane) _PIPE(plane, _DSPAADDR, _DSPBADDR)
>  #define DSPSTRIDE(plane) _PIPE(plane, _DSPASTRIDE, _DSPBSTRIDE)
> -#define DSPPOS(plane) _PIPE(plane, _DSPAPOS, _DSPBPOS)
>  #define DSPSIZE(plane) _PIPE(plane, _DSPASIZE, _DSPBSIZE)
>  #define DSPSURF(plane) _PIPE(plane, _DSPASURF, _DSPBSURF)
>  #define DSPTILEOFF(plane) _PIPE(plane, _DSPATILEOFF, _DSPBTILEOFF)
> @@ -3109,7 +3107,6 @@
>  #define   DISPPLANE_SPRITE_ABOVE_OVERLAY	(1)
>  #define _DSPBADDR		(dev_priv->info->display_mmio_offset + 0x71184)
>  #define _DSPBSTRIDE		(dev_priv->info->display_mmio_offset + 0x71188)
> -#define _DSPBPOS		(dev_priv->info->display_mmio_offset + 0x7118C)
>  #define _DSPBSIZE		(dev_priv->info->display_mmio_offset + 0x71190)
>  #define _DSPBSURF		(dev_priv->info->display_mmio_offset + 0x7119C)
>  #define _DSPBTILEOFF		(dev_priv->info->display_mmio_offset + 0x711A4)
> diff --git a/drivers/gpu/drm/i915/i915_ums.c b/drivers/gpu/drm/i915/i915_ums.c
> index 985a097..0e21925 100644
> --- a/drivers/gpu/drm/i915/i915_ums.c
> +++ b/drivers/gpu/drm/i915/i915_ums.c
> @@ -160,7 +160,6 @@ void i915_save_display_reg(struct drm_device *dev)
>  	dev_priv->regfile.saveDSPACNTR = I915_READ(_DSPACNTR);
>  	dev_priv->regfile.saveDSPASTRIDE = I915_READ(_DSPASTRIDE);
>  	dev_priv->regfile.saveDSPASIZE = I915_READ(_DSPASIZE);
> -	dev_priv->regfile.saveDSPAPOS = I915_READ(_DSPAPOS);
>  	dev_priv->regfile.saveDSPAADDR = I915_READ(_DSPAADDR);
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		dev_priv->regfile.saveDSPASURF = I915_READ(_DSPASURF);
> @@ -217,7 +216,6 @@ void i915_save_display_reg(struct drm_device *dev)
>  	dev_priv->regfile.saveDSPBCNTR = I915_READ(_DSPBCNTR);
>  	dev_priv->regfile.saveDSPBSTRIDE = I915_READ(_DSPBSTRIDE);
>  	dev_priv->regfile.saveDSPBSIZE = I915_READ(_DSPBSIZE);
> -	dev_priv->regfile.saveDSPBPOS = I915_READ(_DSPBPOS);
>  	dev_priv->regfile.saveDSPBADDR = I915_READ(_DSPBADDR);
>  	if (INTEL_INFO(dev)->gen >= 4) {
>  		dev_priv->regfile.saveDSPBSURF = I915_READ(_DSPBSURF);
> @@ -390,7 +388,6 @@ void i915_restore_display_reg(struct drm_device *dev)
>  
>  	/* Restore plane info */
>  	I915_WRITE(_DSPASIZE, dev_priv->regfile.saveDSPASIZE);
> -	I915_WRITE(_DSPAPOS, dev_priv->regfile.saveDSPAPOS);
>  	I915_WRITE(_PIPEASRC, dev_priv->regfile.savePIPEASRC);
>  	I915_WRITE(_DSPAADDR, dev_priv->regfile.saveDSPAADDR);
>  	I915_WRITE(_DSPASTRIDE, dev_priv->regfile.saveDSPASTRIDE);
> @@ -459,7 +456,6 @@ void i915_restore_display_reg(struct drm_device *dev)
>  
>  	/* Restore plane info */
>  	I915_WRITE(_DSPBSIZE, dev_priv->regfile.saveDSPBSIZE);
> -	I915_WRITE(_DSPBPOS, dev_priv->regfile.saveDSPBPOS);
>  	I915_WRITE(_PIPEBSRC, dev_priv->regfile.savePIPEBSRC);
>  	I915_WRITE(_DSPBADDR, dev_priv->regfile.saveDSPBADDR);
>  	I915_WRITE(_DSPBSTRIDE, dev_priv->regfile.saveDSPBSTRIDE);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0451056..b319cd3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4781,7 +4781,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
>  	I915_WRITE(DSPSIZE(plane),
>  		   ((mode->vdisplay - 1) << 16) |
>  		   (mode->hdisplay - 1));
> -	I915_WRITE(DSPPOS(plane), 0);
>  
>  	I915_WRITE(PIPECONF(pipe), pipeconf);
>  	POSTING_READ(PIPECONF(pipe));
> @@ -9314,7 +9313,6 @@ struct intel_display_error_state {
>  		u32 control;
>  		u32 stride;
>  		u32 size;
> -		u32 pos;
>  		u32 addr;
>  		u32 surface;
>  		u32 tile_offset;
> @@ -9350,7 +9348,6 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
>  		if (INTEL_INFO(dev)->gen <= 3)
>  			error->plane[i].size = I915_READ(DSPSIZE(i));
> -		error->plane[i].pos = I915_READ(DSPPOS(i));
>  		if (!IS_HASWELL(dev))
>  			error->plane[i].addr = I915_READ(DSPADDR(i));
>  		if (INTEL_INFO(dev)->gen >= 4) {
> @@ -9396,7 +9393,6 @@ intel_display_print_error_state(struct seq_file *m,
>  		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
>  		if (INTEL_INFO(dev)->gen <= 3)
>  			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
> -		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
>  		if (!IS_HASWELL(dev))
>  			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
>  		if (INTEL_INFO(dev)->gen >= 4) {
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 08/15] drm/i915: remove DSPPOS register

[Daniel Vetter]

On Thu, Mar 07, 2013 at 11:43:14AM +0200, Ville Syrjälä wrote:
> On Wed, Mar 06, 2013 at 08:03:15PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > I couldn't find any evidence that this register exists on Gen2+. On
> > Gen 2/3/4 documents this register is listed as reserved and read-only.
> > On the newer Gens this register is not even documented.
> 
> DSPPOS goes hand in hand with DSPSIZE. IIRC DSPA never had the
> windowing capability, but DSPB and DSPC on older gens had it.

Indeed Ville's right here, gen2/3 have this on sprite B/C (you could move
those things around back then to different pipes ...). So I think we want
a gen <= 3 check here (gen4+ lost this apparently with the fixed
pipe->plane mapping).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 08/15] drm/i915: remove DSPPOS register

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:15PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I couldn't find any evidence that this register exists on Gen2+. On
> Gen 2/3/4 documents this register is listed as reserved and read-only.
> On the newer Gens this register is not even documented.
> 
> Also all we do with this register is:
>   - Write 0 to it on i9xx_crtc_mode_set
>   - Save/restore its value on the UMS code
>   - Read it on intel_display_capture_error_state
> 
> This commit fixes "unclaimed register" messages when there's a GPU
> hang on Haswell.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

[PATCH 6/7] drm/i915: there's no DSPPOS register on gen4+

[Paulo Zanoni]

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

So don't read it when capturing the error state. This solves some
"unclaimed register" messages on Haswell when we hang the GPU.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 98ac3bd..e8fc0ac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9368,9 +9368,10 @@ intel_display_capture_error_state(struct drm_device *dev)
 
 		error->plane[i].control = I915_READ(DSPCNTR(i));
 		error->plane[i].stride = I915_READ(DSPSTRIDE(i));
-		if (INTEL_INFO(dev)->gen <= 3)
+		if (INTEL_INFO(dev)->gen <= 3) {
 			error->plane[i].size = I915_READ(DSPSIZE(i));
-		error->plane[i].pos = I915_READ(DSPPOS(i));
+			error->plane[i].pos = I915_READ(DSPPOS(i));
+		}
 		if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
 			error->plane[i].addr = I915_READ(DSPADDR(i));
 		if (INTEL_INFO(dev)->gen >= 4) {
@@ -9413,9 +9414,10 @@ intel_display_print_error_state(struct seq_file *m,
 		seq_printf(m, "Plane [%d]:\n", i);
 		seq_printf(m, "  CNTR: %08x\n", error->plane[i].control);
 		seq_printf(m, "  STRIDE: %08x\n", error->plane[i].stride);
-		if (INTEL_INFO(dev)->gen <= 3)
+		if (INTEL_INFO(dev)->gen <= 3) {
 			seq_printf(m, "  SIZE: %08x\n", error->plane[i].size);
-		seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
+			seq_printf(m, "  POS: %08x\n", error->plane[i].pos);
+		}
 		if (INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev))
 			seq_printf(m, "  ADDR: %08x\n", error->plane[i].addr);
 		if (INTEL_INFO(dev)->gen >= 4) {
-- 
1.7.10.4

Re: [PATCH 6/7] drm/i915: there's no DSPPOS register on gen4+

[Daniel Vetter]

On Fri, Mar 22, 2013 at 02:20:57PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So don't read it when capturing the error state. This solves some
> "unclaimed register" messages on Haswell when we hang the GPU.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 09/15] drm/i915: there's no PIPESTAT on Gen5+

[Paulo Zanoni]

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

So don't read it when capturing the error state. This solves
"unclaimed register" messages on Haswell when we have a GPU hang.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 2139714..29b1bb1 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1366,8 +1366,9 @@ static void i915_capture_error_state(struct drm_device *dev)
 	else if (INTEL_INFO(dev)->gen == 6)
 		error->forcewake = I915_READ(FORCEWAKE);
 
-	for_each_pipe(pipe)
-		error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+	if (INTEL_INFO(dev)->gen <= 4)
+		for_each_pipe(pipe)
+			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		error->error = I915_READ(ERROR_GEN6);
-- 
1.7.10.4

Re: [PATCH 09/15] drm/i915: there's no PIPESTAT on Gen5+

[Daniel Vetter]

On Wed, Mar 06, 2013 at 08:03:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So don't read it when capturing the error state. This solves
> "unclaimed register" messages on Haswell when we have a GPU hang.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Iirc pipestat exists on vlv, so I think this needs a !HAS_PCH_SPLIT test.
Ville should know for sure.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_irq.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 2139714..29b1bb1 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1366,8 +1366,9 @@ static void i915_capture_error_state(struct drm_device *dev)
>  	else if (INTEL_INFO(dev)->gen == 6)
>  		error->forcewake = I915_READ(FORCEWAKE);
>  
> -	for_each_pipe(pipe)
> -		error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> +	if (INTEL_INFO(dev)->gen <= 4)
> +		for_each_pipe(pipe)
> +			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
>  
>  	if (INTEL_INFO(dev)->gen >= 6) {
>  		error->error = I915_READ(ERROR_GEN6);
> -- 
> 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 09/15] drm/i915: there's no PIPESTAT on Gen5+

[Ville Syrjälä]

On Thu, Mar 07, 2013 at 12:22:43AM +0100, Daniel Vetter wrote:
> On Wed, Mar 06, 2013 at 08:03:16PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > So don't read it when capturing the error state. This solves
> > "unclaimed register" messages on Haswell when we have a GPU hang.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Iirc pipestat exists on vlv, so I think this needs a !HAS_PCH_SPLIT test.
> Ville should know for sure.

Yes, that's right.

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c |    5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 2139714..29b1bb1 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1366,8 +1366,9 @@ static void i915_capture_error_state(struct drm_device *dev)
> >  	else if (INTEL_INFO(dev)->gen == 6)
> >  		error->forcewake = I915_READ(FORCEWAKE);
> >  
> > -	for_each_pipe(pipe)
> > -		error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> > +	if (INTEL_INFO(dev)->gen <= 4)
> > +		for_each_pipe(pipe)
> > +			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
> >  
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		error->error = I915_READ(ERROR_GEN6);
> > -- 
> > 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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[PATCH 7/7] drm/i915: there's no PIPESTAT on HAS_PCH_SPLIT platforms

[Paulo Zanoni]

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

So don't read it when capturing the error state. This solves
"unclaimed register" messages on Haswell when we have a GPU hang.

V2: Check for HAS_PCH_SPLIT instead of Gen5+ because VLV still has
this register.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5fc178e..3c2b05c 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1387,8 +1387,9 @@ static void i915_capture_error_state(struct drm_device *dev)
 	else if (INTEL_INFO(dev)->gen == 6)
 		error->forcewake = I915_READ(FORCEWAKE);
 
-	for_each_pipe(pipe)
-		error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
+	if (!HAS_PCH_SPLIT(dev))
+		for_each_pipe(pipe)
+			error->pipestat[pipe] = I915_READ(PIPESTAT(pipe));
 
 	if (INTEL_INFO(dev)->gen >= 6) {
 		error->error = I915_READ(ERROR_GEN6);
-- 
1.7.10.4

Re: [PATCH 7/7] drm/i915: there's no PIPESTAT on HAS_PCH_SPLIT platforms

[Daniel Vetter]

On Fri, Mar 22, 2013 at 02:24:16PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> So don't read it when capturing the error state. This solves
> "unclaimed register" messages on Haswell when we have a GPU hang.
> 
> V2: Check for HAS_PCH_SPLIT instead of Gen5+ because VLV still has
> this register.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 10/15] drm/i915: check the power well when capturing error state

[Paulo Zanoni]

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

This solves many "unclaimed register" messages when the power well is
down and we get a GPU hang.

Also print the power well register and each pipe's CPU transcoder on
the error state to allow proper interpratation of the registers. And
kzalloc the error state structure since we may not read some of the
registers (to avoid the "unclaimed register" messages).

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b319cd3..835bec2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9290,6 +9290,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;
@@ -9298,6 +9301,7 @@ struct intel_display_error_state {
 	} cursor[I915_MAX_PIPES];
 
 	struct intel_pipe_error_state {
+		enum transcoder cpu_transcoder;
 		u32 conf;
 		u32 source;
 
@@ -9324,15 +9328,35 @@ intel_display_capture_error_state(struct drm_device *dev)
 {
 	drm_i915_private_t *dev_priv = dev->dev_private;
 	struct intel_display_error_state *error;
-	enum transcoder cpu_transcoder;
+	enum transcoder transcoder;
+	bool power_well_is_down;
 	int i;
 
-	error = kmalloc(sizeof(*error), GFP_ATOMIC);
+	error = kzalloc(sizeof(*error), GFP_ATOMIC);
 	if (error == NULL)
 		return NULL;
 
+	power_well_is_down = intel_power_well_is_down(dev);
+
+	if (IS_HASWELL(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);
+		transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
+		error->pipe[i].cpu_transcoder = transcoder;
+
+		if (transcoder == TRANSCODER_EDP || !power_well_is_down) {
+			error->pipe[i].conf = I915_READ(PIPECONF(transcoder));
+			error->pipe[i].htotal = I915_READ(HTOTAL(transcoder));
+			error->pipe[i].hblank = I915_READ(HBLANK(transcoder));
+			error->pipe[i].hsync = I915_READ(HSYNC(transcoder));
+			error->pipe[i].vtotal = I915_READ(VTOTAL(transcoder));
+			error->pipe[i].vblank = I915_READ(VBLANK(transcoder));
+			error->pipe[i].vsync = I915_READ(VSYNC(transcoder));
+		}
+
+		if (power_well_is_down)
+			continue;
 
 		if (INTEL_INFO(dev)->gen <= 6) {
 			error->cursor[i].control = I915_READ(CURCNTR(i));
@@ -9355,14 +9379,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
 		}
 
-		error->pipe[i].conf = I915_READ(PIPECONF(cpu_transcoder));
 		error->pipe[i].source = I915_READ(PIPESRC(i));
-		error->pipe[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
-		error->pipe[i].hblank = I915_READ(HBLANK(cpu_transcoder));
-		error->pipe[i].hsync = I915_READ(HSYNC(cpu_transcoder));
-		error->pipe[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
-		error->pipe[i].vblank = I915_READ(VBLANK(cpu_transcoder));
-		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
 	}
 
 	return error;
@@ -9377,8 +9394,13 @@ intel_display_print_error_state(struct seq_file *m,
 	int i;
 
 	seq_printf(m, "Num Pipes: %d\n", dev_priv->num_pipe);
+	if (IS_HASWELL(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 10/15] drm/i915: check the power well when capturing error state

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This solves many "unclaimed register" messages when the power well is
> down and we get a GPU hang.
> 
> Also print the power well register and each pipe's CPU transcoder on
> the error state to allow proper interpratation of the registers. And
> kzalloc the error state structure since we may not read some of the
> registers (to avoid the "unclaimed register" messages).

Just a thought, but trying to bring the power well up and reading the
registers could also be interesting.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c |   42 ++++++++++++++++++++++++++--------
>  1 file changed, 32 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b319cd3..835bec2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9290,6 +9290,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;
> @@ -9298,6 +9301,7 @@ struct intel_display_error_state {
>  	} cursor[I915_MAX_PIPES];
>  
>  	struct intel_pipe_error_state {
> +		enum transcoder cpu_transcoder;
>  		u32 conf;
>  		u32 source;
>  
Putting this in the error state looks like a nice touch to me.

> @@ -9324,15 +9328,35 @@ intel_display_capture_error_state(struct drm_device *dev)
>  {
>  	drm_i915_private_t *dev_priv = dev->dev_private;
>  	struct intel_display_error_state *error;
> -	enum transcoder cpu_transcoder;
> +	enum transcoder transcoder;
> +	bool power_well_is_down;
>  	int i;
>  
> -	error = kmalloc(sizeof(*error), GFP_ATOMIC);
> +	error = kzalloc(sizeof(*error), GFP_ATOMIC);
>  	if (error == NULL)
>  		return NULL;
>  
> +	power_well_is_down = intel_power_well_is_down(dev);
> +
> +	if (IS_HASWELL(dev))
> +		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);

This is still not a stunning reason to have to pass in dev to
intel_power_well_is_down() since you have an IS_HASWELL check anyway
(from my original bikeshed so many patches ago).

> +
>  	for_each_pipe(i) {
> -		cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> +		transcoder = intel_pipe_to_cpu_transcoder(dev_priv, i);
> +		error->pipe[i].cpu_transcoder = transcoder;
> +
> +		if (transcoder == TRANSCODER_EDP || !power_well_is_down) {
> +			error->pipe[i].conf = I915_READ(PIPECONF(transcoder));
> +			error->pipe[i].htotal = I915_READ(HTOTAL(transcoder));
> +			error->pipe[i].hblank = I915_READ(HBLANK(transcoder));
> +			error->pipe[i].hsync = I915_READ(HSYNC(transcoder));
> +			error->pipe[i].vtotal = I915_READ(VTOTAL(transcoder));
> +			error->pipe[i].vblank = I915_READ(VBLANK(transcoder));
> +			error->pipe[i].vsync = I915_READ(VSYNC(transcoder));
> +		}
> +
> +		if (power_well_is_down)
> +			continue;
>  
>  		if (INTEL_INFO(dev)->gen <= 6) {
>  			error->cursor[i].control = I915_READ(CURCNTR(i));
> @@ -9355,14 +9379,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  			error->plane[i].tile_offset = I915_READ(DSPTILEOFF(i));
>  		}
>  
> -		error->pipe[i].conf = I915_READ(PIPECONF(cpu_transcoder));
>  		error->pipe[i].source = I915_READ(PIPESRC(i));
> -		error->pipe[i].htotal = I915_READ(HTOTAL(cpu_transcoder));
> -		error->pipe[i].hblank = I915_READ(HBLANK(cpu_transcoder));
> -		error->pipe[i].hsync = I915_READ(HSYNC(cpu_transcoder));
> -		error->pipe[i].vtotal = I915_READ(VTOTAL(cpu_transcoder));
> -		error->pipe[i].vblank = I915_READ(VBLANK(cpu_transcoder));
> -		error->pipe[i].vsync = I915_READ(VSYNC(cpu_transcoder));
>  	}
>  
>  	return error;
> @@ -9377,8 +9394,13 @@ intel_display_print_error_state(struct seq_file *m,
>  	int i;
>  
>  	seq_printf(m, "Num Pipes: %d\n", dev_priv->num_pipe);
> +	if (IS_HASWELL(dev))
> +		seq_printf(m, "PWR_WELL_CTL2: %08x\n",
> +			   error->power_well_driver);

I wouldn't bother with the IS_HASWELL check since it's kind of nice to
have error register dumps which are the same length.

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

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 10/15] drm/i915: check the power well when capturing error state

[Daniel Vetter]

On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This solves many "unclaimed register" messages when the power well is
> down and we get a GPU hang.
> 
> Also print the power well register and each pipe's CPU transcoder on
> the error state to allow proper interpratation of the registers. And
> kzalloc the error state structure since we may not read some of the
> registers (to avoid the "unclaimed register" messages).
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Ok, I've thought some more about all and you're all going to hate me.

Essentially I don't like that we have rather invasive patches for a minor
feature with _very_ delicate limits as to what hits the debug yelling and
what doesn't. Furthermore the debug feature isn't that clear-cut since
interrupts get in the way (and we cant fix that since that would also
disable underrun interrupts, which we want). And the audio driver also
randomly inteferes.

Now where it's justified that a register doesn't exist I don't mind
merging the patches - it's always good to have accurate documentation of
what's really used by the hw. But for debug features I'm really uneasy
with runtime checks, since it might very well be that those are wrong.

Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one
will never check for errors, but silently clear them after a read. Note
that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing
random crap could indeed be a serious bug.

So I'll punt on this patch since imo it's too messy for future
maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled
check).

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

Re: [PATCH 10/15] drm/i915: check the power well when capturing error state

[Daniel Vetter]

On Sun, Mar 17, 2013 at 09:46:37PM +0100, Daniel Vetter wrote:
> On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > This solves many "unclaimed register" messages when the power well is
> > down and we get a GPU hang.
> > 
> > Also print the power well register and each pipe's CPU transcoder on
> > the error state to allow proper interpratation of the registers. And
> > kzalloc the error state structure since we may not read some of the
> > registers (to avoid the "unclaimed register" messages).
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Ok, I've thought some more about all and you're all going to hate me.
> 
> Essentially I don't like that we have rather invasive patches for a minor
> feature with _very_ delicate limits as to what hits the debug yelling and
> what doesn't. Furthermore the debug feature isn't that clear-cut since
> interrupts get in the way (and we cant fix that since that would also
> disable underrun interrupts, which we want). And the audio driver also
> randomly inteferes.
> 
> Now where it's justified that a register doesn't exist I don't mind
> merging the patches - it's always good to have accurate documentation of
> what's really used by the hw. But for debug features I'm really uneasy
> with runtime checks, since it might very well be that those are wrong.
> 
> Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one
> will never check for errors, but silently clear them after a read. Note
> that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing
> random crap could indeed be a serious bug.
> 
> So I'll punt on this patch since imo it's too messy for future
> maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled
> check).

Forgotten to add: I like the powerwell related additions to the error
state, but that's imo a separate patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 10/15] drm/i915: check the power well when capturing error state

[Paulo Zanoni]

Hi

2013/3/17 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Mar 06, 2013 at 08:03:17PM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> This solves many "unclaimed register" messages when the power well is
>> down and we get a GPU hang.
>>
>> Also print the power well register and each pipe's CPU transcoder on
>> the error state to allow proper interpratation of the registers. And
>> kzalloc the error state structure since we may not read some of the
>> registers (to avoid the "unclaimed register" messages).
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Ok, I've thought some more about all and you're all going to hate me.
>
> Essentially I don't like that we have rather invasive patches for a minor
> feature with _very_ delicate limits as to what hits the debug yelling and
> what doesn't. Furthermore the debug feature isn't that clear-cut since
> interrupts get in the way (and we cant fix that since that would also
> disable underrun interrupts, which we want). And the audio driver also
> randomly inteferes.
>
> Now where it's justified that a register doesn't exist I don't mind
> merging the patches - it's always good to have accurate documentation of
> what's really used by the hw. But for debug features I'm really uneasy
> with runtime checks, since it might very well be that those are wrong.
>
> Hence I vote for a I915_READ_NOCHECK to avoid all this madness. This one
> will never check for errors, but silently clear them after a read. Note
> that we don't need a I915_WRITE_NOCHECK, and imo that's good since writing
> random crap could indeed be a serious bug.

I don't understand your way of thinking: your argument is that "this
is difficult to maintain", but then you're proposing the addition of a
new i915_read, which IMHO is way much more complex and feels like a
hackish solution to the problem. Every patch to the "normal"
i915_read#x will have to also patch i915_read_nocheck#x. And on patch
0014, which you also suggested to use the "I915_READ_NOCHECK"
function, our code will be relying on the _coincidence_ that when we
read a register that does not exist, we read 0. I really don't think
this is a sane approach.

Our hardware is getting much more complex with respect to these reads,
and I think we should never read ever read from a register that does
not exist or is powered off. I'm open to more suggestions on how to
write this patch, but I really think we shouldn't read registers that
don't exist or are powered off.

>
> So I'll punt on this patch since imo it's too messy for future
> maintenance. Same for the other debug patches (e.g. the assert_pipe_enabled
> check).
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 10/15] drm/i915: check the power well when capturing error state

[Daniel Vetter]

On Thu, Mar 21, 2013 at 11:12 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> I don't understand your way of thinking: your argument is that "this
> is difficult to maintain", but then you're proposing the addition of a
> new i915_read, which IMHO is way much more complex and feels like a
> hackish solution to the problem. Every patch to the "normal"
> i915_read#x will have to also patch i915_read_nocheck#x. And on patch
> 0014, which you also suggested to use the "I915_READ_NOCHECK"
> function, our code will be relying on the _coincidence_ that when we
> read a register that does not exist, we read 0. I really don't think
> this is a sane approach.

I915_READ_NOCHECK already exists, it's called I915_READ_NOTRACE ;-)
We'd lose the tracepoint when just using that, but imo that's no too
horrible for debug checks.

Wrt the assert_pipe check relying on the fact that the hw returns 0,
I'm totally ok on relying on that. If that ever changes in a future
product, we can fix it.

> Our hardware is getting much more complex with respect to these reads,
> and I think we should never read ever read from a register that does
> not exist or is powered off. I'm open to more suggestions on how to
> write this patch, but I really think we shouldn't read registers that
> don't exist or are powered off.

The "much more complex" part is actually why I've looked into
different options for the _debug_ functions. I agree with you for all
the real display code. But debug code (especially the gpu hang stuff)
tends to not be run too often, hence I'm leaning more towards a "just
do it, I know what I'm doing" kind of approach. Currently it's fairly
simple, but with future hw we'll have more chances to screw up and not
dump a few regs we actually want. So I just want to avoid getting a
bug report where the first thing we have to do is patch up the reg
dumper (and we can't really test this any other way than by human
inspection of a dump).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 11/15] drm/i915: add HAS_POWER_WELL

[Paulo Zanoni]

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

We're starting to add many IS_HASWELL checks for the power well code,
so add a HAS_POWER_WELL macro to properly document that we're checking
for hardware that has the power down well.

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

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9bf15e5..60e24d7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1337,6 +1337,7 @@ struct drm_i915_file_private {
 #define HAS_PIPE_CONTROL(dev) (INTEL_INFO(dev)->gen >= 5)
 
 #define HAS_DDI(dev)		(IS_HASWELL(dev))
+#define HAS_POWER_WELL(dev)	(IS_HASWELL(dev))
 
 #define INTEL_PCH_DEVICE_ID_MASK		0xff00
 #define INTEL_PCH_IBX_DEVICE_ID_TYPE		0x3b00
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 835bec2..39f5f72 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9338,7 +9338,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 
 	power_well_is_down = intel_power_well_is_down(dev);
 
-	if (IS_HASWELL(dev))
+	if (HAS_POWER_WELL(dev))
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
@@ -9394,7 +9394,7 @@ intel_display_print_error_state(struct seq_file *m,
 	int i;
 
 	seq_printf(m, "Num Pipes: %d\n", dev_priv->num_pipe);
-	if (IS_HASWELL(dev))
+	if (HAS_POWER_WELL(dev))
 		seq_printf(m, "PWR_WELL_CTL2: %08x\n",
 			   error->power_well_driver);
 	for_each_pipe(i) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 90562bc..9e22ad3 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4080,7 +4080,7 @@ bool intel_power_well_is_down(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (IS_HASWELL(dev))
+	if (HAS_POWER_WELL(dev))
 		return !(I915_READ(HSW_PWR_WELL_DRIVER) & HSW_PWR_WELL_ENABLE);
 	else
 		return false;
@@ -4092,7 +4092,7 @@ void intel_set_power_well(struct drm_device *dev, bool enable)
 	bool is_enabled, enable_requested;
 	uint32_t tmp;
 
-	if (!IS_HASWELL(dev))
+	if (!HAS_POWER_WELL(dev))
 		return;
 
 	tmp = I915_READ(HSW_PWR_WELL_DRIVER);
@@ -4127,7 +4127,7 @@ void intel_init_power_well(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!IS_HASWELL(dev))
+	if (!HAS_POWER_WELL(dev))
 		return;
 
 	/* For now, we need the power well to be always enabled. */
-- 
1.7.10.4

Re: [PATCH 11/15] drm/i915: add HAS_POWER_WELL

[Jesse Barnes]

On Wed,  6 Mar 2013 20:03:18 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We're starting to add many IS_HASWELL checks for the power well code,
> so add a HAS_POWER_WELL macro to properly document that we're checking
> for hardware that has the power down well.
> 

To be more future proof, we'll need to differentiate multiple power
wells.  Not sure we have good names for them though...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH 11/15] drm/i915: add HAS_POWER_WELL

[Daniel Vetter]

On Thu, Mar 07, 2013 at 09:14:56AM -0800, Jesse Barnes wrote:
> On Wed,  6 Mar 2013 20:03:18 -0300
> Paulo Zanoni <przanoni@gmail.com> wrote:
> 
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > We're starting to add many IS_HASWELL checks for the power well code,
> > so add a HAS_POWER_WELL macro to properly document that we're checking
> > for hardware that has the power down well.
> > 
> 
> To be more future proof, we'll need to differentiate multiple power
> wells.  Not sure we have good names for them though...

I think we'll just invent names once they show up ... Queued for -next,
thanks for the patch. I've had to drop the first hunk though since that
has been introduced by the previous patch, which I've rejected.
-Daniel
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH 12/15] drm/i915: reorganize intel_lvds_supported

[Paulo Zanoni]

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

Now it returns false for all platforms unless they're explicitly
listed on the function. There should be no real difference, except for
the fact that it now returns false on Haswell.

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

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6b24fc5..53bd5fd 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -1020,15 +1020,15 @@ static bool intel_lvds_supported(struct drm_device *dev)
 {
 	/* With the introduction of the PCH we gained a dedicated
 	 * LVDS presence pin, use it. */
-	if (HAS_PCH_SPLIT(dev))
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
 		return true;
 
-	if (IS_VALLEYVIEW(dev))
-		return false;
-
 	/* Otherwise LVDS was only attached to mobile products,
 	 * except for the inglorious 830gm */
-	return IS_MOBILE(dev) && !IS_I830(dev);
+	if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
+		return true;
+
+	return false;
 }
 
 /**
-- 
1.7.10.4

Re: [PATCH 12/15] drm/i915: reorganize intel_lvds_supported

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:19PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Now it returns false for all platforms unless they're explicitly
> listed on the function. There should be no real difference, except for
> the fact that it now returns false on Haswell.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I wouldn't mind comments in the commit or code reminding me that IBX and
CPT cover ILk->IVB, and gm45 counts as gen4, but that's just 'cause I
have a bad memory.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 6b24fc5..53bd5fd 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -1020,15 +1020,15 @@ static bool intel_lvds_supported(struct drm_device *dev)
>  {
>  	/* With the introduction of the PCH we gained a dedicated
>  	 * LVDS presence pin, use it. */
> -	if (HAS_PCH_SPLIT(dev))
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
>  		return true;
>  
> -	if (IS_VALLEYVIEW(dev))
> -		return false;
> -
>  	/* Otherwise LVDS was only attached to mobile products,
>  	 * except for the inglorious 830gm */
> -	return IS_MOBILE(dev) && !IS_I830(dev);
> +	if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
> +		return true;
> +
> +	return false;
>  }



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 12/15] drm/i915: reorganize intel_lvds_supported

[Daniel Vetter]

On Fri, Mar 15, 2013 at 01:57:52PM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:19PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Now it returns false for all platforms unless they're explicitly
> > listed on the function. There should be no real difference, except for
> > the fact that it now returns false on Haswell.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> I wouldn't mind comments in the commit or code reminding me that IBX and
> CPT cover ILk->IVB, and gm45 counts as gen4, but that's just 'cause I
> have a bad memory.
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 13/15] drm/i915: don't save/restore PCH_LVDS on LPT

[Paulo Zanoni]

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

Because the register does not exist on LPT. The interesting fact is
that reading/writing PCH_LVDS on LPT does *not* give us "unclaimed
register" messages, but the register value is always 0.

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

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index c1e02b0..41f0fde 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -209,7 +209,8 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->regfile.saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_PCH_CTL2);
 		dev_priv->regfile.saveBLC_CPU_PWM_CTL = I915_READ(BLC_PWM_CPU_CTL);
 		dev_priv->regfile.saveBLC_CPU_PWM_CTL2 = I915_READ(BLC_PWM_CPU_CTL2);
-		dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
+		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
+			dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
 	} else {
 		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
 		dev_priv->regfile.savePFIT_PGM_RATIOS = I915_READ(PFIT_PGM_RATIOS);
@@ -271,9 +272,9 @@ static void i915_restore_display(struct drm_device *dev)
 	if (drm_core_check_feature(dev, DRIVER_MODESET))
 		mask = ~LVDS_PORT_EN;
 
-	if (HAS_PCH_SPLIT(dev)) {
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
 		I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
-	} else if (IS_MOBILE(dev) && !IS_I830(dev))
+	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
 		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
 
 	if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
-- 
1.7.10.4

Re: [PATCH 13/15] drm/i915: don't save/restore PCH_LVDS on LPT

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:20PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because the register does not exist on LPT. The interesting fact is
> that reading/writing PCH_LVDS on LPT does *not* give us "unclaimed
> register" messages, but the register value is always 0.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index c1e02b0..41f0fde 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -209,7 +209,8 @@ static void i915_save_display(struct drm_device *dev)
>  		dev_priv->regfile.saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_PCH_CTL2);
>  		dev_priv->regfile.saveBLC_CPU_PWM_CTL = I915_READ(BLC_PWM_CPU_CTL);
>  		dev_priv->regfile.saveBLC_CPU_PWM_CTL2 = I915_READ(BLC_PWM_CPU_CTL2);
> -		dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
> +		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> +			dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
>  	} else {
>  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
>  		dev_priv->regfile.savePFIT_PGM_RATIOS = I915_READ(PFIT_PGM_RATIOS);
> @@ -271,9 +272,9 @@ static void i915_restore_display(struct drm_device *dev)
>  	if (drm_core_check_feature(dev, DRIVER_MODESET))
>  		mask = ~LVDS_PORT_EN;
>  
> -	if (HAS_PCH_SPLIT(dev)) {
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
>  		I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
> -	} else if (IS_MOBILE(dev) && !IS_I830(dev))
> +	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
>  		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
>  
>  	if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))

We don't support UMS on gen6+ so yuo can probably just check IS_GEN5
instead of if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 13/15] drm/i915: don't save/restore PCH_LVDS on LPT

[Daniel Vetter]

On Fri, Mar 15, 2013 at 02:04:22PM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:20PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Because the register does not exist on LPT. The interesting fact is
> > that reading/writing PCH_LVDS on LPT does *not* give us "unclaimed
> > register" messages, but the register value is always 0.
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c |    7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index c1e02b0..41f0fde 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -209,7 +209,8 @@ static void i915_save_display(struct drm_device *dev)
> >  		dev_priv->regfile.saveBLC_PWM_CTL2 = I915_READ(BLC_PWM_PCH_CTL2);
> >  		dev_priv->regfile.saveBLC_CPU_PWM_CTL = I915_READ(BLC_PWM_CPU_CTL);
> >  		dev_priv->regfile.saveBLC_CPU_PWM_CTL2 = I915_READ(BLC_PWM_CPU_CTL2);
> > -		dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
> > +		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> > +			dev_priv->regfile.saveLVDS = I915_READ(PCH_LVDS);
> >  	} else {
> >  		dev_priv->regfile.savePP_CONTROL = I915_READ(PP_CONTROL);
> >  		dev_priv->regfile.savePFIT_PGM_RATIOS = I915_READ(PFIT_PGM_RATIOS);
> > @@ -271,9 +272,9 @@ static void i915_restore_display(struct drm_device *dev)
> >  	if (drm_core_check_feature(dev, DRIVER_MODESET))
> >  		mask = ~LVDS_PORT_EN;
> >  
> > -	if (HAS_PCH_SPLIT(dev)) {
> > +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> >  		I915_WRITE(PCH_LVDS, dev_priv->regfile.saveLVDS & mask);
> > -	} else if (IS_MOBILE(dev) && !IS_I830(dev))
> > +	else if (INTEL_INFO(dev)->gen <= 4 && IS_MOBILE(dev) && !IS_I830(dev))
> >  		I915_WRITE(LVDS, dev_priv->regfile.saveLVDS & mask);
> >  
> >  	if (!IS_I830(dev) && !IS_845G(dev) && !HAS_PCH_SPLIT(dev))
> 
> We don't support UMS on gen6+ so yuo can probably just check IS_GEN5
> instead of if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))

Hah, trapped! We need to restore the LVDS reg even on kms safe for bit31
(which would enable the panel port) since we don't track all the bits of
the panel state. And the bios doesn't really restore that ...

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

[PATCH 14/15] 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.

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 29b1bb1..aefc674 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -129,6 +129,9 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
 	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
 								      pipe);
 
+	if (intel_power_well_is_down(dev) && cpu_transcoder != TRANSCODER_EDP)
+		return false;
+
 	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;
 }
 
-- 
1.7.10.4

Re: [PATCH 14/15] drm/i915: check the power well on i915_pipe_enabled

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:21PM -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.

This is really beginning to smell like we need special display register
reads which can do the check below.

> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

> ---
>  drivers/gpu/drm/i915/i915_irq.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 29b1bb1..aefc674 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -129,6 +129,9 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
>  
> +	if (intel_power_well_is_down(dev) && cpu_transcoder != TRANSCODER_EDP)
> +		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

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 14/15] drm/i915: check the power well on i915_pipe_enabled

[Daniel Vetter]

On Wed, Mar 06, 2013 at 08:03:21PM -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.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c |    3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 29b1bb1..aefc674 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -129,6 +129,9 @@ i915_pipe_enabled(struct drm_device *dev, int pipe)
>  	enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv,
>  								      pipe);
>  
> +	if (intel_power_well_is_down(dev) && cpu_transcoder != TRANSCODER_EDP)
> +		return false;
> +
>  	return I915_READ(PIPECONF(cpu_transcoder)) & PIPECONF_ENABLE;

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

[PATCH 15/15] drm/i915: add missing space in error message

[Paulo Zanoni]

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

To avoid this:
[  256.798060] [drm] capturing error event; look for more information
in/sys/kernel/debug/dri/0/i915_error_state

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

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index aefc674..0abd8ec 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1340,7 +1340,7 @@ static void i915_capture_error_state(struct drm_device *dev)
 		return;
 	}
 
-	DRM_INFO("capturing error event; look for more information in"
+	DRM_INFO("capturing error event; look for more information in "
 		 "/sys/kernel/debug/dri/%d/i915_error_state\n",
 		 dev->primary->index);
 
-- 
1.7.10.4

Re: [PATCH 15/15] drm/i915: add missing space in error message

[Ben Widawsky]

On Wed, Mar 06, 2013 at 08:03:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> To avoid this:
> [  256.798060] [drm] capturing error event; look for more information
> in/sys/kernel/debug/dri/0/i915_error_state
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>


You may want to add to the commit message that this fixes the issue introduced below:
commit 2f86f1916504525a6fdd6b412374b4ebf1102cbe
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Mon Jan 28 15:32:15 2013 -0800

    drm/i915: Error state should print /sys/kernel/debug
        ...
    [danvet: split up long line.] <----- he did it
    Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

I personally still believe we shouldn't chop up strings like this.



> ---
>  drivers/gpu/drm/i915/i915_irq.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index aefc674..0abd8ec 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1340,7 +1340,7 @@ static void i915_capture_error_state(struct drm_device *dev)
>  		return;
>  	}
>  
> -	DRM_INFO("capturing error event; look for more information in"
> +	DRM_INFO("capturing error event; look for more information in "
>  		 "/sys/kernel/debug/dri/%d/i915_error_state\n",
>  		 dev->primary->index);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH 15/15] drm/i915: add missing space in error message

[Daniel Vetter]

On Fri, Mar 15, 2013 at 02:10:20PM -0700, Ben Widawsky wrote:
> On Wed, Mar 06, 2013 at 08:03:22PM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > To avoid this:
> > [  256.798060] [drm] capturing error event; look for more information
> > in/sys/kernel/debug/dri/0/i915_error_state
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>

Queued for -next, thanks for the patch.

> You may want to add to the commit message that this fixes the issue introduced below:
> commit 2f86f1916504525a6fdd6b412374b4ebf1102cbe
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Mon Jan 28 15:32:15 2013 -0800
> 
>     drm/i915: Error state should print /sys/kernel/debug
>         ...
>     [danvet: split up long line.] <----- he did it
>     Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> I personally still believe we shouldn't chop up strings like this.

/me hides in shame

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