[PATCH] drm/i915: After hibernation, discard the unbound list

[PATCH] drm/i915: After hibernation, discard the unbound list

[Chris Wilson]

The unbound list is an optimisation to track objects that have been
evicted from the GTT but remain untouched by the CPU. After hibernation,
all memory is in the CPU domain (having been restored from a disk image)
and so we need to restore the domain tracking upon the objects. However,
for the unbound list we can simply discard those objects and lazily wait
for them to be reused.

v2: Perform the unbound discard explicitly during thawing after
hibernation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9069e71..f5ccb3d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -592,6 +592,30 @@ static int __i915_drm_thaw(struct drm_device *dev)
 	return error;
 }
 
+static void i915_gem_discard_unbound(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_i915_gem_object *obj, *next;
+
+	/* After hibernation all memory is in the CPU domain and so we need
+	 * to restore our domain tracking. However, the unbound list is
+	 * merely an optimisation to track memory that has been evicted from
+	 * the GTT but remains in the GTT domain. After hibernation, that
+	 * is no longer the case and we can trim the unbound list.
+	 */
+	list_for_each_entry_safe(obj, next,
+				 &dev_priv->mm.unbound_list, gtt_list) {
+		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
+		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
+		if (i915_gem_object_put_pages(obj)) {
+			/* Abandon hope all ye who enter here */
+			obj->base.read_domains = I915_GEM_DOMAIN_GTT;
+			obj->base.write_domain = I915_GEM_DOMAIN_GTT;
+			/* Will be clflushed during restore_gtt_mappings */
+		}
+	}
+}
+
 static int i915_drm_thaw(struct drm_device *dev)
 {
 	int error = 0;
@@ -600,6 +624,7 @@ static int i915_drm_thaw(struct drm_device *dev)
 
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
 		mutex_lock(&dev->struct_mutex);
+		i915_gem_discard_unbound(dev);
 		i915_gem_restore_gtt_mappings(dev);
 		mutex_unlock(&dev->struct_mutex);
 	}
-- 
1.7.10.4

Re: [PATCH] drm/i915: After hibernation, discard the unbound list

[Ben Widawsky]

On Thu, Feb 07, 2013 at 10:27:47AM +0000, Chris Wilson wrote:
> The unbound list is an optimisation to track objects that have been
> evicted from the GTT but remain untouched by the CPU. After hibernation,
> all memory is in the CPU domain (having been restored from a disk image)
> and so we need to restore the domain tracking upon the objects. However,
> for the unbound list we can simply discard those objects and lazily wait
> for them to be reused.
> 
> v2: Perform the unbound discard explicitly during thawing after
> hibernation.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |   25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 9069e71..f5ccb3d 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -592,6 +592,30 @@ static int __i915_drm_thaw(struct drm_device *dev)
>  	return error;
>  }
>  
> +static void i915_gem_discard_unbound(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_i915_gem_object *obj, *next;
> +
> +	/* After hibernation all memory is in the CPU domain and so we need
> +	 * to restore our domain tracking. However, the unbound list is
> +	 * merely an optimisation to track memory that has been evicted from
> +	 * the GTT but remains in the GTT domain. After hibernation, that
> +	 * is no longer the case and we can trim the unbound list.
> +	 */
> +	list_for_each_entry_safe(obj, next,
> +				 &dev_priv->mm.unbound_list, gtt_list) {
> +		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> +		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> +		if (i915_gem_object_put_pages(obj)) {
> +			/* Abandon hope all ye who enter here */
> +			obj->base.read_domains = I915_GEM_DOMAIN_GTT;
> +			obj->base.write_domain = I915_GEM_DOMAIN_GTT;
> +			/* Will be clflushed during restore_gtt_mappings */
> +		}
> +	}
> +}
> +

I'm confused by how you're going about this. To reorder what you said,
if an object is on the unbound list the GPU is done with it, and it may
require clflushing if on a non-LLC system.

Then a hibernate comes along. Almost certainly the PM subsystem would
have to invalidate all the caches before moving the image to disk.

Assuming the above is true:
First question, why do you want to i915_gem_object_put_pages(). This
would seem to do a bunch of unnecessary stuff (primarily setting pages
as dirty when they need not be).
and second, why do you set I915_GEM_DOMAIN_GTT when put_pages fails?
Is that just a hack to get the clflush?

I think if you can convince me why we need to clflush after resume, I'd
be happy.

>  static int i915_drm_thaw(struct drm_device *dev)
>  {
>  	int error = 0;
> @@ -600,6 +624,7 @@ static int i915_drm_thaw(struct drm_device *dev)
>  
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
>  		mutex_lock(&dev->struct_mutex);
> +		i915_gem_discard_unbound(dev);
>  		i915_gem_restore_gtt_mappings(dev);
>  		mutex_unlock(&dev->struct_mutex);
>  	}
> -- 
> 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] drm/i915: After hibernation, discard the unbound list

[Chris Wilson]

On Sun, Feb 10, 2013 at 11:04:17AM -0800, Ben Widawsky wrote:
> I'm confused by how you're going about this. To reorder what you said,
> if an object is on the unbound list the GPU is done with it, and it may
> require clflushing if on a non-LLC system.

Right, the objects on the unbound list are explicitly not in the CPU
domain and so we believe we can skip the clflush next time we try and
use them on the GPU.
 
> Then a hibernate comes along. Almost certainly the PM subsystem would
> have to invalidate all the caches before moving the image to disk.

s/PM/we/ The PM subsystem just concerns itself with moving pages from
ram to disk and back.

> Assuming the above is true:
> First question, why do you want to i915_gem_object_put_pages(). This
> would seem to do a bunch of unnecessary stuff (primarily setting pages
> as dirty when they need not be).

Not unnecessary. Even after hibernation since we need to keep the VM
bookkeeping happy and working in our favour.

> and second, why do you set I915_GEM_DOMAIN_GTT when put_pages fails?
> Is that just a hack to get the clflush?

Right. So that our invariants about the unbound list hold.
 
> I think if you can convince me why we need to clflush after resume, I'd
> be happy.

Because moving pages around trashed the CPU caches and upset our domain
tracking. The clflush is then required to move the object back into the
domain we believe it to be. Otherwise we end up with the GPU reading
stale data and hanging.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915: After hibernation, discard the unbound list

[Chris Wilson]

The unbound list is an optimisation to track objects that have been
evicted from the GTT but remain untouched by the CPU. After hibernation,
all memory is in the CPU domain (having been restored from a disk image)
and so we need to restore the domain tracking upon the objects. However,
for the unbound list we can simply discard those objects and lazily wait
for them to be reused.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 368c821..5b49496 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -404,8 +404,17 @@ void i915_gem_restore_gtt_mappings(struct drm_device *dev)
 		i915_gem_gtt_bind_object(obj, obj->cache_level);
 	}
 
-	list_for_each_entry(obj, &dev_priv->mm.unbound_list, gtt_list)
-		i915_gem_clflush_object(obj);
+	/* Discard any unpinned unbound objects after they have been
+	 * polluted by the hibernation. For pinned objects, we need
+	 * to restore them back to the GTT domain.
+	 */
+	list_for_each_entry(obj, &dev_priv->mm.unbound_list, gtt_list) {
+		/* Update the domains after being touched during hibernate */
+		obj->base.read_domains = I915_GEM_DOMAIN_CPU;
+		obj->base.write_domain = I915_GEM_DOMAIN_CPU;
+		if (i915_gem_object_put_pages(obj))
+			i915_gem_object_set_to_gtt_domain(obj, true);
+	}
 
 	i915_gem_chipset_flush(dev);
 }
-- 
1.7.10.4