[PATCH] Revert "drm/i915: Allocate context objects from stolen"

[PATCH] Revert "drm/i915: Allocate context objects from stolen"

[ville.syrjala]

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

Stolen gets trashed during hibernation, so storing contexts there
is not a very good idea. On my IVB machines this leads to a totally
dead GPU on resume. A reboot is required to resurrect it. So let's
not store contexts where they will get trampled.

This reverts commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_context.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index a7e58a8..5c7ccf4 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -157,9 +157,7 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 	struct drm_i915_gem_object *obj;
 	int ret;
 
-	obj = i915_gem_object_create_stolen(dev, size);
-	if (obj == NULL)
-		obj = i915_gem_alloc_object(dev, size);
+	obj = i915_gem_alloc_object(dev, size);
 	if (obj == NULL)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.3.6

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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Chris Wilson]

On Mon, Jun 29, 2015 at 08:28:35PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Stolen gets trashed during hibernation, so storing contexts there
> is not a very good idea. On my IVB machines this leads to a totally
> dead GPU on resume. A reboot is required to resurrect it. So let's
> not store contexts where they will get trampled.

We need to disable use of stolen entirely then. Fortunately ring buffers
are idle (and so trashable), but all plans for actually using stolen are
off the cards then (e.g. the create2 request to allocate from stolen).

I think it is fairer to say that stolen memory is not restored, and to
do so for hibernation of non-volatile objects would require allocation
of ordinary buffers. Ack on the patch in the short term, does a long
term plan of migrating I915_MADV_WILLNEED stolen objects to normal
memory on hibernation sound acceptable? (I don't like how it makes
hibernation special in terms of shutdown/suspend, but meh)

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

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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Chris Wilson]

On Mon, Jun 29, 2015 at 08:53:12PM +0100, Chris Wilson wrote:
> On Mon, Jun 29, 2015 at 08:28:35PM +0300, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Stolen gets trashed during hibernation, so storing contexts there
> > is not a very good idea. On my IVB machines this leads to a totally
> > dead GPU on resume. A reboot is required to resurrect it. So let's
> > not store contexts where they will get trampled.
> 
> We need to disable use of stolen entirely then. Fortunately ring buffers
> are idle (and so trashable), but all plans for actually using stolen are
> off the cards then (e.g. the create2 request to allocate from stolen).
> 
> I think it is fairer to say that stolen memory is not restored, and to
> do so for hibernation of non-volatile objects would require allocation
> of ordinary buffers. Ack on the patch in the short term, does a long
> term plan of migrating I915_MADV_WILLNEED stolen objects to normal
> memory on hibernation sound acceptable? (I don't like how it makes
> hibernation special in terms of shutdown/suspend, but meh)

I can get a patch to do the migration ready tomorrow, though it will
certainly be quite a few interesting patches.

What's the timeframe for a fix/revert here? (i.e. did this get merged
for 4.2 or was it 4.3?)
-Chris

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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Akash Goel]

On Mon, 2015-06-29 at 21:05 +0100, Chris Wilson wrote:
> On Mon, Jun 29, 2015 at 08:53:12PM +0100, Chris Wilson wrote:
> > On Mon, Jun 29, 2015 at 08:28:35PM +0300, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > Stolen gets trashed during hibernation, so storing contexts there
> > > is not a very good idea. On my IVB machines this leads to a totally
> > > dead GPU on resume. A reboot is required to resurrect it. So let's
> > > not store contexts where they will get trampled.
> > 
> > We need to disable use of stolen entirely then. Fortunately ring buffers
> > are idle (and so trashable), but all plans for actually using stolen are
> > off the cards then (e.g. the create2 request to allocate from stolen).
> > 
> > I think it is fairer to say that stolen memory is not restored, and to
> > do so for hibernation of non-volatile objects would require allocation
> > of ordinary buffers. Ack on the patch in the short term, does a long
> > term plan of migrating I915_MADV_WILLNEED stolen objects to normal
> > memory on hibernation sound acceptable? (I don't like how it makes
> > hibernation special in terms of shutdown/suspend, but meh)
> 
> I can get a patch to do the migration ready tomorrow, though it will
> certainly be quite a few interesting patches.
> 
Hi Chris,

Thanks for keeping us in the loop. 
As you are aware, that we started working on the patches for utilizing
the Stolen area, based on the placement preference flag passed by the
User (extended gem_create ioctl for it). We are trying to get them
reviewed. Would this use case of hibernation thwart that efforts ?

Are you planning to add the support for taking the back up of the
objects allocated from Stolen memory area, by copying them to shmem at
the time of hibernation. Should the objects be copied back to Stolen
area on resume ? 
Is this something, which we can implement and add a new patch to the
series ? 

Best regards
Akash

> What's the timeframe for a fix/revert here? (i.e. did this get merged
> for 4.2 or was it 4.3?)
> -Chris


> 


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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Chris Wilson]

On Tue, Jun 30, 2015 at 12:07:06PM +0530, Akash Goel wrote:
> On Mon, 2015-06-29 at 21:05 +0100, Chris Wilson wrote:
> > On Mon, Jun 29, 2015 at 08:53:12PM +0100, Chris Wilson wrote:
> > > On Mon, Jun 29, 2015 at 08:28:35PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > Stolen gets trashed during hibernation, so storing contexts there
> > > > is not a very good idea. On my IVB machines this leads to a totally
> > > > dead GPU on resume. A reboot is required to resurrect it. So let's
> > > > not store contexts where they will get trampled.
> > > 
> > > We need to disable use of stolen entirely then. Fortunately ring buffers
> > > are idle (and so trashable), but all plans for actually using stolen are
> > > off the cards then (e.g. the create2 request to allocate from stolen).
> > > 
> > > I think it is fairer to say that stolen memory is not restored, and to
> > > do so for hibernation of non-volatile objects would require allocation
> > > of ordinary buffers. Ack on the patch in the short term, does a long
> > > term plan of migrating I915_MADV_WILLNEED stolen objects to normal
> > > memory on hibernation sound acceptable? (I don't like how it makes
> > > hibernation special in terms of shutdown/suspend, but meh)
> > 
> > I can get a patch to do the migration ready tomorrow, though it will
> > certainly be quite a few interesting patches.
> > 
> Hi Chris,
> 
> Thanks for keeping us in the loop. 
> As you are aware, that we started working on the patches for utilizing
> the Stolen area, based on the placement preference flag passed by the
> User (extended gem_create ioctl for it). We are trying to get them
> reviewed. Would this use case of hibernation thwart that efforts ?

Yes. Since if we are not careful we can effectively loose the user data
across hibernation. So long as we can allocate fresh storage for those
objects during hibernation (and we can use the madv flags to limit what
we need to copy) then creating stolen objects for general use is fine.
I'll leave the debate as to whether we want to migrate such objects back
to stolen on thaw/first-use for later. (Gut feeling hibernate + stolen
will be so rare that the justification for migrating back will be
small.)
 
> Are you planning to add the support for taking the back up of the
> objects allocated from Stolen memory area, by copying them to shmem at
> the time of hibernation. Should the objects be copied back to Stolen
> area on resume ? 

Yes. Maybe.

> Is this something, which we can implement and add a new patch to the
> series ? 

It's something I need now as an alternative to reverting
context-in-stolen.
-Chris

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

[PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

Ville reminded us that stolen memory is not preserved across
hibernation, and a result of this was that context objects now being
allocated from stolen were being corrupted on S4 and promptly hanging
the GPU on resume.

We want to utilise stolen for as much as possible (nothing else will use
that wasted memory otherwise), so we need a strategy for handling
general objects allocated from stolen and hibernation. A simple solution
is to do a CPU copy through the GTT of the stolen object into a fresh
shmemfs backing store and thenceforth treat it as a normal objects. This
can be refined in future to either use a GPU copy to avoid the slow
uncached reads (though it's hibernation!) and recreate stolen objects
upon resume/first-use. For now, a simple approach should suffice for
testing the object migration.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
Cc: Akash Goel <akash.goel@intel.com>
Cc: Deepak S <deepak.s@linux.intel.com>
Cc: Imre Deak <imre.deak@linux.intel.com>
---
This uses a few constructs that are not upstream yet, but it should be
enough for sanity checking that my conversion of a stolen object to a 
shmemfs object is sane.
---
 drivers/gpu/drm/i915/i915_drv.c         |  17 +++-
 drivers/gpu/drm/i915/i915_drv.h         |   5 +
 drivers/gpu/drm/i915/i915_gem.c         | 156 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_gem_context.c |   3 +
 drivers/gpu/drm/i915/i915_gem_stolen.c  |   9 ++
 drivers/gpu/drm/i915/intel_overlay.c    |   3 +
 6 files changed, 180 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index c45d5722e987..3ece56a98827 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -972,6 +972,21 @@ static int i915_pm_suspend(struct device *dev)
 	return i915_drm_suspend(drm_dev);
 }
 
+static int i915_pm_freeze(struct device *dev)
+{
+	int ret;
+
+	ret = i915_gem_freeze(pci_get_drvdata(to_pci_dev(dev)));
+	if (ret)
+		return ret;
+
+	ret = i915_pm_suspend(dev);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int i915_pm_suspend_late(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
@@ -1618,7 +1633,7 @@ static const struct dev_pm_ops i915_pm_ops = {
 	 * @restore, @restore_early : called after rebooting and restoring the
 	 *                            hibernation image [PMSG_RESTORE]
 	 */
-	.freeze = i915_pm_suspend,
+	.freeze = i915_pm_freeze,
 	.freeze_late = i915_pm_suspend_late,
 	.thaw_early = i915_pm_resume_early,
 	.thaw = i915_pm_resume,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ea7881367084..ec10f389886e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1258,6 +1258,9 @@ struct intel_l3_parity {
 struct i915_gem_mm {
 	/** Memory allocator for GTT stolen memory */
 	struct drm_mm stolen;
+	/** List of all objects allocated from stolen */
+	struct list_head stolen_list;
+
 	/** List of all objects in gtt_space. Used to restore gtt
 	 * mappings on resume */
 	struct list_head bound_list;
@@ -2024,6 +2027,7 @@ struct drm_i915_gem_object {
 	struct drm_mm_node *stolen;
 	struct list_head global_list;
 
+	struct list_head stolen_link;
 	struct list_head batch_pool_link;
 	struct list_head tmp_link;
 
@@ -3003,6 +3007,7 @@ int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
 void i915_gem_init_swizzling(struct drm_device *dev);
 void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
 int __must_check i915_gpu_idle(struct drm_device *dev);
+int __must_check i915_gem_freeze(struct drm_device *dev);
 int __must_check i915_gem_suspend(struct drm_device *dev);
 void __i915_add_request(struct drm_i915_gem_request *req,
 			struct i915_vma *batch,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 477cd1cb7679..c501e20b7ed0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4580,12 +4580,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.put_pages = i915_gem_object_put_pages_gtt,
 };
 
+static struct address_space *
+i915_gem_set_inode_gfp(struct drm_device *dev, struct file *file)
+{
+	struct address_space *mapping = file_inode(file)->i_mapping;
+	gfp_t mask;
+
+	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
+		/* 965gm cannot relocate objects above 4GiB. */
+		mask &= ~__GFP_HIGHMEM;
+		mask |= __GFP_DMA32;
+	}
+	mapping_set_gfp_mask(mapping, mask);
+
+	return mapping;
+}
+
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
 	struct drm_i915_gem_object *obj;
-	struct address_space *mapping;
-	gfp_t mask;
 	int ret;
 
 	obj = i915_gem_object_alloc(dev);
@@ -4597,16 +4612,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		i915_gem_object_free(obj);
 		return ERR_PTR(ret);
 	}
-
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
-	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
-		/* 965gm cannot relocate objects above 4GiB. */
-		mask &= ~__GFP_HIGHMEM;
-		mask |= __GFP_DMA32;
-	}
-
-	mapping = file_inode(obj->base.filp)->i_mapping;
-	mapping_set_gfp_mask(mapping, mask);
+	i915_gem_set_inode_gfp(dev, obj->base.filp);
 
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
@@ -4738,6 +4744,132 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
 		dev_priv->gt.stop_ring(ring);
 }
 
+static int
+i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *i915 = to_i915(obj->base.dev);
+	struct i915_vma *vma, *vn;
+	struct drm_mm_node node;
+	struct file *file;
+	struct address_space *mapping;
+	int ret, i;
+
+	if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
+		return -EINVAL;
+
+	list_for_each_entry_safe(vma, vn, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			break;
+
+	ret = i915_gem_object_pin_pages(obj);
+	if (ret)
+		return ret;
+
+	memset(&node, 0, sizeof(node));
+	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
+						  &node,
+						  4096, 0, I915_CACHE_NONE,
+						  0, i915->gtt.mappable_end,
+						  DRM_MM_SEARCH_DEFAULT,
+						  DRM_MM_CREATE_DEFAULT);
+	if (ret)
+		goto err_unpin;
+
+	file = shmem_file_setup("drm mm object", obj->base.size, VM_NORESERVE);
+	if (IS_ERR(file)) {
+		ret = PTR_ERR(file);
+		goto err_unpin;
+	}
+
+	mapping = i915_gem_set_inode_gfp(obj->base.dev, file);
+	for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
+		struct page *page;
+		void *dst, *src;
+
+		wmb();
+		i915->gtt.base.insert_page(&i915->gtt.base,
+					   i915_gem_object_get_dma_address(obj, i),
+					   node.start,
+					   I915_CACHE_NONE,
+					   0);
+		wmb();
+
+		page = shmem_read_mapping_page(mapping, i);
+		if (IS_ERR(page)) {
+			ret = PTR_ERR(page);
+			goto err_file;
+		}
+
+		src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start);
+		dst = kmap_atomic(page);
+		memcpy(dst, src, PAGE_SIZE);
+		kunmap_atomic(dst);
+		io_mapping_unmap_atomic(src);
+
+		page_cache_release(page);
+	}
+
+	wmb();
+	i915->gtt.base.clear_range(&i915->gtt.base,
+				   node.start, node.size,
+				   true);
+	drm_mm_remove_node(&node);
+
+	i915_gem_object_unpin_pages(obj);
+	ret = i915_gem_object_put_pages(obj);
+	if (ret) {
+		fput(file);
+		return ret;
+	}
+
+	if (obj->ops->release)
+		obj->ops->release(obj);
+
+	obj->base.filp = file;
+	obj->ops = &i915_gem_object_ops;
+
+	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
+	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
+
+	return 0;
+
+err_file:
+	fput(file);
+err_unpin:
+	i915_gem_object_unpin_pages(obj);
+	return ret;
+}
+
+int
+i915_gem_freeze(struct drm_device *dev)
+{
+	/* Called before freeze when hibernating */
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct drm_i915_gem_object *obj, *tmp;
+	int ret;
+
+	/* Across hibernation, the stolen area is not preserved.
+	 * Anything inside stolen must copied back to normal
+	 * memory if we wish to preserve it.
+	 */
+
+	list_for_each_entry_safe(obj, tmp, &i915->mm.stolen_list, stolen_link) {
+		if (obj->madv != I915_MADV_WILLNEED) {
+			/* XXX any point in setting this? The objects for which
+			 * we preserve never unset it afterwards.
+			 */
+			obj->madv = __I915_MADV_PURGED;
+			continue;
+		}
+
+		ret = i915_gem_object_migrate_stolen_to_shmemfs(obj);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 int
 i915_gem_suspend(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 03fa6e87f5ac..01e8fe7ae632 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -244,6 +244,9 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
 	if (IS_ERR_OR_NULL(obj))
 		return ERR_PTR(-ENOMEM);
 
+	/* contexts need to always be preserved across hibernation/swap-out */
+	obj->madv = I915_MADV_WILLNEED;
+
 	/*
 	 * Try to make the context utilize L3 as well as LLC.
 	 *
diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
index 21ee83bf6307..86e84554e8ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_stolen.c
+++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
@@ -331,6 +331,7 @@ int i915_gem_init_stolen(struct drm_device *dev)
 	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
 		    bios_reserved);
 
+	INIT_LIST_HEAD(&dev_priv->mm.stolen_list);
 	return 0;
 }
 
@@ -387,6 +388,7 @@ static void
 i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
 {
 	if (obj->stolen) {
+		list_del(&obj->stolen_link);
 		drm_mm_remove_node(obj->stolen);
 		kfree(obj->stolen);
 		obj->stolen = NULL;
@@ -419,6 +421,13 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
 	__i915_gem_object_pin_pages(obj);
 	obj->has_dma_mapping = true;
 	obj->stolen = stolen;
+	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
+
+	/* By default, treat the contexts of stolen as volatile. If the object
+	 * must be saved across hibernation, then the caller must take
+	 * action and flag it as WILLNEED.
+	 */
+	obj->madv = I915_MADV_DONTNEED;
 
 	obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
 	obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index cebe857f6df2..ec3ab351e9ff 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -1401,6 +1401,9 @@ void intel_setup_overlay(struct drm_device *dev)
 		goto out_free;
 	overlay->reg_bo = reg_bo;
 
+	/* XXX preserve register values across hibernation */
+	reg_bo->madv = I915_MADV_WILLNEED;
+
 	if (OVERLAY_NEEDS_PHYSICAL(dev)) {
 		ret = i915_gem_object_attach_phys(reg_bo, PAGE_SIZE);
 		if (ret) {
-- 
2.1.4

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:58:08AM +0100, Chris Wilson wrote:
> +static int
> +i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct i915_vma *vma, *vn;
> +	struct drm_mm_node node;
> +	struct file *file;
> +	struct address_space *mapping;
> +	int ret, i;
> +
> +	if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
> +		return -EINVAL;

Missed:

ret = i915_gem_object_set_to_gtt_domain(obj, false);
if (ret)
	return ret;

The unbinding would have been enough to serialise with the GPU, but we
may as well be explicit in our domain management.
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 10:58:08AM +0100, Chris Wilson wrote:
> +int
> +i915_gem_freeze(struct drm_device *dev)
> +{
> +	/* Called before freeze when hibernating */
> +	struct drm_i915_private *i915 = to_i915(dev);
> +	struct drm_i915_gem_object *obj, *tmp;
> +	int ret;
> +
> +	/* Across hibernation, the stolen area is not preserved.
> +	 * Anything inside stolen must copied back to normal
> +	 * memory if we wish to preserve it.
> +	 */

And I forgot to add the double pass + reference/unreference dance here

> +	list_for_each_entry_safe(obj, tmp, &i915->mm.stolen_list, stolen_link) {
> +		if (obj->madv != I915_MADV_WILLNEED) {
> +			/* XXX any point in setting this? The objects for which
> +			 * we preserve never unset it afterwards.
> +			 */
> +			obj->madv = __I915_MADV_PURGED;
> +			continue;
> +		}
> +
> +		ret = i915_gem_object_migrate_stolen_to_shmemfs(obj);
> +		if (ret)
> +			return ret;
> +	}

should be

+	INIT_LIST_HEAD(&migrate);
+       list_for_each_entry_safe(obj, tmp, &i915->mm.stolen_list, stolen_link) {
+               if (obj->madv != I915_MADV_WILLNEED)
+                       continue;
+
+               /* In the general case, this object may only be alive due to
+                * an active reference, and that may disappear when we unbind
+                * any of the objects (and so wait upon the GPU and retire
+                * requests). To prevent one of the objects from disappearing
+                * beneath us, we need to take a reference to each as we
+                * build the migration list.
+                *
+                * This is similar to the strategy required whilst shrinking
+                * or evicting objects (for the same reason).
+                */
+               drm_gem_object_reference(&obj->base);
+               list_move(&obj->stolen_link, &migrate);
+       }
+
+       ret = 0;
+       list_for_each_entry_safe(obj, tmp, &migrate, stolen_link) {
+               if (ret == 0)
+                       ret = i915_gem_object_migrate_stolen_to_shmemfs(obj);
+               drm_gem_object_unreference(&obj->base);
+       }
+       list_splice(&migrate, &i915->mm.stolen_list);
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 10:58:08AM +0100, Chris Wilson wrote:
> Ville reminded us that stolen memory is not preserved across
> hibernation, and a result of this was that context objects now being
> allocated from stolen were being corrupted on S4 and promptly hanging
> the GPU on resume.
> 
> We want to utilise stolen for as much as possible (nothing else will use
> that wasted memory otherwise), so we need a strategy for handling
> general objects allocated from stolen and hibernation. A simple solution
> is to do a CPU copy through the GTT of the stolen object into a fresh
> shmemfs backing store and thenceforth treat it as a normal objects. This
> can be refined in future to either use a GPU copy to avoid the slow
> uncached reads (though it's hibernation!) and recreate stolen objects
> upon resume/first-use. For now, a simple approach should suffice for
> testing the object migration.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Akash Goel <akash.goel@intel.com>
> Cc: Deepak S <deepak.s@linux.intel.com>
> Cc: Imre Deak <imre.deak@linux.intel.com>
> ---
> This uses a few constructs that are not upstream yet, but it should be
> enough for sanity checking that my conversion of a stolen object to a 
> shmemfs object is sane.
> ---
>  drivers/gpu/drm/i915/i915_drv.c         |  17 +++-
>  drivers/gpu/drm/i915/i915_drv.h         |   5 +
>  drivers/gpu/drm/i915/i915_gem.c         | 156 +++++++++++++++++++++++++++++---
>  drivers/gpu/drm/i915/i915_gem_context.c |   3 +
>  drivers/gpu/drm/i915/i915_gem_stolen.c  |   9 ++
>  drivers/gpu/drm/i915/intel_overlay.c    |   3 +
>  6 files changed, 180 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index c45d5722e987..3ece56a98827 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -972,6 +972,21 @@ static int i915_pm_suspend(struct device *dev)
>  	return i915_drm_suspend(drm_dev);
>  }
>  
> +static int i915_pm_freeze(struct device *dev)
> +{
> +	int ret;
> +
> +	ret = i915_gem_freeze(pci_get_drvdata(to_pci_dev(dev)));
> +	if (ret)
> +		return ret;
> +
> +	ret = i915_pm_suspend(dev);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int i915_pm_suspend_late(struct device *dev)
>  {
>  	struct drm_device *drm_dev = dev_to_i915(dev)->dev;
> @@ -1618,7 +1633,7 @@ static const struct dev_pm_ops i915_pm_ops = {
>  	 * @restore, @restore_early : called after rebooting and restoring the
>  	 *                            hibernation image [PMSG_RESTORE]
>  	 */
> -	.freeze = i915_pm_suspend,
> +	.freeze = i915_pm_freeze,
>  	.freeze_late = i915_pm_suspend_late,
>  	.thaw_early = i915_pm_resume_early,
>  	.thaw = i915_pm_resume,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ea7881367084..ec10f389886e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1258,6 +1258,9 @@ struct intel_l3_parity {
>  struct i915_gem_mm {
>  	/** Memory allocator for GTT stolen memory */
>  	struct drm_mm stolen;
> +	/** List of all objects allocated from stolen */
> +	struct list_head stolen_list;
> +
>  	/** List of all objects in gtt_space. Used to restore gtt
>  	 * mappings on resume */
>  	struct list_head bound_list;
> @@ -2024,6 +2027,7 @@ struct drm_i915_gem_object {
>  	struct drm_mm_node *stolen;
>  	struct list_head global_list;
>  
> +	struct list_head stolen_link;
>  	struct list_head batch_pool_link;
>  	struct list_head tmp_link;
>  
> @@ -3003,6 +3007,7 @@ int i915_gem_l3_remap(struct drm_i915_gem_request *req, int slice);
>  void i915_gem_init_swizzling(struct drm_device *dev);
>  void i915_gem_cleanup_ringbuffer(struct drm_device *dev);
>  int __must_check i915_gpu_idle(struct drm_device *dev);
> +int __must_check i915_gem_freeze(struct drm_device *dev);
>  int __must_check i915_gem_suspend(struct drm_device *dev);
>  void __i915_add_request(struct drm_i915_gem_request *req,
>  			struct i915_vma *batch,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 477cd1cb7679..c501e20b7ed0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4580,12 +4580,27 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
>  	.put_pages = i915_gem_object_put_pages_gtt,
>  };
>  
> +static struct address_space *
> +i915_gem_set_inode_gfp(struct drm_device *dev, struct file *file)
> +{
> +	struct address_space *mapping = file_inode(file)->i_mapping;
> +	gfp_t mask;
> +
> +	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> +	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
> +		/* 965gm cannot relocate objects above 4GiB. */
> +		mask &= ~__GFP_HIGHMEM;
> +		mask |= __GFP_DMA32;
> +	}
> +	mapping_set_gfp_mask(mapping, mask);
> +
> +	return mapping;
> +}
> +
>  struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  						  size_t size)
>  {
>  	struct drm_i915_gem_object *obj;
> -	struct address_space *mapping;
> -	gfp_t mask;
>  	int ret;
>  
>  	obj = i915_gem_object_alloc(dev);
> @@ -4597,16 +4612,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
>  		i915_gem_object_free(obj);
>  		return ERR_PTR(ret);
>  	}
> -
> -	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
> -	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
> -		/* 965gm cannot relocate objects above 4GiB. */
> -		mask &= ~__GFP_HIGHMEM;
> -		mask |= __GFP_DMA32;
> -	}
> -
> -	mapping = file_inode(obj->base.filp)->i_mapping;
> -	mapping_set_gfp_mask(mapping, mask);
> +	i915_gem_set_inode_gfp(dev, obj->base.filp);
>  
>  	i915_gem_object_init(obj, &i915_gem_object_ops);
>  
> @@ -4738,6 +4744,132 @@ i915_gem_stop_ringbuffers(struct drm_device *dev)
>  		dev_priv->gt.stop_ring(ring);
>  }
>  
> +static int
> +i915_gem_object_migrate_stolen_to_shmemfs(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *i915 = to_i915(obj->base.dev);
> +	struct i915_vma *vma, *vn;
> +	struct drm_mm_node node;
> +	struct file *file;
> +	struct address_space *mapping;
> +	int ret, i;
> +
> +	if (WARN_ON(i915_gem_object_needs_bit17_swizzle(obj)))
> +		return -EINVAL;
> +
> +	list_for_each_entry_safe(vma, vn, &obj->vma_list, obj_link)
> +		if (i915_vma_unbind(vma))
> +			break;
> +
> +	ret = i915_gem_object_pin_pages(obj);
> +	if (ret)
> +		return ret;
> +
> +	memset(&node, 0, sizeof(node));
> +	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> +						  &node,
> +						  4096, 0, I915_CACHE_NONE,
> +						  0, i915->gtt.mappable_end,
> +						  DRM_MM_SEARCH_DEFAULT,
> +						  DRM_MM_CREATE_DEFAULT);

Hm, I think the plan with stolen is to mostly use it for giant scanout
buffers where we never plan to access them with the gpu. Maybe go with a
per-page loop here instead? You have a low-level pte writing call below
anyway. Would mean we'd need a 1-entry onstack sg_table too, but that
won't hurt.

And performance doesn't matter either because hibernate is seriously slow.

> +	if (ret)
> +		goto err_unpin;
> +
> +	file = shmem_file_setup("drm mm object", obj->base.size, VM_NORESERVE);
> +	if (IS_ERR(file)) {
> +		ret = PTR_ERR(file);
> +		goto err_unpin;
> +	}
> +
> +	mapping = i915_gem_set_inode_gfp(obj->base.dev, file);
> +	for (i = 0; i < obj->base.size / PAGE_SIZE; i++) {
> +		struct page *page;
> +		void *dst, *src;
> +
> +		wmb();
> +		i915->gtt.base.insert_page(&i915->gtt.base,
> +					   i915_gem_object_get_dma_address(obj, i),
> +					   node.start,
> +					   I915_CACHE_NONE,
> +					   0);
> +		wmb();
> +
> +		page = shmem_read_mapping_page(mapping, i);
> +		if (IS_ERR(page)) {
> +			ret = PTR_ERR(page);
> +			goto err_file;
> +		}
> +
> +		src = io_mapping_map_atomic_wc(i915->gtt.mappable, node.start);
> +		dst = kmap_atomic(page);
> +		memcpy(dst, src, PAGE_SIZE);

memcopy_fromio to appease sparse I think.

> +		kunmap_atomic(dst);
> +		io_mapping_unmap_atomic(src);
> +
> +		page_cache_release(page);
> +	}
> +
> +	wmb();
> +	i915->gtt.base.clear_range(&i915->gtt.base,
> +				   node.start, node.size,
> +				   true);
> +	drm_mm_remove_node(&node);
> +
> +	i915_gem_object_unpin_pages(obj);
> +	ret = i915_gem_object_put_pages(obj);
> +	if (ret) {
> +		fput(file);
> +		return ret;
> +	}
> +
> +	if (obj->ops->release)
> +		obj->ops->release(obj);
> +
> +	obj->base.filp = file;
> +	obj->ops = &i915_gem_object_ops;
> +
> +	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> +	obj->base.write_domain = I915_GEM_DOMAIN_CPU;

maybe wrap up in a shmem_reinit function and reuse in the gem object creation
path for a bit more clarity (and to reduce chances we forget about this).

Wrt igt not sure whether we can do this in general, since it requires a
working swap partition and working initrd support for hibernate. And
hibernate is kinda a disregarded feature anyway, so imo meh. We could do
an igt_system_hibernate_autowake() if we do the test though.

Imo trying to migrate backwards would be serious pain since we'd need to
make sure no shm mmap is around and audit all the other places. Too much
trouble for no gain (userspace can just realloc if needed).

> +
> +	return 0;
> +
> +err_file:
> +	fput(file);
> +err_unpin:
> +	i915_gem_object_unpin_pages(obj);
> +	return ret;
> +}
> +
> +int
> +i915_gem_freeze(struct drm_device *dev)
> +{
> +	/* Called before freeze when hibernating */
> +	struct drm_i915_private *i915 = to_i915(dev);
> +	struct drm_i915_gem_object *obj, *tmp;
> +	int ret;
> +
> +	/* Across hibernation, the stolen area is not preserved.
> +	 * Anything inside stolen must copied back to normal
> +	 * memory if we wish to preserve it.
> +	 */
> +
> +	list_for_each_entry_safe(obj, tmp, &i915->mm.stolen_list, stolen_link) {
> +		if (obj->madv != I915_MADV_WILLNEED) {
> +			/* XXX any point in setting this? The objects for which
> +			 * we preserve never unset it afterwards.
> +			 */
> +			obj->madv = __I915_MADV_PURGED;
> +			continue;
> +		}
> +
> +		ret = i915_gem_object_migrate_stolen_to_shmemfs(obj);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int
>  i915_gem_suspend(struct drm_device *dev)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index 03fa6e87f5ac..01e8fe7ae632 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -244,6 +244,9 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
>  	if (IS_ERR_OR_NULL(obj))
>  		return ERR_PTR(-ENOMEM);
>  
> +	/* contexts need to always be preserved across hibernation/swap-out */
> +	obj->madv = I915_MADV_WILLNEED;
> +
>  	/*
>  	 * Try to make the context utilize L3 as well as LLC.
>  	 *
> diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c
> index 21ee83bf6307..86e84554e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c
> @@ -331,6 +331,7 @@ int i915_gem_init_stolen(struct drm_device *dev)
>  	drm_mm_init(&dev_priv->mm.stolen, 0, dev_priv->gtt.stolen_size -
>  		    bios_reserved);
>  
> +	INIT_LIST_HEAD(&dev_priv->mm.stolen_list);
>  	return 0;
>  }
>  
> @@ -387,6 +388,7 @@ static void
>  i915_gem_object_release_stolen(struct drm_i915_gem_object *obj)
>  {
>  	if (obj->stolen) {
> +		list_del(&obj->stolen_link);
>  		drm_mm_remove_node(obj->stolen);
>  		kfree(obj->stolen);
>  		obj->stolen = NULL;
> @@ -419,6 +421,13 @@ _i915_gem_object_create_stolen(struct drm_device *dev,
>  	__i915_gem_object_pin_pages(obj);
>  	obj->has_dma_mapping = true;
>  	obj->stolen = stolen;
> +	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
> +
> +	/* By default, treat the contexts of stolen as volatile. If the object
> +	 * must be saved across hibernation, then the caller must take
> +	 * action and flag it as WILLNEED.
> +	 */
> +	obj->madv = I915_MADV_DONTNEED;

Won't this interfere with autoreclaim of stolen objects (to make room for
users which really need it like fbc) which are still in use by userspace?
I think we need a new madv flag for "REAP_ON_HIBERNATE" or something
similar. Otherwise this is way too surprising for userspace.

And with the REAP_ON_HIBERNATE we could also try to just purge them if
they're not pinned or similar (otherwise garbage on screen, ugh).

I guess the condition would then be

	if (madv == REAP_ON_HIBERNATE && !pinned)
		purge(obj)
	else
		convert_to_shmem

Also I'd make the REAPING opt-in since hibernate is such a rarely-used
path. And then perhaps only use it with create2.

Thinking about this more maybe igt would be good indeed: Use create2 to
allocate stolen with autoreaping, wrap in framebuffer and use it,
hibernate.

The gpu should be able so pinned for scanout is the only corner case I can
think of atm.

But overall I think the approach looks good, just a lot of details to get
right as usual.
-Daniel

>  
>  	obj->base.read_domains = I915_GEM_DOMAIN_CPU | I915_GEM_DOMAIN_GTT;
>  	obj->cache_level = HAS_LLC(dev) ? I915_CACHE_LLC : I915_CACHE_NONE;
> diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
> index cebe857f6df2..ec3ab351e9ff 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -1401,6 +1401,9 @@ void intel_setup_overlay(struct drm_device *dev)
>  		goto out_free;
>  	overlay->reg_bo = reg_bo;
>  
> +	/* XXX preserve register values across hibernation */
> +	reg_bo->madv = I915_MADV_WILLNEED;
> +
>  	if (OVERLAY_NEEDS_PHYSICAL(dev)) {
>  		ret = i915_gem_object_attach_phys(reg_bo, PAGE_SIZE);
>  		if (ret) {
> -- 
> 2.1.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > +	memset(&node, 0, sizeof(node));
> > +	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > +						  &node,
> > +						  4096, 0, I915_CACHE_NONE,
> > +						  0, i915->gtt.mappable_end,
> > +						  DRM_MM_SEARCH_DEFAULT,
> > +						  DRM_MM_CREATE_DEFAULT);
> 
> Hm, I think the plan with stolen is to mostly use it for giant scanout
> buffers where we never plan to access them with the gpu. Maybe go with a
> per-page loop here instead? You have a low-level pte writing call below
> anyway. Would mean we'd need a 1-entry onstack sg_table too, but that
> won't hurt.

I'm not understanding. This is a per-page loop (because we don't need to
bind the entire stolen vma into GGTT for copying with the CPU and
thereby increase the risk of failure). Speaking of failure, should
hibernation be interruptible? I guess it is usually called from an
interruptible process context.
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 12:03:44PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > +	memset(&node, 0, sizeof(node));
> > > +	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > > +						  &node,
> > > +						  4096, 0, I915_CACHE_NONE,
> > > +						  0, i915->gtt.mappable_end,
> > > +						  DRM_MM_SEARCH_DEFAULT,
> > > +						  DRM_MM_CREATE_DEFAULT);
> > 
> > Hm, I think the plan with stolen is to mostly use it for giant scanout
> > buffers where we never plan to access them with the gpu. Maybe go with a
> > per-page loop here instead? You have a low-level pte writing call below
> > anyway. Would mean we'd need a 1-entry onstack sg_table too, but that
> > won't hurt.
> 
> I'm not understanding. This is a per-page loop (because we don't need to
> bind the entire stolen vma into GGTT for copying with the CPU and
> thereby increase the risk of failure). Speaking of failure, should
> hibernation be interruptible? I guess it is usually called from an
> interruptible process context.

I was blind and confused by the insert_entries we have in upstream, which
takes a sg_table and hence can only map the full view without some jumping
through hoops. Concern fully addressed already ;-)

Wrt uninterruptible: GPU should be idle already completely (and reset if
something went wrong) so no need for interruptible. Hm, thinking about
this: Do we handle a gpu death only detected in gpu_idle? Nasty igt:
- inject hang, but be very careful to not cause any wait at all
- suspend

BOOM or not?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 01:22:59PM +0200, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 12:03:44PM +0100, Chris Wilson wrote:
> > On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > > +	memset(&node, 0, sizeof(node));
> > > > +	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > > > +						  &node,
> > > > +						  4096, 0, I915_CACHE_NONE,
> > > > +						  0, i915->gtt.mappable_end,
> > > > +						  DRM_MM_SEARCH_DEFAULT,
> > > > +						  DRM_MM_CREATE_DEFAULT);
> > > 
> > > Hm, I think the plan with stolen is to mostly use it for giant scanout
> > > buffers where we never plan to access them with the gpu. Maybe go with a
> > > per-page loop here instead? You have a low-level pte writing call below
> > > anyway. Would mean we'd need a 1-entry onstack sg_table too, but that
> > > won't hurt.
> > 
> > I'm not understanding. This is a per-page loop (because we don't need to
> > bind the entire stolen vma into GGTT for copying with the CPU and
> > thereby increase the risk of failure). Speaking of failure, should
> > hibernation be interruptible? I guess it is usually called from an
> > interruptible process context.
> 
> I was blind and confused by the insert_entries we have in upstream, which
> takes a sg_table and hence can only map the full view without some jumping
> through hoops. Concern fully addressed already ;-)
> 
> Wrt uninterruptible: GPU should be idle already completely (and reset if
> something went wrong) so no need for interruptible.

Note that I put the migration loop before the suspend, i.e. before the
gpu_idle. Partly because, I felt the migration has the biggest chance of
failure so should go first, and the gpu idle in suspend is quite
convenient if we do use the GPU for blitting, but mainly because
after i915_gem_suspend() doing GEM operations feels very wrong (there is
a strong possibilty that we kick off some work queue or other that must
be idle).

> Hm, thinking about
> this: Do we handle a gpu death only detected in gpu_idle? Nasty igt:
> - inject hang, but be very careful to not cause any wait at all
> - suspend
> 
> BOOM or not?

In my kernels, no boom. GPU hang waiting for idle is business as usual!
In upstream, we have seen suspend/hibernate fail due to an untimely hang
(iirc, usually worked on the second attempt so the bug report in
question was about something else entirely except the logs contained the
hibernate failure).
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 12:32:38PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 01:22:59PM +0200, Daniel Vetter wrote:
> > On Tue, Jun 30, 2015 at 12:03:44PM +0100, Chris Wilson wrote:
> > > On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > > > +	memset(&node, 0, sizeof(node));
> > > > > +	ret = drm_mm_insert_node_in_range_generic(&i915->gtt.base.mm,
> > > > > +						  &node,
> > > > > +						  4096, 0, I915_CACHE_NONE,
> > > > > +						  0, i915->gtt.mappable_end,
> > > > > +						  DRM_MM_SEARCH_DEFAULT,
> > > > > +						  DRM_MM_CREATE_DEFAULT);
> > > > 
> > > > Hm, I think the plan with stolen is to mostly use it for giant scanout
> > > > buffers where we never plan to access them with the gpu. Maybe go with a
> > > > per-page loop here instead? You have a low-level pte writing call below
> > > > anyway. Would mean we'd need a 1-entry onstack sg_table too, but that
> > > > won't hurt.
> > > 
> > > I'm not understanding. This is a per-page loop (because we don't need to
> > > bind the entire stolen vma into GGTT for copying with the CPU and
> > > thereby increase the risk of failure). Speaking of failure, should
> > > hibernation be interruptible? I guess it is usually called from an
> > > interruptible process context.
> > 
> > I was blind and confused by the insert_entries we have in upstream, which
> > takes a sg_table and hence can only map the full view without some jumping
> > through hoops. Concern fully addressed already ;-)
> > 
> > Wrt uninterruptible: GPU should be idle already completely (and reset if
> > something went wrong) so no need for interruptible.
> 
> Note that I put the migration loop before the suspend, i.e. before the
> gpu_idle. Partly because, I felt the migration has the biggest chance of
> failure so should go first, and the gpu idle in suspend is quite
> convenient if we do use the GPU for blitting, but mainly because
> after i915_gem_suspend() doing GEM operations feels very wrong (there is
> a strong possibilty that we kick off some work queue or other that must
> be idle).

