drm/ttm, drm/amdgpu, drm/xe: Consider hitch moves within bulk sublist moves

To address the problem with hitches moving when bulk move
sublists are lru-bumped, register the list cursors with the
ttm_lru_bulk_move structure when traversing its list, and
when lru-bumping the list, move the cursor hitch to the tail.
This also means it's mandatory for drivers to call
ttm_lru_bulk_move_init() and ttm_lru_bulk_move_fini() when
initializing and finalizing the bulk move structure, so add
those calls to the amdgpu- and xe driver.

Compared to v1 this is slightly more code but less fragile
and hopefully easier to understand.

Changes in previous series:
- Completely rework the functionality
- Avoid a NULL pointer dereference assigning manager->mem_type
- Remove some leftover code causing build problems
v2:
- For hitch bulk tail moves, store the mem_type in the cursor
  instead of with the manager.
v3:
- Remove leftover mem_type member from change in v2.
v6:
- Add some lockdep asserts (Matthew Brost)
- Avoid NULL pointer dereference (Matthew Brost)
- No need to check bo->resource before dereferencing
  bo->bulk_move (Matthew Brost)

Cc: Christian König <christian.koenig@amd.com>
Cc: Somalapuram Amaranath <Amaranath.Somalapuram@amd.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <dri-devel@lists.freedesktop.org>
Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
Reviewed-by: Matthew Brost <matthew.brost@intel.com>
Acked-by: Christian König <christian.koenig@amd.com>
Link: https://patchwork.freedesktop.org/patch/msgid/20240705153206.68526-5-thomas.hellstrom@linux.intel.com
Signed-off-by: Christian König <christian.koenig@amd.com>

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 4e2391c83d7c70b65fbfcd10b6457d3228573ff6..6293f3b54b4a79e5ee1bf083acca3dc777f133e4 100644 (file)
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2422,6 +2422,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
        if (r)
                return r;
 
+       ttm_lru_bulk_move_init(&vm->lru_bulk_move);
+
        vm->is_compute_context = false;
 
        vm->use_cpu_for_update = !!(adev->vm_manager.vm_update_mode &
@@ -2486,6 +2488,7 @@ error_free_root:
 error_free_delayed:
        dma_fence_put(vm->last_tlb_flush);
        dma_fence_put(vm->last_unlocked);
+       ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
        amdgpu_vm_fini_entities(vm);
 
        return r;
@@ -2642,6 +2645,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
                }
        }
 
