Re: [PATCH drm-misc-next v3 6/7] drm/gpuvm: generalize dma_resv/extobj handling and GEM validation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 9/14/23 19:21, Thomas Hellström wrote:
On Thu, 2023-09-14 at 18:36 +0200, Danilo Krummrich wrote:
On 9/14/23 15:48, Thomas Hellström wrote:
Hi, Danilo

Some additional minor comments as xe conversion progresses.

On 9/9/23 17:31, Danilo Krummrich wrote:
So far the DRM GPUVA manager offers common infrastructure to
track GPU VA
allocations and mappings, generically connect GPU VA mappings to
their
backing buffers and perform more complex mapping operations on
the GPU VA
space.

However, there are more design patterns commonly used by drivers,
which
can potentially be generalized in order to make the DRM GPUVA
manager
represent a basic GPU-VM implementation. In this context, this
patch aims
at generalizing the following elements.

1) Provide a common dma-resv for GEM objects not being used
outside of
     this GPU-VM.

2) Provide tracking of external GEM objects (GEM objects which
are
     shared with other GPU-VMs).

3) Provide functions to efficiently lock all GEM objects dma-resv
the
     GPU-VM contains mappings of.

4) Provide tracking of evicted GEM objects the GPU-VM contains
mappings
     of, such that validation of evicted GEM objects is
accelerated.

5) Provide some convinience functions for common patterns.

Rather than being designed as a "framework", the target is to
make all
features appear as a collection of optional helper functions,
such that
drivers are free to make use of the DRM GPUVA managers basic
functionality and opt-in for other features without setting any
feature
flags, just by making use of the corresponding functions.

Big kudos to Boris Brezillon for his help to figure out locking
for drivers
updating the GPU VA space within the fence signalling path.

Suggested-by: Matthew Brost <matthew.brost@xxxxxxxxx>
Signed-off-by: Danilo Krummrich <dakr@xxxxxxxxxx>
---

+/**
+ * drm_gpuvm_bo_evict() - add / remove a &drm_gem_object to /
from a
+ * &drm_gpuvms evicted list
+ * @obj: the &drm_gem_object to add or remove
+ * @evict: indicates whether the object is evicted
+ *
+ * Adds a &drm_gem_object to or removes it from all &drm_gpuvms
evicted
+ * list containing a mapping of this &drm_gem_object.
+ */
+void
+drm_gpuvm_bo_evict(struct drm_gem_object *obj, bool evict)
+{
+    struct drm_gpuvm_bo *vm_bo;
+
+    drm_gem_for_each_gpuvm_bo(vm_bo, obj) {
+        if (evict)
+            drm_gpuvm_bo_list_add(vm_bo, evict);
+        else
+            drm_gpuvm_bo_list_del(vm_bo, evict);
+    }
+}
+EXPORT_SYMBOL_GPL(drm_gpuvm_bo_evict);
+

We need a drm_gpuvm_bo_evict(struct drm_gpuvm_bo *vm_bo, ...) that
puts a single gpuvm_bo on the list, the above function could
perhaps be renamed as drm_gpuvm_gem_obj_evict(obj, ....).

Makes sense - gonna change that.


Reason is some vm's are faulting vms which don't have an evict
list, but validate from the pagefault handler. Also evict == false
is dangerous because if called from within an exec, it might remove
the obj from other vm's evict list before they've had a chance to
rebind their VMAs.

   static int
   __drm_gpuva_insert(struct drm_gpuvm *gpuvm,
              struct drm_gpuva *va)
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index afa50b9059a2..834bb6d6617e 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -26,10 +26,12 @@
    */
   #include <linux/list.h>
+#include <linux/dma-resv.h>
   #include <linux/rbtree.h>
   #include <linux/types.h>
   #include <drm/drm_gem.h>
+#include <drm/drm_exec.h>
   struct drm_gpuvm;
   struct drm_gpuvm_bo;
