Quoting Tvrtko Ursulin (2017-06-19 13:06:18)
>
> On 16/06/2017 17:02, Chris Wilson wrote:
> > @@ -520,7 +511,8 @@ static inline int use_cpu_reloc(const struct reloc_cache *cache,
> > static int eb_reserve_vma(const struct i915_execbuffer *eb,
> > struct i915_vma *vma)
> > {
> > - struct drm_i915_gem_exec_object2 *entry = vma->exec_entry;
> > + struct drm_i915_gem_exec_object2 *entry =
> > + &eb->exec[vma->exec_flags - eb->flags];
>
> There are three instances of this so maybe "entry = exec_entry(eb,
> vma)"? Added benefit that exec_entry implementation could site a comment
> explaining the magic.
You were meant to jump in and suggest some way I would store the index.
:)
> > @@ -870,23 +864,20 @@ static void eb_release_vmas(const struct i915_execbuffer *eb)
> > unsigned int i;
> >
> > for (i = 0; i < count; i++) {
> > - struct drm_i915_gem_exec_object2 *entry = &eb->exec[i];
> > - struct i915_vma *vma = exec_to_vma(entry);
> > + struct i915_vma *vma = eb->vma[i];
> > + unsigned int flags = eb->flags[i];
> >
> > if (!vma)
> > continue;
> >
> > - GEM_BUG_ON(vma->exec_entry != entry);
> > - vma->exec_entry = NULL;
> > + GEM_BUG_ON(vma->exec_flags != &eb->flags[i]);
> > + vma->exec_flags = NULL;
> >
> > - if (entry->flags & __EXEC_OBJECT_HAS_PIN)
> > - __eb_unreserve_vma(vma, entry);
> > + if (flags & __EXEC_OBJECT_HAS_PIN)
> > + __eb_unreserve_vma(vma, flags);
> >
> > - if (entry->flags & __EXEC_OBJECT_HAS_REF)
> > + if (flags & __EXEC_OBJECT_HAS_REF)
> > i915_vma_put(vma);
> > -
> > - entry->flags &=
> > - ~(__EXEC_OBJECT_RESERVED | __EXEC_OBJECT_HAS_REF);
>
> No need to clear these from eb->flags[i] to ensure no double free?
No, the other path that may drop the vma prevents the double-free by
setting count to 0. Along the error path we don't have the vma set for
incomplete loads.
> > @@ -1830,33 +1824,34 @@ static int eb_move_to_gpu(struct i915_execbuffer *eb)
> > return -ENOMEM;
> >
> > capture->next = eb->request->capture_list;
> > - capture->vma = vma;
> > + capture->vma = eb->vma[i];
> > eb->request->capture_list = capture;
> > }
> >
> > - if (entry->flags & EXEC_OBJECT_ASYNC)
> > - goto skip_flushes;
> > + if (flags & EXEC_OBJECT_ASYNC)
> > + continue;
> >
> > + obj = eb->vma[i]->obj;
> > if (unlikely(obj->cache_dirty && !obj->cache_coherent))
> > i915_gem_clflush_object(obj, 0);
> >
> > err = i915_gem_request_await_object
> > - (eb->request, obj, entry->flags & EXEC_OBJECT_WRITE);
> > + (eb->request, obj, flags & EXEC_OBJECT_WRITE);
> > if (err)
> > return err;
> > -
> > -skip_flushes:
> > - i915_vma_move_to_active(vma, eb->request, entry->flags);
> > - __eb_unreserve_vma(vma, entry);
> > - vma->exec_entry = NULL;
>
> Grumble, it would have been better if this change was in a separate patch.
Grumble. It was more meaningful at some earlier version of the patch
where there was a dance here with flags.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
