* [PATCH] drm/i915: Fix ordering of unbind vs unpin pages
[not found] <0131204000714.GA32105@bwidawsk.net>
@ 2013-12-04 9:59 ` Chris Wilson
2013-12-04 11:10 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2013-12-04 9:59 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
It is useful to assert that if the object is bound, then it must have
its pages pinned to prevent the shrinker from reaping its backing store.
This is even more useful with the introduction of real-ppgtt whereupon
we may have the object bound into several vma, with each instance
pinning the backing store. This assertion breaks down during unbind
where we unpinned the backing store before decoupling the vma binding.
This can be fixed with a trivial reording of the unbind sequence, which
reinforces the
pin pages
bind to vma
...
unbind from vma
unpin pages
concept.
v2: Bonus comment
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Tested-by: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6312d61f5198..9b805f0e8387 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2755,7 +2755,6 @@ int i915_vma_unbind(struct i915_vma *vma)
vma->unbind_vma(vma);
i915_gem_gtt_finish_object(obj);
- i915_gem_object_unpin_pages(obj);
list_del(&vma->mm_list);
/* Avoid an unnecessary call to unbind on rebind. */
@@ -2763,7 +2762,6 @@ int i915_vma_unbind(struct i915_vma *vma)
obj->map_and_fenceable = true;
drm_mm_remove_node(&vma->node);
-
i915_gem_vma_destroy(vma);
/* Since the unbound list is global, only move to that list if
@@ -2771,6 +2769,12 @@ int i915_vma_unbind(struct i915_vma *vma)
if (list_empty(&obj->vma_list))
list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
+ /* And finally now the object is completely decoupled from this vma,
+ * we can drop its hold on the backing storage and allow it to be
+ * reaped by the shrinker.
+ */
+ i915_gem_object_unpin_pages(obj);
+
return 0;
}
--
1.8.5
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] drm/i915: Fix ordering of unbind vs unpin pages
2013-12-04 9:59 ` [PATCH] drm/i915: Fix ordering of unbind vs unpin pages Chris Wilson
@ 2013-12-04 11:10 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2013-12-04 11:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx, Ben Widawsky
On Wed, Dec 04, 2013 at 09:59:09AM +0000, Chris Wilson wrote:
> It is useful to assert that if the object is bound, then it must have
> its pages pinned to prevent the shrinker from reaping its backing store.
> This is even more useful with the introduction of real-ppgtt whereupon
> we may have the object bound into several vma, with each instance
> pinning the backing store. This assertion breaks down during unbind
> where we unpinned the backing store before decoupling the vma binding.
> This can be fixed with a trivial reording of the unbind sequence, which
> reinforces the
>
> pin pages
> bind to vma
> ...
> unbind from vma
> unpin pages
>
> concept.
>
> v2: Bonus comment
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
> Tested-by: Ben Widawsky <ben@bwidawsk.net>
Queued for -next, thanks for the patch.
-Daniel
> ---
> drivers/gpu/drm/i915/i915_gem.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6312d61f5198..9b805f0e8387 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2755,7 +2755,6 @@ int i915_vma_unbind(struct i915_vma *vma)
> vma->unbind_vma(vma);
>
> i915_gem_gtt_finish_object(obj);
> - i915_gem_object_unpin_pages(obj);
>
> list_del(&vma->mm_list);
> /* Avoid an unnecessary call to unbind on rebind. */
> @@ -2763,7 +2762,6 @@ int i915_vma_unbind(struct i915_vma *vma)
> obj->map_and_fenceable = true;
>
> drm_mm_remove_node(&vma->node);
> -
> i915_gem_vma_destroy(vma);
>
> /* Since the unbound list is global, only move to that list if
> @@ -2771,6 +2769,12 @@ int i915_vma_unbind(struct i915_vma *vma)
> if (list_empty(&obj->vma_list))
> list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>
> + /* And finally now the object is completely decoupled from this vma,
> + * we can drop its hold on the backing storage and allow it to be
> + * reaped by the shrinker.
> + */
> + i915_gem_object_unpin_pages(obj);
> +
> return 0;
> }
>
> --
> 1.8.5
>
> _______________________________________________
> 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH] drm/i915: Fix ordering of unbind vs unpin pages
@ 2013-12-02 10:08 Chris Wilson
2013-12-04 0:07 ` Ben Widawsky
0 siblings, 1 reply; 4+ messages in thread
From: Chris Wilson @ 2013-12-02 10:08 UTC (permalink / raw)
To: intel-gfx; +Cc: Ben Widawsky
It is useful to assert that if the object is bound, then it must have
its pages pinned to prevent the shrinker from reaping its backing store.
This is even more useful with the introduction of real-ppgtt whereupon
we may have the object bound into several vma, with each instance
pinning the backing store. This assertion breaks down during unbind
where we unpinned the backing store before decoupling the vma binding.
This can be fixed with a trivial reording of the unbind sequence, which
reinforces the
pin pages
bind to vma
...
unbind from vma
unpin pages
concept.
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ben Widawsky <ben@bwidawsk.net>
---
drivers/gpu/drm/i915/i915_gem.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6312d61f5198..5fef29a5d6af 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2755,7 +2755,6 @@ int i915_vma_unbind(struct i915_vma *vma)
vma->unbind_vma(vma);
i915_gem_gtt_finish_object(obj);
- i915_gem_object_unpin_pages(obj);
list_del(&vma->mm_list);
/* Avoid an unnecessary call to unbind on rebind. */
@@ -2763,7 +2762,6 @@ int i915_vma_unbind(struct i915_vma *vma)
obj->map_and_fenceable = true;
drm_mm_remove_node(&vma->node);
-
i915_gem_vma_destroy(vma);
/* Since the unbound list is global, only move to that list if
@@ -2771,6 +2769,8 @@ int i915_vma_unbind(struct i915_vma *vma)
if (list_empty(&obj->vma_list))
list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
+ i915_gem_object_unpin_pages(obj);
+
return 0;
}
--
1.8.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: Fix ordering of unbind vs unpin pages
2013-12-02 10:08 Chris Wilson
@ 2013-12-04 0:07 ` Ben Widawsky
0 siblings, 0 replies; 4+ messages in thread
From: Ben Widawsky @ 2013-12-04 0:07 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, Dec 02, 2013 at 10:08:02AM +0000, Chris Wilson wrote:
> It is useful to assert that if the object is bound, then it must have
> its pages pinned to prevent the shrinker from reaping its backing store.
> This is even more useful with the introduction of real-ppgtt whereupon
> we may have the object bound into several vma, with each instance
> pinning the backing store. This assertion breaks down during unbind
> where we unpinned the backing store before decoupling the vma binding.
> This can be fixed with a trivial reording of the unbind sequence, which
> reinforces the
>
> pin pages
> bind to vma
> ...
> unbind from vma
> unpin pages
>
> concept.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ben Widawsky <ben@bwidawsk.net>
A comment in the code why the page unpinning happens last would be nice,
but not mandatory.
Reviewed-by: Ben Widawsky <ben@bwidawsk.net>
Tested-by: Ben Widawsky <ben@bwidawsk.net>
> ---
> drivers/gpu/drm/i915/i915_gem.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6312d61f5198..5fef29a5d6af 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2755,7 +2755,6 @@ int i915_vma_unbind(struct i915_vma *vma)
> vma->unbind_vma(vma);
>
> i915_gem_gtt_finish_object(obj);
> - i915_gem_object_unpin_pages(obj);
>
> list_del(&vma->mm_list);
> /* Avoid an unnecessary call to unbind on rebind. */
> @@ -2763,7 +2762,6 @@ int i915_vma_unbind(struct i915_vma *vma)
> obj->map_and_fenceable = true;
>
> drm_mm_remove_node(&vma->node);
> -
> i915_gem_vma_destroy(vma);
>
> /* Since the unbound list is global, only move to that list if
> @@ -2771,6 +2769,8 @@ int i915_vma_unbind(struct i915_vma *vma)
> if (list_empty(&obj->vma_list))
> list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list);
>
> + i915_gem_object_unpin_pages(obj);
> +
> return 0;
> }
>
> --
> 1.8.5
>
--
Ben Widawsky, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-12-04 11:09 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <0131204000714.GA32105@bwidawsk.net>
2013-12-04 9:59 ` [PATCH] drm/i915: Fix ordering of unbind vs unpin pages Chris Wilson
2013-12-04 11:10 ` Daniel Vetter
2013-12-02 10:08 Chris Wilson
2013-12-04 0:07 ` Ben Widawsky
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox