[PATCH 0/6] reduce register save/restoring accross suspend/resume

[PATCH 0/6] reduce register save/restoring accross suspend/resume

[Daniel Vetter]

Hi all,

This is a start at stopping to use the register save/restore stuff with kms
enabled. Imo we've had too many bugs in the resume code because no one seems to
care about this code and check whether it still makes sense for new platforms.
Or to check whether a bugfix to a reg write sequence also needs to be applied
there. Or whether restoring some random register is a good idea at all.

So I've started to audit all the register write/restores and check whether our
normal setup code could possible rely on this. If I haven't found anything, I've
braketed the respective block with a if (!KMS) check.

I probably need a few more days of utter boredom to work through it all.

Comments, flames and review highly welcome.

Cheers, Daniel

Daniel Vetter (6):
  drm/i915/crt: don't set HOTPLUG bits on !PCH
  drm/i915/crt: explicitly set up HOTPLUG_BITS on resume
  drm/i915: don't save/restor ADPA for kms
  drm/i915: don't save/restore DP regs for kms
  drm/i915: don't save/restore irq regs for kms
  drm/i915: don't save/restore HWS_PGA reg for kms

 drivers/gpu/drm/i915/i915_drv.c      |   1 -
 drivers/gpu/drm/i915/i915_suspend.c  | 154 +++++++++++++++++++----------------
 drivers/gpu/drm/i915/intel_crt.c     |  32 ++++----
 drivers/gpu/drm/i915/intel_display.c |   2 +
 4 files changed, 103 insertions(+), 86 deletions(-)

-- 
1.7.11.2

[PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Daniel Vetter]

... since they don't apply to pre-pch platforms and could actually be
harmful.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_crt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index c42b980..46c90f5 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -235,7 +235,11 @@ static void intel_crt_mode_set(struct drm_encoder *encoder,
 			   dpll_md & ~DPLL_MD_UDI_MULTIPLIER_MASK);
 	}
 
-	adpa = ADPA_HOTPLUG_BITS;
+	if (HAS_PCH_SPLIT(dev))
+		adpa = ADPA_HOTPLUG_BITS;
+	else
+		adpa = 0;
+
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
 		adpa |= ADPA_HSYNC_ACTIVE_HIGH;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
-- 
1.7.11.2

Re: [PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Paulo Zanoni]

2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
> ... since they don't apply to pre-pch platforms and could actually be
> harmful.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Ok, so I checked the specs and yes, these bit definitions don't exist.
The problem here is that instead of "must-be-zero", the spec says
"Reserved. Software must preserve the contents of these bits" for bits
29:16 (and also some others). So maybe by setting everything to 0
instead of enabling bits 17, 18, 20-23 we could actually be breaking
things? Either way, both the old and new code don't follow the
specification.

Maybe on non-pch-split we could try to read ADPA and erase all the
bits except the "must-be-preserved" ones?

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index c42b980..46c90f5 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -235,7 +235,11 @@ static void intel_crt_mode_set(struct drm_encoder *encoder,
>                            dpll_md & ~DPLL_MD_UDI_MULTIPLIER_MASK);
>         }
>
> -       adpa = ADPA_HOTPLUG_BITS;
> +       if (HAS_PCH_SPLIT(dev))
> +               adpa = ADPA_HOTPLUG_BITS;
> +       else
> +               adpa = 0;
> +
>         if (adjusted_mode->flags & DRM_MODE_FLAG_PHSYNC)
>                 adpa |= ADPA_HSYNC_ACTIVE_HIGH;
>         if (adjusted_mode->flags & DRM_MODE_FLAG_PVSYNC)
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Daniel Vetter]

On Fri, Oct 12, 2012 at 4:47 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
>> ... since they don't apply to pre-pch platforms and could actually be
>> harmful.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Ok, so I checked the specs and yes, these bit definitions don't exist.
> The problem here is that instead of "must-be-zero", the spec says
> "Reserved. Software must preserve the contents of these bits" for bits
> 29:16 (and also some others). So maybe by setting everything to 0
> instead of enabling bits 17, 18, 20-23 we could actually be breaking
> things? Either way, both the old and new code don't follow the
> specification.

Indeed, I've overlooked the "must be preserved" wording, and it goes
back to gen2 when the ADPA reg was added. So I've fired up all my
gen2/gen3 machines, and they have all zeros in these registers. And we
never write anything in there. I suspect this is simply a hint from
the Bspec authors to driver writes that they might eventually use
these reserved bits in future hw platforms. And if the driver
preserves the bit settings, it will automatically work. I think MBZ is
mostly used for bit ranges that have been used once, but are no longer
implemented (e.g. bits 12:13 do something in gen2/3 but not on later
hw gens).

> Maybe on non-pch-split we could try to read ADPA and erase all the
> bits except the "must-be-preserved" ones?

See above, I think we can clear them - at least all my old hw seems to
work perfectly well with all these bits being zero.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Paulo Zanoni]

2012/10/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Fri, Oct 12, 2012 at 4:47 AM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> 2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
>>> ... since they don't apply to pre-pch platforms and could actually be
>>> harmful.
>>>
>>> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>> Ok, so I checked the specs and yes, these bit definitions don't exist.
>> The problem here is that instead of "must-be-zero", the spec says
>> "Reserved. Software must preserve the contents of these bits" for bits
>> 29:16 (and also some others). So maybe by setting everything to 0
>> instead of enabling bits 17, 18, 20-23 we could actually be breaking
>> things? Either way, both the old and new code don't follow the
>> specification.
>
> Indeed, I've overlooked the "must be preserved" wording, and it goes
> back to gen2 when the ADPA reg was added. So I've fired up all my
> gen2/gen3 machines, and they have all zeros in these registers.

