On Thu, Aug 22, 2013 at 09:41:07PM +0200, Daniel Vetter wrote: [snip] > > > > It's not clear from the commit message if this actually fixes the bug > > for you (which you seemed to be able to reproduce). Did it? > > Nope, just hard looking at the Oopses, haven't yet reproduced the bug > here. But now that I have a theory what's busted that should help a > bit with writing igts ... > In that case, I think you need the same check in the failure path in bind_to_vm. I have that fixed locally, but haven't actually tested it. > >> --- > >> drivers/gpu/drm/i915/i915_gem.c | 15 +++++++-------- > >> 1 file changed, 7 insertions(+), 8 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > >> index ef92c69..3e855ff 100644 > >> --- a/drivers/gpu/drm/i915/i915_gem.c > >> +++ b/drivers/gpu/drm/i915/i915_gem.c > >> @@ -2616,6 +2616,9 @@ int i915_vma_unbind(struct i915_vma *vma) > >> drm_i915_private_t *dev_priv = obj->base.dev->dev_private; > >> int ret; > >> > >> + /* For now we only ever use 1 vma per object */ > >> + WARN_ON(!list_is_singular(&obj->vma_list)); > >> + > >> if (list_empty(&vma->vma_link)) > >> return 0; > >> > >> @@ -2661,12 +2664,12 @@ int i915_vma_unbind(struct i915_vma *vma) > >> drm_mm_remove_node(&vma->node); > >> > >> destroy: > >> - i915_gem_vma_destroy(vma); > >> + /* Keep the vma as a placeholder in the execbuffer reservation lists */ > >> + if (!list_empty(&vma->exec_list)) > >> + i915_gem_vma_destroy(vma); > > > > Please see my vma->busy question at the top. To me this looks like the > > behavior I had with vma->busy where you don't unlink from the vma_list > > (which actually defies the original convention I had about the > > vma_list, whereby a vma exists on a vma list IFF it has space allocated > > in some address space). I think this is necessary though, as there are > > really three states: > > 1. Bound > > 2. Temporarily unbound <-- this is the problematic one > > 3. Unbound > > Yes. > > > > >> > >> /* Since the unbound list is global, only move to that list if > >> - * no more VMAs exist. > >> - * NB: Until we have real VMAs there will only ever be one */ > >> - WARN_ON(!list_empty(&obj->vma_list)); > >> + * no more VMAs exist. */ > >> if (list_empty(&obj->vma_list)) > >> list_move_tail(&obj->global_list, &dev_priv->mm.unbound_list); > >> > >> @@ -4171,10 +4174,6 @@ void i915_gem_vma_destroy(struct i915_vma *vma) > >> WARN_ON(vma->node.allocated); > >> list_del(&vma->vma_link); > >> > >> - /* Keep the vma as a placeholder in the execbuffer reservation lists */ > >> - if (!list_empty(&vma->exec_list)) > >> - return; > >> - > >> kfree(vma); > >> } > > > > If you really wan to remove this hunk, I think you should do it as a > > revert (or remove it from the history completely if it isn't too late). > > Instead though I think turning this into a WARN is still a valid thing > > to do. Unless I've missed something. It shouldn't ever be safe to > > destroy a VMA on an exec_list because we use the same list element in > > eviction and if we accidentally destroy in eviction (called from > > execbuf) we're in pretty big trouble. > > I think I'll squash this in when merging with Chris' patch and fixup > the commit message a bit. For more paranoia in the evict code Chris > suggested to add a check in mark_free for list_empty(vma->exec_list). > Would be good in a follow-up patch imo. I have this exact same check in my current code to debug this, I think it makes a lot of sense. > > > Finally, I still think Chris' original suggestion of creating a new > > member for eviction would make a lot of these bugs at least a little bit > > easier to find. It's always hard to say though until you actually do it. > > Yeah, we could just go back to the old ways of using the lru lists to > unwind the eviction roaster (i.e. active/inactive vm lists now). > That's also why I think we could just merge the active/inactive list > together into one, since without that merge it'll be trouble. But when > Chris merged the fair lru eviction code he frobbed it a bit to use the > current temporary list. > > Adding yet another list is imo wasteful ;-) I guess we'll see how difficult the bugs are to fix. It uses extra memory, that much is indisputable. -- Ben Widawsky, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