+       ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
 }
 
 /**

diff --git a/drivers/gpu/drm/ttm/ttm_resource.c b/drivers/gpu/drm/ttm/ttm_resource.c
index 9c8b6499edfbe8dd9c7ebaf16dd0da571ade83ac..b6a2daac55183405feaf85e33f9716245692011d 100644 (file)
--- a/drivers/gpu/drm/ttm/ttm_resource.c
+++ b/drivers/gpu/drm/ttm/ttm_resource.c
@@ -33,6 +33,53 @@
 
 #include <drm/drm_util.h>
 
+/* Detach the cursor from the bulk move list*/
+static void
+ttm_resource_cursor_clear_bulk(struct ttm_resource_cursor *cursor)
+{
+       lockdep_assert_held(&cursor->man->bdev->lru_lock);
+
+       cursor->bulk = NULL;
+       list_del_init(&cursor->bulk_link);
+}
+
+/* Move the cursor to the end of the bulk move list it's in */
+static void ttm_resource_cursor_move_bulk_tail(struct ttm_lru_bulk_move *bulk,
+                                              struct ttm_resource_cursor *cursor)
+{
+       struct ttm_lru_bulk_move_pos *pos;
+
+       lockdep_assert_held(&cursor->man->bdev->lru_lock);
+
+       if (WARN_ON_ONCE(bulk != cursor->bulk)) {
+               list_del_init(&cursor->bulk_link);
+               return;
+       }
+
+       pos = &bulk->pos[cursor->mem_type][cursor->priority];
+       if (pos->last)
+               list_move(&cursor->hitch.link, &pos->last->lru.link);
+       ttm_resource_cursor_clear_bulk(cursor);
+}
+
+/* Move all cursors attached to a bulk move to its end */
+static void ttm_bulk_move_adjust_cursors(struct ttm_lru_bulk_move *bulk)
+{
+       struct ttm_resource_cursor *cursor, *next;
+
+       list_for_each_entry_safe(cursor, next, &bulk->cursor_list, bulk_link)
+               ttm_resource_cursor_move_bulk_tail(bulk, cursor);
+}
+
+/* Remove a cursor from an empty bulk move list */
+static void ttm_bulk_move_drop_cursors(struct ttm_lru_bulk_move *bulk)
+{
+       struct ttm_resource_cursor *cursor, *next;
+
+       list_for_each_entry_safe(cursor, next, &bulk->cursor_list, bulk_link)
+               ttm_resource_cursor_clear_bulk(cursor);
+}
+
 /**
  * ttm_resource_cursor_fini_locked() - Finalize the LRU list cursor usage
  * @cursor: The struct ttm_resource_cursor to finalize.
@@ -45,6 +92,7 @@ void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor)
 {
        lockdep_assert_held(&cursor->man->bdev->lru_lock);
        list_del_init(&cursor->hitch.link);
+       ttm_resource_cursor_clear_bulk(cursor);
 }
 
 /**
@@ -73,9 +121,27 @@ void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor)
 void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk)
 {
        memset(bulk, 0, sizeof(*bulk));
+       INIT_LIST_HEAD(&bulk->cursor_list);
 }
 EXPORT_SYMBOL(ttm_lru_bulk_move_init);
 
+/**
+ * ttm_lru_bulk_move_fini - finalize a bulk move structure
+ * @bdev: The struct ttm_device
+ * @bulk: the structure to finalize
+ *
+ * Sanity checks that bulk moves don't have any
+ * resources left and hence no cursors attached.
+ */
+void ttm_lru_bulk_move_fini(struct ttm_device *bdev,
+                           struct ttm_lru_bulk_move *bulk)
+{
+       spin_lock(&bdev->lru_lock);
+       ttm_bulk_move_drop_cursors(bulk);
+       spin_unlock(&bdev->lru_lock);
+}
+EXPORT_SYMBOL(ttm_lru_bulk_move_fini);
+
 /**
  * ttm_lru_bulk_move_tail - bulk move range of resources to the LRU tail.
  *
@@ -88,6 +154,7 @@ void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk)
 {
        unsigned i, j;
 
+       ttm_bulk_move_adjust_cursors(bulk);
        for (i = 0; i < TTM_NUM_MEM_TYPES; ++i) {
                for (j = 0; j < TTM_MAX_BO_PRIORITY; ++j) {
                        struct ttm_lru_bulk_move_pos *pos = &bulk->pos[i][j];
@@ -515,6 +582,28 @@ void ttm_resource_manager_debug(struct ttm_resource_manager *man,
 }
 EXPORT_SYMBOL(ttm_resource_manager_debug);
 
+static void
+ttm_resource_cursor_check_bulk(struct ttm_resource_cursor *cursor,
+                              struct ttm_lru_item *next_lru)
+{
+       struct ttm_resource *next = ttm_lru_item_to_res(next_lru);
+       struct ttm_lru_bulk_move *bulk = NULL;
+       struct ttm_buffer_object *bo = next->bo;
+
+       lockdep_assert_held(&cursor->man->bdev->lru_lock);
+       bulk = bo->bulk_move;
+
+       if (cursor->bulk != bulk) {
+               if (bulk) {
+                       list_move_tail(&cursor->bulk_link, &bulk->cursor_list);
+                       cursor->mem_type = next->mem_type;
+               } else {
+                       list_del_init(&cursor->bulk_link);
+               }
+               cursor->bulk = bulk;
+       }
+}
+
 /**
  * ttm_resource_manager_first() - Start iterating over the resources
  * of a resource manager
@@ -535,6 +624,7 @@ ttm_resource_manager_first(struct ttm_resource_manager *man,
        cursor->priority = 0;
        cursor->man = man;
        ttm_lru_item_init(&cursor->hitch, TTM_LRU_HITCH);
+       INIT_LIST_HEAD(&cursor->bulk_link);
        list_add(&cursor->hitch.link, &man->lru[cursor->priority]);
 
        return ttm_resource_manager_next(cursor);
@@ -559,6 +649,7 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor)
                lru = &cursor->hitch;
                list_for_each_entry_continue(lru, &man->lru[cursor->priority], link) {
                        if (ttm_lru_item_is_res(lru)) {
+                               ttm_resource_cursor_check_bulk(cursor, lru);
                                list_move(&cursor->hitch.link, &lru->link);
                                return ttm_lru_item_to_res(lru);
                        }
@@ -568,6 +659,7 @@ ttm_resource_manager_next(struct ttm_resource_cursor *cursor)
                        break;
 
                list_move(&cursor->hitch.link, &man->lru[cursor->priority]);
+               ttm_resource_cursor_clear_bulk(cursor);
        }
 
        ttm_resource_cursor_fini_locked(cursor);

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 4aa3943e6f292263f1bc353009bfddd03c2ca689..47b737f10e7430681e12fd40a15772d97044261e 100644 (file)
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -1252,6 +1252,8 @@ struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
        init_rwsem(&vm->userptr.notifier_lock);
        spin_lock_init(&vm->userptr.invalidated_lock);
 
+       ttm_lru_bulk_move_init(&vm->lru_bulk_move);
+
        INIT_LIST_HEAD(&vm->preempt.exec_queues);
        vm->preempt.min_run_period_ms = 10;     /* FIXME: Wire up to uAPI */
 