Ok, so please do a final test: try to write something to those
"must-be-preserved" bits and check if the values stay or not. If after
writing 1 to bits 17-18, 20-23 you read 0, then you have my
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan
to test the 6 patches of the series on hsw later btw.

> And we
> never write anything in there. I suspect this is simply a hint from
> the Bspec authors to driver writes that they might eventually use
> these reserved bits in future hw platforms. And if the driver
> preserves the bit settings, it will automatically work. I think MBZ is
> mostly used for bit ranges that have been used once, but are no longer
> implemented (e.g. bits 12:13 do something in gen2/3 but not on later
> hw gens).
>
>> Maybe on non-pch-split we could try to read ADPA and erase all the
>> bits except the "must-be-preserved" ones?
>
> See above, I think we can clear them - at least all my old hw seems to
> work perfectly well with all these bits being zero.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Daniel Vetter]

On Fri, Oct 12, 2012 at 7:17 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Ok, so please do a final test: try to write something to those
> "must-be-preserved" bits and check if the values stay or not. If after
> writing 1 to bits 17-18, 20-23 you read 0, then you have my
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan
> to test the 6 patches of the series on hsw later btw.

I've tested a few bits on a few machines, and some do stick and some
cause ... strange things (like a seemingly random set of other
register bits also being set). I guess that's not the answer you've
been looking for. I'd still prefer if we just try to clear things,
worst case we can easily read out the current state of these reserved
bits at boot up and restore them at resume time. But I'd really prefer
if we reduce our reliance on the boot-up state from the bios as much
as possible. And in this case here the only way to figure things out
is to merge it (it's really early for 3.8 anyway) and see what
happens.

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

Re: [PATCH 1/6] drm/i915/crt: don't set HOTPLUG bits on !PCH

[Paulo Zanoni]

2012/10/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> On Fri, Oct 12, 2012 at 7:17 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Ok, so please do a final test: try to write something to those
>> "must-be-preserved" bits and check if the values stay or not. If after
>> writing 1 to bits 17-18, 20-23 you read 0, then you have my
>> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>. I still do plan
>> to test the 6 patches of the series on hsw later btw.
>
> I've tested a few bits on a few machines, and some do stick and some
> cause ... strange things (like a seemingly random set of other
> register bits also being set). I guess that's not the answer you've
> been looking for. I'd still prefer if we just try to clear things,
> worst case we can easily read out the current state of these reserved
> bits at boot up and restore them at resume time. But I'd really prefer
> if we reduce our reliance on the boot-up state from the bios as much
> as possible. And in this case here the only way to figure things out
> is to merge it (it's really early for 3.8 anyway) and see what
> happens.

Well, ok then... Both the old and the new version don't exactly follow
the spec, but I guess writing zero makes more sense than enabling
random bits, especially because you said you checked and your machines
actually have zeros on those bits.

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

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



-- 
Paulo Zanoni

[PATCH 2/6] drm/i915/crt: explicitly set up HOTPLUG_BITS on resume

[Daniel Vetter]

... instead of relying on the register save/restore madness to do this.

To extract a bit of code call drm_mode_config_reset both on resume
and boot-up and move the hw state frobbing from the crt_init to the
->reset callback. The crt connector is the only one with a ->reset
callback, hence we can easily do this.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.c      |  1 -
 drivers/gpu/drm/i915/intel_crt.c     | 26 +++++++++++++-------------
 drivers/gpu/drm/i915/intel_display.c |  2 ++
 3 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a7837e5..2aabce7 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -547,7 +547,6 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 		intel_modeset_init_hw(dev);
 		intel_modeset_setup_hw_state(dev);
-		drm_mode_config_reset(dev);
 		drm_irq_install(dev);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 46c90f5..53f3e87 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -662,10 +662,22 @@ static int intel_crt_set_property(struct drm_connector *connector,
 static void intel_crt_reset(struct drm_connector *connector)
 {
 	struct drm_device *dev = connector->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crt *crt = intel_attached_crt(connector);
 
-	if (HAS_PCH_SPLIT(dev))
+	if (HAS_PCH_SPLIT(dev)) {
+		u32 adpa;
+
+		adpa = I915_READ(PCH_ADPA);
+		adpa &= ~ADPA_CRT_HOTPLUG_MASK;
+		adpa |= ADPA_HOTPLUG_BITS;
+		I915_WRITE(PCH_ADPA, adpa);
+		POSTING_READ(PCH_ADPA);
+
+		DRM_DEBUG_KMS("pch crt adpa set to 0x%x\n", adpa);
 		crt->force_hotplug_required = 1;
+	}
+
 }
 
 /*
@@ -784,18 +796,6 @@ void intel_crt_init(struct drm_device *dev)
 	 * Configure the automatic hotplug detection stuff
 	 */
 	crt->force_hotplug_required = 0;
-	if (HAS_PCH_SPLIT(dev)) {
-		u32 adpa;
-
-		adpa = I915_READ(PCH_ADPA);
-		adpa &= ~ADPA_CRT_HOTPLUG_MASK;
-		adpa |= ADPA_HOTPLUG_BITS;
-		I915_WRITE(PCH_ADPA, adpa);
-		POSTING_READ(PCH_ADPA);
-
-		DRM_DEBUG_KMS("pch crt adpa set to 0x%x\n", adpa);
-		crt->force_hotplug_required = 1;
-	}
 
 	dev_priv->hotplug_supported_mask |= CRT_HOTPLUG_INT_STATUS;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d469b42..ffc3758 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8334,6 +8334,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev)
 	intel_modeset_update_staged_output_state(dev);
 
 	intel_modeset_check_state(dev);
+
+	drm_mode_config_reset(dev);
 }
 
 void intel_modeset_gem_init(struct drm_device *dev)
-- 
1.7.11.2

Re: [PATCH 2/6] drm/i915/crt: explicitly set up HOTPLUG_BITS on resume

[Paulo Zanoni]

Hi

2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
> ... instead of relying on the register save/restore madness to do this.
>
> To extract a bit of code call drm_mode_config_reset both on resume
> and boot-up and move the hw state frobbing from the crt_init to the
> ->reset callback. The crt connector is the only one with a ->reset
> callback, hence we can easily do this.

The patch looks correct, but doesn't it make more sense to add some
encoder-specific "intel_sanitize_encoder" callback instead of reusing
drm's ->reset ? Maybe we're distorting the meaning of the ->reset
callback?

This is just a bikeshed, not a strong opinion, so feel free to merge
it as it is:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |  1 -
>  drivers/gpu/drm/i915/intel_crt.c     | 26 +++++++++++++-------------
>  drivers/gpu/drm/i915/intel_display.c |  2 ++
>  3 files changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a7837e5..2aabce7 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -547,7 +547,6 @@ static int i915_drm_thaw(struct drm_device *dev)
>
>                 intel_modeset_init_hw(dev);
>                 intel_modeset_setup_hw_state(dev);
> -               drm_mode_config_reset(dev);
>                 drm_irq_install(dev);
>         }
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 46c90f5..53f3e87 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -662,10 +662,22 @@ static int intel_crt_set_property(struct drm_connector *connector,
>  static void intel_crt_reset(struct drm_connector *connector)
>  {
>         struct drm_device *dev = connector->dev;
> +       struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crt *crt = intel_attached_crt(connector);
>
> -       if (HAS_PCH_SPLIT(dev))
> +       if (HAS_PCH_SPLIT(dev)) {
> +               u32 adpa;
> +
> +               adpa = I915_READ(PCH_ADPA);
> +               adpa &= ~ADPA_CRT_HOTPLUG_MASK;
> +               adpa |= ADPA_HOTPLUG_BITS;
> +               I915_WRITE(PCH_ADPA, adpa);
> +               POSTING_READ(PCH_ADPA);
> +
> +               DRM_DEBUG_KMS("pch crt adpa set to 0x%x\n", adpa);
>                 crt->force_hotplug_required = 1;
> +       }
> +
>  }
>
>  /*
> @@ -784,18 +796,6 @@ void intel_crt_init(struct drm_device *dev)
>          * Configure the automatic hotplug detection stuff
>          */
>         crt->force_hotplug_required = 0;
> -       if (HAS_PCH_SPLIT(dev)) {
> -               u32 adpa;
> -
> -               adpa = I915_READ(PCH_ADPA);
> -               adpa &= ~ADPA_CRT_HOTPLUG_MASK;
> -               adpa |= ADPA_HOTPLUG_BITS;
> -               I915_WRITE(PCH_ADPA, adpa);
> -               POSTING_READ(PCH_ADPA);
> -
> -               DRM_DEBUG_KMS("pch crt adpa set to 0x%x\n", adpa);
> -               crt->force_hotplug_required = 1;
> -       }
>
>         dev_priv->hotplug_supported_mask |= CRT_HOTPLUG_INT_STATUS;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d469b42..ffc3758 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8334,6 +8334,8 @@ void intel_modeset_setup_hw_state(struct drm_device *dev)
>         intel_modeset_update_staged_output_state(dev);
>
>         intel_modeset_check_state(dev);
> +
> +       drm_mode_config_reset(dev);
>  }
>
>  void intel_modeset_gem_init(struct drm_device *dev)
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 3/6] drm/i915: don't save/restor ADPA for kms