Hm, conceptually I think this should be part of i915_gem_suspend, but
means we'd get to wire a bool hibernate around a lot. And I also think we
should do this after gpu idle and forget about optimizations like using
the blitter - too much of a slowpath really to matter.

> > Hm, thinking about
> > this: Do we handle a gpu death only detected in gpu_idle? Nasty igt:
> > - inject hang, but be very careful to not cause any wait at all
> > - suspend
> > 
> > BOOM or not?
> 
> In my kernels, no boom. GPU hang waiting for idle is business as usual!
> In upstream, we have seen suspend/hibernate fail due to an untimely hang
> (iirc, usually worked on the second attempt so the bug report in
> question was about something else entirely except the logs contained the
> hibernate failure).

Ok, reality still matches with my expectations then. I'll wish for an igt
and your fix extracted ;-)

Without looking, do you just lock-drop and then retry (and block in the
interruptible acquisition of dev->struct_mutex)?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 01:54:14PM +0200, Daniel Vetter wrote:
> Ok, reality still matches with my expectations then. I'll wish for an igt
> and your fix extracted ;-)
> 
> Without looking, do you just lock-drop and then retry (and block in the
> interruptible acquisition of dev->struct_mutex)?

It will have to change behaviour with TDR and partial resets, but the
obversation is that with a total GPU reset pending, the current contents
of the active bo are irrelevant. The reset will clobber state and it
doesn't matter if that happens before this ioctl or the next. Therefore
there is only place (waiting for ring space) where it matters that a
reset is pending, and moving the special case handling there is much
easier than adding the recovery protocol everywhere.
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > +	if (obj->ops->release)
> > +		obj->ops->release(obj);
> > +
> > +	obj->base.filp = file;
> > +	obj->ops = &i915_gem_object_ops;
> > +
> > +	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> > +	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> 
> maybe wrap up in a shmem_reinit function and reuse in the gem object creation
> path for a bit more clarity (and to reduce chances we forget about this).