@@ -1369,6 +1371,7 @@ err_no_resv:
        mutex_destroy(&vm->snap_mutex);
        for_each_tile(tile, xe, id)
                xe_range_fence_tree_fini(&vm->rftree[id]);
+       ttm_lru_bulk_move_fini(&xe->ttm, &vm->lru_bulk_move);
        kfree(vm);
        if (!(flags & XE_VM_FLAG_MIGRATION))
                xe_pm_runtime_put(xe);
@@ -1511,6 +1514,7 @@ static void xe_vm_free(struct drm_gpuvm *gpuvm)
                XE_WARN_ON(vm->pt_root[id]);
 
        trace_xe_vm_free(vm);
+       ttm_lru_bulk_move_fini(&xe->ttm, &vm->lru_bulk_move);
        kfree(vm);
 }
 

diff --git a/include/drm/ttm/ttm_resource.h b/include/drm/ttm/ttm_resource.h
index 8fac781f641ea44210967ef236fff718e5e71e26..571abb4861a6d73aebc1e9c25d4e5aa98f7384c7 100644 (file)
--- a/include/drm/ttm/ttm_resource.h
+++ b/include/drm/ttm/ttm_resource.h
@@ -269,26 +269,6 @@ ttm_lru_item_to_res(struct ttm_lru_item *item)
        return container_of(item, struct ttm_resource, lru);
 }
 
-/**
- * struct ttm_resource_cursor
- *
- * @man: The resource manager currently being iterated over.
- * @hitch: A hitch list node inserted before the next resource
- * to iterate over.
- * @priority: the current priority
- *
- * Cursor to iterate over the resources in a manager.
- */
-struct ttm_resource_cursor {
-       struct ttm_resource_manager *man;
-       struct ttm_lru_item hitch;
-       unsigned int priority;
-};
-
-void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor);
-
-void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor);
-
 /**
  * struct ttm_lru_bulk_move_pos
  *
@@ -304,8 +284,9 @@ struct ttm_lru_bulk_move_pos {
 
 /**
  * struct ttm_lru_bulk_move
- *
  * @pos: first/last lru entry for resources in the each domain/priority
+ * @cursor_list: The list of cursors currently traversing any of
+ * the sublists of @pos. Protected by the ttm device's lru_lock.
  *
  * Container for the current bulk move state. Should be used with
  * ttm_lru_bulk_move_init() and ttm_bo_set_bulk_move().
@@ -315,8 +296,39 @@ struct ttm_lru_bulk_move_pos {
  */
 struct ttm_lru_bulk_move {
        struct ttm_lru_bulk_move_pos pos[TTM_NUM_MEM_TYPES][TTM_MAX_BO_PRIORITY];
+       struct list_head cursor_list;
 };
 
+/**
+ * struct ttm_resource_cursor
+ * @man: The resource manager currently being iterated over
+ * @hitch: A hitch list node inserted before the next resource
+ * to iterate over.
+ * @bulk_link: A list link for the list of cursors traversing the
+ * bulk sublist of @bulk. Protected by the ttm device's lru_lock.
+ * @bulk: Pointer to struct ttm_lru_bulk_move whose subrange @hitch is
+ * inserted to. NULL if none. Never dereference this pointer since
+ * the struct ttm_lru_bulk_move object pointed to might have been
+ * freed. The pointer is only for comparison.
+ * @mem_type: The memory type of the LRU list being traversed.
+ * This field is valid iff @bulk != NULL.
+ * @priority: the current priority
+ *
+ * Cursor to iterate over the resources in a manager.
+ */
+struct ttm_resource_cursor {
+       struct ttm_resource_manager *man;
+       struct ttm_lru_item hitch;
+       struct list_head bulk_link;
+       struct ttm_lru_bulk_move *bulk;
+       unsigned int mem_type;
+       unsigned int priority;
+};
+
+void ttm_resource_cursor_fini_locked(struct ttm_resource_cursor *cursor);
+
+void ttm_resource_cursor_fini(struct ttm_resource_cursor *cursor);
+
 /**
  * struct ttm_kmap_iter_iomap - Specialization for a struct io_mapping +
  * struct sg_table backed struct ttm_resource.
@@ -405,6 +417,8 @@ ttm_resource_manager_cleanup(struct ttm_resource_manager *man)
 
 void ttm_lru_bulk_move_init(struct ttm_lru_bulk_move *bulk);
 void ttm_lru_bulk_move_tail(struct ttm_lru_bulk_move *bulk);
+void ttm_lru_bulk_move_fini(struct ttm_device *bdev,
+                           struct ttm_lru_bulk_move *bulk);
 
 void ttm_resource_add_bulk_move(struct ttm_resource *res,
                                struct ttm_buffer_object *bo);