[Daniel Vetter]

We now no longer rely on this.

This is step 1 on a long journey to rid us of the save/restore
madness, which tends to lightly paper over many issues, and cause
tons of bad things itself ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 4776ccf..6e398a8 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -395,6 +395,13 @@ static void i915_save_modeset_reg(struct drm_device *dev)
 		break;
 	}
 
+	/* CRT state */
+	if (HAS_PCH_SPLIT(dev)) {
+		dev_priv->saveADPA = I915_READ(PCH_ADPA);
+	} else {
+		dev_priv->saveADPA = I915_READ(ADPA);
+	}
+
 	return;
 }
 
@@ -601,6 +608,12 @@ static void i915_restore_modeset_reg(struct drm_device *dev)
 	if (IS_GEN2(dev))
 		I915_WRITE(CURSIZE, dev_priv->saveCURSIZE);
 
+	/* CRT state */
+	if (HAS_PCH_SPLIT(dev))
+		I915_WRITE(PCH_ADPA, dev_priv->saveADPA);
+	else
+		I915_WRITE(ADPA, dev_priv->saveADPA);
+
 	return;
 }
 
@@ -615,13 +628,6 @@ static void i915_save_display(struct drm_device *dev)
 	/* Don't save them in KMS mode */
 	i915_save_modeset_reg(dev);
 
-	/* CRT state */
-	if (HAS_PCH_SPLIT(dev)) {
-		dev_priv->saveADPA = I915_READ(PCH_ADPA);
-	} else {
-		dev_priv->saveADPA = I915_READ(ADPA);
-	}
-
 	/* LVDS state */
 	if (HAS_PCH_SPLIT(dev)) {
 		dev_priv->savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
@@ -719,12 +725,6 @@ static void i915_restore_display(struct drm_device *dev)
 	/* Don't restore them in KMS mode */
 	i915_restore_modeset_reg(dev);
 
-	/* CRT state */
-	if (HAS_PCH_SPLIT(dev))
-		I915_WRITE(PCH_ADPA, dev_priv->saveADPA);
-	else
-		I915_WRITE(ADPA, dev_priv->saveADPA);
-
 	/* LVDS state */
 	if (INTEL_INFO(dev)->gen >= 4 && !HAS_PCH_SPLIT(dev))
 		I915_WRITE(BLC_PWM_CTL2, dev_priv->saveBLC_PWM_CTL2);
-- 
1.7.11.2

Re: [PATCH 3/6] drm/i915: don't save/restor ADPA for kms

[Paulo Zanoni]

Hi

2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
> We now no longer rely on this.
>
> This is step 1 on a long journey to rid us of the save/restore
> madness, which tends to lightly paper over many issues, and cause
> tons of bad things itself ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 4776ccf..6e398a8 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -395,6 +395,13 @@ static void i915_save_modeset_reg(struct drm_device *dev)
>                 break;
>         }
>
> +       /* CRT state */
> +       if (HAS_PCH_SPLIT(dev)) {
> +               dev_priv->saveADPA = I915_READ(PCH_ADPA);
> +       } else {
> +               dev_priv->saveADPA = I915_READ(ADPA);
> +       }
> +

My OCD is telling me to ask you to remove '}' and '{' here (yes, I
know, they're here because of copy/paste).

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

>         return;
>  }
>
> @@ -601,6 +608,12 @@ static void i915_restore_modeset_reg(struct drm_device *dev)
>         if (IS_GEN2(dev))
>                 I915_WRITE(CURSIZE, dev_priv->saveCURSIZE);
>
> +       /* CRT state */
> +       if (HAS_PCH_SPLIT(dev))
> +               I915_WRITE(PCH_ADPA, dev_priv->saveADPA);
> +       else
> +               I915_WRITE(ADPA, dev_priv->saveADPA);
> +
>         return;
>  }
>
> @@ -615,13 +628,6 @@ static void i915_save_display(struct drm_device *dev)
>         /* Don't save them in KMS mode */
>         i915_save_modeset_reg(dev);
>
> -       /* CRT state */
> -       if (HAS_PCH_SPLIT(dev)) {
> -               dev_priv->saveADPA = I915_READ(PCH_ADPA);
> -       } else {
> -               dev_priv->saveADPA = I915_READ(ADPA);
> -       }
> -
>         /* LVDS state */
>         if (HAS_PCH_SPLIT(dev)) {
>                 dev_priv->savePP_CONTROL = I915_READ(PCH_PP_CONTROL);
> @@ -719,12 +725,6 @@ static void i915_restore_display(struct drm_device *dev)
>         /* Don't restore them in KMS mode */
>         i915_restore_modeset_reg(dev);
>
> -       /* CRT state */
> -       if (HAS_PCH_SPLIT(dev))
> -               I915_WRITE(PCH_ADPA, dev_priv->saveADPA);
> -       else
> -               I915_WRITE(ADPA, dev_priv->saveADPA);
> -
>         /* LVDS state */
>         if (INTEL_INFO(dev)->gen >= 4 && !HAS_PCH_SPLIT(dev))
>                 I915_WRITE(BLC_PWM_CTL2, dev_priv->saveBLC_PWM_CTL2);
> --
> 1.7.11.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 3/6] drm/i915: don't save/restor ADPA for kms

[Daniel Vetter]

On Wed, Oct 17, 2012 at 06:49:00PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/10/11 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > We now no longer rely on this.
> >
> > This is step 1 on a long journey to rid us of the save/restore
> > madness, which tends to lightly paper over many issues, and cause
> > tons of bad things itself ...
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/i915_suspend.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> > index 4776ccf..6e398a8 100644
> > --- a/drivers/gpu/drm/i915/i915_suspend.c
> > +++ b/drivers/gpu/drm/i915/i915_suspend.c
> > @@ -395,6 +395,13 @@ static void i915_save_modeset_reg(struct drm_device *dev)
> >                 break;
> >         }
> >
> > +       /* CRT state */
> > +       if (HAS_PCH_SPLIT(dev)) {
> > +               dev_priv->saveADPA = I915_READ(PCH_ADPA);
> > +       } else {
> > +               dev_priv->saveADPA = I915_READ(ADPA);
> > +       }
> > +
> 
> My OCD is telling me to ask you to remove '}' and '{' here (yes, I
> know, they're here because of copy/paste).

OCD is satisfied, patches merged, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 4/6] drm/i915: don't save/restore DP regs for kms

[Daniel Vetter]

We completely compute these anew in each modeset, hence we don't rely
on them containing anything valid after resume.

To avoid breaking any ums setup due to reordering of the reads/writes
simply don't reorder anything, but bracket the reads/writes into if
(!kms) conditionals. More churn, but safer.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 68 ++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 6e398a8..eaf1e4a 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -660,21 +660,23 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->savePP_DIVISOR = I915_READ(PP_DIVISOR);
 	}
 
-	/* Display Port state */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		dev_priv->saveDP_B = I915_READ(DP_B);
-		dev_priv->saveDP_C = I915_READ(DP_C);
-		dev_priv->saveDP_D = I915_READ(DP_D);
-		dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
-		dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
-		dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
-		dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
-		dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
-		dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
-		dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
-		dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
-	}
-	/* FIXME: save TV & SDVO state */
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display Port state */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			dev_priv->saveDP_B = I915_READ(DP_B);
+			dev_priv->saveDP_C = I915_READ(DP_C);
+			dev_priv->saveDP_D = I915_READ(DP_D);
+			dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
+			dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
+			dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
+			dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
+			dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
+			dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
+			dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
+			dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
+		}
+		/* FIXME: save TV & SDVO state */
+	}
 
 	/* Only save FBC state on the platform that supports FBC */
 	if (I915_HAS_FBC(dev)) {
@@ -709,16 +711,18 @@ static void i915_restore_display(struct drm_device *dev)
 	/* Display arbitration */
 	I915_WRITE(DSPARB, dev_priv->saveDSPARB);
 
-	/* Display port ratios (must be done before clock is set) */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
-		I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
-		I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
-		I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
-		I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
-		I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
-		I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
-		I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display port ratios (must be done before clock is set) */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
+			I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
+			I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
+			I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
+			I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
+			I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
+			I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
+			I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
+		}
 	}
 
 	/* This is only meaningful in non-KMS mode */