A bit dubious about this. I like the explicit domain handling as it
clearly matches the status of obj->file. The only thing that is then
shared with the normal creation is obj->ops, which is actually set in
i915_gem_object_init(). It's not clear that we actually have semantic
reuse with gem object creation.

> Wrt igt not sure whether we can do this in general, since it requires a
> working swap partition and working initrd support for hibernate. And
> hibernate is kinda a disregarded feature anyway, so imo meh. We could do
> an igt_system_hibernate_autowake() if we do the test though.

I was thinking of just a flush-stolen-to-shmemfs debugfs interface. With
create2 we will be able to create the stolen objects and can use the
existing debugfs files to verify placement.
 
> Imo trying to migrate backwards would be serious pain since we'd need to
> make sure no shm mmap is around and audit all the other places. Too much
> trouble for no gain (userspace can just realloc if needed).

Otoh, that would be a userspace error since part of the stolen ABI is
that CPU mmaps are verboten. The simplest way to do this would be keep
the obj->base.filp private (so not to change the ABI of initially stolen
objects) and then fix the existing i915_gem_object_get_pages_stolen()
function (which is currently a BUG()) to do the stolen recreation.
However, that introduces the possibilty of an error being returned to
userspace, so at that point to hide the failure we probably do want to a
full shmemfs conversion.

Hmm, honestly that seems quite tractable and self-contained.
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 12:16:53PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > +	if (obj->ops->release)
> > > +		obj->ops->release(obj);
> > > +
> > > +	obj->base.filp = file;
> > > +	obj->ops = &i915_gem_object_ops;
> > > +
> > > +	obj->base.read_domains = I915_GEM_DOMAIN_CPU;
> > > +	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
> > 
> > maybe wrap up in a shmem_reinit function and reuse in the gem object creation
> > path for a bit more clarity (and to reduce chances we forget about this).
> 
> A bit dubious about this. I like the explicit domain handling as it
> clearly matches the status of obj->file. The only thing that is then
> shared with the normal creation is obj->ops, which is actually set in
> i915_gem_object_init(). It's not clear that we actually have semantic
> reuse with gem object creation.

Yeah was just wondering really, maybe just as a marker. Doesn't sound like
it'd be useful at all.

> > Wrt igt not sure whether we can do this in general, since it requires a
> > working swap partition and working initrd support for hibernate. And
> > hibernate is kinda a disregarded feature anyway, so imo meh. We could do
> > an igt_system_hibernate_autowake() if we do the test though.
> 
> I was thinking of just a flush-stolen-to-shmemfs debugfs interface. With
> create2 we will be able to create the stolen objects and can use the
> existing debugfs files to verify placement.

