Re: [PATCH v5 1/6] drm/i915: Initial introduction of vma resources

On 04/01/2022 12:51, Thomas Hellström wrote:

Introduce vma resources, sort of similar to TTM resources, needed for asynchronous bind management. Initially we will use them to hold completion of unbinding when we capture data from a vma, but they will be used extensively in upcoming patches for asynchronous vma unbinding.

Signed-off-by: Thomas Hellström thomas.hellstrom@linux.intel.com

drivers/gpu/drm/i915/Makefile | 1 + .../gpu/drm/i915/gem/i915_gem_execbuffer.c | 2 +- drivers/gpu/drm/i915/i915_vma.c | 55 +++++++- drivers/gpu/drm/i915/i915_vma.h | 19 ++- drivers/gpu/drm/i915/i915_vma_resource.c | 124 ++++++++++++++++++ drivers/gpu/drm/i915/i915_vma_resource.h | 70 ++++++++++ drivers/gpu/drm/i915/i915_vma_snapshot.c | 15 +-- drivers/gpu/drm/i915/i915_vma_snapshot.h | 7 +- drivers/gpu/drm/i915/i915_vma_types.h | 5 + drivers/gpu/drm/i915/selftests/i915_gem_gtt.c | 99 ++++++++------ 10 files changed, 334 insertions(+), 63 deletions(-) create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.c create mode 100644 drivers/gpu/drm/i915/i915_vma_resource.h

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index 1b62b9f65196..98433ad74194 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -174,6 +174,7 @@ i915-y += \ i915_trace_points.o \ i915_ttm_buddy_manager.o \ i915_vma.o \

•  i915_vma_resource.o \
  

  i915_vma_snapshot.o \ intel_wopcm.o

diff --git a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c index e9541244027a..72e497745c12 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/gem/i915_gem_execbuffer.c @@ -1422,7 +1422,7 @@ eb_relocate_entry(struct i915_execbuffer *eb, mutex_lock(&vma->vm->mutex); err = i915_vma_bind(target->vma, target->vma->obj->cache_level,

• 			    PIN_GLOBAL, NULL);
  

• 			    PIN_GLOBAL, NULL, NULL);
  mutex_unlock(&vma->vm->mutex);
  reloc_cache_remap(&eb->reloc_cache, ev->vma->obj);
  if (err)
  

diff --git a/drivers/gpu/drm/i915/i915_vma.c b/drivers/gpu/drm/i915/i915_vma.c index be208a8f1ed0..7097c5016431 100644 --- a/drivers/gpu/drm/i915/i915_vma.c +++ b/drivers/gpu/drm/i915/i915_vma.c @@ -37,6 +37,7 @@ #include "i915_sw_fence_work.h" #include "i915_trace.h" #include "i915_vma.h" +#include "i915_vma_resource.h"

static struct kmem_cache *slab_vmas;

@@ -380,6 +381,8 @@ static int i915_vma_verify_bind_complete(struct i915_vma *vma)

• @cache_level: mapping cache level
• @flags: flags like global or local mapping
• @work: preallocated worker for allocating and binding the PTE

• • @vma_res: pointer to a preallocated vma resource. The resource is either
• • consumed or freed.
  • DMA addresses are taken from the scatter-gather table of this object (or of
  • this VMA in case of non-default GGTT views) and PTE entries set up.

@@ -388,7 +391,8 @@ static int i915_vma_verify_bind_complete(struct i915_vma *vma) int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags,

•   struct i915_vma_work *work)
  

•   struct i915_vma_work *work,
  

•   struct i915_vma_resource *vma_res)
  

  { u32 bind_flags; u32 vma_flags;

@@ -399,11 +403,15 @@ int i915_vma_bind(struct i915_vma *vma,

if (GEM_DEBUG_WARN_ON(range_overflows(vma->node.start, vma->node.size,

• 			      vma->vm->total)))
  

• 			      vma->vm->total))) {
  

• kfree(vma_res);
  

  return -ENODEV;
• }

• if (GEM_DEBUG_WARN_ON(!flags))

• if (GEM_DEBUG_WARN_ON(!flags)) {

• kfree(vma_res);
  

  return -EINVAL;

• }

  bind_flags = flags; bind_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;