@@ -259,6 +261,38 @@ struct drm_gpuvm {
        * space
        */
       struct dma_resv *resv;
+
+    /**
+     * @extobj: structure holding the extobj list
+     */
+    struct {
+        /**
+         * @list: &list_head storing &drm_gpuvm_bos serving as
+         * external object
+         */
+        struct list_head list;
+
+        /**
+         * @lock: spinlock to protect the extobj list
+         */
+        spinlock_t lock;
+    } extobj;
+
+    /**
+     * @evict: structure holding the evict list and evict list
lock
+     */
+    struct {
+        /**
+         * @list: &list_head storing &drm_gpuvm_bos currently
being
+         * evicted
+         */
+        struct list_head list;
+
+        /**
+         * @lock: spinlock to protect the evict list
+         */
+        spinlock_t lock;
+    } evict;
   };
   void drm_gpuvm_init(struct drm_gpuvm *gpuvm, struct drm_device
*drm,
@@ -268,6 +302,21 @@ void drm_gpuvm_init(struct drm_gpuvm *gpuvm,
struct drm_device *drm,
               const struct drm_gpuvm_ops *ops);
   void drm_gpuvm_destroy(struct drm_gpuvm *gpuvm);
+/**
+ * drm_gpuvm_is_extobj() - indicates whether the given
&drm_gem_object is an
+ * external object
+ * @gpuvm: the &drm_gpuvm to check
+ * @obj: the &drm_gem_object to check
+ *
+ * Returns: true if the &drm_gem_object &dma_resv differs from
the
+ * &drm_gpuvms &dma_resv, false otherwise
+ */
+static inline bool drm_gpuvm_is_extobj(struct drm_gpuvm *gpuvm,
+                       struct drm_gem_object *obj)
+{
+    return obj && obj->resv != gpuvm->resv;
+}
+
   static inline struct drm_gpuva *
   __drm_gpuva_next(struct drm_gpuva *va)
   {
@@ -346,6 +395,128 @@ __drm_gpuva_next(struct drm_gpuva *va)
   #define drm_gpuvm_for_each_va_safe(va__, next__, gpuvm__) \
       list_for_each_entry_safe(va__, next__, &(gpuvm__)->rb.list,
rb.entry)
+/**
+ * struct drm_gpuvm_exec - &drm_gpuvm abstraction of &drm_exec
+ *
+ * This structure should be created on the stack as &drm_exec
should be.
+ *
+ * Optionally, @extra can be set in order to lock additional
&drm_gem_objects.
+ */
+struct drm_gpuvm_exec {
+    /**
+     * @exec: the &drm_exec structure
+     */
+    struct drm_exec exec;
+
+    /**
+     * @vm: the &drm_gpuvm to lock its DMA reservations
+     */
+    struct drm_gpuvm *vm;
+
+    /**
+     * @extra: Callback and corresponding private data for the
driver to
+     * lock arbitrary additional &drm_gem_objects.
+     */
+    struct {
+        /**
+         * @fn: The driver callback to lock additional
&drm_gem_objects.
+         */
+        int (*fn)(struct drm_gpuvm_exec *vm_exec,
+              unsigned int num_fences);
+
+        /**
+         * @priv: driver private data for the @fn callback
+         */
+        void *priv;
+    } extra;
+};
+
+/**
+ * drm_gpuvm_prepare_vm() - prepare the GPUVMs common dma-resv
+ * @gpuvm: the &drm_gpuvm
+ * @exec: the &drm_exec context
+ * @num_fences: the amount of &dma_fences to reserve
+ *
+ * Calls drm_exec_prepare_obj() for the GPUVMs dummy
&drm_gem_object.
+ *
+ * Using this function directly, it is the drivers
responsibility to call
+ * drm_exec_init() and drm_exec_fini() accordingly.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+static inline int
+drm_gpuvm_prepare_vm(struct drm_gpuvm *gpuvm,
+             struct drm_exec *exec,
+             unsigned int num_fences)
+{
+    return drm_exec_prepare_obj(exec, &gpuvm->d_obj,
num_fences);
+}
+
+int drm_gpuvm_prepare_objects(struct drm_gpuvm *gpuvm,
+                  struct drm_exec *exec,
+                  unsigned int num_fences);
+
+int drm_gpuvm_prepare_range(struct drm_gpuvm *gpuvm,
+                struct drm_exec *exec,
+                u64 addr, u64 range,
+                unsigned int num_fences);
+
+int drm_gpuvm_exec_lock(struct drm_gpuvm_exec *vm_exec,
+            unsigned int num_fences,
+            bool interruptible);
+
+int drm_gpuvm_exec_lock_array(struct drm_gpuvm_exec *vm_exec,
+                  struct drm_gem_object **objs,
+                  unsigned int num_objs,
+                  unsigned int num_fences,
+                  bool interruptible);
+
+int drm_gpuvm_exec_lock_range(struct drm_gpuvm_exec *vm_exec,
+                  u64 addr, u64 range,
+                  unsigned int num_fences,
+                  bool interruptible);
+
+/**
+ * drm_gpuvm_lock() - lock all dma-resv of all assoiciated BOs
+ * @gpuvm: the &drm_gpuvm
+ *
+ * Releases all dma-resv locks of all &drm_gem_objects
previously acquired
+ * through drm_gpuvm_lock() or its variants.
+ *
+ * Returns: 0 on success, negative error code on failure.
+ */
+static inline void
+drm_gpuvm_exec_unlock(struct drm_gpuvm_exec *vm_exec)
+{
+    drm_exec_fini(&vm_exec->exec);
+}
+
+int drm_gpuvm_validate(struct drm_gpuvm *gpuvm);
+void drm_gpuvm_resv_add_fence(struct drm_gpuvm *gpuvm,
+                  struct drm_exec *exec,
+                  struct dma_fence *fence,
+                  enum dma_resv_usage private_usage,
+                  enum dma_resv_usage extobj_usage);
+
+/**
+ * drm_gpuvm_exec_resv_add_fence()
+ * @vm_exec: the &drm_gpuvm_exec abstraction
+ * @fence: fence to add
+ * @private_usage: private dma-resv usage
+ * @extobj_usage: extobj dma-resv usage
+ *
+ * See drm_gpuvm_resv_add_fence().
+ */
+static inline void
+drm_gpuvm_exec_resv_add_fence(struct drm_gpuvm_exec *vm_exec,
+                  struct dma_fence *fence,
+                  enum dma_resv_usage private_usage,
+                  enum dma_resv_usage extobj_usage)
+{
+    drm_gpuvm_resv_add_fence(vm_exec->vm, &vm_exec->exec, fence,
+                 private_usage, extobj_usage);
+}
+
   /**
    * struct drm_gpuvm_bo - structure representing a &drm_gpuvm
and
    * &drm_gem_object combination
@@ -398,6 +569,18 @@ struct drm_gpuvm_bo {
                * gpuva list.
                */
               struct list_head gem;
+
+            /**
+             * @evict: List entry to attach to the &drm_gpuvms
+             * extobj list.
+             */
+            struct list_head extobj;
+
+            /**
+             * @evict: List entry to attach to the &drm_gpuvms
evict
+             * list.
+             */
+            struct list_head evict;
           } entry;
       } list;
   };
