On 26/11/15 10:01, Daniel Vetter wrote:
On Wed, Nov 25, 2015 at 10:16:37AM +0000, Tvrtko Ursulin wrote:
On 24/11/15 17:47, Daniel Vetter wrote:
On Mon, Nov 23, 2015 at 03:12:35PM +0000, Tvrtko Ursulin wrote:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Current code moves _any_ VMA to the inactive list only when
_all_ rendering on an object (so from any context or VM) has
been completed.
This creates an un-natural situation where the context (and
VM) destructors can run with VMAs still on the respective
active list.
Change here is to move VMAs to the inactive list as the
requests are getting retired.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92638
Testcase: igt/gem_request_retire/retire-vma-not-inactive
Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gem.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cd7e102720f4..47a743246d2c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2413,17 +2413,32 @@ static void
i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
{
struct i915_vma *vma;
+ struct i915_address_space *vm;
RQ_BUG_ON(obj->last_read_req[ring] == NULL);
RQ_BUG_ON(!(obj->active & (1 << ring)));
list_del_init(&obj->ring_list[ring]);
- i915_gem_request_assign(&obj->last_read_req[ring], NULL);
if (obj->last_write_req && obj->last_write_req->ring->id == ring)
i915_gem_object_retire__write(obj);
obj->active &= ~(1 << ring);
+
+ if (obj->last_read_req[ring]->ctx->ppgtt)
+ vm = &obj->last_read_req[ring]->ctx->ppgtt->base;
+ else
+ vm = &obj->last_read_req[ring]->i915->gtt.base;
+
+ list_for_each_entry(vma, &obj->vma_list, vma_link) {
+ if (vma->vm == vm &&
+ vma->ggtt_view.type == I915_GGTT_VIEW_NORMAL &&
+ !list_empty(&vma->mm_list))
+ list_move_tail(&vma->mm_list, &vma->vm->inactive_list);
+ }
This is only a partial solution since with schedulers and semaphores and a
few depencies on a given object, but in different vm you can still end up
with an object that is idle in a vm, but slipped through here.
Could you describe the exact scenario you had in mind? I won't pretend it
this is simple code and I have it all figured out.
Well tbh I don't fully understand what exactly your code will do in all
corner-cases since there's a lot of ifs and special cases. But
fundamentally the problem is that an object can be active in a given vm
and there's no request pointing at the corresponding context in either
last_read_req or last_write_req. It works like this:
- ctx A uses obj 1
- ctx B uses obj 1 on the same engine
Your code above will miss to retire obj 1 on ctx 1's vm, so if you then
destroy ctx A you'll hit the nice warn above (presuming ctx B keeps obj 1
busy for all that time). So not even scheduler needed.
Ah yes... I was even close to figuring this one out but got too confused
and decided I am imagining things...
Fundamentally if you really want to accurately keep track of vma instaed
of obj activeness, then you need to have per-vma tracking of all of it,
i.e. all the last_*_req stuff.
Without that we essentially only keep track of an lru on each vm, and the
active/inactive split is totally bogus (well not quite, but since it
reflects obj->active there's not really any value, but it does create tons
of confusion).
To unconfuse this we'd need to have proper vma active tracking (not sure
whether that's worth it) or just merge the per-vm active/inactive lists
since they're really just one big list.
... so all in all best to drop this for now. Since it is a half
solution, plus Chris says Synmark, if I read it right, likes to use an
object from a zillion of contexts simultaneously so would be hurt by
traversing the obj->vma_list here.
Also, checking for the view type is some strange layering. Why that?
!ppgtt to skip potential other views I thought, no?
The vma lrus should be irrespective of ggtt vs. ppgtt or exactly what kind
of view it is. If we exclude special case everywhere the might pop up but
should the code will become unreadable (and the abstraction useless). What
do you fear could blow up without this check?
Nothing really, since other view types will never end up on the active
list. Just a never ending dilemma of whether to be generic or optimal. :)
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