Hm yeah manually exercising this code would be great for testing indeed.

> > Imo trying to migrate backwards would be serious pain since we'd need to
> > make sure no shm mmap is around and audit all the other places. Too much
> > trouble for no gain (userspace can just realloc if needed).
> 
> Otoh, that would be a userspace error since part of the stolen ABI is
> that CPU mmaps are verboten. The simplest way to do this would be keep
> the obj->base.filp private (so not to change the ABI of initially stolen
> objects) and then fix the existing i915_gem_object_get_pages_stolen()
> function (which is currently a BUG()) to do the stolen recreation.
> However, that introduces the possibilty of an error being returned to
> userspace, so at that point to hide the failure we probably do want to a
> full shmemfs conversion.
> 
> Hmm, honestly that seems quite tractable and self-contained.

Imo your minimal approach here to convert the bo into a shmem backed one
behind the scenes has a lot of appeal - we don't need to audit any ioctls
at all for userspace to be able to sneak in something nasty. And hence
also no need for negative igt testcases, only for one that exercises the
actual stolen-to-shmem function in a few corner cases (display pinning is
still the only one I can think of). Downside is that once converted it'll
stay converted, but I really don't see the downside of that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > +	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
> > +
> > +	/* By default, treat the contexts of stolen as volatile. If the object
> > +	 * must be saved across hibernation, then the caller must take
> > +	 * action and flag it as WILLNEED.
> > +	 */
> > +	obj->madv = I915_MADV_DONTNEED;
> 
> Won't this interfere with autoreclaim of stolen objects (to make room for
> users which really need it like fbc) which are still in use by userspace?
> I think we need a new madv flag for "REAP_ON_HIBERNATE" or something
> similar. Otherwise this is way too surprising for userspace.

