[PATCH] drm/i915: Restore global mappings upon resume

[PATCH] drm/i915: Restore global mappings upon resume

[Chris Wilson]

commit c6916417c8575637659686ff0e4f744babf0cb4e
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue Sep 24 09:58:00 2013 -0700

    drm/i915: Use the new vm [un]bind functions

changes the interpretation of the bind routines, and in particular we
only bind into the global GTT if we pass a flag to the routine. So upon
resume, we need to ask for whatever we thing is bound into the global
GTT (cursors, fences, scanouts) to be rebound or else glorious
corruption ensues.

Fixes: i-g-t/gem_suspend
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69834
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e053f14..022cc75 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -509,7 +509,7 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
 							   &dev_priv->gtt.base);
 		i915_gem_clflush_object(obj, obj->pin_display);
-		vma->vm->bind_vma(vma, obj->cache_level, 0);
+		vma->vm->bind_vma(vma, obj->cache_level, obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
 	}
 
 	i915_gem_chipset_flush(dev);
-- 
1.8.4.rc3

[PATCH] drm/i915: Restore global mappings upon resume

[Chris Wilson]

commit c6916417c8575637659686ff0e4f744babf0cb4e
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue Sep 24 09:58:00 2013 -0700

    drm/i915: Use the new vm [un]bind functions

changes the interpretation of the bind routines, and in particular we
only bind into the global GTT if we pass a flag to the routine. So upon
resume, we need to ask for whatever we thing is bound into the global
GTT (cursors, fences, scanouts) to be rebound or else glorious
corruption ensues.

v2: Clear the is-bound flags before attempting to rebind.

Fixes: i-g-t/gem_suspend
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69834
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e053f14..854d28f 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -509,7 +509,11 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
 							   &dev_priv->gtt.base);
 		i915_gem_clflush_object(obj, obj->pin_display);
-		vma->vm->bind_vma(vma, obj->cache_level, 0);
+
+		obj->has_global_gtt_mapping = false;
+		obj->has_aliasing_ppgtt_mapping = false;
+
+		vma->vm->bind_vma(vma, obj->cache_level, obj->has_global_gtt_mapping ? GLOBAL_BIND : 0);
 	}
 
 	i915_gem_chipset_flush(dev);
-- 
1.8.4.rc3

[PATCH] drm/i915: Restore global mappings upon resume

[Chris Wilson]

commit c6916417c8575637659686ff0e4f744babf0cb4e
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Tue Sep 24 09:58:00 2013 -0700

    drm/i915: Use the new vm [un]bind functions

changes the interpretation of the bind routines, and in particular we
only bind into the global GTT if we pass a flag to the routine. So upon
resume, we need to ask for whatever we thing is bound into the global
GTT (cursors, fences, scanouts) to be rebound or else glorious
corruption ensues.

v2.1: Clear the is-bound flags before attempting to rebind.

Fixes: i-g-t/gem_suspend
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69834
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e053f14..55a8d87 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -508,8 +508,14 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
 		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
 							   &dev_priv->gtt.base);
+		unsigned flags = obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
+
 		i915_gem_clflush_object(obj, obj->pin_display);
-		vma->vm->bind_vma(vma, obj->cache_level, 0);
+
+		obj->has_global_gtt_mapping = false;
+		obj->has_aliasing_ppgtt_mapping = false;
+
+		vma->vm->bind_vma(vma, obj->cache_level, flags);
 	}
 
 	i915_gem_chipset_flush(dev);
-- 
1.8.4.rc3

Re: [PATCH] drm/i915: Restore global mappings upon resume

[Ben Widawsky]

On Thu, Sep 26, 2013 at 09:22:45AM +0100, Chris Wilson wrote:
> commit c6916417c8575637659686ff0e4f744babf0cb4e
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue Sep 24 09:58:00 2013 -0700
> 
>     drm/i915: Use the new vm [un]bind functions
> 
> changes the interpretation of the bind routines, and in particular we
> only bind into the global GTT if we pass a flag to the routine. So upon
> resume, we need to ask for whatever we thing is bound into the global
> GTT (cursors, fences, scanouts) to be rebound or else glorious
> corruption ensues.
> 
> v2.1: Clear the is-bound flags before attempting to rebind.

Probably wouldn't hurt to add a comment on why you are clearing flags on
resume (which would seemingly want to be cleared on suspend, but for
pretty good reason cannot).

> 
> Fixes: i-g-t/gem_suspend
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69834
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e053f14..55a8d87 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -508,8 +508,14 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
>  							   &dev_priv->gtt.base);
> +		unsigned flags = obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
> +
>  		i915_gem_clflush_object(obj, obj->pin_display);
> -		vma->vm->bind_vma(vma, obj->cache_level, 0);
> +
> +		obj->has_global_gtt_mapping = false;
> +		obj->has_aliasing_ppgtt_mapping = false;

Actually, the aliasing PPGTT mapping may very well still be in tact.
Unless I'm not seeing something that you are.

> +
> +		vma->vm->bind_vma(vma, obj->cache_level, flags);
>  	}
>  
>  	i915_gem_chipset_flush(dev);
> -- 
> 1.8.4.rc3
> 

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Restore global mappings upon resume

[Chris Wilson]

On Thu, Sep 26, 2013 at 09:04:53AM -0700, Ben Widawsky wrote:
> On Thu, Sep 26, 2013 at 09:22:45AM +0100, Chris Wilson wrote:
> > +		obj->has_global_gtt_mapping = false;
> > +		obj->has_aliasing_ppgtt_mapping = false;
> 
> Actually, the aliasing PPGTT mapping may very well still be in tact.
> Unless I'm not seeing something that you are.

I was about to agree with you, but we zap the PDEs in the clear_range()
above and don't otherwise restore them, so it looks like without that
optimisation we need to clear the ppgtt flag as well.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Restore global mappings upon resume

[Ben Widawsky]

On Thu, Sep 26, 2013 at 05:16:53PM +0100, Chris Wilson wrote:
> On Thu, Sep 26, 2013 at 09:04:53AM -0700, Ben Widawsky wrote:
> > On Thu, Sep 26, 2013 at 09:22:45AM +0100, Chris Wilson wrote:
> > > +		obj->has_global_gtt_mapping = false;
> > > +		obj->has_aliasing_ppgtt_mapping = false;
> > 
> > Actually, the aliasing PPGTT mapping may very well still be in tact.
> > Unless I'm not seeing something that you are.
> 
> I was about to agree with you, but we zap the PDEs in the clear_range()
> above and don't otherwise restore them, so it looks like without that
> optimisation we need to clear the ppgtt flag as well.
> -Chris
> 

Having trouble finding it on the interwebs. Please see:
1375315222-4785-15-git-send-email-ben@bwidawsk.net

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: Restore global mappings upon resume

[Chris Wilson]

On Thu, Sep 26, 2013 at 09:38:41AM -0700, Ben Widawsky wrote:
> On Thu, Sep 26, 2013 at 05:16:53PM +0100, Chris Wilson wrote:
> > On Thu, Sep 26, 2013 at 09:04:53AM -0700, Ben Widawsky wrote:
> > > On Thu, Sep 26, 2013 at 09:22:45AM +0100, Chris Wilson wrote:
> > > > +		obj->has_global_gtt_mapping = false;
> > > > +		obj->has_aliasing_ppgtt_mapping = false;
> > > 
> > > Actually, the aliasing PPGTT mapping may very well still be in tact.
> > > Unless I'm not seeing something that you are.
> > 
> > I was about to agree with you, but we zap the PDEs in the clear_range()
> > above and don't otherwise restore them, so it looks like without that
> > optimisation we need to clear the ppgtt flag as well.
> > -Chris
> > 
> 
> Having trouble finding it on the interwebs. Please see:
> 1375315222-4785-15-git-send-email-ben@bwidawsk.net

Right, that will allow us to skip the per-object reload of the
aliasing_ppgtt PTE.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Restore global mappings upon resume

[Daniel Vetter]

On Thu, Sep 26, 2013 at 09:22:45AM +0100, Chris Wilson wrote:
> commit c6916417c8575637659686ff0e4f744babf0cb4e
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Tue Sep 24 09:58:00 2013 -0700
> 
>     drm/i915: Use the new vm [un]bind functions
> 
> changes the interpretation of the bind routines, and in particular we
> only bind into the global GTT if we pass a flag to the routine. So upon
> resume, we need to ask for whatever we thing is bound into the global
> GTT (cursors, fences, scanouts) to be rebound or else glorious
> corruption ensues.
> 
> v2.1: Clear the is-bound flags before attempting to rebind.
> 
> Fixes: i-g-t/gem_suspend
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69834
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>

Merged with the aliasing clearing dropped as discussed on irc.
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e053f14..55a8d87 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -508,8 +508,14 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
>  		struct i915_vma *vma = i915_gem_obj_to_vma(obj,
>  							   &dev_priv->gtt.base);
> +		unsigned flags = obj->has_global_gtt_mapping ? GLOBAL_BIND : 0;
> +
>  		i915_gem_clflush_object(obj, obj->pin_display);
> -		vma->vm->bind_vma(vma, obj->cache_level, 0);
> +
> +		obj->has_global_gtt_mapping = false;
> +		obj->has_aliasing_ppgtt_mapping = false;
> +
> +		vma->vm->bind_vma(vma, obj->cache_level, flags);
>  	}
>  
>  	i915_gem_chipset_flush(dev);
> -- 
> 1.8.4.rc3
> 
> _______________________________________________
> 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