* [PATCH 0/4] drm/msm: Submit overhead opts
@ 2023-08-02 22:21 Rob Clark
2023-08-02 22:21 ` [PATCH 1/4] drm/msm: Take lru lock once per job_run Rob Clark
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Rob Clark @ 2023-08-02 22:21 UTC (permalink / raw
To: dri-devel
Cc: Rob Clark, Elliot Berman, linux-arm-msm, Bjorn Andersson,
Adam Skladowski, open list, Sean Paul, Marijn Suijten,
Dmitry Baryshkov, Guru Das Srinagesh, freedreno
From: Rob Clark <robdclark@chromium.org>
I recently wrote myself a submitoverhead igt test[1] and spent a bit of
time profiling. The end result ranges from 1.6x faster for
NO_IMPLICIT_SYNC commits with 100 BOs to 2.5x faster for 1000 BOs.
[1] https://patchwork.freedesktop.org/series/121909/
Rob Clark (4):
drm/msm: Take lru lock once per job_run
drm/msm: Use drm_gem_object in submit bos table
drm/msm: Take lru lock once per submit_pin_objects()
drm/msm: Remove vma use tracking
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +--
drivers/gpu/drm/msm/msm_gem.c | 57 ++++++++++++-----------
drivers/gpu/drm/msm/msm_gem.h | 15 ++----
drivers/gpu/drm/msm/msm_gem_submit.c | 62 +++++++++++++------------
drivers/gpu/drm/msm/msm_gem_vma.c | 67 +--------------------------
drivers/gpu/drm/msm/msm_gpu.c | 20 ++++----
drivers/gpu/drm/msm/msm_rd.c | 8 ++--
drivers/gpu/drm/msm/msm_ringbuffer.c | 10 ++--
8 files changed, 91 insertions(+), 154 deletions(-)
--
2.41.0
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/4] drm/msm: Take lru lock once per job_run
2023-08-02 22:21 [PATCH 0/4] drm/msm: Submit overhead opts Rob Clark
@ 2023-08-02 22:21 ` Rob Clark
2023-08-02 22:21 ` [PATCH 2/4] drm/msm: Use drm_gem_object in submit bos table Rob Clark
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2023-08-02 22:21 UTC (permalink / raw
To: dri-devel
Cc: Rob Clark, linux-arm-msm, Abhinav Kumar, open list, Sean Paul,
Dmitry Baryshkov, Marijn Suijten, freedreno
From: Rob Clark <robdclark@chromium.org>
Rather than acquiring it and dropping it for each individual obj.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_gem.c | 3 ---
drivers/gpu/drm/msm/msm_ringbuffer.c | 5 +++++
2 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 20cfd86d2b32..6d1dbffc3905 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -509,14 +509,11 @@ void msm_gem_unpin_locked(struct drm_gem_object *obj)
*/
void msm_gem_unpin_active(struct drm_gem_object *obj)
{
- struct msm_drm_private *priv = obj->dev->dev_private;
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- mutex_lock(&priv->lru.lock);
msm_obj->pin_count--;
GEM_WARN_ON(msm_obj->pin_count < 0);
update_lru_active(obj);
- mutex_unlock(&priv->lru.lock);
}
struct msm_gem_vma *msm_gem_get_vma_locked(struct drm_gem_object *obj,
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index b60199184409..8b8353dcde9f 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -16,10 +16,13 @@ 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;
+ struct msm_drm_private *priv = gpu->dev->dev_private;
int i;
msm_fence_init(submit->hw_fence, fctx);
+ mutex_lock(&priv->lru.lock);
+
for (i = 0; i < submit->nr_bos; i++) {
struct drm_gem_object *obj = &submit->bos[i].obj->base;
@@ -28,6 +31,8 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)
submit->bos[i].flags &= ~(BO_VMA_PINNED | BO_OBJ_PINNED);
}
+ mutex_unlock(&priv->lru.lock);
+
/* TODO move submit path over to using a per-ring lock.. */
mutex_lock(&gpu->lock);
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/4] drm/msm: Use drm_gem_object in submit bos table
2023-08-02 22:21 [PATCH 0/4] drm/msm: Submit overhead opts Rob Clark
2023-08-02 22:21 ` [PATCH 1/4] drm/msm: Take lru lock once per job_run Rob Clark
@ 2023-08-02 22:21 ` Rob Clark
2023-08-02 22:21 ` [PATCH 3/4] drm/msm: Take lru lock once per submit_pin_objects() Rob Clark
2023-08-02 22:21 ` [PATCH 4/4] drm/msm: Remove vma use tracking Rob Clark
3 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2023-08-02 22:21 UTC (permalink / raw
To: dri-devel
Cc: Rob Clark, Elliot Berman, Akhil P Oommen, linux-arm-msm,
Adam Skladowski, Abhinav Kumar, open list, Sean Paul, Mukesh Ojha,
Dmitry Baryshkov, Marijn Suijten, freedreno
From: Rob Clark <robdclark@chromium.org>
Basically everywhere wants the base ptr type. So store that instead of
msm_gem_object.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 ++--
drivers/gpu/drm/msm/msm_gem.h | 2 +-
drivers/gpu/drm/msm/msm_gem_submit.c | 42 +++++++++++++--------------
drivers/gpu/drm/msm/msm_gpu.c | 20 ++++++-------
drivers/gpu/drm/msm/msm_rd.c | 8 ++---
drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +-
6 files changed, 40 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index a98c97977e01..888f714ceccd 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -66,7 +66,7 @@ void a5xx_flush(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct msm_gem_submit *submit)
{
struct msm_ringbuffer *ring = submit->ring;
- struct msm_gem_object *obj;
+ struct drm_gem_object *obj;
uint32_t *ptr, dwords;
unsigned int i;
@@ -83,7 +83,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct msm_gem_submit *submit
obj = submit->bos[submit->cmd[i].idx].obj;
dwords = submit->cmd[i].size;
- ptr = msm_gem_get_vaddr(&obj->base);
+ ptr = msm_gem_get_vaddr(obj);
/* _get_vaddr() shouldn't fail at this point,
* since we've already mapped it once in
@@ -103,7 +103,7 @@ static void a5xx_submit_in_rb(struct msm_gpu *gpu, struct msm_gem_submit *submit
OUT_RING(ring, ptr[i]);
}
- msm_gem_put_vaddr(&obj->base);
+ msm_gem_put_vaddr(obj);
break;
}
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 2bd6846c83a9..31b370474fa8 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -301,7 +301,7 @@ struct msm_gem_submit {
#define BO_VMA_PINNED 0x1000 /* vma (virtual address) is pinned */
uint32_t flags;
union {
- struct msm_gem_object *obj;
+ struct drm_gem_object *obj;
uint32_t handle;
};
uint64_t iova;
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 3b908f9f5493..a03bdded1a15 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -165,7 +165,7 @@ static int submit_lookup_objects(struct msm_gem_submit *submit,
drm_gem_object_get(obj);
- submit->bos[i].obj = to_msm_bo(obj);
+ submit->bos[i].obj = obj;
}
out_unlock:
@@ -251,7 +251,7 @@ static int submit_lookup_cmds(struct msm_gem_submit *submit,
static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
unsigned cleanup_flags)
{
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
unsigned flags = submit->bos[i].flags & cleanup_flags;
/*
@@ -287,7 +287,7 @@ static int submit_lock_objects(struct msm_gem_submit *submit)
retry:
for (i = 0; i < submit->nr_bos; i++) {
- struct msm_gem_object *msm_obj = submit->bos[i].obj;
+ struct drm_gem_object *obj = submit->bos[i].obj;
if (slow_locked == i)
slow_locked = -1;
@@ -295,7 +295,7 @@ static int submit_lock_objects(struct msm_gem_submit *submit)
contended = i;
if (!(submit->bos[i].flags & BO_LOCKED)) {
- ret = dma_resv_lock_interruptible(msm_obj->base.resv,
+ ret = dma_resv_lock_interruptible(obj->resv,
&submit->ticket);
if (ret)
goto fail;
@@ -321,9 +321,9 @@ static int submit_lock_objects(struct msm_gem_submit *submit)
submit_unlock_unpin_bo(submit, slow_locked);
if (ret == -EDEADLK) {
- struct msm_gem_object *msm_obj = submit->bos[contended].obj;
+ struct drm_gem_object *obj = submit->bos[contended].obj;
/* we lost out in a seqno race, lock and retry.. */
- ret = dma_resv_lock_slow_interruptible(msm_obj->base.resv,
+ ret = dma_resv_lock_slow_interruptible(obj->resv,
&submit->ticket);
if (!ret) {
submit->bos[contended].flags |= BO_LOCKED;
@@ -346,7 +346,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
int i, ret = 0;
for (i = 0; i < submit->nr_bos; i++) {
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
bool write = submit->bos[i].flags & MSM_SUBMIT_BO_WRITE;
/* NOTE: _reserve_shared() must happen before
@@ -389,7 +389,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
submit->valid = true;
for (i = 0; i < submit->nr_bos; i++) {
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
struct msm_gem_vma *vma;
/* if locking succeeded, pin bo: */
@@ -424,7 +424,7 @@ static void submit_attach_object_fences(struct msm_gem_submit *submit)
int i;
for (i = 0; i < submit->nr_bos; i++) {
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
dma_resv_add_fence(obj->resv, submit->user_fence,
@@ -436,7 +436,7 @@ static void submit_attach_object_fences(struct msm_gem_submit *submit)
}
static int submit_bo(struct msm_gem_submit *submit, uint32_t idx,
- struct msm_gem_object **obj, uint64_t *iova, bool *valid)
+ struct drm_gem_object **obj, uint64_t *iova, bool *valid)
{
if (idx >= submit->nr_bos) {
DRM_ERROR("invalid buffer index: %u (out of %u)\n",
@@ -455,7 +455,7 @@ static int submit_bo(struct msm_gem_submit *submit, uint32_t idx,
}
/* process the reloc's and patch up the cmdstream as needed: */
-static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *obj,
+static int submit_reloc(struct msm_gem_submit *submit, struct drm_gem_object *obj,
uint32_t offset, uint32_t nr_relocs, struct drm_msm_gem_submit_reloc *relocs)
{
uint32_t i, last_offset = 0;
@@ -473,7 +473,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
/* For now, just map the entire thing. Eventually we probably
* to do it page-by-page, w/ kmap() if not vmap()d..
*/
- ptr = msm_gem_get_vaddr_locked(&obj->base);
+ ptr = msm_gem_get_vaddr_locked(obj);
if (IS_ERR(ptr)) {
ret = PTR_ERR(ptr);
@@ -497,7 +497,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
/* offset in dwords: */
off = submit_reloc.submit_offset / 4;
- if ((off >= (obj->base.size / 4)) ||
+ if ((off >= (obj->size / 4)) ||
(off < last_offset)) {
DRM_ERROR("invalid offset %u at reloc %u\n", off, i);
ret = -EINVAL;
@@ -524,7 +524,7 @@ static int submit_reloc(struct msm_gem_submit *submit, struct msm_gem_object *ob
}
out:
- msm_gem_put_vaddr_locked(&obj->base);
+ msm_gem_put_vaddr_locked(obj);
return ret;
}
@@ -542,10 +542,10 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)
cleanup_flags |= BO_VMA_PINNED | BO_OBJ_PINNED;
for (i = 0; i < submit->nr_bos; i++) {
- struct msm_gem_object *msm_obj = submit->bos[i].obj;
+ struct drm_gem_object *obj = submit->bos[i].obj;
submit_cleanup_bo(submit, i, cleanup_flags);
if (error)
- drm_gem_object_put(&msm_obj->base);
+ drm_gem_object_put(obj);
}
}
@@ -554,7 +554,7 @@ void msm_submit_retire(struct msm_gem_submit *submit)
int i;
for (i = 0; i < submit->nr_bos; i++) {
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
drm_gem_object_put(obj);
}
@@ -861,17 +861,17 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
goto out;
for (i = 0; i < args->nr_cmds; i++) {
- struct msm_gem_object *msm_obj;
+ struct drm_gem_object *obj;
uint64_t iova;
ret = submit_bo(submit, submit->cmd[i].idx,
- &msm_obj, &iova, NULL);
+ &obj, &iova, NULL);
if (ret)
goto out;
if (!submit->cmd[i].size ||
((submit->cmd[i].size + submit->cmd[i].offset) >
- msm_obj->base.size / 4)) {
+ obj->size / 4)) {
DRM_ERROR("invalid cmdstream size: %u\n", submit->cmd[i].size * 4);
ret = -EINVAL;
goto out;
@@ -892,7 +892,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
continue;
}
- ret = submit_reloc(submit, msm_obj, submit->cmd[i].offset * 4,
+ ret = submit_reloc(submit, obj, submit->cmd[i].offset * 4,
submit->cmd[i].nr_relocs, submit->cmd[i].relocs);
if (ret)
goto out;
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 52db90e34ead..243f988c65b7 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -219,36 +219,36 @@ static void msm_gpu_devcoredump_free(void *data)
}
static void msm_gpu_crashstate_get_bo(struct msm_gpu_state *state,
- struct msm_gem_object *obj, u64 iova, bool full)
+ struct drm_gem_object *obj, u64 iova, bool full)
{
struct msm_gpu_state_bo *state_bo = &state->bos[state->nr_bos];
/* Don't record write only objects */
- state_bo->size = obj->base.size;
+ state_bo->size = obj->size;
state_bo->iova = iova;
- BUILD_BUG_ON(sizeof(state_bo->name) != sizeof(obj->name));
+ BUILD_BUG_ON(sizeof(state_bo->name) != sizeof(to_msm_bo(obj)->name));
- memcpy(state_bo->name, obj->name, sizeof(state_bo->name));
+ memcpy(state_bo->name, to_msm_bo(obj)->name, sizeof(state_bo->name));
if (full) {
void *ptr;
- state_bo->data = kvmalloc(obj->base.size, GFP_KERNEL);
+ state_bo->data = kvmalloc(obj->size, GFP_KERNEL);
if (!state_bo->data)
goto out;
- msm_gem_lock(&obj->base);
- ptr = msm_gem_get_vaddr_active(&obj->base);
- msm_gem_unlock(&obj->base);
+ msm_gem_lock(obj);
+ ptr = msm_gem_get_vaddr_active(obj);
+ msm_gem_unlock(obj);
if (IS_ERR(ptr)) {
kvfree(state_bo->data);
state_bo->data = NULL;
goto out;
}
- memcpy(state_bo->data, ptr, obj->base.size);
- msm_gem_put_vaddr(&obj->base);
+ memcpy(state_bo->data, ptr, obj->size);
+ msm_gem_put_vaddr(obj);
}
out:
state->nr_bos++;
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
index 8d5687d5ed78..5adc51f7ab59 100644
--- a/drivers/gpu/drm/msm/msm_rd.c
+++ b/drivers/gpu/drm/msm/msm_rd.c
@@ -310,7 +310,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
struct msm_gem_submit *submit, int idx,
uint64_t iova, uint32_t size, bool full)
{
- struct msm_gem_object *obj = submit->bos[idx].obj;
+ struct drm_gem_object *obj = submit->bos[idx].obj;
unsigned offset = 0;
const char *buf;
@@ -318,7 +318,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
offset = iova - submit->bos[idx].iova;
} else {
iova = submit->bos[idx].iova;
- size = obj->base.size;
+ size = obj->size;
}
/*
@@ -335,7 +335,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
if (!(submit->bos[idx].flags & MSM_SUBMIT_BO_READ))
return;
- buf = msm_gem_get_vaddr_active(&obj->base);
+ buf = msm_gem_get_vaddr_active(obj);
if (IS_ERR(buf))
return;
@@ -343,7 +343,7 @@ static void snapshot_buf(struct msm_rd_state *rd,
rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
- msm_gem_put_vaddr_locked(&obj->base);
+ msm_gem_put_vaddr_locked(obj);
}
/* called under gpu->lock */
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 8b8353dcde9f..6fa427d2992e 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -24,7 +24,7 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)
mutex_lock(&priv->lru.lock);
for (i = 0; i < submit->nr_bos; i++) {
- struct drm_gem_object *obj = &submit->bos[i].obj->base;
+ struct drm_gem_object *obj = submit->bos[i].obj;
msm_gem_vma_unpin_fenced(submit->bos[i].vma, fctx);
msm_gem_unpin_active(obj);
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/4] drm/msm: Take lru lock once per submit_pin_objects()
2023-08-02 22:21 [PATCH 0/4] drm/msm: Submit overhead opts Rob Clark
2023-08-02 22:21 ` [PATCH 1/4] drm/msm: Take lru lock once per job_run Rob Clark
2023-08-02 22:21 ` [PATCH 2/4] drm/msm: Use drm_gem_object in submit bos table Rob Clark
@ 2023-08-02 22:21 ` Rob Clark
2023-08-02 22:21 ` [PATCH 4/4] drm/msm: Remove vma use tracking Rob Clark
3 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2023-08-02 22:21 UTC (permalink / raw
To: dri-devel
Cc: Rob Clark, linux-arm-msm, Abhinav Kumar, open list, Sean Paul,
Dmitry Baryshkov, Marijn Suijten, freedreno
From: Rob Clark <robdclark@chromium.org>
Split out pin_count incrementing and lru updating into a separate loop
so we can take the lru lock only once for all objs. Since we are still
holding the obj lock, it is safe to split this up.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_gem.c | 45 ++++++++++++++++++----------
drivers/gpu/drm/msm/msm_gem.h | 1 +
drivers/gpu/drm/msm/msm_gem_submit.c | 10 ++++++-
3 files changed, 39 insertions(+), 17 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 6d1dbffc3905..1c81ff6115ac 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -222,9 +222,7 @@ static void put_pages(struct drm_gem_object *obj)
static struct page **msm_gem_pin_pages_locked(struct drm_gem_object *obj,
unsigned madv)
{
- struct msm_drm_private *priv = obj->dev->dev_private;
struct msm_gem_object *msm_obj = to_msm_bo(obj);
- struct page **p;
msm_gem_assert_locked(obj);
@@ -234,16 +232,29 @@ static struct page **msm_gem_pin_pages_locked(struct drm_gem_object *obj,
return ERR_PTR(-EBUSY);
}
- p = get_pages(obj);
- if (IS_ERR(p))
- return p;
+ return get_pages(obj);
+}
+
+/*
+ * Update the pin count of the object, call under lru.lock
+ */
+void msm_gem_pin_obj_locked(struct drm_gem_object *obj)
+{
+ struct msm_drm_private *priv = obj->dev->dev_private;
+
+ msm_gem_assert_locked(obj);
+
+ to_msm_bo(obj)->pin_count++;
+ drm_gem_lru_move_tail_locked(&priv->lru.pinned, obj);
+}
+
+static void pin_obj_locked(struct drm_gem_object *obj)
+{
+ struct msm_drm_private *priv = obj->dev->dev_private;
mutex_lock(&priv->lru.lock);
- msm_obj->pin_count++;
- update_lru_locked(obj);
+ msm_gem_pin_obj_locked(obj);
mutex_unlock(&priv->lru.lock);
-
- return p;
}
struct page **msm_gem_pin_pages(struct drm_gem_object *obj)
@@ -252,6 +263,8 @@ struct page **msm_gem_pin_pages(struct drm_gem_object *obj)
msm_gem_lock(obj);
p = msm_gem_pin_pages_locked(obj, MSM_MADV_WILLNEED);
+ if (!IS_ERR(p))
+ pin_obj_locked(obj);
msm_gem_unlock(obj);
return p;
@@ -463,7 +476,7 @@ 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;
- int ret, prot = IOMMU_READ;
+ int prot = IOMMU_READ;
if (!(msm_obj->flags & MSM_BO_GPU_READONLY))
prot |= IOMMU_WRITE;
@@ -480,11 +493,7 @@ int msm_gem_pin_vma_locked(struct drm_gem_object *obj, struct msm_gem_vma *vma)
if (IS_ERR(pages))
return PTR_ERR(pages);
- ret = msm_gem_vma_map(vma, prot, msm_obj->sgt, obj->size);
- if (ret)
- msm_gem_unpin_locked(obj);
-
- return ret;
+ return msm_gem_vma_map(vma, prot, msm_obj->sgt, obj->size);
}
void msm_gem_unpin_locked(struct drm_gem_object *obj)
@@ -536,8 +545,10 @@ static int get_and_pin_iova_range_locked(struct drm_gem_object *obj,
return PTR_ERR(vma);
ret = msm_gem_pin_vma_locked(obj, vma);
- if (!ret)
+ if (!ret) {
*iova = vma->iova;
+ pin_obj_locked(obj);
+ }
return ret;
}
@@ -700,6 +711,8 @@ static void *get_vaddr(struct drm_gem_object *obj, unsigned madv)
if (IS_ERR(pages))
return ERR_CAST(pages);
+ pin_obj_locked(obj);
+
/* increment vmap_count *before* vmap() call, so shrinker can
* check vmap_count (is_vunmapable()) outside of msm_obj lock.
* This guarantees that we won't try to msm_gem_vunmap() this
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 31b370474fa8..2ddd896aac68 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -142,6 +142,7 @@ 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(struct drm_gem_object *obj,
struct msm_gem_address_space *aspace);
+void msm_gem_pin_obj_locked(struct drm_gem_object *obj);
struct page **msm_gem_pin_pages(struct drm_gem_object *obj);
void msm_gem_unpin_pages(struct drm_gem_object *obj);
int msm_gem_dumb_create(struct drm_file *file, struct drm_device *dev,
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index a03bdded1a15..b17561ebd518 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -384,6 +384,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
static int submit_pin_objects(struct msm_gem_submit *submit)
{
+ struct msm_drm_private *priv = submit->dev->dev_private;
int i, ret = 0;
submit->valid = true;
@@ -403,7 +404,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
if (ret)
break;
- submit->bos[i].flags |= BO_OBJ_PINNED | BO_VMA_PINNED;
+ submit->bos[i].flags |= BO_VMA_PINNED;
submit->bos[i].vma = vma;
if (vma->iova == submit->bos[i].iova) {
@@ -416,6 +417,13 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
}
}
+ mutex_lock(&priv->lru.lock);
+ for (i = 0; i < submit->nr_bos; i++) {
+ msm_gem_pin_obj_locked(submit->bos[i].obj);
+ submit->bos[i].flags |= BO_OBJ_PINNED;
+ }
+ mutex_unlock(&priv->lru.lock);
+
return ret;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 4/4] drm/msm: Remove vma use tracking
2023-08-02 22:21 [PATCH 0/4] drm/msm: Submit overhead opts Rob Clark
` (2 preceding siblings ...)
2023-08-02 22:21 ` [PATCH 3/4] drm/msm: Take lru lock once per submit_pin_objects() Rob Clark
@ 2023-08-02 22:21 ` Rob Clark
2023-08-03 8:37 ` Daniel Vetter
3 siblings, 1 reply; 7+ messages in thread
From: Rob Clark @ 2023-08-02 22:21 UTC (permalink / raw
To: dri-devel
Cc: Rob Clark, linux-arm-msm, Abhinav Kumar, open list, Sean Paul,
Dmitry Baryshkov, Marijn Suijten, freedreno
From: Rob Clark <robdclark@chromium.org>
This was not strictly necessary, as page unpinning (ie. shrinker) only
cares about the resv. It did give us some extra sanity checking for
userspace controlled iova, and was useful to catch issues on kernel and
userspace side when enabling userspace iova. But if userspace screws
this up, it just corrupts it's own gpu buffers and/or gets iova faults.
So we can just let userspace shoot it's own foot and drop the extra per-
buffer SUBMIT overhead.
Signed-off-by: Rob Clark <robdclark@chromium.org>
---
drivers/gpu/drm/msm/msm_gem.c | 9 +---
drivers/gpu/drm/msm/msm_gem.h | 12 +----
drivers/gpu/drm/msm/msm_gem_submit.c | 14 ++----
drivers/gpu/drm/msm/msm_gem_vma.c | 67 +---------------------------
drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +-
5 files changed, 9 insertions(+), 96 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 1c81ff6115ac..ce1ed0f9ad2d 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -607,9 +607,6 @@ static int clear_iova(struct drm_gem_object *obj,
if (!vma)
return 0;
- if (msm_gem_vma_inuse(vma))
- return -EBUSY;
-
msm_gem_vma_purge(vma);
msm_gem_vma_close(vma);
del_vma(vma);
@@ -660,7 +657,6 @@ void msm_gem_unpin_iova(struct drm_gem_object *obj,
msm_gem_lock(obj);
vma = lookup_vma(obj, aspace);
if (!GEM_WARN_ON(!vma)) {
- msm_gem_vma_unpin(vma);
msm_gem_unpin_locked(obj);
}
msm_gem_unlock(obj);
@@ -991,11 +987,10 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m,
} else {
name = comm = NULL;
}
- seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s,inuse=%d]",
+ seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s]",
name, comm ? ":" : "", comm ? comm : "",
vma->aspace, vma->iova,
- vma->mapped ? "mapped" : "unmapped",
- msm_gem_vma_inuse(vma));
+ vma->mapped ? "mapped" : "unmapped");
kfree(comm);
}
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index 2ddd896aac68..8ddef5443140 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -59,24 +59,16 @@ struct msm_fence_context;
struct msm_gem_vma {
struct drm_mm_node node;
- spinlock_t lock;
uint64_t iova;
struct msm_gem_address_space *aspace;
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];
};
struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace);
int msm_gem_vma_init(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_vma_purge(struct msm_gem_vma *vma);
-void msm_gem_vma_unpin(struct msm_gem_vma *vma);
-void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx);
int msm_gem_vma_map(struct msm_gem_vma *vma, int prot, struct sg_table *sgt, int size);
void msm_gem_vma_close(struct msm_gem_vma *vma);
@@ -298,15 +290,13 @@ struct msm_gem_submit {
/* 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_OBJ_PINNED 0x2000 /* obj (pages) is pinned and on active list */
-#define BO_VMA_PINNED 0x1000 /* vma (virtual address) is pinned */
+#define BO_PINNED 0x2000 /* obj (pages) is pinned and on active list */
uint32_t flags;
union {
struct drm_gem_object *obj;
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 b17561ebd518..5f90cc8e7b7f 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -261,10 +261,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
*/
submit->bos[i].flags &= ~cleanup_flags;
- if (flags & BO_VMA_PINNED)
- msm_gem_vma_unpin(submit->bos[i].vma);
-
- if (flags & BO_OBJ_PINNED)
+ if (flags & BO_PINNED)
msm_gem_unpin_locked(obj);
if (flags & BO_LOCKED)
@@ -273,7 +270,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)
{
- unsigned cleanup_flags = BO_VMA_PINNED | BO_OBJ_PINNED | BO_LOCKED;
+ unsigned cleanup_flags = BO_PINNED | BO_LOCKED;
submit_cleanup_bo(submit, i, cleanup_flags);
if (!(submit->bos[i].flags & BO_VALID))
@@ -404,9 +401,6 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
if (ret)
break;
- submit->bos[i].flags |= BO_VMA_PINNED;
- submit->bos[i].vma = vma;
-
if (vma->iova == submit->bos[i].iova) {
submit->bos[i].flags |= BO_VALID;
} else {
@@ -420,7 +414,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
mutex_lock(&priv->lru.lock);
for (i = 0; i < submit->nr_bos; i++) {
msm_gem_pin_obj_locked(submit->bos[i].obj);
- submit->bos[i].flags |= BO_OBJ_PINNED;
+ submit->bos[i].flags |= BO_PINNED;
}
mutex_unlock(&priv->lru.lock);
@@ -547,7 +541,7 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)
unsigned i;
if (error)
- cleanup_flags |= BO_VMA_PINNED | BO_OBJ_PINNED;
+ cleanup_flags |= BO_PINNED;
for (i = 0; i < submit->nr_bos; i++) {
struct drm_gem_object *obj = submit->bos[i].obj;
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 98287ed99960..11e842dda73c 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -38,41 +38,12 @@ msm_gem_address_space_get(struct msm_gem_address_space *aspace)
return aspace;
}
-bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
-{
- bool ret = true;
-
- spin_lock(&vma->lock);
-
- if (vma->inuse > 0)
- goto out;
-
- while (vma->fence_mask) {
- unsigned idx = ffs(vma->fence_mask) - 1;
-
- if (!msm_fence_completed(vma->fctx[idx], vma->fence[idx]))
- goto out;
-
- vma->fence_mask &= ~BIT(idx);
- }
-
- ret = false;
-
-out:
- spin_unlock(&vma->lock);
-
- return ret;
-}
-
/* Actually unmap memory for the vma */
void msm_gem_vma_purge(struct msm_gem_vma *vma)
{
struct msm_gem_address_space *aspace = vma->aspace;
unsigned size = vma->node.size;
- /* Print a message if we try to purge a vma in use */
- GEM_WARN_ON(msm_gem_vma_inuse(vma));
-
/* Don't do anything if the memory isn't mapped */
if (!vma->mapped)
return;
@@ -82,33 +53,6 @@ void msm_gem_vma_purge(struct msm_gem_vma *vma)
vma->mapped = false;
}
-static void vma_unpin_locked(struct msm_gem_vma *vma)
-{
- if (GEM_WARN_ON(!vma->inuse))
- return;
- if (!GEM_WARN_ON(!vma->iova))
- vma->inuse--;
-}
-
-/* Remove reference counts for the mapping */
-void msm_gem_vma_unpin(struct msm_gem_vma *vma)
-{
- spin_lock(&vma->lock);
- vma_unpin_locked(vma);
- spin_unlock(&vma->lock);
-}
-
-/* Replace pin reference with fence: */
-void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx)
-{
- spin_lock(&vma->lock);
- vma->fctx[fctx->index] = fctx;
- vma->fence[fctx->index] = fctx->last_fence;
- vma->fence_mask |= BIT(fctx->index);
- vma_unpin_locked(vma);
- spin_unlock(&vma->lock);
-}
-
/* Map and pin vma: */
int
msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
@@ -120,11 +64,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
if (GEM_WARN_ON(!vma->iova))
return -EINVAL;
- /* Increase the usage counter */
- spin_lock(&vma->lock);
- vma->inuse++;
- spin_unlock(&vma->lock);
-
if (vma->mapped)
return 0;
@@ -146,9 +85,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
if (ret) {
vma->mapped = false;
- spin_lock(&vma->lock);
- vma->inuse--;
- spin_unlock(&vma->lock);
}
return ret;
@@ -159,7 +95,7 @@ void msm_gem_vma_close(struct msm_gem_vma *vma)
{
struct msm_gem_address_space *aspace = vma->aspace;
- GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped);
+ GEM_WARN_ON(vma->mapped);
spin_lock(&aspace->lock);
if (vma->iova)
@@ -179,7 +115,6 @@ struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace)
if (!vma)
return NULL;
- spin_lock_init(&vma->lock);
vma->aspace = aspace;
return vma;
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 6fa427d2992e..7f5e0a961bba 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -26,9 +26,8 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)
for (i = 0; i < submit->nr_bos; i++) {
struct drm_gem_object *obj = submit->bos[i].obj;
- msm_gem_vma_unpin_fenced(submit->bos[i].vma, fctx);
msm_gem_unpin_active(obj);
- submit->bos[i].flags &= ~(BO_VMA_PINNED | BO_OBJ_PINNED);
+ submit->bos[i].flags &= ~BO_PINNED;
}
mutex_unlock(&priv->lru.lock);
--
2.41.0
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 4/4] drm/msm: Remove vma use tracking
2023-08-02 22:21 ` [PATCH 4/4] drm/msm: Remove vma use tracking Rob Clark
@ 2023-08-03 8:37 ` Daniel Vetter
2023-08-03 16:11 ` Rob Clark
0 siblings, 1 reply; 7+ messages in thread
From: Daniel Vetter @ 2023-08-03 8:37 UTC (permalink / raw
To: Rob Clark
Cc: Rob Clark, linux-arm-msm, Abhinav Kumar, dri-devel, open list,
Sean Paul, Dmitry Baryshkov, Marijn Suijten, freedreno
On Wed, Aug 02, 2023 at 03:21:52PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> This was not strictly necessary, as page unpinning (ie. shrinker) only
> cares about the resv. It did give us some extra sanity checking for
> userspace controlled iova, and was useful to catch issues on kernel and
> userspace side when enabling userspace iova. But if userspace screws
> this up, it just corrupts it's own gpu buffers and/or gets iova faults.
> So we can just let userspace shoot it's own foot and drop the extra per-
> buffer SUBMIT overhead.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
I did check a few things (like that the gem lru helpers have all the
needed lockdep_assert_held) and I think aside from the optimization this
is a nice semantic cleanup. Since iirc we've had a locking inversion
discussion and the vma tracking here came up as a culprit. On the series:
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/msm/msm_gem.c | 9 +---
> drivers/gpu/drm/msm/msm_gem.h | 12 +----
> drivers/gpu/drm/msm/msm_gem_submit.c | 14 ++----
> drivers/gpu/drm/msm/msm_gem_vma.c | 67 +---------------------------
> drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +-
> 5 files changed, 9 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> index 1c81ff6115ac..ce1ed0f9ad2d 100644
> --- a/drivers/gpu/drm/msm/msm_gem.c
> +++ b/drivers/gpu/drm/msm/msm_gem.c
> @@ -607,9 +607,6 @@ static int clear_iova(struct drm_gem_object *obj,
> if (!vma)
> return 0;
>
> - if (msm_gem_vma_inuse(vma))
> - return -EBUSY;
> -
> msm_gem_vma_purge(vma);
> msm_gem_vma_close(vma);
> del_vma(vma);
> @@ -660,7 +657,6 @@ void msm_gem_unpin_iova(struct drm_gem_object *obj,
> msm_gem_lock(obj);
> vma = lookup_vma(obj, aspace);
> if (!GEM_WARN_ON(!vma)) {
> - msm_gem_vma_unpin(vma);
> msm_gem_unpin_locked(obj);
> }
> msm_gem_unlock(obj);
> @@ -991,11 +987,10 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m,
> } else {
> name = comm = NULL;
> }
> - seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s,inuse=%d]",
> + seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s]",
> name, comm ? ":" : "", comm ? comm : "",
> vma->aspace, vma->iova,
> - vma->mapped ? "mapped" : "unmapped",
> - msm_gem_vma_inuse(vma));
> + vma->mapped ? "mapped" : "unmapped");
> kfree(comm);
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> index 2ddd896aac68..8ddef5443140 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -59,24 +59,16 @@ struct msm_fence_context;
>
> struct msm_gem_vma {
> struct drm_mm_node node;
> - spinlock_t lock;
> uint64_t iova;
> struct msm_gem_address_space *aspace;
> 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];
> };
>
> struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace);
> int msm_gem_vma_init(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_vma_purge(struct msm_gem_vma *vma);
> -void msm_gem_vma_unpin(struct msm_gem_vma *vma);
> -void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx);
> int msm_gem_vma_map(struct msm_gem_vma *vma, int prot, struct sg_table *sgt, int size);
> void msm_gem_vma_close(struct msm_gem_vma *vma);
>
> @@ -298,15 +290,13 @@ struct msm_gem_submit {
> /* 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_OBJ_PINNED 0x2000 /* obj (pages) is pinned and on active list */
> -#define BO_VMA_PINNED 0x1000 /* vma (virtual address) is pinned */
> +#define BO_PINNED 0x2000 /* obj (pages) is pinned and on active list */
> uint32_t flags;
> union {
> struct drm_gem_object *obj;
> 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 b17561ebd518..5f90cc8e7b7f 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -261,10 +261,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
> */
> submit->bos[i].flags &= ~cleanup_flags;
>
> - if (flags & BO_VMA_PINNED)
> - msm_gem_vma_unpin(submit->bos[i].vma);
> -
> - if (flags & BO_OBJ_PINNED)
> + if (flags & BO_PINNED)
> msm_gem_unpin_locked(obj);
>
> if (flags & BO_LOCKED)
> @@ -273,7 +270,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
>
> static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)
> {
> - unsigned cleanup_flags = BO_VMA_PINNED | BO_OBJ_PINNED | BO_LOCKED;
> + unsigned cleanup_flags = BO_PINNED | BO_LOCKED;
> submit_cleanup_bo(submit, i, cleanup_flags);
>
> if (!(submit->bos[i].flags & BO_VALID))
> @@ -404,9 +401,6 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> if (ret)
> break;
>
> - submit->bos[i].flags |= BO_VMA_PINNED;
> - submit->bos[i].vma = vma;
> -
> if (vma->iova == submit->bos[i].iova) {
> submit->bos[i].flags |= BO_VALID;
> } else {
> @@ -420,7 +414,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> mutex_lock(&priv->lru.lock);
> for (i = 0; i < submit->nr_bos; i++) {
> msm_gem_pin_obj_locked(submit->bos[i].obj);
> - submit->bos[i].flags |= BO_OBJ_PINNED;
> + submit->bos[i].flags |= BO_PINNED;
> }
> mutex_unlock(&priv->lru.lock);
>
> @@ -547,7 +541,7 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)
> unsigned i;
>
> if (error)
> - cleanup_flags |= BO_VMA_PINNED | BO_OBJ_PINNED;
> + cleanup_flags |= BO_PINNED;
>
> for (i = 0; i < submit->nr_bos; i++) {
> struct drm_gem_object *obj = submit->bos[i].obj;
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 98287ed99960..11e842dda73c 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -38,41 +38,12 @@ msm_gem_address_space_get(struct msm_gem_address_space *aspace)
> return aspace;
> }
>
> -bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
> -{
> - bool ret = true;
> -
> - spin_lock(&vma->lock);
> -
> - if (vma->inuse > 0)
> - goto out;
> -
> - while (vma->fence_mask) {
> - unsigned idx = ffs(vma->fence_mask) - 1;
> -
> - if (!msm_fence_completed(vma->fctx[idx], vma->fence[idx]))
> - goto out;
> -
> - vma->fence_mask &= ~BIT(idx);
> - }
> -
> - ret = false;
> -
> -out:
> - spin_unlock(&vma->lock);
> -
> - return ret;
> -}
> -
> /* Actually unmap memory for the vma */
> void msm_gem_vma_purge(struct msm_gem_vma *vma)
> {
> struct msm_gem_address_space *aspace = vma->aspace;
> unsigned size = vma->node.size;
>
> - /* Print a message if we try to purge a vma in use */
> - GEM_WARN_ON(msm_gem_vma_inuse(vma));
> -
> /* Don't do anything if the memory isn't mapped */
> if (!vma->mapped)
> return;
> @@ -82,33 +53,6 @@ void msm_gem_vma_purge(struct msm_gem_vma *vma)
> vma->mapped = false;
> }
>
> -static void vma_unpin_locked(struct msm_gem_vma *vma)
> -{
> - if (GEM_WARN_ON(!vma->inuse))
> - return;
> - if (!GEM_WARN_ON(!vma->iova))
> - vma->inuse--;
> -}
> -
> -/* Remove reference counts for the mapping */
> -void msm_gem_vma_unpin(struct msm_gem_vma *vma)
> -{
> - spin_lock(&vma->lock);
> - vma_unpin_locked(vma);
> - spin_unlock(&vma->lock);
> -}
> -
> -/* Replace pin reference with fence: */
> -void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx)
> -{
> - spin_lock(&vma->lock);
> - vma->fctx[fctx->index] = fctx;
> - vma->fence[fctx->index] = fctx->last_fence;
> - vma->fence_mask |= BIT(fctx->index);
> - vma_unpin_locked(vma);
> - spin_unlock(&vma->lock);
> -}
> -
> /* Map and pin vma: */
> int
> msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
> @@ -120,11 +64,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
> if (GEM_WARN_ON(!vma->iova))
> return -EINVAL;
>
> - /* Increase the usage counter */
> - spin_lock(&vma->lock);
> - vma->inuse++;
> - spin_unlock(&vma->lock);
> -
> if (vma->mapped)
> return 0;
>
> @@ -146,9 +85,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
>
> if (ret) {
> vma->mapped = false;
> - spin_lock(&vma->lock);
> - vma->inuse--;
> - spin_unlock(&vma->lock);
> }
>
> return ret;
> @@ -159,7 +95,7 @@ void msm_gem_vma_close(struct msm_gem_vma *vma)
> {
> struct msm_gem_address_space *aspace = vma->aspace;
>
> - GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped);
> + GEM_WARN_ON(vma->mapped);
>
> spin_lock(&aspace->lock);
> if (vma->iova)
> @@ -179,7 +115,6 @@ struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace)
> if (!vma)
> return NULL;
>
> - spin_lock_init(&vma->lock);
> vma->aspace = aspace;
>
> return vma;
> diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
> index 6fa427d2992e..7f5e0a961bba 100644
> --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> @@ -26,9 +26,8 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)
> for (i = 0; i < submit->nr_bos; i++) {
> struct drm_gem_object *obj = submit->bos[i].obj;
>
> - msm_gem_vma_unpin_fenced(submit->bos[i].vma, fctx);
> msm_gem_unpin_active(obj);
> - submit->bos[i].flags &= ~(BO_VMA_PINNED | BO_OBJ_PINNED);
> + submit->bos[i].flags &= ~BO_PINNED;
> }
>
> mutex_unlock(&priv->lru.lock);
> --
> 2.41.0
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 4/4] drm/msm: Remove vma use tracking
2023-08-03 8:37 ` Daniel Vetter
@ 2023-08-03 16:11 ` Rob Clark
0 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2023-08-03 16:11 UTC (permalink / raw
To: Rob Clark, dri-devel, freedreno, linux-arm-msm, Rob Clark,
Abhinav Kumar, Dmitry Baryshkov, Sean Paul, Marijn Suijten,
David Airlie, open list
On Thu, Aug 3, 2023 at 1:38 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Wed, Aug 02, 2023 at 03:21:52PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > This was not strictly necessary, as page unpinning (ie. shrinker) only
> > cares about the resv. It did give us some extra sanity checking for
> > userspace controlled iova, and was useful to catch issues on kernel and
> > userspace side when enabling userspace iova. But if userspace screws
> > this up, it just corrupts it's own gpu buffers and/or gets iova faults.
> > So we can just let userspace shoot it's own foot and drop the extra per-
> > buffer SUBMIT overhead.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
>
> I did check a few things (like that the gem lru helpers have all the
> needed lockdep_assert_held) and I think aside from the optimization this
> is a nice semantic cleanup. Since iirc we've had a locking inversion
> discussion and the vma tracking here came up as a culprit. On the series:
yup, I try to make sure there are sufficient locking asserts when it
comes to gem stuff ;-)
There shouldn't have been any locking inversion with the vma lock (no
other locks acquired under it) but it was hurting cpu overhead. The
big remaining locking headache for me is run-pm / pm-qos
BR,
-R
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> > ---
> > drivers/gpu/drm/msm/msm_gem.c | 9 +---
> > drivers/gpu/drm/msm/msm_gem.h | 12 +----
> > drivers/gpu/drm/msm/msm_gem_submit.c | 14 ++----
> > drivers/gpu/drm/msm/msm_gem_vma.c | 67 +---------------------------
> > drivers/gpu/drm/msm/msm_ringbuffer.c | 3 +-
> > 5 files changed, 9 insertions(+), 96 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
> > index 1c81ff6115ac..ce1ed0f9ad2d 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.c
> > +++ b/drivers/gpu/drm/msm/msm_gem.c
> > @@ -607,9 +607,6 @@ static int clear_iova(struct drm_gem_object *obj,
> > if (!vma)
> > return 0;
> >
> > - if (msm_gem_vma_inuse(vma))
> > - return -EBUSY;
> > -
> > msm_gem_vma_purge(vma);
> > msm_gem_vma_close(vma);
> > del_vma(vma);
> > @@ -660,7 +657,6 @@ void msm_gem_unpin_iova(struct drm_gem_object *obj,
> > msm_gem_lock(obj);
> > vma = lookup_vma(obj, aspace);
> > if (!GEM_WARN_ON(!vma)) {
> > - msm_gem_vma_unpin(vma);
> > msm_gem_unpin_locked(obj);
> > }
> > msm_gem_unlock(obj);
> > @@ -991,11 +987,10 @@ void msm_gem_describe(struct drm_gem_object *obj, struct seq_file *m,
> > } else {
> > name = comm = NULL;
> > }
> > - seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s,inuse=%d]",
> > + seq_printf(m, " [%s%s%s: aspace=%p, %08llx,%s]",
> > name, comm ? ":" : "", comm ? comm : "",
> > vma->aspace, vma->iova,
> > - vma->mapped ? "mapped" : "unmapped",
> > - msm_gem_vma_inuse(vma));
> > + vma->mapped ? "mapped" : "unmapped");
> > kfree(comm);
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
> > index 2ddd896aac68..8ddef5443140 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > @@ -59,24 +59,16 @@ struct msm_fence_context;
> >
> > struct msm_gem_vma {
> > struct drm_mm_node node;
> > - spinlock_t lock;
> > uint64_t iova;
> > struct msm_gem_address_space *aspace;
> > 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];
> > };
> >
> > struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace);
> > int msm_gem_vma_init(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_vma_purge(struct msm_gem_vma *vma);
> > -void msm_gem_vma_unpin(struct msm_gem_vma *vma);
> > -void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx);
> > int msm_gem_vma_map(struct msm_gem_vma *vma, int prot, struct sg_table *sgt, int size);
> > void msm_gem_vma_close(struct msm_gem_vma *vma);
> >
> > @@ -298,15 +290,13 @@ struct msm_gem_submit {
> > /* 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_OBJ_PINNED 0x2000 /* obj (pages) is pinned and on active list */
> > -#define BO_VMA_PINNED 0x1000 /* vma (virtual address) is pinned */
> > +#define BO_PINNED 0x2000 /* obj (pages) is pinned and on active list */
> > uint32_t flags;
> > union {
> > struct drm_gem_object *obj;
> > 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 b17561ebd518..5f90cc8e7b7f 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -261,10 +261,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
> > */
> > submit->bos[i].flags &= ~cleanup_flags;
> >
> > - if (flags & BO_VMA_PINNED)
> > - msm_gem_vma_unpin(submit->bos[i].vma);
> > -
> > - if (flags & BO_OBJ_PINNED)
> > + if (flags & BO_PINNED)
> > msm_gem_unpin_locked(obj);
> >
> > if (flags & BO_LOCKED)
> > @@ -273,7 +270,7 @@ static void submit_cleanup_bo(struct msm_gem_submit *submit, int i,
> >
> > static void submit_unlock_unpin_bo(struct msm_gem_submit *submit, int i)
> > {
> > - unsigned cleanup_flags = BO_VMA_PINNED | BO_OBJ_PINNED | BO_LOCKED;
> > + unsigned cleanup_flags = BO_PINNED | BO_LOCKED;
> > submit_cleanup_bo(submit, i, cleanup_flags);
> >
> > if (!(submit->bos[i].flags & BO_VALID))
> > @@ -404,9 +401,6 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> > if (ret)
> > break;
> >
> > - submit->bos[i].flags |= BO_VMA_PINNED;
> > - submit->bos[i].vma = vma;
> > -
> > if (vma->iova == submit->bos[i].iova) {
> > submit->bos[i].flags |= BO_VALID;
> > } else {
> > @@ -420,7 +414,7 @@ static int submit_pin_objects(struct msm_gem_submit *submit)
> > mutex_lock(&priv->lru.lock);
> > for (i = 0; i < submit->nr_bos; i++) {
> > msm_gem_pin_obj_locked(submit->bos[i].obj);
> > - submit->bos[i].flags |= BO_OBJ_PINNED;
> > + submit->bos[i].flags |= BO_PINNED;
> > }
> > mutex_unlock(&priv->lru.lock);
> >
> > @@ -547,7 +541,7 @@ static void submit_cleanup(struct msm_gem_submit *submit, bool error)
> > unsigned i;
> >
> > if (error)
> > - cleanup_flags |= BO_VMA_PINNED | BO_OBJ_PINNED;
> > + cleanup_flags |= BO_PINNED;
> >
> > for (i = 0; i < submit->nr_bos; i++) {
> > struct drm_gem_object *obj = submit->bos[i].obj;
> > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> > index 98287ed99960..11e842dda73c 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> > @@ -38,41 +38,12 @@ msm_gem_address_space_get(struct msm_gem_address_space *aspace)
> > return aspace;
> > }
> >
> > -bool msm_gem_vma_inuse(struct msm_gem_vma *vma)
> > -{
> > - bool ret = true;
> > -
> > - spin_lock(&vma->lock);
> > -
> > - if (vma->inuse > 0)
> > - goto out;
> > -
> > - while (vma->fence_mask) {
> > - unsigned idx = ffs(vma->fence_mask) - 1;
> > -
> > - if (!msm_fence_completed(vma->fctx[idx], vma->fence[idx]))
> > - goto out;
> > -
> > - vma->fence_mask &= ~BIT(idx);
> > - }
> > -
> > - ret = false;
> > -
> > -out:
> > - spin_unlock(&vma->lock);
> > -
> > - return ret;
> > -}
> > -
> > /* Actually unmap memory for the vma */
> > void msm_gem_vma_purge(struct msm_gem_vma *vma)
> > {
> > struct msm_gem_address_space *aspace = vma->aspace;
> > unsigned size = vma->node.size;
> >
> > - /* Print a message if we try to purge a vma in use */
> > - GEM_WARN_ON(msm_gem_vma_inuse(vma));
> > -
> > /* Don't do anything if the memory isn't mapped */
> > if (!vma->mapped)
> > return;
> > @@ -82,33 +53,6 @@ void msm_gem_vma_purge(struct msm_gem_vma *vma)
> > vma->mapped = false;
> > }
> >
> > -static void vma_unpin_locked(struct msm_gem_vma *vma)
> > -{
> > - if (GEM_WARN_ON(!vma->inuse))
> > - return;
> > - if (!GEM_WARN_ON(!vma->iova))
> > - vma->inuse--;
> > -}
> > -
> > -/* Remove reference counts for the mapping */
> > -void msm_gem_vma_unpin(struct msm_gem_vma *vma)
> > -{
> > - spin_lock(&vma->lock);
> > - vma_unpin_locked(vma);
> > - spin_unlock(&vma->lock);
> > -}
> > -
> > -/* Replace pin reference with fence: */
> > -void msm_gem_vma_unpin_fenced(struct msm_gem_vma *vma, struct msm_fence_context *fctx)
> > -{
> > - spin_lock(&vma->lock);
> > - vma->fctx[fctx->index] = fctx;
> > - vma->fence[fctx->index] = fctx->last_fence;
> > - vma->fence_mask |= BIT(fctx->index);
> > - vma_unpin_locked(vma);
> > - spin_unlock(&vma->lock);
> > -}
> > -
> > /* Map and pin vma: */
> > int
> > msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
> > @@ -120,11 +64,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
> > if (GEM_WARN_ON(!vma->iova))
> > return -EINVAL;
> >
> > - /* Increase the usage counter */
> > - spin_lock(&vma->lock);
> > - vma->inuse++;
> > - spin_unlock(&vma->lock);
> > -
> > if (vma->mapped)
> > return 0;
> >
> > @@ -146,9 +85,6 @@ msm_gem_vma_map(struct msm_gem_vma *vma, int prot,
> >
> > if (ret) {
> > vma->mapped = false;
> > - spin_lock(&vma->lock);
> > - vma->inuse--;
> > - spin_unlock(&vma->lock);
> > }
> >
> > return ret;
> > @@ -159,7 +95,7 @@ void msm_gem_vma_close(struct msm_gem_vma *vma)
> > {
> > struct msm_gem_address_space *aspace = vma->aspace;
> >
> > - GEM_WARN_ON(msm_gem_vma_inuse(vma) || vma->mapped);
> > + GEM_WARN_ON(vma->mapped);
> >
> > spin_lock(&aspace->lock);
> > if (vma->iova)
> > @@ -179,7 +115,6 @@ struct msm_gem_vma *msm_gem_vma_new(struct msm_gem_address_space *aspace)
> > if (!vma)
> > return NULL;
> >
> > - spin_lock_init(&vma->lock);
> > vma->aspace = aspace;
> >
> > return vma;
> > diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
> > index 6fa427d2992e..7f5e0a961bba 100644
> > --- a/drivers/gpu/drm/msm/msm_ringbuffer.c
> > +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
> > @@ -26,9 +26,8 @@ static struct dma_fence *msm_job_run(struct drm_sched_job *job)
> > for (i = 0; i < submit->nr_bos; i++) {
> > struct drm_gem_object *obj = submit->bos[i].obj;
> >
> > - msm_gem_vma_unpin_fenced(submit->bos[i].vma, fctx);
> > msm_gem_unpin_active(obj);
> > - submit->bos[i].flags &= ~(BO_VMA_PINNED | BO_OBJ_PINNED);
> > + submit->bos[i].flags &= ~BO_PINNED;
> > }
> >
> > mutex_unlock(&priv->lru.lock);
> > --
> > 2.41.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-08-03 16:12 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-08-02 22:21 [PATCH 0/4] drm/msm: Submit overhead opts Rob Clark
2023-08-02 22:21 ` [PATCH 1/4] drm/msm: Take lru lock once per job_run Rob Clark
2023-08-02 22:21 ` [PATCH 2/4] drm/msm: Use drm_gem_object in submit bos table Rob Clark
2023-08-02 22:21 ` [PATCH 3/4] drm/msm: Take lru lock once per submit_pin_objects() Rob Clark
2023-08-02 22:21 ` [PATCH 4/4] drm/msm: Remove vma use tracking Rob Clark
2023-08-03 8:37 ` Daniel Vetter
2023-08-03 16:11 ` Rob Clark
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).