@@ -412,11 +420,21 @@ int i915_vma_bind(struct i915_vma *vma, vma_flags &= I915_VMA_GLOBAL_BIND | I915_VMA_LOCAL_BIND;

bind_flags &= ~vma_flags;

• if (bind_flags == 0)

• if (bind_flags == 0) {

• kfree(vma_res);
  

  return 0;

• }

  GEM_BUG_ON(!atomic_read(&vma->pages_count));

• if (vma->resource || !vma_res) {

• /* Rebinding with an additional I915_VMA_*_BIND */
  

• GEM_WARN_ON(!vma_flags);
  

• kfree(vma_res);
  

• } else {

• i915_vma_resource_init(vma_res);
  

• vma->resource = vma_res;
  

• } trace_i915_vma_bind(vma, bind_flags); if (work && bind_flags & vma->vm->bind_async_flags) { struct dma_fence *prev;

@@ -1279,6 +1297,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, { struct i915_vma_work *work = NULL; struct dma_fence *moving = NULL;

• struct i915_vma_resource *vma_res = NULL; intel_wakeref_t wakeref = 0; unsigned int bound; int err;

@@ -1333,6 +1352,12 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, } }

• vma_res = i915_vma_resource_alloc();
• if (IS_ERR(vma_res)) {

• err = PTR_ERR(vma_res);
  

• goto err_fence;
  

• }
• /*
  • Differentiate between user/kernel vma inside the aliasing-ppgtt.

@@ -1353,7 +1378,7 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, err = mutex_lock_interruptible_nested(&vma->vm->mutex, !(flags & PIN_GLOBAL)); if (err)

• goto err_fence;
  

• goto err_vma_res;
  
  

  /* No more allocations allowed now we hold vm->mutex */

@@ -1394,7 +1419,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, GEM_BUG_ON(!vma->pages); err = i915_vma_bind(vma, vma->obj->cache_level,

• 	    flags, work);
  

• 	    flags, work, vma_res);
  

• vma_res = NULL; if (err) goto err_remove;

@@ -1417,6 +1443,8 @@ int i915_vma_pin_ww(struct i915_vma *vma, struct i915_gem_ww_ctx *ww, i915_active_release(&vma->active); err_unlock: mutex_unlock(&vma->vm->mutex); +err_vma_res:

• kfree(vma_res); err_fence: if (work) dma_fence_work_commit_imm(&work->base);

@@ -1567,6 +1595,7 @@ void i915_vma_release(struct kref *ref) i915_vm_put(vma->vm);

i915_active_fini(&vma->active);

• GEM_WARN_ON(vma->resource); i915_vma_free(vma); }

@@ -1715,6 +1744,8 @@ int _i915_vma_move_to_active(struct i915_vma *vma,

void __i915_vma_evict(struct i915_vma *vma) {

• struct dma_fence *unbind_fence;

• GEM_BUG_ON(i915_vma_is_pinned(vma));

  if (i915_vma_is_map_and_fenceable(vma)) {

@@ -1752,8 +1783,20 @@ void __i915_vma_evict(struct i915_vma *vma) atomic_and(~(I915_VMA_BIND_MASK | I915_VMA_ERROR | I915_VMA_GGTT_WRITE), &vma->flags);

• unbind_fence = i915_vma_resource_unbind(vma->resource);

• i915_vma_resource_put(vma->resource);

• vma->resource = NULL;

• i915_vma_detach(vma); vma_unbind_pages(vma);

• /*

• * This uninterruptible wait under the vm mutex is currently
  

• * only ever blocking while the vma is being captured from.
  

• * With async unbinding, this wait here will be removed.
  

• */
  

• dma_fence_wait(unbind_fence, false);

• dma_fence_put(unbind_fence); }

  int __i915_vma_unbind(struct i915_vma *vma)

diff --git a/drivers/gpu/drm/i915/i915_vma.h b/drivers/gpu/drm/i915/i915_vma.h index 32719431b3df..de0f3e44cdfa 100644 --- a/drivers/gpu/drm/i915/i915_vma.h +++ b/drivers/gpu/drm/i915/i915_vma.h @@ -37,6 +37,7 @@

#include "i915_active.h" #include "i915_request.h" +#include "i915_vma_resource.h" #include "i915_vma_types.h"

struct i915_vma * @@ -204,7 +205,8 @@ struct i915_vma_work *i915_vma_work(void); int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags,

•   struct i915_vma_work *work);
  

•   struct i915_vma_work *work,
  

•   struct i915_vma_resource *vma_res);
  
  

  bool i915_gem_valid_gtt_space(struct i915_vma *vma, unsigned long color); bool i915_vma_misplaced(const struct i915_vma *vma,

@@ -428,6 +430,21 @@ static inline int i915_vma_sync(struct i915_vma *vma) return i915_active_wait(&vma->active); }

+/**

• • i915_vma_get_current_resource - Get the current resource of the vma
• • @vma: The vma to get the current resource from.
• • It's illegal to call this function if the vma is not bound.
• • Return: A refcounted pointer to the current vma resource
• • of the vma, assuming the vma is bound.
• */

+static inline struct i915_vma_resource * +i915_vma_get_current_resource(struct i915_vma *vma) +{

• return i915_vma_resource_get(vma->resource);

+}

• void i915_vma_module_exit(void); int i915_vma_module_init(void);

diff --git a/drivers/gpu/drm/i915/i915_vma_resource.c b/drivers/gpu/drm/i915/i915_vma_resource.c new file mode 100644 index 000000000000..833e987bed2a --- /dev/null +++ b/drivers/gpu/drm/i915/i915_vma_resource.c @@ -0,0 +1,124 @@ +// SPDX-License-Identifier: MIT +/*

• • Copyright © 2021 Intel Corporation
• */

+#include <linux/slab.h>

+#include "i915_vma_resource.h"