@@ -761,13 +765,15 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PP_CONTROL, dev_priv->savePP_CONTROL);
 	}
 
-	/* Display Port state */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		I915_WRITE(DP_B, dev_priv->saveDP_B);
-		I915_WRITE(DP_C, dev_priv->saveDP_C);
-		I915_WRITE(DP_D, dev_priv->saveDP_D);
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display Port state */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			I915_WRITE(DP_B, dev_priv->saveDP_B);
+			I915_WRITE(DP_C, dev_priv->saveDP_C);
+			I915_WRITE(DP_D, dev_priv->saveDP_D);
+		}
+		/* FIXME: restore TV & SDVO state */
 	}
-	/* FIXME: restore TV & SDVO state */
 
 	/* only restore FBC info on the platform that supports FBC*/
 	intel_disable_fbc(dev);
-- 
1.7.11.2

[PATCH 5/6] drm/i915: don't save/restore irq regs for kms

[Daniel Vetter]

We already call drm_irq_install/uninstall at the right time, which
will set up the irq registers with the correct values (through the
preinstall hooks).

For kms this is at best harmless, in the worst case we get an
interrupt when we don't really expect it.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 56 ++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index eaf1e4a..44893be 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -818,20 +818,22 @@ int i915_save_state(struct drm_device *dev)
 
 	i915_save_display(dev);
 
-	/* Interrupt state */
-	if (HAS_PCH_SPLIT(dev)) {
-		dev_priv->saveDEIER = I915_READ(DEIER);
-		dev_priv->saveDEIMR = I915_READ(DEIMR);
-		dev_priv->saveGTIER = I915_READ(GTIER);
-		dev_priv->saveGTIMR = I915_READ(GTIMR);
-		dev_priv->saveFDI_RXA_IMR = I915_READ(_FDI_RXA_IMR);
-		dev_priv->saveFDI_RXB_IMR = I915_READ(_FDI_RXB_IMR);
-		dev_priv->saveMCHBAR_RENDER_STANDBY =
-			I915_READ(RSTDBYCTL);
-		dev_priv->savePCH_PORT_HOTPLUG = I915_READ(PCH_PORT_HOTPLUG);
-	} else {
-		dev_priv->saveIER = I915_READ(IER);
-		dev_priv->saveIMR = I915_READ(IMR);
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Interrupt state */
+		if (HAS_PCH_SPLIT(dev)) {
+			dev_priv->saveDEIER = I915_READ(DEIER);
+			dev_priv->saveDEIMR = I915_READ(DEIMR);
+			dev_priv->saveGTIER = I915_READ(GTIER);
+			dev_priv->saveGTIMR = I915_READ(GTIMR);
+			dev_priv->saveFDI_RXA_IMR = I915_READ(_FDI_RXA_IMR);
+			dev_priv->saveFDI_RXB_IMR = I915_READ(_FDI_RXB_IMR);
+			dev_priv->saveMCHBAR_RENDER_STANDBY =
+				I915_READ(RSTDBYCTL);
+			dev_priv->savePCH_PORT_HOTPLUG = I915_READ(PCH_PORT_HOTPLUG);
+		} else {
+			dev_priv->saveIER = I915_READ(IER);
+			dev_priv->saveIMR = I915_READ(IMR);
+		}
 	}
 
 	intel_disable_gt_powersave(dev);
@@ -869,18 +871,20 @@ int i915_restore_state(struct drm_device *dev)
 
 	i915_restore_display(dev);
 
-	/* Interrupt state */
-	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(DEIER, dev_priv->saveDEIER);
-		I915_WRITE(DEIMR, dev_priv->saveDEIMR);
-		I915_WRITE(GTIER, dev_priv->saveGTIER);
-		I915_WRITE(GTIMR, dev_priv->saveGTIMR);
-		I915_WRITE(_FDI_RXA_IMR, dev_priv->saveFDI_RXA_IMR);
-		I915_WRITE(_FDI_RXB_IMR, dev_priv->saveFDI_RXB_IMR);
-		I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->savePCH_PORT_HOTPLUG);
-	} else {
-		I915_WRITE(IER, dev_priv->saveIER);
-		I915_WRITE(IMR, dev_priv->saveIMR);
+	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Interrupt state */
+		if (HAS_PCH_SPLIT(dev)) {
+			I915_WRITE(DEIER, dev_priv->saveDEIER);
+			I915_WRITE(DEIMR, dev_priv->saveDEIMR);
+			I915_WRITE(GTIER, dev_priv->saveGTIER);
+			I915_WRITE(GTIMR, dev_priv->saveGTIMR);
+			I915_WRITE(_FDI_RXA_IMR, dev_priv->saveFDI_RXA_IMR);
+			I915_WRITE(_FDI_RXB_IMR, dev_priv->saveFDI_RXB_IMR);
+			I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->savePCH_PORT_HOTPLUG);
+		} else {
+			I915_WRITE(IER, dev_priv->saveIER);
+			I915_WRITE(IMR, dev_priv->saveIMR);
+		}
 	}
 
 	/* Cache mode state */
-- 
1.7.11.2

[PATCH 6/6] drm/i915: don't save/restore HWS_PGA reg for kms

[Daniel Vetter]

We already do that as part of the ringbuffer re-setup at resume time.
Furthermore the register offset has moved on gen6+ around quite a bit,
and on ilk/gm45 we also need to restore the HWS reg for the bsd ring,
not just the render ring.

So again in kms mode this is only confusing a best, hence don't
bother.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 44893be..f9630d9 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -814,7 +814,8 @@ int i915_save_state(struct drm_device *dev)
 	mutex_lock(&dev->struct_mutex);
 
 	/* Hardware status page */
-	dev_priv->saveHWS = I915_READ(HWS_PGA);
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		dev_priv->saveHWS = I915_READ(HWS_PGA);
 
 	i915_save_display(dev);
 
@@ -867,7 +868,8 @@ int i915_restore_state(struct drm_device *dev)
 	mutex_lock(&dev->struct_mutex);
 
 	/* Hardware status page */
-	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
+	if (drm_core_check_feature(dev, DRIVER_MODESET))
+		I915_WRITE(HWS_PGA, dev_priv->saveHWS);
 
 	i915_restore_display(dev);
 
-- 
1.7.11.2

[PATCH 1/3] drm/i915: don't save/restore DP regs for kms

[Daniel Vetter]

We completely compute these anew in each modeset, hence we don't rely
on them containing anything valid after resume.

To avoid breaking any ums setup due to reordering of the reads/writes
simply don't reorder anything, but bracket the reads/writes into if
(!kms) conditionals. More churn, but safer.

v2: Fixup the logic, noticed by Paulo Zanoni.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 68 ++++++++++++++++++++-----------------
 1 file changed, 37 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 6e398a8..3d12147 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -660,21 +660,23 @@ static void i915_save_display(struct drm_device *dev)
 		dev_priv->savePP_DIVISOR = I915_READ(PP_DIVISOR);
 	}
 
-	/* Display Port state */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		dev_priv->saveDP_B = I915_READ(DP_B);
-		dev_priv->saveDP_C = I915_READ(DP_C);
-		dev_priv->saveDP_D = I915_READ(DP_D);
-		dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
-		dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
-		dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
-		dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
-		dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
-		dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
-		dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
-		dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
-	}
-	/* FIXME: save TV & SDVO state */
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display Port state */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			dev_priv->saveDP_B = I915_READ(DP_B);
+			dev_priv->saveDP_C = I915_READ(DP_C);
+			dev_priv->saveDP_D = I915_READ(DP_D);
+			dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
+			dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
+			dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
+			dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
+			dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
+			dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
+			dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
+			dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
+		}
+		/* FIXME: save TV & SDVO state */
+	}
 
 	/* Only save FBC state on the platform that supports FBC */
 	if (I915_HAS_FBC(dev)) {
@@ -709,16 +711,18 @@ static void i915_restore_display(struct drm_device *dev)
 	/* Display arbitration */
 	I915_WRITE(DSPARB, dev_priv->saveDSPARB);
 
-	/* Display port ratios (must be done before clock is set) */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
-		I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
-		I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
-		I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
-		I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
-		I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
-		I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
-		I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display port ratios (must be done before clock is set) */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
+			I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
+			I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
+			I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
+			I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
+			I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
+			I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
+			I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
+		}
 	}
 
 	/* This is only meaningful in non-KMS mode */
@@ -761,13 +765,15 @@ static void i915_restore_display(struct drm_device *dev)
 		I915_WRITE(PP_CONTROL, dev_priv->savePP_CONTROL);
 	}
 
-	/* Display Port state */
-	if (SUPPORTS_INTEGRATED_DP(dev)) {
-		I915_WRITE(DP_B, dev_priv->saveDP_B);
-		I915_WRITE(DP_C, dev_priv->saveDP_C);
-		I915_WRITE(DP_D, dev_priv->saveDP_D);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Display Port state */
+		if (SUPPORTS_INTEGRATED_DP(dev)) {
+			I915_WRITE(DP_B, dev_priv->saveDP_B);
+			I915_WRITE(DP_C, dev_priv->saveDP_C);
+			I915_WRITE(DP_D, dev_priv->saveDP_D);
+		}
+		/* FIXME: restore TV & SDVO state */
 	}
-	/* FIXME: restore TV & SDVO state */
 
 	/* only restore FBC info on the platform that supports FBC*/
 	intel_disable_fbc(dev);
-- 
1.7.11.7

[PATCH 2/3] drm/i915: don't save/restore irq regs for kms

[Daniel Vetter]

We already call drm_irq_install/uninstall at the right time, which
will set up the irq registers with the correct values (through the
preinstall hooks).

For kms this is at best harmless, in the worst case we get an
interrupt when we don't really expect it.

v2: Fixup the logic, noticed by Paulo Zanoni.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 56 ++++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index 3d12147..ac6d412 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -818,20 +818,22 @@ int i915_save_state(struct drm_device *dev)
 
 	i915_save_display(dev);
 
