On Thu, Nov 26, 2015 at 02:07:57PM +0000, Tvrtko Ursulin wrote: > > 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. Yeah, walking obj->vma_list pretty much means unclean design. Unfortunately we still have a pile of those left from the original ppgtt enabling, and the fixes for those are burried in Chris' tree somewhere. > >>>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. :) Well in this case there shouldn't be a dilemma: Since the view special case can't happen and doesn't matter, being generic also means less code and hence more optimal ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
