drm/msm/gem: Add fenced vma unpin

With userspace allocated iova (next patch), we can have a race condition
where userspace observes the fence completion and deletes the vma before
retire_submit() gets around to unpinning the vma.  To handle this, add a
fenced unpin which drops the refcount but tracks the fence, and update
msm_gem_vma_inuse() to check any previously unsignaled fences.

v2: Fix inuse underflow (duplicate unpin)
v3: Fix msm_job_run() vs submit_cleanup() race condition

Signed-off-by: Rob Clark <robdclark@chromium.org>
Link: https://lore.kernel.org/r/20220411215849.297838-10-robdclark@gmail.com
Signed-off-by: Rob Clark <robdclark@chromium.org>

diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index f2cece542c3f72d5f307c9a33841103bdae1fa4c..3df255402a33c1d9de601768cfc8c60631242a34 100644 (file)
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -15,6 +15,7 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
                const char *name)
 {
        struct msm_fence_context *fctx;
+       static int index = 0;
 
        fctx = kzalloc(sizeof(*fctx), GFP_KERNEL);
        if (!fctx)
@@ -23,6 +24,7 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
        fctx->dev = dev;
        strncpy(fctx->name, name, sizeof(fctx->name));
        fctx->context = dma_fence_context_alloc(1);
+       fctx->index = index++;
        fctx->fenceptr = fenceptr;
        spin_lock_init(&fctx->spinlock);
 
@@ -34,7 +36,7 @@ void msm_fence_context_free(struct msm_fence_context *fctx)
        kfree(fctx);
 }
 
-static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fence)
+bool msm_fence_completed(struct msm_fence_context *fctx, uint32_t fence)
 {
        /*
         * Note: Check completed_fence first, as fenceptr is in a write-combine
@@ -76,7 +78,7 @@ static const char *msm_fence_get_timeline_name(struct dma_fence *fence)
 static bool msm_fence_signaled(struct dma_fence *fence)
 {
        struct msm_fence *f = to_msm_fence(fence);
-       return fence_completed(f->fctx, f->base.seqno);
+       return msm_fence_completed(f->fctx, f->base.seqno);
 }
 
 static const struct dma_fence_ops msm_fence_ops = {

diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
index 17ee3822b4237d27ae81bbde03a539431b2898d1..7f1798c54cd15195353ee5ecbadde6169ae89dd2 100644 (file)
--- a/drivers/gpu/drm/msm/msm_fence.h
+++ b/drivers/gpu/drm/msm/msm_fence.h
@@ -21,6 +21,8 @@ struct msm_fence_context {
        char name[32];
        /** context: see dma_fence_context_alloc() */
        unsigned context;
+       /** index: similar to context, but local to msm_fence_context's */
+       unsigned index;
 
        /**
         * last_fence:
@@ -56,6 +58,7 @@ struct msm_fence_context * msm_fence_context_alloc(struct drm_device *dev,
                volatile uint32_t *fenceptr, const char *name);
 void msm_fence_context_free(struct msm_fence_context *fctx);
 
+bool msm_fence_completed(struct msm_fence_context *fctx, uint32_t fence);
 void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence);
 
 struct dma_fence * msm_fence_alloc(struct msm_fence_context *fctx);

diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index b31c4a374917fbec8a5f21d1f0964a4851710658..21986c3214588201b4b68e88ebfeaf242d8e3d7f 100644 (file)
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -445,7 +445,7 @@ void msm_gem_unpin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vm
 
        GEM_WARN_ON(!msm_gem_is_locked(obj));
 
-       msm_gem_unmap_vma(vma->aspace, vma);
+       msm_gem_unpin_vma(vma);
 
        msm_obj->pin_count--;
        GEM_WARN_ON(msm_obj->pin_count < 0);

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index f98264cf130dceab297db4297cae61fcce3ded41..580b6eb95edd2953bc7be5ee778d3ea8033c8ad1 100644 (file)
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -49,6 +49,8 @@ struct msm_gem_address_space *
 msm_gem_address_space_create(struct msm_mmu *mmu, const char *name,
                u64 va_start, u64 size);
 
+struct msm_fence_context;
+
 struct msm_gem_vma {
        struct drm_mm_node node;
        uint64_t iova;
@@ -56,6 +58,9 @@ struct msm_gem_vma {
        struct list_head list;    /* node in msm_gem_object::vmas */
        bool mapped;
        int inuse;
+       uint32_t fence_mask;
+       uint32_t fence[MSM_GPU_MAX_RINGS];
+       struct msm_fence_context *fctx[MSM_GPU_MAX_RINGS];
 };
 
 int msm_gem_init_vma(struct msm_gem_address_space *aspace,
@@ -64,8 +69,8 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace,
 bool msm_gem_vma_inuse(struct msm_gem_vma *vma);
 void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
                struct msm_gem_vma *vma);
-void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
-               struct msm_gem_vma *vma);
+void msm_gem_unpin_vma(struct msm_gem_vma *vma);
+void msm_gem_unpin_vma_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx);
 int msm_gem_map_vma(struct msm_gem_address_space *aspace,
                struct msm_gem_vma *vma, int prot,
                struct sg_table *sgt, int size);