+/* Callbacks for the unbind dma-fence. */ +static const char *get_driver_name(struct dma_fence *fence) +{

• return "vma unbind fence";

+}

+static const char *get_timeline_name(struct dma_fence *fence) +{

• return "unbound";

+}

+static struct dma_fence_ops unbind_fence_ops = {

• .get_driver_name = get_driver_name,
• .get_timeline_name = get_timeline_name,

+};

+/**

• • i915_vma_resource_init - Initialize a vma resource.
• • @vma_res: The vma resource to initialize
• • Initializes a vma resource allocated using i915_vma_resource_alloc().
• • The reason for having separate allocate and initialize function is that
• • initialization may need to be performed from under a lock where
• • allocation is not allowed.
• */

+void i915_vma_resource_init(struct i915_vma_resource *vma_res) +{

• spin_lock_init(&vma_res->lock);
• dma_fence_init(&vma_res->unbind_fence, &unbind_fence_ops,

•        &vma_res->lock, 0, 0);
  

• refcount_set(&vma_res->hold_count, 1);

+}

+/**

• • i915_vma_resource_alloc - Allocate a vma resource
• • Return: A pointer to a cleared struct i915_vma_resource or
• • a -ENOMEM error pointer if allocation fails.
• */

+struct i915_vma_resource *i915_vma_resource_alloc(void) +{

• struct i915_vma_resource *vma_res =

• kzalloc(sizeof(*vma_res), GFP_KERNEL);
  

• return vma_res ? vma_res : ERR_PTR(-ENOMEM);

+}

+static void __i915_vma_resource_unhold(struct i915_vma_resource *vma_res) +{

• if (refcount_dec_and_test(&vma_res->hold_count))

• dma_fence_signal(&vma_res->unbind_fence);
  

+}

+/**

• • i915_vma_resource_unhold - Unhold the signaling of the vma resource unbind
• • fence.
• • @vma_res: The vma resource.
• • @lockdep_cookie: The lockdep cookie returned from i915_vma_resource_hold.
• • The function may leave a dma_fence critical section.
• */

+void i915_vma_resource_unhold(struct i915_vma_resource *vma_res,

• 	      bool lockdep_cookie)
  

+{

• dma_fence_end_signalling(lockdep_cookie);
• if (IS_ENABLED(CONFIG_PROVE_LOCKING)) {

• unsigned long irq_flags;
  

• /* Inefficient open-coded might_lock_irqsave() */
  

• spin_lock_irqsave(&vma_res->lock, irq_flags);
  

• spin_unlock_irqrestore(&vma_res->lock, irq_flags);
  

• }
• __i915_vma_resource_unhold(vma_res);

+}

+/**

• • i915_vma_resource_hold - Hold the signaling of the vma resource unbind fence.
• • @vma_res: The vma resource.
• • @lockdep_cookie: Pointer to a bool serving as a lockdep cooke that should
• • be given as an argument to the pairing i915_vma_resource_unhold.
• • If returning true, the function enters a dma_fence signalling critical
• • section is not in one already.

if not?

• • Return: true if holding successful, false if not.
• */

+bool i915_vma_resource_hold(struct i915_vma_resource *vma_res,

• 	    bool *lockdep_cookie)
  

+{

• bool held = refcount_inc_not_zero(&vma_res->hold_count);
• if (held)

• *lockdep_cookie = dma_fence_begin_signalling();
  

• return held;

+}

+/**

• • i915_vma_resource_unbind - Unbind a vma resource
• • @vma_res: The vma resource to unbind.
• • At this point this function does little more than publish a fence that
• • signals immediately unless signaling is held back.
• • Return: A refcounted pointer to a dma-fence that signals when unbinding is
• • complete.
• */

+struct dma_fence * +i915_vma_resource_unbind(struct i915_vma_resource *vma_res) +{

• __i915_vma_resource_unhold(vma_res);
• dma_fence_get(&vma_res->unbind_fence);
• return &vma_res->unbind_fence;

+} diff --git a/drivers/gpu/drm/i915/i915_vma_resource.h b/drivers/gpu/drm/i915/i915_vma_resource.h new file mode 100644 index 000000000000..34744da23072 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_vma_resource.h @@ -0,0 +1,70 @@ +/* SPDX-License-Identifier: MIT */ +/*

• • Copyright © 2021 Intel Corporation
• */

+#ifndef __I915_VMA_RESOURCE_H__ +#define __I915_VMA_RESOURCE_H__

+#include <linux/dma-fence.h> +#include <linux/refcount.h>

+/**

• • struct i915_vma_resource - Snapshotted unbind information.
• • @unbind_fence: Fence to mark unbinding complete. Note that this fence
• • is not considered published until unbind is scheduled, and as such it
• • is illegal to access this fence before scheduled unbind other than
• • for refcounting.
• • @lock: The @unbind_fence lock. We're also using it to protect the weak
• • pointer to the struct i915_vma, @vma during lookup and takedown.

Not sure what the @vma here is referring to?

Otherwise, Reviewed-by: Matthew Auld matthew.auld@intel.com