-	/* Interrupt state */
-	if (HAS_PCH_SPLIT(dev)) {
-		dev_priv->saveDEIER = I915_READ(DEIER);
-		dev_priv->saveDEIMR = I915_READ(DEIMR);
-		dev_priv->saveGTIER = I915_READ(GTIER);
-		dev_priv->saveGTIMR = I915_READ(GTIMR);
-		dev_priv->saveFDI_RXA_IMR = I915_READ(_FDI_RXA_IMR);
-		dev_priv->saveFDI_RXB_IMR = I915_READ(_FDI_RXB_IMR);
-		dev_priv->saveMCHBAR_RENDER_STANDBY =
-			I915_READ(RSTDBYCTL);
-		dev_priv->savePCH_PORT_HOTPLUG = I915_READ(PCH_PORT_HOTPLUG);
-	} else {
-		dev_priv->saveIER = I915_READ(IER);
-		dev_priv->saveIMR = I915_READ(IMR);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Interrupt state */
+		if (HAS_PCH_SPLIT(dev)) {
+			dev_priv->saveDEIER = I915_READ(DEIER);
+			dev_priv->saveDEIMR = I915_READ(DEIMR);
+			dev_priv->saveGTIER = I915_READ(GTIER);
+			dev_priv->saveGTIMR = I915_READ(GTIMR);
+			dev_priv->saveFDI_RXA_IMR = I915_READ(_FDI_RXA_IMR);
+			dev_priv->saveFDI_RXB_IMR = I915_READ(_FDI_RXB_IMR);
+			dev_priv->saveMCHBAR_RENDER_STANDBY =
+				I915_READ(RSTDBYCTL);
+			dev_priv->savePCH_PORT_HOTPLUG = I915_READ(PCH_PORT_HOTPLUG);
+		} else {
+			dev_priv->saveIER = I915_READ(IER);
+			dev_priv->saveIMR = I915_READ(IMR);
+		}
 	}
 
 	intel_disable_gt_powersave(dev);
@@ -869,18 +871,20 @@ int i915_restore_state(struct drm_device *dev)
 
 	i915_restore_display(dev);
 
-	/* Interrupt state */
-	if (HAS_PCH_SPLIT(dev)) {
-		I915_WRITE(DEIER, dev_priv->saveDEIER);
-		I915_WRITE(DEIMR, dev_priv->saveDEIMR);
-		I915_WRITE(GTIER, dev_priv->saveGTIER);
-		I915_WRITE(GTIMR, dev_priv->saveGTIMR);
-		I915_WRITE(_FDI_RXA_IMR, dev_priv->saveFDI_RXA_IMR);
-		I915_WRITE(_FDI_RXB_IMR, dev_priv->saveFDI_RXB_IMR);
-		I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->savePCH_PORT_HOTPLUG);
-	} else {
-		I915_WRITE(IER, dev_priv->saveIER);
-		I915_WRITE(IMR, dev_priv->saveIMR);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
+		/* Interrupt state */
+		if (HAS_PCH_SPLIT(dev)) {
+			I915_WRITE(DEIER, dev_priv->saveDEIER);
+			I915_WRITE(DEIMR, dev_priv->saveDEIMR);
+			I915_WRITE(GTIER, dev_priv->saveGTIER);
+			I915_WRITE(GTIMR, dev_priv->saveGTIMR);
+			I915_WRITE(_FDI_RXA_IMR, dev_priv->saveFDI_RXA_IMR);
+			I915_WRITE(_FDI_RXB_IMR, dev_priv->saveFDI_RXB_IMR);
+			I915_WRITE(PCH_PORT_HOTPLUG, dev_priv->savePCH_PORT_HOTPLUG);
+		} else {
+			I915_WRITE(IER, dev_priv->saveIER);
+			I915_WRITE(IMR, dev_priv->saveIMR);
+		}
 	}
 
 	/* Cache mode state */
-- 
1.7.11.7

[PATCH 3/3] drm/i915: don't save/restore HWS_PGA reg for kms

[Daniel Vetter]

We already do that as part of the ringbuffer re-setup at resume time.
Furthermore the register offset has moved on gen6+ around quite a bit,
and on ilk/gm45 we also need to restore the HWS reg for the bsd ring,
not just the render ring.

So again in kms mode this is only confusing a best, hence don't
bother.

v2: Fixup logic, noticed by Paulo Zanoni.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_suspend.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
index ac6d412..33121aa 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -814,7 +814,8 @@ int i915_save_state(struct drm_device *dev)
 	mutex_lock(&dev->struct_mutex);
 
 	/* Hardware status page */
-	dev_priv->saveHWS = I915_READ(HWS_PGA);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		dev_priv->saveHWS = I915_READ(HWS_PGA);
 
 	i915_save_display(dev);
 
@@ -867,7 +868,8 @@ int i915_restore_state(struct drm_device *dev)
 	mutex_lock(&dev->struct_mutex);
 
 	/* Hardware status page */
-	I915_WRITE(HWS_PGA, dev_priv->saveHWS);
+	if (!drm_core_check_feature(dev, DRIVER_MODESET))
+		I915_WRITE(HWS_PGA, dev_priv->saveHWS);
 
 	i915_restore_display(dev);
 
-- 
1.7.11.7

Re: [PATCH 1/3] drm/i915: don't save/restore DP regs for kms

[Paulo Zanoni]

Hi

2012/10/17 Daniel Vetter <daniel.vetter@ffwll.ch>:
> We completely compute these anew in each modeset, hence we don't rely
> on them containing anything valid after resume.
>
> To avoid breaking any ums setup due to reordering of the reads/writes
> simply don't reorder anything, but bracket the reads/writes into if
> (!kms) conditionals. More churn, but safer.
>
> v2: Fixup the logic, noticed by Paulo Zanoni.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

For the 3 patches:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Tested only on HSW: S3 and S4 suspend still works.