I didn't like this much either, and even in the hibernate loop, we
confuse userspace madv with internal objects. I went with a new

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ec10f389886e..c8ea71713ab8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2064,6 +2064,12 @@ struct drm_i915_gem_object {
         * Advice: are the backing pages purgeable?
         */
        unsigned int madv:2;
+       /**
+        * Whereas madv is for userspace, there are certain situations
+        * where we want I915_MADV_DONTNEED behaviour on internal objects
+        * without conflating the userspace setting.
+        */
+       unsigned int nonvolatile:1;

(I don't like the double negative, but it's better than int _volatile:1;)

So that we could easily distinguish the internal objects from userspace.
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 12:20:19PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > +	list_add(&obj->stolen_link, &to_i915(dev)->mm.stolen_list);
> > > +
> > > +	/* By default, treat the contexts of stolen as volatile. If the object
> > > +	 * must be saved across hibernation, then the caller must take
> > > +	 * action and flag it as WILLNEED.
> > > +	 */
> > > +	obj->madv = I915_MADV_DONTNEED;
> > 
> > Won't this interfere with autoreclaim of stolen objects (to make room for
> > users which really need it like fbc) which are still in use by userspace?
> > I think we need a new madv flag for "REAP_ON_HIBERNATE" or something
> > similar. Otherwise this is way too surprising for userspace.
> 
> I didn't like this much either, and even in the hibernate loop, we
> confuse userspace madv with internal objects. I went with a new
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ec10f389886e..c8ea71713ab8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2064,6 +2064,12 @@ struct drm_i915_gem_object {
>          * Advice: are the backing pages purgeable?
>          */
>         unsigned int madv:2;
> +       /**
> +        * Whereas madv is for userspace, there are certain situations
> +        * where we want I915_MADV_DONTNEED behaviour on internal objects
> +        * without conflating the userspace setting.
> +        */
> +       unsigned int nonvolatile:1;
> 
> (I don't like the double negative, but it's better than int _volatile:1;)
> 
> So that we could easily distinguish the internal objects from userspace.

I'd go with an explicit volatile_stolen or so, which must be explicitly
set. Just to avoid surprises since everywhere else (and let's be honest,
not many use hibernate compared to suspend) we do preserve objects. Hence
I think a safe-by-default setting is called for here.

But yeah separate flag sounds cleaner.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> The gpu should be able so pinned for scanout is the only corner case I can
> think of atm.

Hmm. That's a nuisance. But... We can throw away all the VM bindings
that are unpinned, and then rewrite those that are left with the shmemfs
pages.

It's ugly. On migration back we would have to do a similar trick, and we
need to tell a few white lies to get past some of our internal BUG_ON.
However, as the contents are the same and the PTE writes are atomic into
the GGTT, it should be invisible to the user.

For internally allocated frontbuffers, I was expecting to mark them as
volatile and let them lose their contents across hibernation - because
they will be immediately cleared afterwards (at least so I expect).
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 12:25:54PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > The gpu should be able so pinned for scanout is the only corner case I can
> > think of atm.
> 
> Hmm. That's a nuisance. But... We can throw away all the VM bindings
> that are unpinned, and then rewrite those that are left with the shmemfs
> pages.
> 
> It's ugly. On migration back we would have to do a similar trick, and we
> need to tell a few white lies to get past some of our internal BUG_ON.
> However, as the contents are the same and the PTE writes are atomic into
> the GGTT, it should be invisible to the user.
> 
> For internally allocated frontbuffers, I was expecting to mark them as
> volatile and let them lose their contents across hibernation - because
> they will be immediately cleared afterwards (at least so I expect).

New upcoming complications: On some still super-secret upcoming platform
we need special ggtt vmas for scanout, to pin the backing storage into
suitable memory. And because the hw implementation does replace the ggtt
pte with the physical page entry (i.e. resolving dmar and stuff) it
disallows any writes to the ptes. So we can't replace the ggtt ptes while
the vma is pinned at all.

So I guess the scheme would need to be:
- On hibernate migrate the backing storage around shmem.
- On resume we need to repin (need to do that anyway) with the new vma
  settings.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Tue, Jun 30, 2015 at 02:07:41PM +0200, Daniel Vetter wrote:
> On Tue, Jun 30, 2015 at 12:25:54PM +0100, Chris Wilson wrote:
> > On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > The gpu should be able so pinned for scanout is the only corner case I can
> > > think of atm.
> > 
> > Hmm. That's a nuisance. But... We can throw away all the VM bindings
> > that are unpinned, and then rewrite those that are left with the shmemfs
> > pages.
> > 
> > It's ugly. On migration back we would have to do a similar trick, and we
> > need to tell a few white lies to get past some of our internal BUG_ON.
> > However, as the contents are the same and the PTE writes are atomic into
> > the GGTT, it should be invisible to the user.
> > 
> > For internally allocated frontbuffers, I was expecting to mark them as
> > volatile and let them lose their contents across hibernation - because
> > they will be immediately cleared afterwards (at least so I expect).
> 
> New upcoming complications: On some still super-secret upcoming platform
> we need special ggtt vmas for scanout, to pin the backing storage into
> suitable memory. And because the hw implementation does replace the ggtt
> pte with the physical page entry (i.e. resolving dmar and stuff) it
> disallows any writes to the ptes. So we can't replace the ggtt ptes while
> the vma is pinned at all.

In that scheme, our backing storage is incoherent with the actual
contents. So we don't need the backing storage at all after setting the
PTE and the entire object is volatile (so not saved across hibernate
anyway). We wouldn't want to reserve stolen space for that, just a
custom allocator and discard the pages asap. (Or we have a phys object
scheme that does a lot of copying...)
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Tue, Jun 30, 2015 at 01:47:06PM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 02:07:41PM +0200, Daniel Vetter wrote:
> > On Tue, Jun 30, 2015 at 12:25:54PM +0100, Chris Wilson wrote:
> > > On Tue, Jun 30, 2015 at 12:54:02PM +0200, Daniel Vetter wrote:
> > > > The gpu should be able so pinned for scanout is the only corner case I can
> > > > think of atm.
> > > 
> > > Hmm. That's a nuisance. But... We can throw away all the VM bindings
> > > that are unpinned, and then rewrite those that are left with the shmemfs
> > > pages.
> > > 
> > > It's ugly. On migration back we would have to do a similar trick, and we
> > > need to tell a few white lies to get past some of our internal BUG_ON.
> > > However, as the contents are the same and the PTE writes are atomic into
> > > the GGTT, it should be invisible to the user.
> > > 
> > > For internally allocated frontbuffers, I was expecting to mark them as
> > > volatile and let them lose their contents across hibernation - because
> > > they will be immediately cleared afterwards (at least so I expect).
> > 
> > New upcoming complications: On some still super-secret upcoming platform
> > we need special ggtt vmas for scanout, to pin the backing storage into
> > suitable memory. And because the hw implementation does replace the ggtt
> > pte with the physical page entry (i.e. resolving dmar and stuff) it
> > disallows any writes to the ptes. So we can't replace the ggtt ptes while
> > the vma is pinned at all.
> 
> In that scheme, our backing storage is incoherent with the actual
> contents. So we don't need the backing storage at all after setting the
> PTE and the entire object is volatile (so not saved across hibernate
> anyway). We wouldn't want to reserve stolen space for that, just a
> custom allocator and discard the pages asap. (Or we have a phys object
> scheme that does a lot of copying...)

The backing storage will still be coherent, it's just that the specific
view used to scan it out can't be touched at all (i.e. not even ptes
rewritten) while in use. Which means you can't rewrite pts, that's all.

We can still copy the actual backing storage using a new ggtt view
(they'll have different types) and convert to shmem. It's only that
rewriting the ptes for the scanout view can only happen on resume from
hibernate (we need to restore it all ofc again). Youre description above
sounded like you wanted to rewrite all ptes right away which I think isn't
need and at least with that fancy platform not possible - if we have
concurrent writes while we do the conversion there's a bug anyway.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Chris Wilson]

On Wed, Jul 01, 2015 at 02:47:15PM +0200, Daniel Vetter wrote:
> The backing storage will still be coherent, it's just that the specific
> view used to scan it out can't be touched at all (i.e. not even ptes
> rewritten) while in use. Which means you can't rewrite pts, that's all.

Hmm, I thought there was going to be some fenced off memory. I was
hoping to avoid allocating any ourseleves :)
 
> We can still copy the actual backing storage using a new ggtt view
> (they'll have different types) and convert to shmem. It's only that
> rewriting the ptes for the scanout view can only happen on resume from
> hibernate (we need to restore it all ofc again). Youre description above
> sounded like you wanted to rewrite all ptes right away which I think isn't
> need and at least with that fancy platform not possible - if we have
> concurrent writes while we do the conversion there's a bug anyway.

It's just the difference between making the function a generic
migrate-stolen-to-shmem, and making it peculiar to hibernate.

But yes, since we need to rebind all the vma after hibernate, we can add
a parameter to tell us to skip it during migrate() and expect everything
to come out in the wash (since the stolen memory won't be reused).
-Chris

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

Re: [PATCH] drm/i915: Migrate stolen objects before hibernation

[Daniel Vetter]

On Wed, Jul 01, 2015 at 01:59:23PM +0100, Chris Wilson wrote:
> On Wed, Jul 01, 2015 at 02:47:15PM +0200, Daniel Vetter wrote:
> > The backing storage will still be coherent, it's just that the specific
> > view used to scan it out can't be touched at all (i.e. not even ptes
> > rewritten) while in use. Which means you can't rewrite pts, that's all.
> 
> Hmm, I thought there was going to be some fenced off memory. I was
> hoping to avoid allocating any ourseleves :)

It works like a cache, but with some funky pinning which needs a separate
view because that's how the hw works.

> > We can still copy the actual backing storage using a new ggtt view
> > (they'll have different types) and convert to shmem. It's only that
> > rewriting the ptes for the scanout view can only happen on resume from
> > hibernate (we need to restore it all ofc again). Youre description above
> > sounded like you wanted to rewrite all ptes right away which I think isn't
> > need and at least with that fancy platform not possible - if we have
> > concurrent writes while we do the conversion there's a bug anyway.
> 
> It's just the difference between making the function a generic
> migrate-stolen-to-shmem, and making it peculiar to hibernate.
> 
> But yes, since we need to rebind all the vma after hibernate, we can add
> a parameter to tell us to skip it during migrate() and expect everything
> to come out in the wash (since the stolen memory won't be reused).

Yeah generic stolen-to-shmem is probably overshooting for this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Jani Nikula]

On Mon, 29 Jun 2015, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Stolen gets trashed during hibernation, so storing contexts there
> is not a very good idea. On my IVB machines this leads to a totally
> dead GPU on resume. A reboot is required to resurrect it. So let's
> not store contexts where they will get trampled.
>
> This reverts commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261.

Looks like a *partial* revert of

commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Tue Apr 7 16:21:11 2015 +0100

    drm/i915: Allocate context objects from stolen

and the not reverted hunk has been touched since in 

commit 3126a660f352b3fe48125a8a0b4fdbf85935d8bf
Author: Dan Carpenter <dan.carpenter@oracle.com>
Date:   Thu Apr 30 17:30:50 2015 +0300

    drm/i915: checking IS_ERR() instead of NULL

The commit message should say it's a partial revert and not claim
otherwise, and document why not everything was reverted.

The above commits are in Linus' tree.

BR,
Jani.

>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_context.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
> index a7e58a8..5c7ccf4 100644
> --- a/drivers/gpu/drm/i915/i915_gem_context.c
> +++ b/drivers/gpu/drm/i915/i915_gem_context.c
> @@ -157,9 +157,7 @@ i915_gem_alloc_context_obj(struct drm_device *dev, size_t size)
>  	struct drm_i915_gem_object *obj;
>  	int ret;
>  
> -	obj = i915_gem_object_create_stolen(dev, size);
> -	if (obj == NULL)
> -		obj = i915_gem_alloc_object(dev, size);
> +	obj = i915_gem_alloc_object(dev, size);
>  	if (obj == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> -- 
> 2.3.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Chris Wilson]

On Tue, Jun 30, 2015 at 11:31:54AM +0300, Jani Nikula wrote:
> On Mon, 29 Jun 2015, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Stolen gets trashed during hibernation, so storing contexts there
> > is not a very good idea. On my IVB machines this leads to a totally
> > dead GPU on resume. A reboot is required to resurrect it. So let's
> > not store contexts where they will get trampled.
> >
> > This reverts commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261.
> 
> Looks like a *partial* revert of
> 
> commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Tue Apr 7 16:21:11 2015 +0100
> 
>     drm/i915: Allocate context objects from stolen
> 
> and the not reverted hunk has been touched since in 
> 
> commit 3126a660f352b3fe48125a8a0b4fdbf85935d8bf
> Author: Dan Carpenter <dan.carpenter@oracle.com>
> Date:   Thu Apr 30 17:30:50 2015 +0300
> 
>     drm/i915: checking IS_ERR() instead of NULL
> 
> The commit message should say it's a partial revert and not claim
> otherwise, and document why not everything was reverted.
> 
> The above commits are in Linus' tree.

The fixes I have planned to migrate context objects on hibernation are
not of the simple variety! Revert now, fix later.

Given Ville's discovery, it also makes

commit 8040513870399f1cb032cb8bc805df5042fedcdf
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu Nov 15 11:32:29 2012 +0000

    drm/i915: Allocate overlay registers from stolen memory

suspect.
-Chris

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

Re: [PATCH] Revert "drm/i915: Allocate context objects from stolen"

[Ville Syrjälä]

On Tue, Jun 30, 2015 at 10:44:15AM +0100, Chris Wilson wrote:
> On Tue, Jun 30, 2015 at 11:31:54AM +0300, Jani Nikula wrote:
> > On Mon, 29 Jun 2015, ville.syrjala@linux.intel.com wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Stolen gets trashed during hibernation, so storing contexts there
> > > is not a very good idea. On my IVB machines this leads to a totally
> > > dead GPU on resume. A reboot is required to resurrect it. So let's
> > > not store contexts where they will get trampled.
> > >
> > > This reverts commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261.
> > 
> > Looks like a *partial* revert of
> > 
> > commit 149c86e74fe44dcbac5e9f8d145c5fbc5dc21261
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Tue Apr 7 16:21:11 2015 +0100
> > 
> >     drm/i915: Allocate context objects from stolen
> > 
> > and the not reverted hunk has been touched since in 
> > 
> > commit 3126a660f352b3fe48125a8a0b4fdbf85935d8bf
> > Author: Dan Carpenter <dan.carpenter@oracle.com>
> > Date:   Thu Apr 30 17:30:50 2015 +0300
> > 
> >     drm/i915: checking IS_ERR() instead of NULL
> > 
> > The commit message should say it's a partial revert and not claim
> > otherwise, and document why not everything was reverted.
> > 
> > The above commits are in Linus' tree.
> 
> The fixes I have planned to migrate context objects on hibernation are
> not of the simple variety! Revert now, fix later.
> 
> Given Ville's discovery, it also makes
> 
> commit 8040513870399f1cb032cb8bc805df5042fedcdf
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu Nov 15 11:32:29 2012 +0000
> 
>     drm/i915: Allocate overlay registers from stolen memory
> 
> suspect.

Hmm. It does look like it might not reinitialize the entire register
block on suspend.

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