Quoting Tvrtko Ursulin (2019-09-17 13:37:55)
> On 02/09/2019 05:02, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > index 82db2b783123..9a8c307c5aeb 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
> > @@ -251,16 +251,6 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> > goto err_rpm;
> > }
> >
> > - ret = i915_mutex_lock_interruptible(dev);
> > - if (ret)
> > - goto err_reset;
> > -
> > - /* Access to snoopable pages through the GTT is incoherent. */
> > - if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(i915)) {
> > - ret = -EFAULT;
> > - goto err_unlock;
> > - }
> > -
> > /* Now pin it into the GTT as needed */
> > vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0,
> > PIN_MAPPABLE |
> > @@ -293,7 +283,13 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> > }
> > if (IS_ERR(vma)) {
> > ret = PTR_ERR(vma);
> > - goto err_unlock;
> > + goto err_reset;
> > + }
> > +
> > + /* Access to snoopable pages through the GTT is incoherent. */
> > + if (obj->cache_level != I915_CACHE_NONE && !HAS_LLC(i915)) {
> > + ret = -EFAULT;
> > + goto err_unpin;
> > }
>
> Why have you moved this check to after pinning?
Since we've dropped the lock around this check, the intent is to use the
pin as a guarantee that the vma cannot be changed. What we actually want
here, for clarity, is vma->cache_level.
> >
> > ret = i915_vma_pin_fence(vma);
> > @@ -321,14 +317,12 @@ vm_fault_t i915_gem_fault(struct vm_fault *vmf)
> > intel_wakeref_auto(&i915->ggtt.userfault_wakeref,
> > msecs_to_jiffies_timeout(CONFIG_DRM_I915_USERFAULT_AUTOSUSPEND));
> >
> > - i915_vma_set_ggtt_write(vma);
> > -
> > + if (write)
> > + i915_vma_set_ggtt_write(vma);
>
> Noise for what this patch is concerned?
Yeah, I had to stare at an incorrect __set_bit(GGTT_WRITE) for too long.
>
> > err_fence:
> > i915_vma_unpin_fence(vma);
> > err_unpin:
> > __i915_vma_unpin(vma);
> > -err_unlock:
> > - mutex_unlock(&dev->struct_mutex);
> > err_reset:
> > intel_gt_reset_unlock(ggtt->vm.gt, srcu);
> > err_rpm:
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object.c b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > index 0ef60dae23a7..dbf9be9a79f4 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_object.c
> > @@ -155,21 +155,30 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
> >
> > wakeref = intel_runtime_pm_get(&i915->runtime_pm);
> > llist_for_each_entry_safe(obj, on, freed, freed) {
> > - struct i915_vma *vma, *vn;
> > -
> > trace_i915_gem_object_destroy(obj);
> >
> > - mutex_lock(&i915->drm.struct_mutex);
> > -
> > - list_for_each_entry_safe(vma, vn, &obj->vma.list, obj_link) {
> > - GEM_BUG_ON(i915_vma_is_active(vma));
> > - atomic_and(~I915_VMA_PIN_MASK, &vma->flags);
> > - i915_vma_destroy(vma);
> > + if (!list_empty(&obj->vma.list)) {
> > + struct i915_vma *vma;
> > +
> > + /*
> > + * Note that the vma keeps an object reference while
> > + * it is active, so it *should* not sleep while we
> > + * destroy it. Our debug code errs insits it *might*.
> > + * For the moment, play along.
> > + */
> > + spin_lock(&obj->vma.lock);
> > + while ((vma = list_first_entry_or_null(&obj->vma.list,
> > + struct i915_vma,
> > + obj_link)))
>
> What is the point of having a while loop inside top-level if !list_empty
> check? Looks theoretically racy, and even if that is irrelevant, it
> would be clearer to just do the while loop.
We can't add more vma to the object, as there are no more refs to the
object. The challenge is in dropping the links. In the end it makes no
difference, it's an unlocked compare before the spinlock (and ugly
loop).
> > GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
> >
> > + GEM_BUG_ON(vma->pages);
> > vma->pages = obj->mm.pages;
> > + atomic_set(&vma->pages_count, I915_VMA_PAGES_ACTIVE);
>
> What is I915_VMA_PAGES_ACTIVE used for? Pinned? Has pages?
It's a counter for the number of unique binds as opposed to callers
merely acquiring the pages prior to binding; so that we can balance
vma_unbind_pages() which is called once (i915_vma_unbind) but
i915_vma_bind() can be called for both PIN_GLOBAL and PIN_USER.
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > index ca0c2f451742..b9cfae0e4435 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_tiling.c
> > @@ -181,22 +181,25 @@ static int
> > i915_gem_object_fence_prepare(struct drm_i915_gem_object *obj,
> > int tiling_mode, unsigned int stride)
> > {
> > + struct i915_ggtt *ggtt = &to_i915(obj->base.dev)->ggtt;
> > struct i915_vma *vma;
> > - int ret;
> > + int ret = 0;
> >
> > if (tiling_mode == I915_TILING_NONE)
> > return 0;
> >
> > + mutex_lock(&ggtt->vm.mutex);
> > for_each_ggtt_vma(vma, obj) {
> > if (i915_vma_fence_prepare(vma, tiling_mode, stride))
> > continue;
>
> vma_fence_prepare doesn't need to be under mutex, but it's much easier
> for this loop, yes?
fence_prepare is just a simple predicate, so no. What we do need though
is to pull it under the object-lock so that we don't prepare for one
tiling and bind another. Hmm.
> > @@ -333,7 +344,11 @@ static int igt_check_page_sizes(struct i915_vma *vma)
> > struct drm_i915_private *i915 = vma->vm->i915;
> > unsigned int supported = INTEL_INFO(i915)->page_sizes;
> > struct drm_i915_gem_object *obj = vma->obj;
> > - int err = 0;
> > + int err;
> > +
> > + err = i915_active_wait(&vma->active);
> > + if (err)
> > + return err;
>
> Touched upon this in the previous email, but I think we need to put some
> clear notes in writing, in some good place, regarding when callers need
> to explicitly do this and when not. Don't know where though.. commit
> message is weak. Maybe kernel doc next to i915_vma_unbind?
The couple of places that care are the exception. Though I'm
contemplating using the async bind to circumvent the stop_machine()...
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > index da54a718c712..aa67c02ba98c 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_context.c
> > @@ -747,10 +747,7 @@ emit_rpcs_query(struct drm_i915_gem_object *obj,
> > if (err)
> > goto skip_request;
> >
> > - i915_vma_unpin(batch);
> > - i915_vma_close(batch);
> > - i915_vma_put(batch);
> > -
> > + i915_vma_unpin_and_release(&batch, 0);
>
> Is this consolidation enabled by this patch or was otherwise possible?
At this point, it is deduplication. What prompted the change were a few
callers mixed up the potential use after free of vma->obj.
> Something rings familiar about some problem with
> i915_vma_unpin_and_release but I can't remember what.. Semantics are a
> bit weird though.. whereas pattern before was to i915_vma_put after
> instantiating a vma, now it is unpin_and_release.. release being a bit
> of an odd term in this space. There was/is no symmetry with get/put
> anyway so I guess it's fine.
It'll be back to i915_vma_put() eventually.
> > +void i915_active_set_exclusive(struct i915_active *ref, struct dma_fence *f)
> > +{
> > + GEM_BUG_ON(i915_active_is_idle(ref));
> > +
> > + dma_fence_get(f);
> > +
> > + rcu_read_lock();
> > + if (rcu_access_pointer(ref->excl)) {
> > + struct dma_fence *old;
> > +
> > + old = dma_fence_get_rcu_safe(&ref->excl);
> > + if (old) {
> > + if (dma_fence_remove_callback(old, &ref->excl_cb))
> > + atomic_dec(&ref->count);
> > + dma_fence_put(old);
> > + }
>
> Put some commentary in describing the business done with ref->excl and
> callbacks. Or this goes away later?
void i915_active_set_exclusive(struct i915_active *ref, struct dma_fence *f)
{
GEM_BUG_ON(i915_active_is_idle(ref));
mutex_acquire(&ref->mutex.dep_map, 0, 0, _THIS_IP_);
if (!__i915_active_fence_set(&ref->excl, f))
atomic_inc(&ref->count);
mutex_release(&ref->mutex.dep_map, 0, _THIS_IP_);
}
> > +static int excl_wait(struct i915_active *ref)
> > +{
> > + struct dma_fence *old;
> > + int err = 0;
> > +
> > + if (!rcu_access_pointer(ref->excl))
> > + return 0;
> > +
> > + rcu_read_lock();
> > + old = dma_fence_get_rcu_safe(&ref->excl);
> > + rcu_read_unlock();
>
> ref->excl can go something to NULL but not NULL to something in here?
Yes, but the wait is on the snapshot of fences at that moment in time.
For things like unbind if it becomes busy again after we start, what is
the best course of action -- currently we swear and try again.
The problem is more evident later in the comment:
/* Any fence added after the wait begins will not be auto-signaled */
which means the wait may end waiting for the background worker to flush
fences.
> > int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
> > {
> > - struct active_node *it, *n;
> > - int err;
> > -
> > - if (RB_EMPTY_ROOT(&ref->tree))
> > - return 0;
> > + int err = 0;
> >
> > - /* await allocates and so we need to avoid hitting the shrinker */
> > - err = i915_active_acquire(ref);
> > - if (err)
> > - return err;
> > + if (rcu_access_pointer(ref->excl)) {
>
> excl_wait used if-not-return pattern, but essentialy the same question
> about possible races. I guess there aren't any since only retirement is
> relevant which is something-to-NULL and that's handled fine. But I am
> not sure, possibly comments are in order.
It's the same as any other wait on a set-of-fence, the wait is on the
snapshot of fences at that moment.
If you need stricter control (such as ensuring ordering within a
particular timeline) you have to provide the ordering on that external
timeline. That's the difference between simply waiting on the
i915_active, and setting a fence within an i915_active_fence (the latter
has a strict order).
> > @@ -109,20 +121,31 @@ int i915_gem_object_unbind(struct drm_i915_gem_object *obj,
> > LIST_HEAD(still_in_list);
> > int ret = 0;
> >
> > - lockdep_assert_held(&obj->base.dev->struct_mutex);
> > -
> > spin_lock(&obj->vma.lock);
> > while (!ret && (vma = list_first_entry_or_null(&obj->vma.list,
> > struct i915_vma,
> > obj_link))) {
> > + struct i915_address_space *vm = vma->vm;
> > +
> > + ret = -EBUSY;
> > + if (!i915_vm_tryopen(vm))
> > + break;
>
> This is some race between different paths of vm closing/destruction and
> vma cleanup? We need a comment about it somewhere.. Here I guess is a
> good place.
4 different paths all fighting; and coordinate by preventing the other 2
destruction paths from running as they run, and spinlocks to prevent the
other.
> And why break and not continue, there will be a second call coming from
> somewhere when it fails?
It failed to unbind the object, so it would end up in a busy spin
preventing the actual vm release from removing the vma.
> > @@ -1015,14 +1016,9 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
> > return ERR_PTR(-ENOSPC);
> > }
> >
> > - WARN(i915_vma_is_pinned(vma),
> > - "bo is already pinned in ggtt with incorrect alignment:"
> > - " offset=%08x, req.alignment=%llx,"
> > - " req.map_and_fenceable=%d, vma->map_and_fenceable=%d\n",
> > - i915_ggtt_offset(vma), alignment,
> > - !!(flags & PIN_MAPPABLE),
> > - i915_vma_is_map_and_fenceable(vma));
>
> Why removing the WARN?
It's fundamentally racy. Not until you lock, flush the active, do you
know whether or not the vma is actually pinned. (Whereas previously it
was locked under struct_mutex.) With multiple simultaneous pinners, the
chance that we end up with a pin we do not like is increased. (Hmm,
early on this entire function was under a single vm->mutex, that removes
some race conditions that currently cause an unlikely failure at the
end, but such races would be ping-ponging, so both strategies have their
faults.)
> > + mutex_lock(&vma->vm->mutex);
> > ret = i915_vma_unbind(vma);
> > + mutex_unlock(&vma->vm->mutex);
> > if (ret)
> > return ERR_PTR(ret);
> > }
> > @@ -127,15 +126,6 @@ i915_gem_evict_something(struct i915_address_space *vm,
> > min_size, alignment, cache_level,
> > start, end, mode);
> >
> > - /*
> > - * Retire before we search the active list. Although we have
> > - * reasonable accuracy in our retirement lists, we may have
> > - * a stray pin (preventing eviction) that can only be resolved by
> > - * retiring.
> > - */
> > - if (!(flags & PIN_NONBLOCK))
> > - i915_retire_requests(dev_priv);
>
> Not needed any more because?
It's illegal, requires struct_mutex, we do not hold struct_mutex. (Until
the end of this series when i915_retire_requests is independent of
struct_mutex and we restore it.)
> > @@ -175,7 +178,8 @@ static int ppgtt_bind_vma(struct i915_vma *vma,
> >
> > static void ppgtt_unbind_vma(struct i915_vma *vma)
> > {
> > - vma->vm->clear_range(vma->vm, vma->node.start, vma->size);
> > + if (test_and_clear_bit(I915_VMA_ALLOC_BIT, __i915_vma_flags(vma)))
> > + vma->vm->clear_range(vma->vm, vma->node.start, vma->size);
>
> VMA bound and allocated are now synonyms?
clear_range decrements the pt use counters that alloc incremented.
> > +struct i915_vma_work *i915_vma_work(void)
> > +{
> > + struct i915_vma_work *vw;
> > +
> > + vw = kzalloc(sizeof(*vw), GFP_KERNEL);
>
> This could be reasonably high traffic to warrant a dedicated slab.
It's also not going to survive for more than 2 kernel releases. :(
> > + if (!vw)
> > + return NULL;
> > +
> > + dma_fence_work_init(&vw->base, &bind_ops);
>
> Hm, new (for me) API.. what is the advantage/need compared to plain workers?
It is a plain worker with the dma-fence coupling we've all grown to
cherish.
> > @@ -333,9 +375,32 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level,
> > GEM_BUG_ON(!vma->pages);
> >
> > trace_i915_vma_bind(vma, bind_flags);
> > - ret = vma->ops->bind_vma(vma, cache_level, bind_flags);
> > - if (ret)
> > - return ret;
> > + if (work && (bind_flags & ~vma_flags) & vma->vm->bind_alloc) {
>
> What is the bitwise logic checking for?
Whether we are adding a bind that requires allocations that the caller
requires to be async.
> > + work->vma = vma;
> > + work->cache_level = cache_level;
> > + work->flags = bind_flags | I915_VMA_ALLOC;
> > +
> > + /*
> > + * Note we only want to chain up to the migration fence on
> > + * the pages (not the object itself). As we don't track that,
> > + * yet, we have to use the exclusive fence instead.
> > + *
> > + * Also note that we do not want to track the async vma as
> > + * part of the obj->resv->excl_fence as it only affects
> > + * execution and not content or object's backing store lifetime.
> > + */
> > + GEM_BUG_ON(i915_active_has_exclusive(&vma->active));
> > + i915_active_set_exclusive(&vma->active, &work->base.dma);
>
> Oh right, dma_fence_work since it's not a worker but callback on
> signalling the fence... From what context it gets called (holding any
> locks?) and which locks the callback can/will take?
dma_fence_work has a dma_fence_cb that schedules a work_stuct which
executes our function. (And it also has a dma-fence through which we can
signal the completion/error of that function.)
> > @@ -368,7 +431,8 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma *vma)
> > goto err;
> > }
> >
> > - vma->iomap = ptr;
> > + if (unlikely(cmpxchg(&vma->iomap, NULL, ptr)))
> > + io_mapping_unmap(ptr);
>
> Why this change?
Multiple pinners race to see who win the io_mapping competition. Each
thread does their own allocation, if first they pass ownership to the
vma, if last they delete their own io_mapping and go on to use the
winner's vma->iomap. Except an idiot forgot to update the loser's ptr
here.
> > @@ -580,35 +640,21 @@ i915_vma_insert(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> > return -ENOSPC;
> > }
> >
> > - if (vma->obj) {
> > - ret = i915_gem_object_pin_pages(vma->obj);
> > - if (ret)
> > - return ret;
> > -
> > + cache_level = 0;
> > + if (vma->obj)
> > cache_level = vma->obj->cache_level;
> > - } else {
> > - cache_level = 0;
> > - }
> > -
> > - GEM_BUG_ON(vma->pages);
> > -
> > - ret = vma->ops->set_pages(vma);
> > - if (ret)
> > - goto err_unpin;
> >
> > if (flags & PIN_OFFSET_FIXED) {
> > u64 offset = flags & PIN_OFFSET_MASK;
> > if (!IS_ALIGNED(offset, alignment) ||
> > - range_overflows(offset, size, end)) {
> > - ret = -EINVAL;
> > - goto err_clear;
> > - }
> > + range_overflows(offset, size, end))
>
> A lot of checks/ops removed from here.. I think I'll need to apply to
> figure out how flows work. Or review on high level and have faith in CI.
Checks didn't get dropped, just the ops moved to the caller. Those that
need to be done before the mutex have to be in the caller. This function
is now only responsible for find/reserving the place in the drm_mm.
> > +static bool try_fast_pin(struct i915_vma *vma, unsigned int flags)
> > {
> > - const unsigned int bound = atomic_read(&vma->flags);
> > - int ret;
> > + unsigned int bound;
> > + bool pinned = true;
> >
> > - lockdep_assert_held(&vma->vm->i915->drm.struct_mutex);
> > - GEM_BUG_ON((flags & (PIN_GLOBAL | PIN_USER)) == 0);
> > - GEM_BUG_ON((flags & PIN_GLOBAL) && !i915_vma_is_ggtt(vma));
> > + bound = atomic_read(&vma->flags);
> > + do {
> > + if (unlikely(flags & ~bound))
> > + return false;
>
> What is this checking for?
If the requested binding (flags) is already available (bound).
> > - if (WARN_ON(bound & I915_VMA_PIN_OVERFLOW)) {
> > - ret = -EBUSY;
> > - goto err_unpin;
> > + if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR)))
> > + return false;
>
> What will the caller try to do in these cases?
Not do the early return.
> > +
> > + if (!(bound & I915_VMA_PIN_MASK))
> > + goto slow;
>
> Since fast path can go to slow, not sure what is fast referring to.
>
> > +
> > + GEM_BUG_ON(((bound + 1) & I915_VMA_PIN_MASK) == 0);
> > + } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1));
> > +
> > + return true;
> > +
> > +slow:
> > + /*
> > + * If pin_count==0, but we are bound, check under the lock to avoid
> > + * racing with a concurrent i915_vma_unbind().
> > + */
> > + mutex_lock(&vma->vm->mutex);
>
> This is never from process context to need to be interruptible?
I started with a tristate return, didn't like it much (try() means bool
to me). It can be _interruptible here, we then do an alloc before giving
up.
> > + do {
> > + if (unlikely(bound & (I915_VMA_OVERFLOW | I915_VMA_ERROR))) {
> > + pinned = false;
> > + break;
> > + }
> > +
> > + if (unlikely(flags & ~bound)) {
> > + pinned = false;
> > + break;
> > + }
> > + } while (!atomic_try_cmpxchg(&vma->flags, &bound, bound + 1));
> > + mutex_unlock(&vma->vm->mutex);
>
> slow = 0 -> 1 pin transition, the rest are fast?
1+ are fast; unbound is slowest.
> > +
> > + return pinned;
> > +}
> > +
> > +static int vma_get_pages(struct i915_vma *vma)
> > +{
> > + int err = 0;
> > +
> > + if (atomic_add_unless(&vma->pages_count, 1, 0))
> > + return 0;
> > +
> > + if (mutex_lock_interruptible(&vma->pages_mutex))
> > + return -EINTR;
> > +
> > + if (!atomic_read(&vma->pages_count)) {
> > + if (vma->obj) {
> > + err = i915_gem_object_pin_pages(vma->obj);
> > + if (err)
> > + goto unlock;
> > + }
> > +
> > + err = vma->ops->set_pages(vma);
> > + if (err)
> > + goto unlock;
> > }
> > + atomic_inc(&vma->pages_count);
> >
> > - if ((bound & I915_VMA_BIND_MASK) == 0) {
> > - ret = i915_vma_insert(vma, size, alignment, flags);
> > - if (ret)
> > - goto err_unpin;
> > +unlock:
> > + mutex_unlock(&vma->pages_mutex);
> > +
> > + return err;
> > +}
> > +
> > +static void __vma_put_pages(struct i915_vma *vma, unsigned int count)
> > +{
> > + mutex_lock_nested(&vma->pages_mutex, SINGLE_DEPTH_NESTING);
>
> Nesting annotation only needed on the put path? Comment to describe why
> it is needed would be good.
It's an allocating mutex, so we can be called from underneath another
vma->pages_mutex. Standard misery.
> > + GEM_BUG_ON(atomic_read(&vma->pages_count) < count);
> > + if (atomic_sub_return(count, &vma->pages_count) == 0) {
> > + vma->ops->clear_pages(vma);
> > + GEM_BUG_ON(vma->pages);
> > + if (vma->obj)
> > + i915_gem_object_unpin_pages(vma->obj);
> > }
> > - GEM_BUG_ON(!drm_mm_node_allocated(&vma->node));
> > + mutex_unlock(&vma->pages_mutex);
> > +}
> >
> > - ret = i915_vma_bind(vma, vma->obj ? vma->obj->cache_level : 0, flags);
> > - if (ret)
> > - goto err_remove;
> > +static void vma_put_pages(struct i915_vma *vma)
> > +{
> > + if (atomic_add_unless(&vma->pages_count, -1, 1))
> > + return;
> >
> > - GEM_BUG_ON(!i915_vma_is_bound(vma, I915_VMA_BIND_MASK));
> > + __vma_put_pages(vma, 1);
> > +}
> > +
> > +static void vma_unbind_pages(struct i915_vma *vma)
> > +{
> > + unsigned int count;
> > +
> > + lockdep_assert_held(&vma->vm->mutex);
> > +
> > + count = atomic_read(&vma->pages_count);
> > + count >>= I915_VMA_PAGES_BIAS;
> > + GEM_BUG_ON(!count);
> > +
> > + __vma_put_pages(vma, count | count << I915_VMA_PAGES_BIAS);
>
> I think we need documentation on various flags and possible states.
It literally is just a bias! (2 counters within one.)
> > +int i915_vma_pin(struct i915_vma *vma, u64 size, u64 alignment, u64 flags)
> > +{
> > + struct i915_vma_work *work = NULL;
> > + unsigned int bound;
> > + int err;
> > +
> > + BUILD_BUG_ON(PIN_GLOBAL != I915_VMA_GLOBAL_BIND);
> > + BUILD_BUG_ON(PIN_USER != I915_VMA_LOCAL_BIND);
> > +
> > + GEM_BUG_ON(flags & PIN_UPDATE);
> > + GEM_BUG_ON(!(flags & (PIN_USER | PIN_GLOBAL)));
> >
> > - if ((bound ^ atomic_read(&vma->flags)) & I915_VMA_GLOBAL_BIND)
> > - __i915_vma_set_map_and_fenceable(vma);
> > + if (try_fast_pin(vma, flags & I915_VMA_BIND_MASK))
> > + return 0;
> > +
> > + err = vma_get_pages(vma);
> > + if (err)
> > + return err;
> > +
> > + if (flags & PIN_USER) {
> > + work = i915_vma_work();
> > + if (!work) {
> > + err = -ENOMEM;
> > + goto err_pages;
> > + }
> > + }
>
> Good place for a comment to explain why PIN_USER needs to go via worker
> and the rest dont.
Probably will change if my plans for bsw/bxt come to fruition. And
certainly will change again as we will need to pass down the fence...
> > + err = mutex_lock_interruptible(&vma->vm->mutex);
> > + if (err)
> > + goto err_fence;
> > +
> > + bound = atomic_read(&vma->flags);
> > + if (unlikely(bound & I915_VMA_ERROR)) {
> > + err = -ENOMEM;
> > + goto err_unlock;
> > + }
> >
> > + if (unlikely(!((bound + 1) & I915_VMA_PIN_MASK))) {
> > + err = -EAGAIN; /* pins are meant to be fairly temporary */
> > + goto err_unlock;
> > + }
>
> Did not get what this is?
The overflow test.
> > + if (unlikely(!(flags & ~bound & I915_VMA_BIND_MASK))) {
> > + __i915_vma_pin(vma);
> > + goto err_unlock;
> > + }
>
> Or this. (Comment please.)
Already bound by another thread.
> > +
> > + err = i915_active_acquire(&vma->active);
>
> It's dropped somewhere to release it into normal flows?
Not many lines below before we exit. The active is acquired again for
the async operation to avoid the early release (on the success path).
> > + if (err)
> > + goto err_unlock;
> > +
> > + if (!(bound & I915_VMA_BIND_MASK)) {
> > + err = i915_vma_insert(vma, size, alignment, flags);
> > + if (err)
> > + goto err_active;
> > +
> > + if (i915_is_ggtt(vma->vm))
> > + __i915_vma_set_map_and_fenceable(vma);
> > + }
> > +
> > + GEM_BUG_ON(!vma->pages);
> > + err = i915_vma_bind(vma,
> > + vma->obj ? vma->obj->cache_level : 0,
> > + flags, work);
> > + if (err)
> > + goto err_remove;
> > +
> > + /* There should only be at most 2 active bindings (user, global) */
>
> s/bindings/pins/ ? I thought one vma is one binding by definition.. so I
> am confused.
Oh no, that would be far too simple. On gen6-7 & bsw, the vma is aliased.
One vma, two binding (PIN_USER -> ggtt->alias, PIN_GLOBAL -> ggtt).
> > + GEM_BUG_ON(bound + I915_VMA_PAGES_ACTIVE < bound);
> > + atomic_add(I915_VMA_PAGES_ACTIVE, &vma->pages_count);
> > + list_move_tail(&vma->vm_link, &vma->vm->bound_list);
> > +
> > + __i915_vma_pin(vma);
> > + GEM_BUG_ON(!i915_vma_is_pinned(vma));
> > + GEM_BUG_ON(!i915_vma_is_bound(vma, flags));
> > GEM_BUG_ON(i915_vma_misplaced(vma, size, alignment, flags));
> > - return 0;
> >
> > err_remove:
> > - if ((bound & I915_VMA_BIND_MASK) == 0) {
> > + if (!i915_vma_is_bound(vma, I915_VMA_BIND_MASK))
> > i915_vma_remove(vma);
> > - GEM_BUG_ON(vma->pages);
> > - GEM_BUG_ON(atomic_read(&vma->flags) & I915_VMA_BIND_MASK);
> > - }
> > -err_unpin:
> > - __i915_vma_unpin(vma);
> > - return ret;
> > +err_active:
> > + i915_active_release(&vma->active);
> > +err_unlock:
> > + mutex_unlock(&vma->vm->mutex);
> > +err_fence:
> > + if (work)
> > + dma_fence_work_commit(&work->base);
> > +err_pages:
> > + vma_put_pages(vma);
> > + return err;
> > }
> > void i915_vma_parked(struct drm_i915_private *i915)
> > @@ -831,12 +1011,32 @@ void i915_vma_parked(struct drm_i915_private *i915)
> >
> > spin_lock_irq(&i915->gt.closed_lock);
> > list_for_each_entry_safe(vma, next, &i915->gt.closed_vma, closed_link) {
> > - list_del_init(&vma->closed_link);
> > + struct drm_i915_gem_object *obj = vma->obj;
> > + struct i915_address_space *vm = vma->vm;
> > +
> > + /* XXX All to avoid keeping a reference on i915_vma itself */
>
> Does XXX signify it is a temporary state of code, or just like a comment
> but more important in some way?
It's a critique of the ugly code (the comment you asked for before) that
is required because I didn't want to have to throw away the entire lmem
object handling code.
> > @@ -976,7 +977,6 @@ static int igt_vma_remapped_gtt(void *arg)
> >
> > out:
> > intel_runtime_pm_put(&i915->runtime_pm, wakeref);
> > - mutex_unlock(&i915->drm.struct_mutex);
> > i915_gem_object_put(obj);
> >
> > return err;
> >
>
> In general, even when I did not explicitly asked for a comment, places
> where things weren't clear to me should be considered to have them added
> and probably more.
How else do I know what isn't clear to you... ;)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