@@ -432,6 +615,9 @@ struct drm_gpuvm_bo *
   drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm,
             struct drm_gem_object *obj);
+void drm_gpuvm_bo_evict(struct drm_gem_object *obj, bool evict);
+void drm_gpuvm_bo_extobj_add(struct drm_gpuvm_bo *vm_bo);
+
   /**
    * drm_gpuvm_bo_for_each_va() - iterator to walk over a list of
&drm_gpuva
    * @va__: &drm_gpuva structure to assign to in each iteration
step
@@ -837,6 +1023,17 @@ struct drm_gpuvm_ops {
        * used.
        */
       int (*sm_step_unmap)(struct drm_gpuva_op *op, void *priv);
+
+    /**
+     * @bo_validate: called from drm_gpuvm_validate()
+     *
+     * Drivers receive this callback for every evicted
&drm_gem_object being
+     * mapped in the corresponding &drm_gpuvm.
+     *
+     * Typically, drivers would call their driver specific
variant of
+     * ttm_bo_validate() from within this callback.
+     */
+    int (*bo_validate)(struct drm_gem_object *obj);

Same here. Could we have a vm_bo as an argument instead, so that
the callback knows what gpuvm we're targeting and can mark all its
gpu_vas for revalidation? Or is that intended to be done elsewhere?

Makes sense as well. I'll change that too.

I forgot, drm_gpuvm_validate() would preferably take an drm_gpuvm_exec
argument because we need it in the validate callback. It's also easy
for the driver to subclass further if needed, to pass even more
arguments to its validate callback.

Hm.. that implies that a driver open coding the drm_exec loop, still needs
to use a struct drm_gpuvm_exec rather than just a struct drm_exec. What is
this needed for in Xe? Do we expect other drivers needing it? Might a priv
void pointer maybe make more sense?


/Thomas




   };
   int drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm, void *priv,

Thanks,

Thomas








[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux