On 25-10-2021 17:02, Matthew Auld wrote:
> On Thu, 21 Oct 2021 at 11:37, Maarten Lankhorst
> <maarten.lankhorst@xxxxxxxxxxxxxxx> wrote:
>> Add a flag PIN_VALIDATE, to indicate we don't need to pin and only
>> protected by the object lock.
>>
>> This removes the need to unpin, which is done by just releasing the
>> lock.
>>
>> eb_reserve is slightly reworked for readability, but the same steps
>> are still done:
>> - First pass pins with NONBLOCK.
>> - Second pass unbinds all objects first, then pins.
>> - Third pass is only called when not all objects are softpinned, and
>> unbinds all objects, then calls i915_gem_evict_vm(), then pins.
>>
>> When evicting the entire vm in eb_reserve() we do temporarily pin objects
>> that are marked with EXEC_OBJECT_PINNED. This is because they are already
>> at their destination, and i915_gem_evict_vm() would otherwise unbind them.
>>
>> However, we reduce the visibility of those pins by limiting the pin
>> to our call to i915_gem_evict_vm() only, and pin with vm->mutex held,
>> instead of the entire duration of the execbuf.
>>
>> Not sure the latter matters, one can hope..
>> In theory we could kill the pinning by adding an extra flag to the vma
>> to temporarily prevent unbinding for gtt for i915_gem_evict_vm only, but
>> I think that might be overkill. We're still holding the object lock, and
>> we don't have blocking eviction yet. It's likely sufficient to simply
>> enforce EXEC_OBJECT_PINNED for all objects on >= gen12.
>>
>> Changes since v1:
>> - Split out eb_reserve() into separate functions for readability.
>> Changes since v2:
>> - Make batch buffer mappable on platforms where only GGTT is available,
>> to prevent moving the batch buffer during relocations.
>> Changes since v3:
>> - Preserve current behavior for batch buffer, instead be cautious when
>> calling i915_gem_object_ggtt_pin_ww, and re-use the current batch vma
>> if it's inside ggtt and map-and-fenceable.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>> ---
>> .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 252 ++++++++++--------
>> drivers/gpu/drm/i915/i915_gem_gtt.h | 1 +
>> drivers/gpu/drm/i915/i915_vma.c | 24 +-
>> 3 files changed, 161 insertions(+), 116 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> index bbf2a10738f7..19f91143cfcf 100644
>> --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c
>> @@ -439,7 +439,7 @@ eb_pin_vma(struct i915_execbuffer *eb,
>> else
>> pin_flags = entry->offset & PIN_OFFSET_MASK;
>>
>> - pin_flags |= PIN_USER | PIN_NOEVICT | PIN_OFFSET_FIXED;
>> + pin_flags |= PIN_USER | PIN_NOEVICT | PIN_OFFSET_FIXED | PIN_VALIDATE;
>> if (unlikely(ev->flags & EXEC_OBJECT_NEEDS_GTT))
>> pin_flags |= PIN_GLOBAL;
>>
>> @@ -457,17 +457,15 @@ eb_pin_vma(struct i915_execbuffer *eb,
>> entry->pad_to_size,
>> entry->alignment,
>> eb_pin_flags(entry, ev->flags) |
>> - PIN_USER | PIN_NOEVICT);
>> + PIN_USER | PIN_NOEVICT | PIN_VALIDATE);
>> if (unlikely(err))
>> return err;
>> }
>>
>> if (unlikely(ev->flags & EXEC_OBJECT_NEEDS_FENCE)) {
>> err = i915_vma_pin_fence(vma);
>> - if (unlikely(err)) {
>> - i915_vma_unpin(vma);
>> + if (unlikely(err))
>> return err;
>> - }
>>
>> if (vma->fence)
>> ev->flags |= __EXEC_OBJECT_HAS_FENCE;
>> @@ -483,13 +481,9 @@ eb_pin_vma(struct i915_execbuffer *eb,
>> static inline void
>> eb_unreserve_vma(struct eb_vma *ev)
>> {
>> - if (!(ev->flags & __EXEC_OBJECT_HAS_PIN))
>> - return;
>> -
>> if (unlikely(ev->flags & __EXEC_OBJECT_HAS_FENCE))
>> __i915_vma_unpin_fence(ev->vma);
>>
>> - __i915_vma_unpin(ev->vma);
>> ev->flags &= ~__EXEC_OBJECT_RESERVED;
>> }
>>
>> @@ -682,10 +676,8 @@ static int eb_reserve_vma(struct i915_execbuffer *eb,
>>
>> if (unlikely(ev->flags & EXEC_OBJECT_NEEDS_FENCE)) {
>> err = i915_vma_pin_fence(vma);
>> - if (unlikely(err)) {
>> - i915_vma_unpin(vma);
>> + if (unlikely(err))
>> return err;
>> - }
>>
>> if (vma->fence)
>> ev->flags |= __EXEC_OBJECT_HAS_FENCE;
>> @@ -697,85 +689,129 @@ static int eb_reserve_vma(struct i915_execbuffer *eb,
>> return 0;
>> }
>>
>> -static int eb_reserve(struct i915_execbuffer *eb)
>> +static int eb_evict_vm(struct i915_execbuffer *eb)
>> +{
>> + const unsigned int count = eb->buffer_count;
>> + unsigned int i;
>> + int err;
>> +
>> + err = mutex_lock_interruptible(&eb->context->vm->mutex);
>> + if (err)
>> + return err;
>> +
>> + /* pin to protect against i915_gem_evict_vm evicting below */
>> + for (i = 0; i < count; i++) {
>> + struct eb_vma *ev = &eb->vma[i];
>> +
>> + if (ev->flags & __EXEC_OBJECT_HAS_PIN)
>> + __i915_vma_pin(ev->vma);
>> + }
>> +
>> + /* Too fragmented, unbind everything and retry */
>> + err = i915_gem_evict_vm(eb->context->vm, &eb->ww);
>> +
>> + /* unpin objects.. */
>> + for (i = 0; i < count; i++) {
>> + struct eb_vma *ev = &eb->vma[i];
>> +
>> + if (ev->flags & __EXEC_OBJECT_HAS_PIN)
>> + i915_vma_unpin(ev->vma);
>> + }
>> +
>> + mutex_unlock(&eb->context->vm->mutex);
>> +
>> + return err;
>> +}
>> +
>> +static bool eb_unbind(struct i915_execbuffer *eb)
>> {
>> const unsigned int count = eb->buffer_count;
>> - unsigned int pin_flags = PIN_USER | PIN_NONBLOCK;
>> + unsigned int i;
>> struct list_head last;
>> + bool unpinned = false;
>> +
>> + /* Resort *all* the objects into priority order */
>> + INIT_LIST_HEAD(&eb->unbound);
>> + INIT_LIST_HEAD(&last);
>> +
>> + for (i = 0; i < count; i++) {
>> + struct eb_vma *ev = &eb->vma[i];
>> + unsigned int flags = ev->flags;
>> +
>> + if (flags & EXEC_OBJECT_PINNED &&
>> + flags & __EXEC_OBJECT_HAS_PIN)
>> + continue;
>> +
>> + unpinned = true;
>> + eb_unreserve_vma(ev);
>> +
>> + if (flags & EXEC_OBJECT_PINNED)
>> + /* Pinned must have their slot */
>> + list_add(&ev->bind_link, &eb->unbound);
>> + else if (flags & __EXEC_OBJECT_NEEDS_MAP)
>> + /* Map require the lowest 256MiB (aperture) */
>> + list_add_tail(&ev->bind_link, &eb->unbound);
>> + else if (!(flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS))
>> + /* Prioritise 4GiB region for restricted bo */
>> + list_add(&ev->bind_link, &last);
>> + else
>> + list_add_tail(&ev->bind_link, &last);
>> + }
>> +
>> + list_splice_tail(&last, &eb->unbound);
>> + return unpinned;
>> +}
>> +
>> +static int eb_reserve(struct i915_execbuffer *eb)
>> +{
>> struct eb_vma *ev;
>> - unsigned int i, pass;
>> + unsigned int pass;
>> int err = 0;
>> + bool unpinned;
>>
>> /*
>> * Attempt to pin all of the buffers into the GTT.
>> - * This is done in 3 phases:
>> + * This is done in 2 phases:
>> *
>> - * 1a. Unbind all objects that do not match the GTT constraints for
>> - * the execbuffer (fenceable, mappable, alignment etc).
>> - * 1b. Increment pin count for already bound objects.
>> - * 2. Bind new objects.
>> - * 3. Decrement pin count.
>> + * 1. Unbind all objects that do not match the GTT constraints for
>> + * the execbuffer (fenceable, mappable, alignment etc).
>> + * 2. Bind new objects.
>> *
>> * This avoid unnecessary unbinding of later objects in order to make
>> * room for the earlier objects *unless* we need to defragment.
>> + *
>> + * Defragmenting is skipped if all objects are pinned at a fixed location.
>> */
>> - pass = 0;
>> - do {
>> - list_for_each_entry(ev, &eb->unbound, bind_link) {
>> - err = eb_reserve_vma(eb, ev, pin_flags);
>> - if (err)
>> - break;
>> - }
>> - if (err != -ENOSPC)
>> - return err;
>> + for (pass = 0; pass <= 2; pass++) {
>> + int pin_flags = PIN_USER | PIN_VALIDATE;
>>
>> - /* Resort *all* the objects into priority order */
>> - INIT_LIST_HEAD(&eb->unbound);
>> - INIT_LIST_HEAD(&last);
>> - for (i = 0; i < count; i++) {
>> - unsigned int flags;
>> + if (pass == 0)
>> + pin_flags |= PIN_NONBLOCK;
>>
>> - ev = &eb->vma[i];
>> - flags = ev->flags;
>> - if (flags & EXEC_OBJECT_PINNED &&
>> - flags & __EXEC_OBJECT_HAS_PIN)
>> - continue;
>> + if (pass >= 1)
>> + unpinned = eb_unbind(eb);
>>
>> - eb_unreserve_vma(ev);
>> -
>> - if (flags & EXEC_OBJECT_PINNED)
>> - /* Pinned must have their slot */
>> - list_add(&ev->bind_link, &eb->unbound);
>> - else if (flags & __EXEC_OBJECT_NEEDS_MAP)
>> - /* Map require the lowest 256MiB (aperture) */
>> - list_add_tail(&ev->bind_link, &eb->unbound);
>> - else if (!(flags & EXEC_OBJECT_SUPPORTS_48B_ADDRESS))
>> - /* Prioritise 4GiB region for restricted bo */
>> - list_add(&ev->bind_link, &last);
>> - else
>> - list_add_tail(&ev->bind_link, &last);
>> - }
>> - list_splice_tail(&last, &eb->unbound);
>> -
>> - switch (pass++) {
>> - case 0:
>> - break;
>> + if (pass == 2) {
>> + /* No point in defragmenting gtt if all is pinned */
>> + if (!unpinned)
>> + return -ENOSPC;
> Can this ever happen? If everything is already pinned where it's meant
> to be, then how did we get here?
Hmm no, in previous versions it could, but it looks like I removed that in a refactor. Seems like dead code.
>
>> - case 1:
>> - /* Too fragmented, unbind everything and retry */
>> - mutex_lock(&eb->context->vm->mutex);
>> - err = i915_gem_evict_vm(eb->context->vm, &eb->ww);
>> - mutex_unlock(&eb->context->vm->mutex);
>> + err = eb_evict_vm(eb);
>> if (err)
>> return err;
>> - break;
>> + }
>>
>> - default:
>> - return -ENOSPC;
>> + list_for_each_entry(ev, &eb->unbound, bind_link) {
>> + err = eb_reserve_vma(eb, ev, pin_flags);
>> + if (err)
>> + break;
>> }
>>
>> - pin_flags = PIN_USER;
>> - } while (1);
>> + if (err != -ENOSPC)
>> + break;
>> + }
>> +
>> + return err;
>> }
>>
>> static int eb_select_context(struct i915_execbuffer *eb)
>> @@ -1184,10 +1220,11 @@ static void *reloc_kmap(struct drm_i915_gem_object *obj,
>> return vaddr;
>> }
>>
>> -static void *reloc_iomap(struct drm_i915_gem_object *obj,
>> +static void *reloc_iomap(struct i915_vma *batch,
>> struct i915_execbuffer *eb,
>> unsigned long page)
>> {
>> + struct drm_i915_gem_object *obj = batch->obj;
>> struct reloc_cache *cache = &eb->reloc_cache;
>> struct i915_ggtt *ggtt = cache_to_ggtt(cache);
>> unsigned long offset;
>> @@ -1197,7 +1234,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
>> intel_gt_flush_ggtt_writes(ggtt->vm.gt);
>> io_mapping_unmap_atomic((void __force __iomem *) unmask_page(cache->vaddr));
>> } else {
>> - struct i915_vma *vma;
>> + struct i915_vma *vma = ERR_PTR(-ENODEV);
>> int err;
>>
>> if (i915_gem_object_is_tiled(obj))
>> @@ -1210,10 +1247,23 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
>> if (err)
>> return ERR_PTR(err);
>>
>> - vma = i915_gem_object_ggtt_pin_ww(obj, &eb->ww, NULL, 0, 0,
>> - PIN_MAPPABLE |
>> - PIN_NONBLOCK /* NOWARN */ |
>> - PIN_NOEVICT);
>> + /*
>> + * i915_gem_object_ggtt_pin_ww may attempt to remove the batch
>> + * VMA from the object list because we no longer pin.
>> + *
>> + * Only attempt to pin the batch buffer to ggtt if the current batch
>> + * is not inside ggtt, or the batch buffer is not misplaced.
>> + */
>> + if (!i915_is_ggtt(batch->vm)) {
>> + vma = i915_gem_object_ggtt_pin_ww(obj, &eb->ww, NULL, 0, 0,
>> + PIN_MAPPABLE |
>> + PIN_NONBLOCK /* NOWARN */ |
>> + PIN_NOEVICT);
>> + } else if (i915_vma_is_map_and_fenceable(batch)) {
>> + __i915_vma_pin(batch);
>> + vma = batch;
>> + }
>> +
>> if (vma == ERR_PTR(-EDEADLK))
>> return vma;
>>
>> @@ -1251,7 +1301,7 @@ static void *reloc_iomap(struct drm_i915_gem_object *obj,
>> return vaddr;
>> }
>>
>> -static void *reloc_vaddr(struct drm_i915_gem_object *obj,
>> +static void *reloc_vaddr(struct i915_vma *vma,
>> struct i915_execbuffer *eb,
>> unsigned long page)
>> {
>> @@ -1263,9 +1313,9 @@ static void *reloc_vaddr(struct drm_i915_gem_object *obj,
>> } else {
>> vaddr = NULL;
>> if ((cache->vaddr & KMAP) == 0)
>> - vaddr = reloc_iomap(obj, eb, page);
>> + vaddr = reloc_iomap(vma, eb, page);
>> if (!vaddr)
>> - vaddr = reloc_kmap(obj, cache, page);
>> + vaddr = reloc_kmap(vma->obj, cache, page);
>> }
>>
>> return vaddr;
>> @@ -1306,7 +1356,7 @@ relocate_entry(struct i915_vma *vma,
>> void *vaddr;
>>
>> repeat:
>> - vaddr = reloc_vaddr(vma->obj, eb,
>> + vaddr = reloc_vaddr(vma, eb,
>> offset >> PAGE_SHIFT);
>> if (IS_ERR(vaddr))
>> return PTR_ERR(vaddr);
>> @@ -2074,7 +2124,7 @@ shadow_batch_pin(struct i915_execbuffer *eb,
>> if (IS_ERR(vma))
>> return vma;
>>
>> - err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, flags);
>> + err = i915_vma_pin_ww(vma, &eb->ww, 0, 0, flags | PIN_VALIDATE);
>> if (err)
>> return ERR_PTR(err);
>>
>> @@ -2088,7 +2138,7 @@ static struct i915_vma *eb_dispatch_secure(struct i915_execbuffer *eb, struct i9
>> * batch" bit. Hence we need to pin secure batches into the global gtt.
>> * hsw should have this fixed, but bdw mucks it up again. */
>> if (eb->batch_flags & I915_DISPATCH_SECURE)
>> - return i915_gem_object_ggtt_pin_ww(vma->obj, &eb->ww, NULL, 0, 0, 0);
>> + return i915_gem_object_ggtt_pin_ww(vma->obj, &eb->ww, NULL, 0, 0, PIN_VALIDATE);
>>
>> return NULL;
>> }
>> @@ -2139,13 +2189,12 @@ static int eb_parse(struct i915_execbuffer *eb)
>>
>> err = i915_gem_object_lock(pool->obj, &eb->ww);
>> if (err)
>> - goto err;
>> + return err;
>>
>> shadow = shadow_batch_pin(eb, pool->obj, eb->context->vm, PIN_USER);
>> - if (IS_ERR(shadow)) {
>> - err = PTR_ERR(shadow);
>> - goto err;
>> - }
>> + if (IS_ERR(shadow))
>> + return PTR_ERR(shadow);
>> +
>> intel_gt_buffer_pool_mark_used(pool);
>> i915_gem_object_set_readonly(shadow->obj);
>> shadow->private = pool;
>> @@ -2157,25 +2206,21 @@ static int eb_parse(struct i915_execbuffer *eb)
>> shadow = shadow_batch_pin(eb, pool->obj,
>> &eb->gt->ggtt->vm,
>> PIN_GLOBAL);
>> - if (IS_ERR(shadow)) {
>> - err = PTR_ERR(shadow);
>> - shadow = trampoline;
>> - goto err_shadow;
>> - }
>> + if (IS_ERR(shadow))
>> + return PTR_ERR(shadow);
>> +
>> shadow->private = pool;
>>
>> eb->batch_flags |= I915_DISPATCH_SECURE;
>> }
>>
>> batch = eb_dispatch_secure(eb, shadow);
>> - if (IS_ERR(batch)) {
>> - err = PTR_ERR(batch);
>> - goto err_trampoline;
>> - }
>> + if (IS_ERR(batch))
>> + return PTR_ERR(batch);
>>
>> err = dma_resv_reserve_shared(shadow->obj->base.resv, 1);
>> if (err)
>> - goto err_trampoline;
>> + return err;
>>
>> err = intel_engine_cmd_parser(eb->context->engine,
>> eb->batches[0]->vma,
>> @@ -2183,7 +2228,7 @@ static int eb_parse(struct i915_execbuffer *eb)
>> eb->batch_len[0],
>> shadow, trampoline);
>> if (err)
>> - goto err_unpin_batch;
>> + return err;
>>
>> eb->batches[0] = &eb->vma[eb->buffer_count++];
>> eb->batches[0]->vma = i915_vma_get(shadow);
>> @@ -2202,17 +2247,6 @@ static int eb_parse(struct i915_execbuffer *eb)
>> eb->batches[0]->vma = i915_vma_get(batch);
>> }
>> return 0;
>> -
>> -err_unpin_batch:
>> - if (batch)
>> - i915_vma_unpin(batch);
>> -err_trampoline:
>> - if (trampoline)
>> - i915_vma_unpin(trampoline);
>> -err_shadow:
>> - i915_vma_unpin(shadow);
>> -err:
>> - return err;
>> }
>>
>> static int eb_request_submit(struct i915_execbuffer *eb,
>> @@ -3337,8 +3371,6 @@ i915_gem_do_execbuffer(struct drm_device *dev,
>>
>> err_vma:
>> eb_release_vmas(&eb, true);
>> - if (eb.trampoline)
>> - i915_vma_unpin(eb.trampoline);
>> WARN_ON(err == -EDEADLK);
>> i915_gem_ww_ctx_fini(&eb.ww);
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> index e4938aba3fe9..8c2f57eb5dda 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
>> @@ -44,6 +44,7 @@ int i915_gem_gtt_insert(struct i915_address_space *vm,
>> #define PIN_HIGH BIT_ULL(5)
>> #define PIN_OFFSET_BIAS BIT_ULL(6)
>> #define PIN_OFFSET_FIXED BIT_ULL(7)
>> +#define PIN_VALIDATE BIT_ULL(8) /* validate placement only, no need to call unpin() */
>>
>> #define PIN_GLOBAL BIT_ULL(10) /* I915_VMA_GLOBAL_BIND */
>> #define PIN_USER BIT_ULL(11) /* I915_VMA_LOCAL_BIND */
>> diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c
>> index 65168db534f0..0706731b211d 100644
>> --- a/drivers/gpu/drm/i915/i915_vma.c
>> +++ b/drivers/gpu/drm/i915/i915_vma.c
>> @@ -751,6 +751,15 @@ static bool try_qad_pin(struct i915_vma *vma, unsigned int flags)
>> unsigned int bound;
>>
>> bound = atomic_read(&vma->flags);
>> +
>> + if (flags & PIN_VALIDATE) {
>> + flags &= I915_VMA_BIND_MASK;
>> +
>> + return (flags & bound) == flags;
>> + }
>> +
>> + /* with the lock mandatory for unbind, we don't race here */
>> + flags &= I915_VMA_BIND_MASK;
>> do {
>> if (unlikely(flags & ~bound))
>> return false;
>> @@ -1176,7 +1185,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> GEM_BUG_ON(!(flags & (PIN_USER | PIN_GLOBAL)));
>>
>> /* First try and grab the pin without rebinding the vma */
>> - if (try_qad_pin(vma, flags & I915_VMA_BIND_MASK))
>> + if (try_qad_pin(vma, flags))
>> return 0;
>>
>> err = i915_vma_get_pages(vma);
>> @@ -1255,7 +1264,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> }
>>
>> if (unlikely(!(flags & ~bound & I915_VMA_BIND_MASK))) {
>> - __i915_vma_pin(vma);
>> + if (!(flags & PIN_VALIDATE))
>> + __i915_vma_pin(vma);
>> goto err_unlock;
>> }
>>
>> @@ -1284,8 +1294,10 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww,
>> 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));
>> + if (!(flags & PIN_VALIDATE)) {
>> + __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));
>>
>> @@ -1538,8 +1550,6 @@ static int __i915_vma_move_to_active(struct i915_vma *vma, struct i915_request *
>> {
>> int err;
>>
>> - GEM_BUG_ON(!i915_vma_is_pinned(vma));
>> -
>> /* Wait for the vma to be bound before we start! */
>> err = __i915_request_await_bind(rq, vma);
>> if (err)
>> @@ -1558,6 +1568,8 @@ int _i915_vma_move_to_active(struct i915_vma *vma,
>>
>> assert_object_held(obj);
>>
>> + GEM_BUG_ON(!vma->pages);
>> +
>> err = __i915_vma_move_to_active(vma, rq);
>> if (unlikely(err))
>> return err;
>> --
>> 2.33.0
>>
