From: Rob Clark robdclark@chromium.org
The first six paches are various cleanups and simplifications. The next two get rid of redundant vma lookups in the submit and retire paths. Following that, fenced vma lets us indicate a fence value following which the vma is no longer used, which is needed because otherwise userspace could observe the signaled fence prior to retire_submits() finishing. (With userspace allocated GPU addresses userspace is tracking when a buffer is no longer used and it's vma can be deleted.) And finally the last patch adds the new uabi for user- space allocated iova.
v2: Fix inuse check in last patch, split GEM_WARN_ON() conversion to separate patch, and nits
Rob Clark (10): drm/msm/gem: Move prototypes drm/msm/gpu: Drop duplicate fence counter drm/msm/gem: Convert some missed GEM_WARN_ON()s drm/msm/gem: Split out inuse helper drm/msm/gem: Drop PAGE_SHIFT for address space mm drm/msm: Drop msm_gem_iova() drm/msm/gem: Rework vma lookup and pin drm/msm/gem: Split vma lookup and pin drm/msm/gem: Add fenced vma unpin drm/msm: Add a way for userspace to allocate GPU iova
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 14 ++- drivers/gpu/drm/msm/msm_drv.c | 21 ++++ drivers/gpu/drm/msm/msm_drv.h | 23 ---- drivers/gpu/drm/msm/msm_fb.c | 16 ++- drivers/gpu/drm/msm/msm_fence.c | 6 +- drivers/gpu/drm/msm/msm_fence.h | 3 + drivers/gpu/drm/msm/msm_gem.c | 151 ++++++++++++++---------- drivers/gpu/drm/msm/msm_gem.h | 47 +++++++- drivers/gpu/drm/msm/msm_gem_submit.c | 17 ++- drivers/gpu/drm/msm/msm_gem_vma.c | 59 ++++++--- drivers/gpu/drm/msm/msm_gpu.c | 8 +- drivers/gpu/drm/msm/msm_gpu.h | 2 +- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 +- drivers/gpu/drm/msm/msm_ringbuffer.h | 1 - include/uapi/drm/msm_drm.h | 3 + 18 files changed, 258 insertions(+), 131 deletions(-)
From: Rob Clark robdclark@chromium.org
These belong more cleanly in the gem header.
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/msm_drv.h | 23 ----------------------- drivers/gpu/drm/msm/msm_gem.h | 22 ++++++++++++++++++++++ 2 files changed, 22 insertions(+), 23 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h index 9f68aa685ed7..daf60d219463 100644 --- a/drivers/gpu/drm/msm/msm_drv.h +++ b/drivers/gpu/drm/msm/msm_drv.h @@ -250,29 +250,6 @@ void msm_atomic_state_free(struct drm_atomic_state *state); int msm_crtc_enable_vblank(struct drm_crtc *crtc); void msm_crtc_disable_vblank(struct drm_crtc *crtc);
-int msm_gem_init_vma(struct msm_gem_address_space *aspace, - struct msm_gem_vma *vma, int npages, - u64 range_start, u64 range_end); -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); -int msm_gem_map_vma(struct msm_gem_address_space *aspace, - struct msm_gem_vma *vma, int prot, - struct sg_table *sgt, int npages); -void msm_gem_close_vma(struct msm_gem_address_space *aspace, - struct msm_gem_vma *vma); - - -struct msm_gem_address_space * -msm_gem_address_space_get(struct msm_gem_address_space *aspace); - -void msm_gem_address_space_put(struct msm_gem_address_space *aspace); - -struct msm_gem_address_space * -msm_gem_address_space_create(struct msm_mmu *mmu, const char *name, - u64 va_start, u64 size); - int msm_register_mmu(struct drm_device *dev, struct msm_mmu *mmu); void msm_unregister_mmu(struct drm_device *dev, struct msm_mmu *mmu);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 58e11c282928..947ff7d9b471 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -40,6 +40,15 @@ struct msm_gem_address_space { int faults; };
+struct msm_gem_address_space * +msm_gem_address_space_get(struct msm_gem_address_space *aspace); + +void msm_gem_address_space_put(struct msm_gem_address_space *aspace); + +struct msm_gem_address_space * +msm_gem_address_space_create(struct msm_mmu *mmu, const char *name, + u64 va_start, u64 size); + struct msm_gem_vma { struct drm_mm_node node; uint64_t iova; @@ -49,6 +58,19 @@ struct msm_gem_vma { int inuse; };
+int msm_gem_init_vma(struct msm_gem_address_space *aspace, + struct msm_gem_vma *vma, int npages, + u64 range_start, u64 range_end); +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); +int msm_gem_map_vma(struct msm_gem_address_space *aspace, + struct msm_gem_vma *vma, int prot, + struct sg_table *sgt, int npages); +void msm_gem_close_vma(struct msm_gem_address_space *aspace, + struct msm_gem_vma *vma); + struct msm_gem_object { struct drm_gem_object base;
From: Rob Clark robdclark@chromium.org
The ring seqno counter duplicates the fence-context last_fence counter. They end up getting incremented in lock-step, on the same scheduler thread, but the split just makes things less obvious.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 4 ++-- drivers/gpu/drm/msm/msm_gpu.c | 8 ++++---- drivers/gpu/drm/msm/msm_gpu.h | 2 +- drivers/gpu/drm/msm/msm_ringbuffer.h | 1 - 6 files changed, 9 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c index 407f50a15faa..d31aa87c6c8d 100644 --- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c @@ -1235,7 +1235,7 @@ static void a5xx_fault_detect_irq(struct msm_gpu *gpu) return;
DRM_DEV_ERROR(dev->dev, "gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n", - ring ? ring->id : -1, ring ? ring->seqno : 0, + ring ? ring->id : -1, ring ? ring->fctx->last_fence : 0, gpu_read(gpu, REG_A5XX_RBBM_STATUS), gpu_read(gpu, REG_A5XX_CP_RB_RPTR), gpu_read(gpu, REG_A5XX_CP_RB_WPTR), diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c index 83c31b2ad865..17de46fc4bf2 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c @@ -1390,7 +1390,7 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
DRM_DEV_ERROR(&gpu->pdev->dev, "gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n", - ring ? ring->id : -1, ring ? ring->seqno : 0, + ring ? ring->id : -1, ring ? ring->fctx->last_fence : 0, gpu_read(gpu, REG_A6XX_RBBM_STATUS), gpu_read(gpu, REG_A6XX_CP_RB_RPTR), gpu_read(gpu, REG_A6XX_CP_RB_WPTR), diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 45f2c6084aa7..6385ab06632f 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -578,7 +578,7 @@ int adreno_gpu_state_get(struct msm_gpu *gpu, struct msm_gpu_state *state)
state->ring[i].fence = gpu->rb[i]->memptrs->fence; state->ring[i].iova = gpu->rb[i]->iova; - state->ring[i].seqno = gpu->rb[i]->seqno; + state->ring[i].seqno = gpu->rb[i]->fctx->last_fence; state->ring[i].rptr = get_rptr(adreno_gpu, gpu->rb[i]); state->ring[i].wptr = get_wptr(gpu->rb[i]);
@@ -828,7 +828,7 @@ void adreno_dump_info(struct msm_gpu *gpu)
printk("rb %d: fence: %d/%d\n", i, ring->memptrs->fence, - ring->seqno); + ring->fctx->last_fence);
printk("rptr: %d\n", get_rptr(adreno_gpu, ring)); printk("rb wptr: %d\n", get_wptr(ring)); diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c index 747b89aa9d13..9480bdf875db 100644 --- a/drivers/gpu/drm/msm/msm_gpu.c +++ b/drivers/gpu/drm/msm/msm_gpu.c @@ -534,7 +534,7 @@ static void hangcheck_handler(struct timer_list *t) if (fence != ring->hangcheck_fence) { /* some progress has been made.. ya! */ ring->hangcheck_fence = fence; - } else if (fence_before(fence, ring->seqno)) { + } else if (fence_before(fence, ring->fctx->last_fence)) { /* no progress and not done.. hung! */ ring->hangcheck_fence = fence; DRM_DEV_ERROR(dev->dev, "%s: hangcheck detected gpu lockup rb %d!\n", @@ -542,13 +542,13 @@ static void hangcheck_handler(struct timer_list *t) DRM_DEV_ERROR(dev->dev, "%s: completed fence: %u\n", gpu->name, fence); DRM_DEV_ERROR(dev->dev, "%s: submitted fence: %u\n", - gpu->name, ring->seqno); + gpu->name, ring->fctx->last_fence);
kthread_queue_work(gpu->worker, &gpu->recover_work); }
/* if still more pending work, reset the hangcheck timer: */ - if (fence_after(ring->seqno, ring->hangcheck_fence)) + if (fence_after(ring->fctx->last_fence, ring->hangcheck_fence)) hangcheck_timer_reset(gpu);
/* workaround for missing irq: */ @@ -770,7 +770,7 @@ void msm_gpu_submit(struct msm_gpu *gpu, struct msm_gem_submit *submit)
msm_gpu_hw_init(gpu);
- submit->seqno = ++ring->seqno; + submit->seqno = submit->hw_fence->seqno;
msm_rd_dump_submit(priv->rd, submit, NULL);
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h index 2c0203fd6ce3..e47a42b1244a 100644 --- a/drivers/gpu/drm/msm/msm_gpu.h +++ b/drivers/gpu/drm/msm/msm_gpu.h @@ -291,7 +291,7 @@ static inline bool msm_gpu_active(struct msm_gpu *gpu) for (i = 0; i < gpu->nr_rings; i++) { struct msm_ringbuffer *ring = gpu->rb[i];
- if (fence_after(ring->seqno, ring->memptrs->fence)) + if (fence_after(ring->fctx->last_fence, ring->memptrs->fence)) return true; }
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h index d8c63df4e9ca..2a5045abe46e 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.h +++ b/drivers/gpu/drm/msm/msm_ringbuffer.h @@ -59,7 +59,6 @@ struct msm_ringbuffer { spinlock_t submit_lock;
uint64_t iova; - uint32_t seqno; uint32_t hangcheck_fence; struct msm_rbmemptrs *memptrs; uint64_t memptrs_iova;
From: Rob Clark robdclark@chromium.org
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/msm_gem_vma.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index f914ddbaea89..64906594fc65 100644 --- a/drivers/gpu/drm/msm/msm_gem_vma.c +++ b/drivers/gpu/drm/msm/msm_gem_vma.c @@ -44,7 +44,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace, unsigned size = vma->node.size << PAGE_SHIFT;
/* Print a message if we try to purge a vma in use */ - if (WARN_ON(vma->inuse > 0)) + if (GEM_WARN_ON(vma->inuse > 0)) return;
/* Don't do anything if the memory isn't mapped */ @@ -61,7 +61,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace, void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma) { - if (!WARN_ON(!vma->iova)) + if (!GEM_WARN_ON(!vma->iova)) vma->inuse--; }
@@ -73,7 +73,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace, unsigned size = npages << PAGE_SHIFT; int ret = 0;
- if (WARN_ON(!vma->iova)) + if (GEM_WARN_ON(!vma->iova)) return -EINVAL;
/* Increase the usage counter */ @@ -100,7 +100,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace, void msm_gem_close_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma) { - if (WARN_ON(vma->inuse > 0 || vma->mapped)) + if (GEM_WARN_ON(vma->inuse > 0 || vma->mapped)) return;
spin_lock(&aspace->lock); @@ -120,7 +120,7 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace, { int ret;
- if (WARN_ON(vma->iova)) + if (GEM_WARN_ON(vma->iova)) return -EBUSY;
spin_lock(&aspace->lock);
From: Rob Clark robdclark@chromium.org
Prep for a following patch, where it gets a bit more complicated.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/msm_gem.c | 2 +- drivers/gpu/drm/msm/msm_gem.h | 1 + drivers/gpu/drm/msm/msm_gem_vma.c | 9 +++++++-- 3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index a4f61972667b..f96d1dc72021 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -938,7 +938,7 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m, name, comm ? ":" : "", comm ? comm : "", vma->aspace, vma->iova, vma->mapped ? "mapped" : "unmapped", - vma->inuse); + msm_gem_vma_inuse(vma)); kfree(comm); }
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 947ff7d9b471..1b7f0f0b88bf 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -61,6 +61,7 @@ struct msm_gem_vma { int msm_gem_init_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, int npages, u64 range_start, u64 range_end); +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, diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index 64906594fc65..dc2ae097805e 100644 --- a/drivers/gpu/drm/msm/msm_gem_vma.c +++ b/drivers/gpu/drm/msm/msm_gem_vma.c @@ -37,6 +37,11 @@ msm_gem_address_space_get(struct msm_gem_address_space *aspace) return aspace; }
+bool msm_gem_vma_inuse(struct msm_gem_vma *vma) +{ + return !!vma->inuse; +} + /* Actually unmap memory for the vma */ void msm_gem_purge_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma) @@ -44,7 +49,7 @@ void msm_gem_purge_vma(struct msm_gem_address_space *aspace, unsigned size = vma->node.size << PAGE_SHIFT;
/* Print a message if we try to purge a vma in use */ - if (GEM_WARN_ON(vma->inuse > 0)) + if (GEM_WARN_ON(msm_gem_vma_inuse(vma))) return;
/* Don't do anything if the memory isn't mapped */ @@ -100,7 +105,7 @@ msm_gem_map_vma(struct msm_gem_address_space *aspace, void msm_gem_close_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma) { - if (GEM_WARN_ON(vma->inuse > 0 || vma->mapped)) + if (GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped)) return;
spin_lock(&aspace->lock);
From: Rob Clark robdclark@chromium.org
Get rid of all the unnecessary conversion between address/size and page offsets. It just confuses things.
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 2 +- drivers/gpu/drm/msm/msm_gem.c | 5 ++--- drivers/gpu/drm/msm/msm_gem.h | 4 ++-- drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++++-------- 4 files changed, 13 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c index 3e325e2a2b1b..9f76f5b15759 100644 --- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c +++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c @@ -1172,7 +1172,7 @@ static int a6xx_gmu_memory_alloc(struct a6xx_gmu *gmu, struct a6xx_gmu_bo *bo, return PTR_ERR(bo->obj);
ret = msm_gem_get_and_pin_iova_range(bo->obj, gmu->aspace, &bo->iova, - range_start >> PAGE_SHIFT, range_end >> PAGE_SHIFT); + range_start, range_end); if (ret) { drm_gem_object_put(bo->obj); return ret; diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index f96d1dc72021..f4b68bb28a4d 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -392,7 +392,7 @@ static int get_iova_locked(struct drm_gem_object *obj, if (IS_ERR(vma)) return PTR_ERR(vma);
- ret = msm_gem_init_vma(aspace, vma, obj->size >> PAGE_SHIFT, + ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) { del_vma(vma); @@ -434,8 +434,7 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj, if (IS_ERR(pages)) return PTR_ERR(pages);
- ret = msm_gem_map_vma(aspace, vma, prot, - msm_obj->sgt, obj->size >> PAGE_SHIFT); + ret = msm_gem_map_vma(aspace, vma, prot, msm_obj->sgt, obj->size);
if (!ret) msm_obj->pin_count++; diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 1b7f0f0b88bf..090c3b1a6d9a 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -59,7 +59,7 @@ struct msm_gem_vma { };
int msm_gem_init_vma(struct msm_gem_address_space *aspace, - struct msm_gem_vma *vma, int npages, + struct msm_gem_vma *vma, int size, u64 range_start, u64 range_end); bool msm_gem_vma_inuse(struct msm_gem_vma *vma); void msm_gem_purge_vma(struct msm_gem_address_space *aspace, @@ -68,7 +68,7 @@ void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma); int msm_gem_map_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, int prot, - struct sg_table *sgt, int npages); + struct sg_table *sgt, int size); void msm_gem_close_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma);
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index dc2ae097805e..4949899f1fc7 100644 --- a/drivers/gpu/drm/msm/msm_gem_vma.c +++ b/drivers/gpu/drm/msm/msm_gem_vma.c @@ -46,7 +46,7 @@ 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) { - unsigned size = vma->node.size << PAGE_SHIFT; + unsigned size = vma->node.size;
/* Print a message if we try to purge a vma in use */ if (GEM_WARN_ON(msm_gem_vma_inuse(vma))) @@ -73,9 +73,8 @@ void msm_gem_unmap_vma(struct msm_gem_address_space *aspace, int msm_gem_map_vma(struct msm_gem_address_space *aspace, struct msm_gem_vma *vma, int prot, - struct sg_table *sgt, int npages) + struct sg_table *sgt, int size) { - unsigned size = npages << PAGE_SHIFT; int ret = 0;
if (GEM_WARN_ON(!vma->iova)) @@ -120,7 +119,7 @@ void msm_gem_close_vma(struct msm_gem_address_space *aspace,
/* Initialize a new vma and allocate an iova for it */ int msm_gem_init_vma(struct msm_gem_address_space *aspace, - struct msm_gem_vma *vma, int npages, + struct msm_gem_vma *vma, int size, u64 range_start, u64 range_end) { int ret; @@ -129,14 +128,15 @@ int msm_gem_init_vma(struct msm_gem_address_space *aspace, return -EBUSY;
spin_lock(&aspace->lock); - ret = drm_mm_insert_node_in_range(&aspace->mm, &vma->node, npages, 0, - 0, range_start, range_end, 0); + ret = drm_mm_insert_node_in_range(&aspace->mm, &vma->node, + size, PAGE_SIZE, 0, + range_start, range_end, 0); spin_unlock(&aspace->lock);
if (ret) return ret;
- vma->iova = vma->node.start << PAGE_SHIFT; + vma->iova = vma->node.start; vma->mapped = false;
kref_get(&aspace->kref); @@ -161,7 +161,7 @@ msm_gem_address_space_create(struct msm_mmu *mmu, const char *name, aspace->name = name; aspace->mmu = mmu;
- drm_mm_init(&aspace->mm, va_start >> PAGE_SHIFT, size >> PAGE_SHIFT); + drm_mm_init(&aspace->mm, va_start, size);
kref_init(&aspace->kref);
From: Rob Clark robdclark@chromium.org
There was only a single user, which could just as easily stash the iova when pinning.
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/msm_fb.c | 16 ++++++++++------ drivers/gpu/drm/msm/msm_gem.c | 16 ---------------- drivers/gpu/drm/msm/msm_gem.h | 2 -- 3 files changed, 10 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fb.c b/drivers/gpu/drm/msm/msm_fb.c index 7137492fe78e..d4eef66e29dc 100644 --- a/drivers/gpu/drm/msm/msm_fb.c +++ b/drivers/gpu/drm/msm/msm_fb.c @@ -21,6 +21,9 @@ struct msm_framebuffer {
/* Count of # of attached planes which need dirtyfb: */ refcount_t dirtyfb; + + /* Framebuffer per-plane address, if pinned, else zero: */ + uint64_t iova[DRM_FORMAT_MAX_PLANES]; }; #define to_msm_framebuffer(x) container_of(x, struct msm_framebuffer, base)
@@ -76,14 +79,14 @@ int msm_framebuffer_prepare(struct drm_framebuffer *fb, { struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb); int ret, i, n = fb->format->num_planes; - uint64_t iova;
if (needs_dirtyfb) refcount_inc(&msm_fb->dirtyfb);
for (i = 0; i < n; i++) { - ret = msm_gem_get_and_pin_iova(fb->obj[i], aspace, &iova); - drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)", fb->base.id, i, iova, ret); + ret = msm_gem_get_and_pin_iova(fb->obj[i], aspace, &msm_fb->iova[i]); + drm_dbg_state(fb->dev, "FB[%u]: iova[%d]: %08llx (%d)", + fb->base.id, i, msm_fb->iova[i], ret); if (ret) return ret; } @@ -103,14 +106,15 @@ void msm_framebuffer_cleanup(struct drm_framebuffer *fb,
for (i = 0; i < n; i++) msm_gem_unpin_iova(fb->obj[i], aspace); + + memset(msm_fb->iova, 0, sizeof(msm_fb->iova)); }
uint32_t msm_framebuffer_iova(struct drm_framebuffer *fb, struct msm_gem_address_space *aspace, int plane) { - if (!fb->obj[plane]) - return 0; - return msm_gem_iova(fb->obj[plane], aspace) + fb->offsets[plane]; + struct msm_framebuffer *msm_fb = to_msm_framebuffer(fb); + return msm_fb->iova[plane]; }
struct drm_gem_object *msm_framebuffer_bo(struct drm_framebuffer *fb, int plane) diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index f4b68bb28a4d..deafae6feaa8 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -509,22 +509,6 @@ int msm_gem_get_iova(struct drm_gem_object *obj, return ret; }
-/* get iova without taking a reference, used in places where you have - * already done a 'msm_gem_get_and_pin_iova' or 'msm_gem_get_iova' - */ -uint64_t msm_gem_iova(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace) -{ - struct msm_gem_vma *vma; - - msm_gem_lock(obj); - vma = lookup_vma(obj, aspace); - msm_gem_unlock(obj); - GEM_WARN_ON(!vma); - - return vma ? vma->iova : 0; -} - /* * Locked variant of msm_gem_unpin_iova() */ diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 090c3b1a6d9a..772de010a669 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -142,8 +142,6 @@ int msm_gem_get_and_pin_iova_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); int msm_gem_get_and_pin_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); -uint64_t msm_gem_iova(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace); void msm_gem_unpin_iova_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); void msm_gem_unpin_iova(struct drm_gem_object *obj,
From: Rob Clark robdclark@chromium.org
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index deafae6feaa8..218744a490a4 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) } }
-static int get_iova_locked(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace, uint64_t *iova, +static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace, u64 range_start, u64 range_end) { struct msm_gem_vma *vma; - int ret = 0;
GEM_WARN_ON(!msm_gem_is_locked(obj));
vma = lookup_vma(obj, aspace);
if (!vma) { + int ret; + vma = add_vma(obj, aspace); if (IS_ERR(vma)) - return PTR_ERR(vma); + return vma;
ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) { del_vma(vma); - return ret; + return ERR_PTR(ret); } + } else { + GEM_WARN_ON(vma->iova < range_start); + GEM_WARN_ON((vma->iova + obj->size) > range_end); }
- *iova = vma->iova; - return 0; + return vma; }
-static int msm_gem_pin_iova(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace) +static int msm_gem_pin_iova(struct drm_gem_object *obj, struct msm_gem_vma *vma) { struct msm_gem_object *msm_obj = to_msm_bo(obj); - struct msm_gem_vma *vma; struct page **pages; int ret, prot = IOMMU_READ;
@@ -426,15 +427,11 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj, if (GEM_WARN_ON(msm_obj->madv != MSM_MADV_WILLNEED)) return -EBUSY;
- vma = lookup_vma(obj, aspace); - if (GEM_WARN_ON(!vma)) - return -EINVAL; - pages = get_pages(obj); if (IS_ERR(pages)) return PTR_ERR(pages);
- ret = msm_gem_map_vma(aspace, vma, prot, msm_obj->sgt, obj->size); + ret = msm_gem_map_vma(vma->aspace, vma, prot, msm_obj->sgt, obj->size);
if (!ret) msm_obj->pin_count++; @@ -446,19 +443,18 @@ static int get_and_pin_iova_range_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end) { - u64 local; + struct msm_gem_vma *vma; int ret;
GEM_WARN_ON(!msm_gem_is_locked(obj));
- ret = get_iova_locked(obj, aspace, &local, - range_start, range_end); - - if (!ret) - ret = msm_gem_pin_iova(obj, aspace); + vma = get_vma_locked(obj, aspace, range_start, range_end); + if (IS_ERR(vma)) + return PTR_ERR(vma);
+ ret = msm_gem_pin_iova(obj, vma); if (!ret) - *iova = local; + *iova = vma->iova;
return ret; } @@ -500,10 +496,16 @@ int msm_gem_get_and_pin_iova(struct drm_gem_object *obj, int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova) { - int ret; + struct msm_gem_vma *vma; + int ret = 0;
msm_gem_lock(obj); - ret = get_iova_locked(obj, aspace, iova, 0, U64_MAX); + vma = get_vma_locked(obj, aspace, 0, U64_MAX); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + } else { + *iova = vma->iova; + } msm_gem_unlock(obj);
return ret;
On 3/30/22 23:47, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index deafae6feaa8..218744a490a4 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) } }
-static int get_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,+static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace,{ struct msm_gem_vma *vma;
int ret = 0;
GEM_WARN_ON(!msm_gem_is_locked(obj));
vma = lookup_vma(obj, aspace);
if (!vma) {
int ret;- vma = add_vma(obj, aspace); if (IS_ERR(vma))
return PTR_ERR(vma);
return vma;ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) {
You're allocation range_start -> range_end
del_vma(vma);
return ret;
return ERR_PTR(ret);- } else {
GEM_WARN_ON(vma->iova < range_start);GEM_WARN_ON((vma->iova + obj->size) > range_end);
and then comparing range_start -> range_start + obj->size, hence you're assuming that range_end always equals to obj->size during the allocation.
I'm not sure what is the idea here.. this looks inconsistent. I think you wanted to write:
GEM_WARN_ON(vma->iova < range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end);
But is it really useful to check whether the new range is inside of the old range? Shouldn't it be always a error to change the IOVA range without reallocating vma?
I'd expect to see:
GEM_WARN_ON(vma->iova != range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end);
and then error out if range mismatches.
On 3/31/22 21:27, Dmitry Osipenko wrote:
On 3/30/22 23:47, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index deafae6feaa8..218744a490a4 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) } }
-static int get_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,+static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace,{ struct msm_gem_vma *vma;
int ret = 0;
GEM_WARN_ON(!msm_gem_is_locked(obj));
vma = lookup_vma(obj, aspace);
if (!vma) {
int ret;- vma = add_vma(obj, aspace); if (IS_ERR(vma))
return PTR_ERR(vma);
return vma;ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) {
You're allocation range_start -> range_end
*allocating
del_vma(vma);
return ret;
return ERR_PTR(ret);- } else {
GEM_WARN_ON(vma->iova < range_start);GEM_WARN_ON((vma->iova + obj->size) > range_end);and then comparing range_start -> range_start + obj->size, hence you're assuming that range_end always equals to obj->size during the allocation.
I'm not sure what is the idea here.. this looks inconsistent. I think you wanted to write:
GEM_WARN_ON(vma->iova < range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end);But is it really useful to check whether the new range is inside of the old range? Shouldn't it be always a error to change the IOVA range without reallocating vma?
I'd expect to see:
GEM_WARN_ON(vma->iova != range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end);and then error out if range mismatches.
On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
On 3/30/22 23:47, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index deafae6feaa8..218744a490a4 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) } }
-static int get_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,+static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, u64 range_start, u64 range_end){ struct msm_gem_vma *vma;
int ret = 0; GEM_WARN_ON(!msm_gem_is_locked(obj)); vma = lookup_vma(obj, aspace); if (!vma) {
int ret;vma = add_vma(obj, aspace); if (IS_ERR(vma))
return PTR_ERR(vma);
return vma; ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) {You're allocation range_start -> range_end
del_vma(vma);
return ret;
return ERR_PTR(ret); }} else {GEM_WARN_ON(vma->iova < range_start);GEM_WARN_ON((vma->iova + obj->size) > range_end);and then comparing range_start -> range_start + obj->size, hence you're assuming that range_end always equals to obj->size during the allocation.
I'm not sure what is the idea here.. this looks inconsistent. I think you wanted to write:
GEM_WARN_ON(vma->iova < range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end);But is it really useful to check whether the new range is inside of the old range? Shouldn't it be always a error to change the IOVA range without reallocating vma?
There are a few cases (for allocations for GMU) where the range is larger than the bo.. see a6xx_gmu_memory_alloc()
BR, -R
I'd expect to see:
GEM_WARN_ON(vma->iova != range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) != range_end);and then error out if range mismatches.
On 3/31/22 21:58, Rob Clark wrote:
On Thu, Mar 31, 2022 at 11:27 AM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
On 3/30/22 23:47, Rob Clark wrote:
From: Rob Clark robdclark@chromium.org
Combines duplicate vma lookup in the get_and_pin path.
Signed-off-by: Rob Clark robdclark@chromium.org
drivers/gpu/drm/msm/msm_gem.c | 50 ++++++++++++++++++----------------- 1 file changed, 26 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index deafae6feaa8..218744a490a4 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -376,39 +376,40 @@ put_iova_vmas(struct drm_gem_object *obj) } }
-static int get_iova_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t *iova,+static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, u64 range_start, u64 range_end){ struct msm_gem_vma *vma;
int ret = 0; GEM_WARN_ON(!msm_gem_is_locked(obj)); vma = lookup_vma(obj, aspace); if (!vma) {
int ret;vma = add_vma(obj, aspace); if (IS_ERR(vma))
return PTR_ERR(vma);
return vma; ret = msm_gem_init_vma(aspace, vma, obj->size, range_start, range_end); if (ret) {You're allocation range_start -> range_end
del_vma(vma);
return ret;
return ERR_PTR(ret); }} else {GEM_WARN_ON(vma->iova < range_start);GEM_WARN_ON((vma->iova + obj->size) > range_end);and then comparing range_start -> range_start + obj->size, hence you're assuming that range_end always equals to obj->size during the allocation.
I'm not sure what is the idea here.. this looks inconsistent. I think you wanted to write:
GEM_WARN_ON(vma->iova < range_start); GEM_WARN_ON(vma->iova + (vma->node.size << PAGE_SHIFT) > range_end);But is it really useful to check whether the new range is inside of the old range? Shouldn't it be always a error to change the IOVA range without reallocating vma?
There are a few cases (for allocations for GMU) where the range is larger than the bo.. see a6xx_gmu_memory_alloc()
Ahh, I didn't read the code properly and see now why you're using the obj->size. It's the range where you want to put the BO. Looks good then.
Reviewed-by: Dmitry Osipenko dmitry.osipenko@collabora.com
From: Rob Clark robdclark@chromium.org
This way we only lookup vma once per object per submit, for both the submit and retire path.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/msm_gem.c | 60 +++++++++++++--------------- drivers/gpu/drm/msm/msm_gem.h | 9 +++-- drivers/gpu/drm/msm/msm_gem_submit.c | 17 +++++--- 3 files changed, 44 insertions(+), 42 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index 218744a490a4..e8107a22c33a 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -407,7 +407,7 @@ static struct msm_gem_vma *get_vma_locked(struct drm_gem_object *obj, return vma; }
-static int msm_gem_pin_iova(struct drm_gem_object *obj, struct msm_gem_vma *vma) +int msm_gem_pin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vma) { struct msm_gem_object *msm_obj = to_msm_bo(obj); struct page **pages; @@ -439,6 +439,26 @@ static int msm_gem_pin_iova(struct drm_gem_object *obj, struct msm_gem_vma *vma) return ret; }
+void msm_gem_unpin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vma) +{ + struct msm_gem_object *msm_obj = to_msm_bo(obj); + + GEM_WARN_ON(!msm_gem_is_locked(obj)); + + msm_gem_unmap_vma(vma->aspace, vma); + + msm_obj->pin_count--; + GEM_WARN_ON(msm_obj->pin_count < 0); + + update_inactive(msm_obj); +} + +struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace) +{ + return get_vma_locked(obj, aspace, 0, U64_MAX); +} + static int get_and_pin_iova_range_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end) @@ -452,7 +472,7 @@ static int get_and_pin_iova_range_locked(struct drm_gem_object *obj, if (IS_ERR(vma)) return PTR_ERR(vma);
- ret = msm_gem_pin_iova(obj, vma); + ret = msm_gem_pin_vma_locked(obj, vma); if (!ret) *iova = vma->iova;
@@ -476,12 +496,6 @@ int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, return ret; }
-int msm_gem_get_and_pin_iova_locked(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace, uint64_t *iova) -{ - return get_and_pin_iova_range_locked(obj, aspace, iova, 0, U64_MAX); -} - /* get iova and pin it. Should have a matching put */ int msm_gem_get_and_pin_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova) @@ -511,29 +525,6 @@ int msm_gem_get_iova(struct drm_gem_object *obj, return ret; }
-/* - * Locked variant of msm_gem_unpin_iova() - */ -void msm_gem_unpin_iova_locked(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace) -{ - struct msm_gem_object *msm_obj = to_msm_bo(obj); - struct msm_gem_vma *vma; - - GEM_WARN_ON(!msm_gem_is_locked(obj)); - - vma = lookup_vma(obj, aspace); - - if (!GEM_WARN_ON(!vma)) { - msm_gem_unmap_vma(aspace, vma); - - msm_obj->pin_count--; - GEM_WARN_ON(msm_obj->pin_count < 0); - - update_inactive(msm_obj); - } -} - /* * Unpin a iova by updating the reference counts. The memory isn't actually * purged until something else (shrinker, mm_notifier, destroy, etc) decides @@ -542,8 +533,13 @@ void msm_gem_unpin_iova_locked(struct drm_gem_object *obj, void msm_gem_unpin_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace) { + struct msm_gem_vma *vma; + msm_gem_lock(obj); - msm_gem_unpin_iova_locked(obj, aspace); + vma = lookup_vma(obj, aspace); + if (!GEM_WARN_ON(!vma)) { + msm_gem_unpin_vma_locked(obj, vma); + } msm_gem_unlock(obj); }
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 772de010a669..f98264cf130d 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -133,17 +133,17 @@ struct msm_gem_object { #define to_msm_bo(x) container_of(x, struct msm_gem_object, base)
uint64_t msm_gem_mmap_offset(struct drm_gem_object *obj); +int msm_gem_pin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vma); +void msm_gem_unpin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vma); +struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace); int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end); -int msm_gem_get_and_pin_iova_locked(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace, uint64_t *iova); int msm_gem_get_and_pin_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); -void msm_gem_unpin_iova_locked(struct drm_gem_object *obj, - struct msm_gem_address_space *aspace); void msm_gem_unpin_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); struct page **msm_gem_get_pages(struct drm_gem_object *obj); @@ -369,6 +369,7 @@ struct msm_gem_submit { uint32_t handle; }; uint64_t iova; + struct msm_gem_vma *vma; } bos[]; };
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index c6d60c8d286d..91da05af40ee 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -232,7 +232,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i, unsigned flags = submit->bos[i].flags & cleanup_flags;
if (flags & BO_PINNED) - msm_gem_unpin_iova_locked(obj, submit->aspace); + msm_gem_unpin_vma_locked(obj, submit->bos[i].vma);
if (flags & BO_ACTIVE) msm_gem_active_put(obj); @@ -365,21 +365,26 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
for (i = 0; i < submit->nr_bos; i++) { struct drm_gem_object *obj = &submit->bos[i].obj->base; - uint64_t iova; + struct msm_gem_vma *vma;
/* if locking succeeded, pin bo: */ - ret = msm_gem_get_and_pin_iova_locked(obj, - submit->aspace, &iova); + vma = msm_gem_get_vma_locked(obj, submit->aspace); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + break; + }
+ ret = msm_gem_pin_vma_locked(obj, vma); if (ret) break;
submit->bos[i].flags |= BO_PINNED; + submit->bos[i].vma = vma;
- if (iova == submit->bos[i].iova) { + if (vma->iova == submit->bos[i].iova) { submit->bos[i].flags |= BO_VALID; } else { - submit->bos[i].iova = iova; + submit->bos[i].iova = vma->iova; /* iova changed, so address in cmdstream is not valid: */ submit->bos[i].flags &= ~BO_VALID; submit->valid = false;
From: Rob Clark robdclark@chromium.org
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.
Signed-off-by: Rob Clark robdclark@chromium.org --- drivers/gpu/drm/msm/msm_fence.c | 6 ++++-- drivers/gpu/drm/msm/msm_fence.h | 3 +++ drivers/gpu/drm/msm/msm_gem.c | 2 +- drivers/gpu/drm/msm/msm_gem.h | 9 +++++++-- drivers/gpu/drm/msm/msm_gem_vma.c | 28 +++++++++++++++++++++++++--- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 +++++++++++- 6 files changed, 51 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c index f2cece542c3f..3df255402a33 100644 --- 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 17ee3822b423..7f1798c54cd1 100644 --- 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 e8107a22c33a..bf4af17e2f1e 100644 --- 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 f98264cf130d..38d66e1248b1 100644 --- 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); diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index 4949899f1fc7..6f9a402450f9 100644 --- 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,22 @@ 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->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 3bbf574c3bdc..01f7e4b771ff 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -14,9 +14,19 @@ 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); + msm_gem_unlock(obj); + }
pm_runtime_get_sync(&gpu->pdev->dev);
From: Rob Clark robdclark@chromium.org
The motivation at this point is mainly native userspace mesa driver in a VM guest. The one remaining synchronous "hotpath" is buffer allocation, because guest needs to wait to know the bo's iova before it can start emitting cmdstream/state that references the new bo. By allocating the iova in the guest userspace, we no longer need to wait for a response from the host, but can just rely on the allocation request being processed before the cmdstream submission. Allocation failures (OoM, etc) would just be treated as context-lost (ie. GL_GUILTY_CONTEXT_RESET) or subsequent allocations (or readpix, etc) can raise GL_OUT_OF_MEMORY.
Signed-off-by: Rob Clark robdclark@chromium.org Reviewed-by: Dmitry Baryshkov dmitry.baryshkov@linaro.org --- drivers/gpu/drm/msm/adreno/adreno_gpu.c | 10 ++++++ drivers/gpu/drm/msm/msm_drv.c | 21 +++++++++++ drivers/gpu/drm/msm/msm_gem.c | 48 +++++++++++++++++++++++++ drivers/gpu/drm/msm/msm_gem.h | 8 +++++ drivers/gpu/drm/msm/msm_gem_vma.c | 2 ++ include/uapi/drm/msm_drm.h | 3 ++ 6 files changed, 92 insertions(+)
diff --git a/drivers/gpu/drm/msm/adreno/adreno_gpu.c b/drivers/gpu/drm/msm/adreno/adreno_gpu.c index 6385ab06632f..4caae0229518 100644 --- a/drivers/gpu/drm/msm/adreno/adreno_gpu.c +++ b/drivers/gpu/drm/msm/adreno/adreno_gpu.c @@ -281,6 +281,16 @@ int adreno_get_param(struct msm_gpu *gpu, struct msm_file_private *ctx, case MSM_PARAM_SUSPENDS: *value = gpu->suspend_count; return 0; + case MSM_PARAM_VA_START: + if (ctx->aspace == gpu->aspace) + return -EINVAL; + *value = ctx->aspace->va_start; + return 0; + case MSM_PARAM_VA_SIZE: + if (ctx->aspace == gpu->aspace) + return -EINVAL; + *value = ctx->aspace->va_size; + return 0; default: DBG("%s: invalid param: %u", gpu->name, param); return -EINVAL; diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c index a5eed5738ac8..45523b4123eb 100644 --- a/drivers/gpu/drm/msm/msm_drv.c +++ b/drivers/gpu/drm/msm/msm_drv.c @@ -719,6 +719,23 @@ static int msm_ioctl_gem_info_iova(struct drm_device *dev, return msm_gem_get_iova(obj, ctx->aspace, iova); }
+static int msm_ioctl_gem_info_set_iova(struct drm_device *dev, + struct drm_file *file, struct drm_gem_object *obj, + uint64_t iova) +{ + struct msm_drm_private *priv = dev->dev_private; + struct msm_file_private *ctx = file->driver_priv; + + if (!priv->gpu) + return -EINVAL; + + /* Only supported if per-process address space is supported: */ + if (priv->gpu->aspace == ctx->aspace) + return -EOPNOTSUPP; + + return msm_gem_set_iova(obj, ctx->aspace, iova); +} + static int msm_ioctl_gem_info(struct drm_device *dev, void *data, struct drm_file *file) { @@ -733,6 +750,7 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data, switch (args->info) { case MSM_INFO_GET_OFFSET: case MSM_INFO_GET_IOVA: + case MSM_INFO_SET_IOVA: /* value returned as immediate, not pointer, so len==0: */ if (args->len) return -EINVAL; @@ -757,6 +775,9 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data, case MSM_INFO_GET_IOVA: ret = msm_ioctl_gem_info_iova(dev, file, obj, &args->value); break; + case MSM_INFO_SET_IOVA: + ret = msm_ioctl_gem_info_set_iova(dev, file, obj, args->value); + break; case MSM_INFO_SET_NAME: /* length check should leave room for terminating null: */ if (args->len >= sizeof(msm_obj->name)) { diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c index bf4af17e2f1e..83ded2b7154e 100644 --- a/drivers/gpu/drm/msm/msm_gem.c +++ b/drivers/gpu/drm/msm/msm_gem.c @@ -525,6 +525,54 @@ int msm_gem_get_iova(struct drm_gem_object *obj, return ret; }
+static int clear_iova(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace) +{ + struct msm_gem_vma *vma = lookup_vma(obj, aspace); + + if (!vma) + return 0; + + if (msm_gem_vma_inuse(vma)) + return -EBUSY; + + msm_gem_purge_vma(vma->aspace, vma); + msm_gem_close_vma(vma->aspace, vma); + del_vma(vma); + + return 0; +} + +/* + * Get the requested iova but don't pin it. Fails if the requested iova is + * not available. Doesn't need a put because iovas are currently valid for + * the life of the object. + * + * Setting an iova of zero will clear the vma. + */ +int msm_gem_set_iova(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace, uint64_t iova) +{ + int ret = 0; + + msm_gem_lock(obj); + if (!iova) { + ret = clear_iova(obj, aspace); + } else { + struct msm_gem_vma *vma; + vma = get_vma_locked(obj, aspace, iova, iova + obj->size); + if (IS_ERR(vma)) { + ret = PTR_ERR(vma); + } else if (GEM_WARN_ON(vma->iova != iova)) { + clear_iova(obj, aspace); + ret = -ENOSPC; + } + } + msm_gem_unlock(obj); + + return ret; +} + /* * Unpin a iova by updating the reference counts. The memory isn't actually * purged until something else (shrinker, mm_notifier, destroy, etc) decides diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 38d66e1248b1..efa2e5c19f1e 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -38,6 +38,12 @@ struct msm_gem_address_space {
/* @faults: the number of GPU hangs associated with this address space */ int faults; + + /** @va_start: lowest possible address to allocate */ + uint64_t va_start; + + /** @va_size: the size of the address space (in bytes) */ + uint64_t va_size; };
struct msm_gem_address_space * @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); +int msm_gem_set_iova(struct drm_gem_object *obj, + struct msm_gem_address_space *aspace, uint64_t iova); int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end); diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c index 6f9a402450f9..354f91aff573 100644 --- a/drivers/gpu/drm/msm/msm_gem_vma.c +++ b/drivers/gpu/drm/msm/msm_gem_vma.c @@ -182,6 +182,8 @@ msm_gem_address_space_create(struct msm_mmu *mmu, const char *name, spin_lock_init(&aspace->lock); aspace->name = name; aspace->mmu = mmu; + aspace->va_start = va_start; + aspace->va_size = size;
drm_mm_init(&aspace->mm, va_start, size);
diff --git a/include/uapi/drm/msm_drm.h b/include/uapi/drm/msm_drm.h index 794ad1948497..3c7b097c4e3d 100644 --- a/include/uapi/drm/msm_drm.h +++ b/include/uapi/drm/msm_drm.h @@ -84,6 +84,8 @@ struct drm_msm_timespec { #define MSM_PARAM_SYSPROF 0x0b /* WO: 1 preserves perfcntrs, 2 also disables suspend */ #define MSM_PARAM_COMM 0x0c /* WO: override for task->comm */ #define MSM_PARAM_CMDLINE 0x0d /* WO: override for task cmdline */ +#define MSM_PARAM_VA_START 0x0e /* RO: start of valid GPU iova range */ +#define MSM_PARAM_VA_SIZE 0x0f /* RO: size of valid GPU iova range (bytes) */
/* For backwards compat. The original support for preemption was based on * a single ring per priority level so # of priority levels equals the # @@ -135,6 +137,7 @@ struct drm_msm_gem_new { #define MSM_INFO_GET_IOVA 0x01 /* get iova, returned by value */ #define MSM_INFO_SET_NAME 0x02 /* set the debug name (by pointer) */ #define MSM_INFO_GET_NAME 0x03 /* get debug name, returned by pointer */ +#define MSM_INFO_SET_IOVA 0x04 /* set the iova, passed by value */
struct drm_msm_gem_info { __u32 handle; /* in */
...
+/*
- Get the requested iova but don't pin it. Fails if the requested iova is
- not available. Doesn't need a put because iovas are currently valid for
- the life of the object.
- Setting an iova of zero will clear the vma.
- */
+int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova)+{
- int ret = 0;
nit: No need to initialize the ret
- msm_gem_lock(obj);
- if (!iova) {
ret = clear_iova(obj, aspace);- } else {
struct msm_gem_vma *vma;vma = get_vma_locked(obj, aspace, iova, iova + obj->size);if (IS_ERR(vma)) {ret = PTR_ERR(vma);} else if (GEM_WARN_ON(vma->iova != iova)) {clear_iova(obj, aspace);ret = -ENOSPC;
The (vma->iova != iova) means that vma is already set, but to a different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL looks more natural to me.
}- }
- msm_gem_unlock(obj);
- return ret;
+}
/*
- Unpin a iova by updating the reference counts. The memory isn't actually
- purged until something else (shrinker, mm_notifier, destroy, etc) decides
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 38d66e1248b1..efa2e5c19f1e 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -38,6 +38,12 @@ struct msm_gem_address_space {
/* @faults: the number of GPU hangs associated with this address space */ int faults;
- /** @va_start: lowest possible address to allocate */
- uint64_t va_start;
- /** @va_size: the size of the address space (in bytes) */
- uint64_t va_size;
};
struct msm_gem_address_space * @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); +int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova);int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code :) The uint64_t variant shouldn't be used by kernel code in general and checkpatch should want about it.
On 3/31/22 21:52, Dmitry Osipenko wrote:
...
+/*
- Get the requested iova but don't pin it. Fails if the requested iova is
- not available. Doesn't need a put because iovas are currently valid for
- the life of the object.
- Setting an iova of zero will clear the vma.
- */
+int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova)+{
- int ret = 0;
nit: No need to initialize the ret
- msm_gem_lock(obj);
- if (!iova) {
ret = clear_iova(obj, aspace);- } else {
struct msm_gem_vma *vma;vma = get_vma_locked(obj, aspace, iova, iova + obj->size);if (IS_ERR(vma)) {ret = PTR_ERR(vma);} else if (GEM_WARN_ON(vma->iova != iova)) {clear_iova(obj, aspace);ret = -ENOSPC;The (vma->iova != iova) means that vma is already set, but to a different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL looks more natural to me.
}- }
- msm_gem_unlock(obj);
- return ret;
+}
/*
- Unpin a iova by updating the reference counts. The memory isn't actually
- purged until something else (shrinker, mm_notifier, destroy, etc) decides
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 38d66e1248b1..efa2e5c19f1e 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -38,6 +38,12 @@ struct msm_gem_address_space {
/* @faults: the number of GPU hangs associated with this address space */ int faults;
- /** @va_start: lowest possible address to allocate */
- uint64_t va_start;
- /** @va_size: the size of the address space (in bytes) */
- uint64_t va_size;
};
struct msm_gem_address_space * @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); +int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova);int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code :) The uint64_t variant shouldn't be used by kernel code in general and checkpatch should want about it.
s/want/warn/
On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
...
+/*
- Get the requested iova but don't pin it. Fails if the requested iova is
- not available. Doesn't need a put because iovas are currently valid for
- the life of the object.
- Setting an iova of zero will clear the vma.
- */
+int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova)+{
int ret = 0;nit: No need to initialize the ret
actually, we do
msm_gem_lock(obj);if (!iova) {ret = clear_iova(obj, aspace);} else {struct msm_gem_vma *vma;vma = get_vma_locked(obj, aspace, iova, iova + obj->size);if (IS_ERR(vma)) {ret = PTR_ERR(vma);} else if (GEM_WARN_ON(vma->iova != iova)) {clear_iova(obj, aspace);ret = -ENOSPC;The (vma->iova != iova) means that vma is already set, but to a different address. Is -ENOSPC really appropriate here? -EBUSY or -EINVAL looks more natural to me.
yeah, -EBUSY is better
}}msm_gem_unlock(obj);return ret;+}
/*
- Unpin a iova by updating the reference counts. The memory isn't actually
- purged until something else (shrinker, mm_notifier, destroy, etc) decides
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index 38d66e1248b1..efa2e5c19f1e 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -38,6 +38,12 @@ struct msm_gem_address_space {
/* @faults: the number of GPU hangs associated with this address space */ int faults;
/** @va_start: lowest possible address to allocate */uint64_t va_start;/** @va_size: the size of the address space (in bytes) */uint64_t va_size;};
struct msm_gem_address_space * @@ -144,6 +150,8 @@ struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj, struct msm_gem_address_space *aspace); int msm_gem_get_iova(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova); +int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova);int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code :) The uint64_t variant shouldn't be used by kernel code in general and checkpatch should want about it.
one of many things that I disagree with checkpatch about ;-)
I prefer standard types to custom ones. I _kinda_ get the argument in case of uapi (but IMHO that doesn't apply to how drm uapi headers are used)
BR, -R
On 3/31/22 22:02, Rob Clark wrote:
On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
...
+/*
- Get the requested iova but don't pin it. Fails if the requested iova is
- not available. Doesn't need a put because iovas are currently valid for
- the life of the object.
- Setting an iova of zero will clear the vma.
- */
+int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova)+{
int ret = 0;nit: No need to initialize the ret
actually, we do
Indeed, sorry :)
...
int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code :) The uint64_t variant shouldn't be used by kernel code in general and checkpatch should want about it.
one of many things that I disagree with checkpatch about ;-)
I prefer standard types to custom ones. I _kinda_ get the argument in case of uapi (but IMHO that doesn't apply to how drm uapi headers are used)
I'd understand if it was all either uint64_t or u64, but the mix.. hm.
On Thu, Mar 31, 2022 at 12:41 PM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
On 3/31/22 22:02, Rob Clark wrote:
On Thu, Mar 31, 2022 at 11:52 AM Dmitry Osipenko dmitry.osipenko@collabora.com wrote:
...
+/*
- Get the requested iova but don't pin it. Fails if the requested iova is
- not available. Doesn't need a put because iovas are currently valid for
- the life of the object.
- Setting an iova of zero will clear the vma.
- */
+int msm_gem_set_iova(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace, uint64_t iova)+{
int ret = 0;nit: No need to initialize the ret
actually, we do
Indeed, sorry :)
...
int msm_gem_get_and_pin_iova_range(struct drm_gem_object *obj, struct msm_gem_address_space *aspace, uint64_t *iova, u64 range_start, u64 range_end);
nit: There is an odd mix of uint64_t and u64 (and alike) in the MSM code :) The uint64_t variant shouldn't be used by kernel code in general and checkpatch should want about it.
one of many things that I disagree with checkpatch about ;-)
I prefer standard types to custom ones. I _kinda_ get the argument in case of uapi (but IMHO that doesn't apply to how drm uapi headers are used)
I'd understand if it was all either uint64_t or u64, but the mix.. hm.
yeah, fair, we could be a bit more consistent
BR, -R
dri-devel@lists.freedesktop.org
-
Dmitry Osipenko -
Rob Clark