> ---
>  drivers/gpu/drm/i915/i915_suspend.c | 68 ++++++++++++++++++++-----------------
>  1 file changed, 37 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_suspend.c b/drivers/gpu/drm/i915/i915_suspend.c
> index 6e398a8..3d12147 100644
> --- a/drivers/gpu/drm/i915/i915_suspend.c
> +++ b/drivers/gpu/drm/i915/i915_suspend.c
> @@ -660,21 +660,23 @@ static void i915_save_display(struct drm_device *dev)
>                 dev_priv->savePP_DIVISOR = I915_READ(PP_DIVISOR);
>         }
>
> -       /* Display Port state */
> -       if (SUPPORTS_INTEGRATED_DP(dev)) {
> -               dev_priv->saveDP_B = I915_READ(DP_B);
> -               dev_priv->saveDP_C = I915_READ(DP_C);
> -               dev_priv->saveDP_D = I915_READ(DP_D);
> -               dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
> -               dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
> -               dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
> -               dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
> -               dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
> -               dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
> -               dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
> -               dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
> -       }
> -       /* FIXME: save TV & SDVO state */
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Display Port state */
> +               if (SUPPORTS_INTEGRATED_DP(dev)) {
> +                       dev_priv->saveDP_B = I915_READ(DP_B);
> +                       dev_priv->saveDP_C = I915_READ(DP_C);
> +                       dev_priv->saveDP_D = I915_READ(DP_D);
> +                       dev_priv->savePIPEA_GMCH_DATA_M = I915_READ(_PIPEA_GMCH_DATA_M);
> +                       dev_priv->savePIPEB_GMCH_DATA_M = I915_READ(_PIPEB_GMCH_DATA_M);
> +                       dev_priv->savePIPEA_GMCH_DATA_N = I915_READ(_PIPEA_GMCH_DATA_N);
> +                       dev_priv->savePIPEB_GMCH_DATA_N = I915_READ(_PIPEB_GMCH_DATA_N);
> +                       dev_priv->savePIPEA_DP_LINK_M = I915_READ(_PIPEA_DP_LINK_M);
> +                       dev_priv->savePIPEB_DP_LINK_M = I915_READ(_PIPEB_DP_LINK_M);
> +                       dev_priv->savePIPEA_DP_LINK_N = I915_READ(_PIPEA_DP_LINK_N);
> +                       dev_priv->savePIPEB_DP_LINK_N = I915_READ(_PIPEB_DP_LINK_N);
> +               }
> +               /* FIXME: save TV & SDVO state */
> +       }
>
>         /* Only save FBC state on the platform that supports FBC */
>         if (I915_HAS_FBC(dev)) {
> @@ -709,16 +711,18 @@ static void i915_restore_display(struct drm_device *dev)
>         /* Display arbitration */
>         I915_WRITE(DSPARB, dev_priv->saveDSPARB);
>
> -       /* Display port ratios (must be done before clock is set) */
> -       if (SUPPORTS_INTEGRATED_DP(dev)) {
> -               I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
> -               I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
> -               I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
> -               I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
> -               I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
> -               I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
> -               I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
> -               I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Display port ratios (must be done before clock is set) */
> +               if (SUPPORTS_INTEGRATED_DP(dev)) {
> +                       I915_WRITE(_PIPEA_GMCH_DATA_M, dev_priv->savePIPEA_GMCH_DATA_M);
> +                       I915_WRITE(_PIPEB_GMCH_DATA_M, dev_priv->savePIPEB_GMCH_DATA_M);
> +                       I915_WRITE(_PIPEA_GMCH_DATA_N, dev_priv->savePIPEA_GMCH_DATA_N);
> +                       I915_WRITE(_PIPEB_GMCH_DATA_N, dev_priv->savePIPEB_GMCH_DATA_N);
> +                       I915_WRITE(_PIPEA_DP_LINK_M, dev_priv->savePIPEA_DP_LINK_M);
> +                       I915_WRITE(_PIPEB_DP_LINK_M, dev_priv->savePIPEB_DP_LINK_M);
> +                       I915_WRITE(_PIPEA_DP_LINK_N, dev_priv->savePIPEA_DP_LINK_N);
> +                       I915_WRITE(_PIPEB_DP_LINK_N, dev_priv->savePIPEB_DP_LINK_N);
> +               }
>         }
>
>         /* This is only meaningful in non-KMS mode */
> @@ -761,13 +765,15 @@ static void i915_restore_display(struct drm_device *dev)
>                 I915_WRITE(PP_CONTROL, dev_priv->savePP_CONTROL);
>         }
>
> -       /* Display Port state */
> -       if (SUPPORTS_INTEGRATED_DP(dev)) {
> -               I915_WRITE(DP_B, dev_priv->saveDP_B);
> -               I915_WRITE(DP_C, dev_priv->saveDP_C);
> -               I915_WRITE(DP_D, dev_priv->saveDP_D);
> +       if (!drm_core_check_feature(dev, DRIVER_MODESET)) {
> +               /* Display Port state */
> +               if (SUPPORTS_INTEGRATED_DP(dev)) {
> +                       I915_WRITE(DP_B, dev_priv->saveDP_B);
> +                       I915_WRITE(DP_C, dev_priv->saveDP_C);
> +                       I915_WRITE(DP_D, dev_priv->saveDP_D);
> +               }
> +               /* FIXME: restore TV & SDVO state */
>         }
> -       /* FIXME: restore TV & SDVO state */
>
>         /* only restore FBC info on the platform that supports FBC*/
>         intel_disable_fbc(dev);
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 1/3] drm/i915: don't save/restore DP regs for kms

[Daniel Vetter]

On Wed, Oct 17, 2012 at 04:52:28PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/10/17 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > We completely compute these anew in each modeset, hence we don't rely
> > on them containing anything valid after resume.
> >
> > To avoid breaking any ums setup due to reordering of the reads/writes
> > simply don't reorder anything, but bracket the reads/writes into if
> > (!kms) conditionals. More churn, but safer.
> >
> > v2: Fixup the logic, noticed by Paulo Zanoni.
> >
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> For the 3 patches:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Tested only on HSW: S3 and S4 suspend still works.

All three merged, thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch