All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
@ 2016-05-06 18:35 ville.syrjala
  2016-05-06 20:22 ` Chris Wilson
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: ville.syrjala @ 2016-05-06 18:35 UTC (permalink / raw)
  To: intel-gfx; +Cc: Daniel Vetter, drm-intel-fixes

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Move the intel_enable_gtt() call to happen before we touch the GTT
during resume. Right now it's done way too late. Before
commit ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
it was actually done earlier on account of also getting called from
the resume hook of the fake agp driver. With the fake agp driver
no longer getting registered we must move the call up.

The symptoms I've seen on my 830 machine include lowmem corruption,
other kinds of memory corruption, and straight up hung machine during
or just after resume. Not really sure what causes the memory corruption,
but so far I've not seen any with this fix.

I think we shouldn't really need to call this during init, but we have
been doing that so I've decided to keep the call. However moving that
call earlier could be prudent as well. Doing it right after the
intel-gtt probe seems appropriate.

Also tested this on 946gz,elk,ilk and all seemed quite happy with
this change.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: drm-intel-fixes@lists.freedesktop.org
Fixes: ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_dma.c     | 6 ++++++
 drivers/gpu/drm/i915/i915_drv.c     | 5 +++++
 drivers/gpu/drm/i915/i915_gem.c     | 3 ---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 9 +++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
 5 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index ad7abe517700..c225d92158a9 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1234,6 +1234,12 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		return ret;
 
+	ret = i915_ggtt_enable_hw(dev);
+	if (ret) {
+		DRM_ERROR("failed to enable GGTT\n");
+		goto out_ggtt;
+	}
+
 	/* WARNING: Apparently we must kick fbdev drivers before vgacon,
 	 * otherwise the vga fbdev driver falls over. */
 	ret = i915_kick_out_firmware_fb(dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9fd221c97275..3f41e8aed54f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
 static int i915_drm_resume(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	int ret;
 
 	disable_rpm_wakeref_asserts(dev_priv);
 
+	ret = i915_ggtt_enable_hw(dev);
+	if (ret)
+		DRM_ERROR("failed to re-enable GGTT\n");
+
 	intel_csr_ucode_resume(dev_priv);
 
 	mutex_lock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a88e6c9e9516..98b2a1c48f39 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4823,9 +4823,6 @@ i915_gem_init_hw(struct drm_device *dev)
 	struct intel_engine_cs *engine;
 	int ret;
 
-	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
-		return -EIO;
-
 	/* Double layer security blanket, see i915_gem_init() */
 	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index d284b17af431..10138a0ff03c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3157,6 +3157,15 @@ static void i915_gmch_remove(struct i915_address_space *vm)
 	intel_gmch_remove();
 }
 
+int
+i915_ggtt_enable_hw(struct drm_device *dev)
+{
+	if (INTEL_INFO(dev)->gen < 6 && !intel_enable_gtt())
+		return -EIO;
+
+	return 0;
+}
+
 /**
  * i915_ggtt_init_hw - Initialize GGTT hardware
  * @dev: DRM device
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 18af3af18754..6e34f2e9f080 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -516,6 +516,7 @@ i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
 		px_dma(ppgtt->base.scratch_pd);
 }
 
+int i915_ggtt_enable_hw(struct drm_device *dev);
 int i915_ggtt_init_hw(struct drm_device *dev);
 void i915_gem_init_ggtt(struct drm_device *dev);
 void i915_ggtt_cleanup_hw(struct drm_device *dev);
-- 
2.7.4

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

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-06 18:35 [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms ville.syrjala
@ 2016-05-06 20:22 ` Chris Wilson
  2016-05-07 18:18   ` Ville Syrjälä
  2016-05-09 13:52 ` ✓ Fi.CI.BAT: success for " Patchwork
  2016-05-10  9:14 ` [PATCH] " Chris Wilson
  2 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2016-05-06 20:22 UTC (permalink / raw)
  To: ville.syrjala; +Cc: Daniel Vetter, intel-gfx, drm-intel-fixes

On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> @@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
>  static int i915_drm_resume(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> +	int ret;
>  
>  	disable_rpm_wakeref_asserts(dev_priv);
>  
> +	ret = i915_ggtt_enable_hw(dev);
> +	if (ret)
> +		DRM_ERROR("failed to re-enable GGTT\n");

Would it not be fatal for resume as well? Failure means we can't use the
GGTT, so all subsequent writes will be going into a random address.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-06 20:22 ` Chris Wilson
@ 2016-05-07 18:18   ` Ville Syrjälä
  2016-05-08 13:09     ` Chris Wilson
  2016-05-20 13:06     ` David Weinehall
  0 siblings, 2 replies; 9+ messages in thread
From: Ville Syrjälä @ 2016-05-07 18:18 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx, Daniel Vetter, drm-intel-fixes

On Fri, May 06, 2016 at 09:22:49PM +0100, Chris Wilson wrote:
> On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > @@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
> >  static int i915_drm_resume(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int ret;
> >  
> >  	disable_rpm_wakeref_asserts(dev_priv);
> >  
> > +	ret = i915_ggtt_enable_hw(dev);
> > +	if (ret)
> > +		DRM_ERROR("failed to re-enable GGTT\n");
> 
> Would it not be fatal for resume as well? Failure means we can't use the
> GGTT, so all subsequent writes will be going into a random address.

Yeah, I assume things would blow up. The question is however, what can
we do in this case? We'd basically have to shut the entire driver down.
I don't think we have a way to do that?

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-07 18:18   ` Ville Syrjälä
@ 2016-05-08 13:09     ` Chris Wilson
  2016-05-20 13:06     ` David Weinehall
  1 sibling, 0 replies; 9+ messages in thread
From: Chris Wilson @ 2016-05-08 13:09 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, drm-intel-fixes

On Sat, May 07, 2016 at 09:18:24PM +0300, Ville Syrjälä wrote:
> On Fri, May 06, 2016 at 09:22:49PM +0100, Chris Wilson wrote:
> > On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > > @@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
> > >  static int i915_drm_resume(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int ret;
> > >  
> > >  	disable_rpm_wakeref_asserts(dev_priv);
> > >  
> > > +	ret = i915_ggtt_enable_hw(dev);
> > > +	if (ret)
> > > +		DRM_ERROR("failed to re-enable GGTT\n");
> > 
> > Would it not be fatal for resume as well? Failure means we can't use the
> > GGTT, so all subsequent writes will be going into a random address.
> 
> Yeah, I assume things would blow up. The question is however, what can
> we do in this case? We'd basically have to shut the entire driver down.
> I don't think we have a way to do that?

No, I was naively thinking that erring out during resume would magically
shut the driver down.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-06 18:35 [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms ville.syrjala
  2016-05-06 20:22 ` Chris Wilson
@ 2016-05-09 13:52 ` Patchwork
  2016-05-10  9:14 ` [PATCH] " Chris Wilson
  2 siblings, 0 replies; 9+ messages in thread
From: Patchwork @ 2016-05-09 13:52 UTC (permalink / raw)
  To: ville.syrjala; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
URL   : https://patchwork.freedesktop.org/series/6845/
State : success

== Summary ==

Series 6845v1 drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
http://patchwork.freedesktop.org/api/1.0/series/6845/revisions/1/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                incomplete -> PASS       (snb-dellxps)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-b:
                skip       -> PASS       (skl-nuci5)
        Subgroup suspend-read-crc-pipe-c:
                skip       -> PASS       (hsw-brixbox)

bdw-nuci7-2      total:219  pass:206  dwarn:0   dfail:0   fail:0   skip:13 
bdw-ultra        total:219  pass:193  dwarn:0   dfail:0   fail:0   skip:26 
bsw-nuc-2        total:218  pass:174  dwarn:0   dfail:0   fail:2   skip:42 
byt-nuc          total:218  pass:174  dwarn:0   dfail:0   fail:3   skip:41 
hsw-brixbox      total:219  pass:193  dwarn:0   dfail:0   fail:0   skip:26 
hsw-gt2          total:219  pass:197  dwarn:0   dfail:0   fail:1   skip:21 
ilk-hp8440p      total:219  pass:155  dwarn:0   dfail:0   fail:1   skip:63 
ivb-t430s        total:219  pass:188  dwarn:0   dfail:0   fail:0   skip:31 
skl-i7k-2        total:219  pass:191  dwarn:0   dfail:0   fail:0   skip:28 
skl-nuci5        total:219  pass:207  dwarn:0   dfail:0   fail:0   skip:12 
snb-dellxps      total:195  pass:159  dwarn:0   dfail:0   fail:0   skip:35 
snb-x220t        total:219  pass:176  dwarn:0   dfail:0   fail:1   skip:42 

Results at /archive/results/CI_IGT_test/Patchwork_2153/

bcc6a843e7e4a3f4794b90dbefb00174171365bd drm-intel-nightly: 2016y-05m-09d-12h-47m-46s UTC integration manifest
eee5a86 drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms

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

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-06 18:35 [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms ville.syrjala
  2016-05-06 20:22 ` Chris Wilson
  2016-05-09 13:52 ` ✓ Fi.CI.BAT: success for " Patchwork
@ 2016-05-10  9:14 ` Chris Wilson
  2016-05-10  9:39   ` Ville Syrjälä
  2 siblings, 1 reply; 9+ messages in thread
From: Chris Wilson @ 2016-05-10  9:14 UTC (permalink / raw)
  To: ville.syrjala; +Cc: Daniel Vetter, intel-gfx, drm-intel-fixes

On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Move the intel_enable_gtt() call to happen before we touch the GTT
> during resume. Right now it's done way too late. Before
> commit ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
> it was actually done earlier on account of also getting called from
> the resume hook of the fake agp driver. With the fake agp driver
> no longer getting registered we must move the call up.
> 
> The symptoms I've seen on my 830 machine include lowmem corruption,
> other kinds of memory corruption, and straight up hung machine during
> or just after resume. Not really sure what causes the memory corruption,
> but so far I've not seen any with this fix.
> 
> I think we shouldn't really need to call this during init, but we have
> been doing that so I've decided to keep the call. However moving that
> call earlier could be prudent as well. Doing it right after the
> intel-gtt probe seems appropriate.

So why not in i915_ggtt_init_hw()? Because you want to keep the current
semantics of doing it everytime.
 
> Also tested this on 946gz,elk,ilk and all seemed quite happy with
> this change.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: drm-intel-fixes@lists.freedesktop.org
> Fixes: ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 18af3af18754..6e34f2e9f080 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -516,6 +516,7 @@ i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
>  		px_dma(ppgtt->base.scratch_pd);
>  }
>  
> +int i915_ggtt_enable_hw(struct drm_device *dev);
>  int i915_ggtt_init_hw(struct drm_device *dev);

Minor nit, probably best to put it after init_hw, since we do init first
then enable.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-10  9:14 ` [PATCH] " Chris Wilson
@ 2016-05-10  9:39   ` Ville Syrjälä
  0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2016-05-10  9:39 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx, Daniel Vetter, drm-intel-fixes

On Tue, May 10, 2016 at 10:14:19AM +0100, Chris Wilson wrote:
> On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Move the intel_enable_gtt() call to happen before we touch the GTT
> > during resume. Right now it's done way too late. Before
> > commit ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
> > it was actually done earlier on account of also getting called from
> > the resume hook of the fake agp driver. With the fake agp driver
> > no longer getting registered we must move the call up.
> > 
> > The symptoms I've seen on my 830 machine include lowmem corruption,
> > other kinds of memory corruption, and straight up hung machine during
> > or just after resume. Not really sure what causes the memory corruption,
> > but so far I've not seen any with this fix.
> > 
> > I think we shouldn't really need to call this during init, but we have
> > been doing that so I've decided to keep the call. However moving that
> > call earlier could be prudent as well. Doing it right after the
> > intel-gtt probe seems appropriate.
> 
> So why not in i915_ggtt_init_hw()? Because you want to keep the current
> semantics of doing it everytime.
>  
> > Also tested this on 946gz,elk,ilk and all seemed quite happy with
> > this change.
> > 
> > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Cc: drm-intel-fixes@lists.freedesktop.org
> > Fixes: ebb7c78d358b ("agp/intel-gtt: Only register fake agp driver for gen1")
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > index 18af3af18754..6e34f2e9f080 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> > @@ -516,6 +516,7 @@ i915_page_dir_dma_addr(const struct i915_hw_ppgtt *ppgtt, const unsigned n)
> >  		px_dma(ppgtt->base.scratch_pd);
> >  }
> >  
> > +int i915_ggtt_enable_hw(struct drm_device *dev);
> >  int i915_ggtt_init_hw(struct drm_device *dev);
> 
> Minor nit, probably best to put it after init_hw, since we do init first
> then enable.

Pushed to dinq, with a fresh coat of paint. Thanks for the review.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-07 18:18   ` Ville Syrjälä
  2016-05-08 13:09     ` Chris Wilson
@ 2016-05-20 13:06     ` David Weinehall
  2016-05-23 17:10       ` Ville Syrjälä
  1 sibling, 1 reply; 9+ messages in thread
From: David Weinehall @ 2016-05-20 13:06 UTC (permalink / raw)
  To: Ville Syrjälä; +Cc: Daniel Vetter, intel-gfx, drm-intel-fixes

On Sat, May 07, 2016 at 09:18:24PM +0300, Ville Syrjälä wrote:
> On Fri, May 06, 2016 at 09:22:49PM +0100, Chris Wilson wrote:
> > On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > > @@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
> > >  static int i915_drm_resume(struct drm_device *dev)
> > >  {
> > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > +	int ret;
> > >  
> > >  	disable_rpm_wakeref_asserts(dev_priv);
> > >  
> > > +	ret = i915_ggtt_enable_hw(dev);
> > > +	if (ret)
> > > +		DRM_ERROR("failed to re-enable GGTT\n");
> > 
> > Would it not be fatal for resume as well? Failure means we can't use the
> > GGTT, so all subsequent writes will be going into a random address.
> 
> Yeah, I assume things would blow up. The question is however, what can
> we do in this case? We'd basically have to shut the entire driver down.
> I don't think we have a way to do that?

panic() seems like the answer here.  If we risk scribbling into
random memory we should make sure that we just drop everything.


Kind regards, David
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms
  2016-05-20 13:06     ` David Weinehall
@ 2016-05-23 17:10       ` Ville Syrjälä
  0 siblings, 0 replies; 9+ messages in thread
From: Ville Syrjälä @ 2016-05-23 17:10 UTC (permalink / raw)
  To: Chris Wilson, intel-gfx, Daniel Vetter, drm-intel-fixes

On Fri, May 20, 2016 at 04:06:17PM +0300, David Weinehall wrote:
> On Sat, May 07, 2016 at 09:18:24PM +0300, Ville Syrjälä wrote:
> > On Fri, May 06, 2016 at 09:22:49PM +0100, Chris Wilson wrote:
> > > On Fri, May 06, 2016 at 09:35:55PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > @@ -730,9 +730,14 @@ int i915_suspend_switcheroo(struct drm_device *dev, pm_message_t state)
> > > >  static int i915_drm_resume(struct drm_device *dev)
> > > >  {
> > > >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > > > +	int ret;
> > > >  
> > > >  	disable_rpm_wakeref_asserts(dev_priv);
> > > >  
> > > > +	ret = i915_ggtt_enable_hw(dev);
> > > > +	if (ret)
> > > > +		DRM_ERROR("failed to re-enable GGTT\n");
> > > 
> > > Would it not be fatal for resume as well? Failure means we can't use the
> > > GGTT, so all subsequent writes will be going into a random address.
> > 
> > Yeah, I assume things would blow up. The question is however, what can
> > we do in this case? We'd basically have to shut the entire driver down.
> > I don't think we have a way to do that?
> 
> panic() seems like the answer here.  If we risk scribbling into
> random memory we should make sure that we just drop everything.

Yeah, maybe. OTOH I really hate it when resume gives you hung machine, a
bit of memory corruption might be preferable. I wonder if we could
force a remount-ro for all disks to at least avoid scribbling over
anything permanently. But this might all be entirely theoretical, and
this will never happen. The fake agp code just ignored the return value
and peopled seemed happy.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-05-23 17:10 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-06 18:35 [PATCH] drm/i915: Re-enable GGTT earlier during resume on pre-gen6 platforms ville.syrjala
2016-05-06 20:22 ` Chris Wilson
2016-05-07 18:18   ` Ville Syrjälä
2016-05-08 13:09     ` Chris Wilson
2016-05-20 13:06     ` David Weinehall
2016-05-23 17:10       ` Ville Syrjälä
2016-05-09 13:52 ` ✓ Fi.CI.BAT: success for " Patchwork
2016-05-10  9:14 ` [PATCH] " Chris Wilson
2016-05-10  9:39   ` Ville Syrjälä

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.