@@ -363,6 +368,11 @@ struct msm_gem_submit {
                struct drm_msm_gem_submit_reloc *relocs;
        } *cmd;  /* array of size nr_cmds */
        struct {
+/* make sure these don't conflict w/ MSM_SUBMIT_BO_x */
+#define BO_VALID    0x8000   /* is current addr in cmdstream correct/valid? */
+#define BO_LOCKED   0x4000   /* obj lock is held */
+#define BO_ACTIVE   0x2000   /* active refcnt is held */
+#define BO_PINNED   0x1000   /* obj is pinned and on active list */
                uint32_t flags;
                union {
                        struct msm_gem_object *obj;

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 91da05af40ee37f10fd6aff9d8f8b8d4a4c4be71..23b68bc945f6ee363db71ca64bb587daa5cde01a 100644 (file)
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -21,12 +21,6 @@
  * Cmdstream submission:
  */
 
-/* make sure these don't conflict w/ MSM_SUBMIT_BO_x */
-#define BO_VALID    0x8000   /* is current addr in cmdstream correct/valid? */
-#define BO_LOCKED   0x4000   /* obj lock is held */
-#define BO_ACTIVE   0x2000   /* active refcnt is held */
-#define BO_PINNED   0x1000   /* obj is pinned and on active list */
-
 static struct msm_gem_submit *submit_create(struct drm_device *dev,
                struct msm_gpu *gpu,
                struct msm_gpu_submitqueue *queue, uint32_t nr_bos,
@@ -231,6 +225,13 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
        struct drm_gem_object *obj = &submit->bos[i].obj->base;
        unsigned flags = submit->bos[i].flags & cleanup_flags;
 
+       /*
+        * Clear flags bit before dropping lock, so that the msm_job_run()
+        * path isn't racing with submit_cleanup() (ie. the read/modify/
+        * write is protected by the obj lock in all paths)
+        */
+       submit->bos[i].flags &= ~cleanup_flags;
+
        if (flags & BO_PINNED)
                msm_gem_unpin_vma_locked(obj, submit->bos[i].vma);
 
@@ -239,8 +240,6 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
 
        if (flags & BO_LOCKED)
                dma_resv_unlock(obj->resv);
-
-       submit->bos[i].flags &= ~cleanup_flags;
 }
 
 static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)

diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 4949899f1fc7acb4f8581d59a547427953c9ce4e..0cd6770faf418ae3c5fefd15d8abb962b0de0895 100644 (file)
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -5,6 +5,7 @@
  */
 
 #include "msm_drv.h"
+#include "msm_fence.h"
 #include "msm_gem.h"
 #include "msm_mmu.h"
 
@@ -39,7 +40,19 @@ msm_gem_address_space_get(struct msm_gem_address_space *aspace)
 
 bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
 {
-       return !!vma->inuse;
+       if (vma->inuse > 0)
+               return true;
+
+       while (vma->fence_mask) {
+               unsigned idx = ffs(vma->fence_mask) - 1;
+
+               if (!msm_fence_completed(vma->fctx[idx], vma->fence[idx]))
+                       return true;
+
+               vma->fence_mask &= ~BIT(idx);
+       }
+
+       return false;
 }
 
 /* Actually unmap memory for the vma */
@@ -63,13 +76,24 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace,
 }
 
 /* Remove reference counts for the mapping */
-void msm_gem_unmap_vma(struct msm_gem_address_space *aspace,
-               struct msm_gem_vma *vma)
+void msm_gem_unpin_vma(struct msm_gem_vma *vma)
 {
+       if (GEM_WARN_ON(!vma->inuse))
+               return;
        if (!GEM_WARN_ON(!vma->iova))
                vma->inuse--;
 }
 
+/* Replace pin reference with fence: */
+void msm_gem_unpin_vma_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx)
+{
+       vma->fctx[fctx->index] = fctx;
+       vma->fence[fctx->index] = fctx->last_fence;
+       vma->fence_mask |= BIT(fctx->index);
+       msm_gem_unpin_vma(vma);
+}
+
+/* Map and pin vma: */
 int
 msm_gem_map_vma(struct msm_gem_address_space *aspace,
                struct msm_gem_vma *vma, int prot,

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 367a6aaa3a202a8d5329d17aa2bad9fae7a53091..eb67fce5bc62b8bcf6b1b28858b710bc69299953 100644 (file)
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -14,9 +14,20 @@ module_param(num_hw_submissions, uint, 0600);
 static struct dma_fence *msm_job_run(struct drm_sched_job *job)
 {
        struct msm_gem_submit *submit = to_msm_submit(job);
+       struct msm_fence_context *fctx = submit->ring->fctx;
        struct msm_gpu *gpu = submit->gpu;
+       int i;
 
-       submit->hw_fence = msm_fence_alloc(submit->ring->fctx);
+       submit->hw_fence = msm_fence_alloc(fctx);
+
+       for (i = 0; i < submit->nr_bos; i++) {
+               struct drm_gem_object *obj = &submit->bos[i].obj->base;
+
+               msm_gem_lock(obj);
+               msm_gem_unpin_vma_fenced(submit->bos[i].vma, fctx);
+               submit->bos[i].flags &= ~BO_PINNED;
+               msm_gem_unlock(obj);
+       }
 
        pm_runtime_get_sync(&gpu->pdev->dev);