* [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups
@ 2021-07-28 1:06 Rob Clark
2021-07-28 1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
2021-07-28 1:06 ` [PATCH v4 09/13] drm/msm: Conversion to drm scheduler Rob Clark
0 siblings, 2 replies; 7+ messages in thread
From: Rob Clark @ 2021-07-28 1:06 UTC (permalink / raw)
To: dri-devel
Cc: freedreno, Rob Clark, Akhil P Oommen, AngeloGioacchino Del Regno,
Bernard Zhao, Bjorn Andersson, Christian König, Dave Airlie,
Douglas Anderson, Emma Anholt, Jonathan Marek, Jordan Crouse,
Konrad Dybcio, Kristian H. Kristensen, Lee Jones,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, open list,
open list:DMA BUFFER SHARING FRAMEWORK, Marijn Suijten,
Sai Prakash Ranjan, Sharat Masetty, Zhenzhong Duan
From: Rob Clark <robdclark@chromium.org>
Conversion to gpu_scheduler, and bonus removal of
drm_gem_object_put_locked()
v2: Fix priority mixup (msm UAPI has lower numeric priority value as
higher priority, inverse of drm/scheduler) and add some comments
in the UAPI header to clarify.
Now that we move active refcnt get into msm_gem_submit, add a
patch to mark all bos busy before pinning, to avoid evicting bos
used in same batch.
Fix bo locking for cmdstream dumping ($debugfs/n/{rd,hangrd})
v3: Add a patch to drop submit bo_list and instead use -EALREADY
to detect errors with same obj appearing multiple times in the
submit ioctl bos table. Otherwise, with struct_mutex locking
dropped, we'd need to move insertion into and removal from
bo_list under the obj lock.
v4: One last small tweak, drop unused wait_queue_head_t in
msm_fence_context
Rob Clark (13):
drm/msm: Docs and misc cleanup
drm/msm: Small submitqueue creation cleanup
drm/msm: drop drm_gem_object_put_locked()
drm: Drop drm_gem_object_put_locked()
drm/msm/submit: Simplify out-fence-fd handling
drm/msm: Consolidate submit bo state
drm/msm: Track "seqno" fences by idr
drm/msm: Return ERR_PTR() from submit_create()
drm/msm: Conversion to drm scheduler
drm/msm: Drop submit bo_list
drm/msm: Drop struct_mutex in submit path
drm/msm: Utilize gpu scheduler priorities
drm/msm/gem: Mark active before pinning
drivers/gpu/drm/drm_gem.c | 22 --
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/adreno/a5xx_debugfs.c | 4 +-
drivers/gpu/drm/msm/adreno/a5xx_gpu.c | 6 +-
drivers/gpu/drm/msm/adreno/a5xx_power.c | 2 +-
drivers/gpu/drm/msm/adreno/a5xx_preempt.c | 7 +-
drivers/gpu/drm/msm/adreno/a6xx_gmu.c | 12 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 2 +-
drivers/gpu/drm/msm/adreno/a6xx_gpu_state.c | 4 +-
drivers/gpu/drm/msm/adreno/adreno_gpu.c | 6 +-
drivers/gpu/drm/msm/msm_drv.c | 30 +-
drivers/gpu/drm/msm/msm_fence.c | 42 ---
drivers/gpu/drm/msm/msm_fence.h | 3 -
drivers/gpu/drm/msm/msm_gem.c | 94 +-----
drivers/gpu/drm/msm/msm_gem.h | 47 +--
drivers/gpu/drm/msm/msm_gem_submit.c | 344 ++++++++++++--------
drivers/gpu/drm/msm/msm_gpu.c | 46 +--
drivers/gpu/drm/msm/msm_gpu.h | 78 ++++-
drivers/gpu/drm/msm/msm_rd.c | 6 +-
drivers/gpu/drm/msm/msm_ringbuffer.c | 70 +++-
drivers/gpu/drm/msm/msm_ringbuffer.h | 12 +
drivers/gpu/drm/msm/msm_submitqueue.c | 53 ++-
include/drm/drm_gem.h | 2 -
include/uapi/drm/msm_drm.h | 14 +-
24 files changed, 516 insertions(+), 391 deletions(-)
--
2.31.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
2021-07-28 1:06 [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups Rob Clark
@ 2021-07-28 1:06 ` Rob Clark
2021-11-10 15:28 ` Akhil P Oommen
2021-07-28 1:06 ` [PATCH v4 09/13] drm/msm: Conversion to drm scheduler Rob Clark
1 sibling, 1 reply; 7+ messages in thread
From: Rob Clark @ 2021-07-28 1:06 UTC (permalink / raw)
To: dri-devel
Cc: freedreno, Rob Clark, Christian König, Rob Clark, Sean Paul,
David Airlie, Daniel Vetter, Sumit Semwal,
open list:DRM DRIVER FOR MSM ADRENO GPU, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK
From: Rob Clark <robdclark@chromium.org>
Previously the (non-fd) fence returned from submit ioctl was a raw
seqno, which is scoped to the ring. But from UABI standpoint, the
ioctls related to seqno fences all specify a submitqueue. We can
take advantage of that to replace the seqno fences with a cyclic idr
handle.
This is in preperation for moving to drm scheduler, at which point
the submit ioctl will return after queuing the submit job to the
scheduler, but before the submit is written into the ring (and
therefore before a ring seqno has been assigned). Which means we
need to replace the dma_fence that userspace may need to wait on
with a scheduler fence.
Signed-off-by: Rob Clark <robdclark@chromium.org>
Acked-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
drivers/gpu/drm/msm/msm_fence.h | 3 --
drivers/gpu/drm/msm/msm_gem.h | 1 +
drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
7 files changed, 61 insertions(+), 48 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9b8fa2ad0d84..1594ae39d54f 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
ktime_t timeout = to_ktime(args->timeout);
struct msm_gpu_submitqueue *queue;
struct msm_gpu *gpu = priv->gpu;
+ struct dma_fence *fence;
int ret;
if (args->pad) {
@@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
if (!queue)
return -ENOENT;
- ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
- true);
+ /*
+ * Map submitqueue scoped "seqno" (which is actually an idr key)
+ * back to underlying dma-fence
+ *
+ * The fence is removed from the fence_idr when the submit is
+ * retired, so if the fence is not found it means there is nothing
+ * to wait for
+ */
+ ret = mutex_lock_interruptible(&queue->lock);
+ if (ret)
+ return ret;
+ fence = idr_find(&queue->fence_idr, args->fence);
+ if (fence)
+ fence = dma_fence_get_rcu(fence);
+ mutex_unlock(&queue->lock);
+
+ if (!fence)
+ return 0;
+ ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
+ if (ret == 0) {
+ ret = -ETIMEDOUT;
+ } else if (ret != -ERESTARTSYS) {
+ ret = 0;
+ }
+
+ dma_fence_put(fence);
msm_submitqueue_put(queue);
+
return ret;
}
diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
index b92a9091a1e2..f2cece542c3f 100644
--- a/drivers/gpu/drm/msm/msm_fence.c
+++ b/drivers/gpu/drm/msm/msm_fence.c
@@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
strncpy(fctx->name, name, sizeof(fctx->name));
fctx->context = dma_fence_context_alloc(1);
fctx->fenceptr = fenceptr;
- init_waitqueue_head(&fctx->event);
spin_lock_init(&fctx->spinlock);
return fctx;
@@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
(int32_t)(*fctx->fenceptr - fence) >= 0;
}
-/* legacy path for WAIT_FENCE ioctl: */
-int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
- ktime_t *timeout, bool interruptible)
-{
- int ret;
-
- if (fence > fctx->last_fence) {
- DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
- fctx->name, fence, fctx->last_fence);
- return -EINVAL;
- }
-
- if (!timeout) {
- /* no-wait: */
- ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
- } else {
- unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
-
- if (interruptible)
- ret = wait_event_interruptible_timeout(fctx->event,
- fence_completed(fctx, fence),
- remaining_jiffies);
- else
- ret = wait_event_timeout(fctx->event,
- fence_completed(fctx, fence),
- remaining_jiffies);
-
- if (ret == 0) {
- DBG("timeout waiting for fence: %u (completed: %u)",
- fence, fctx->completed_fence);
- ret = -ETIMEDOUT;
- } else if (ret != -ERESTARTSYS) {
- ret = 0;
- }
- }
-
- return ret;
-}
-
/* called from workqueue */
void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
{
spin_lock(&fctx->spinlock);
fctx->completed_fence = max(fence, fctx->completed_fence);
spin_unlock(&fctx->spinlock);
-
- wake_up_all(&fctx->event);
}
struct msm_fence {
diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
index 6ab97062ff1a..4783db528bcc 100644
--- a/drivers/gpu/drm/msm/msm_fence.h
+++ b/drivers/gpu/drm/msm/msm_fence.h
@@ -49,7 +49,6 @@ struct msm_fence_context {
*/
volatile uint32_t *fenceptr;
- wait_queue_head_t event;
spinlock_t spinlock;
};
@@ -57,8 +56,6 @@ 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);
-int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
- ktime_t *timeout, bool interruptible);
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.h b/drivers/gpu/drm/msm/msm_gem.h
index da3af702a6c8..e0579abda5b9 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -320,6 +320,7 @@ struct msm_gem_submit {
struct ww_acquire_ctx ticket;
uint32_t seqno; /* Sequence number of the submit on the ring */
struct dma_fence *fence;
+ int fence_id; /* key into queue->fence_idr */
struct msm_gpu_submitqueue *queue;
struct pid *pid; /* submitting process */
bool fault_dumped; /* Limit devcoredump dumping to one per submit */
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index 4f02fa3c78f9..f6f595aae2c5 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
container_of(kref, struct msm_gem_submit, ref);
unsigned i;
+ if (submit->fence_id) {
+ mutex_lock(&submit->queue->lock);
+ idr_remove(&submit->queue->fence_idr, submit->fence_id);
+ mutex_unlock(&submit->queue->lock);
+ }
+
dma_fence_put(submit->fence);
+
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
@@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
goto out;
}
+ /*
+ * Allocate an id which can be used by WAIT_FENCE ioctl to map back
+ * to the underlying fence.
+ */
+ mutex_lock(&queue->lock);
+ submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
+ submit->fence, 0, INT_MAX, GFP_KERNEL);
+ mutex_unlock(&queue->lock);
+ if (submit->fence_id < 0) {
+ ret = submit->fence_id = 0;
+ submit->fence_id = 0;
+ goto out;
+ }
+
if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
struct sync_file *sync_file = sync_file_create(submit->fence);
if (!sync_file) {
@@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
msm_gpu_submit(gpu, submit);
- args->fence = submit->fence->seqno;
+ args->fence = submit->fence_id;
msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
msm_process_post_deps(post_deps, args->nr_out_syncobjs,
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 96efcb31e502..579627252540 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
* which set of pgtables do submits jobs associated with the
* submitqueue use)
* @node: node in the context's list of submitqueues
+ * @fence_idr: maps fence-id to dma_fence for userspace visible fence
+ * seqno, protected by submitqueue lock
+ * @lock: submitqueue lock
* @ref: reference count
*/
struct msm_gpu_submitqueue {
@@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
int faults;
struct msm_file_private *ctx;
struct list_head node;
+ struct idr fence_idr;
+ struct mutex lock;
struct kref ref;
};
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 9e9fec61d629..66f8d0fb38b0 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
struct msm_gpu_submitqueue *queue = container_of(kref,
struct msm_gpu_submitqueue, ref);
+ idr_destroy(&queue->fence_idr);
+
msm_file_private_put(queue->ctx);
kfree(queue);
@@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
if (id)
*id = queue->id;
+ idr_init(&queue->fence_idr);
+ mutex_init(&queue->lock);
+
list_add_tail(&queue->node, &ctx->submitqueues);
write_unlock(&ctx->queuelock);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH v4 09/13] drm/msm: Conversion to drm scheduler
2021-07-28 1:06 [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups Rob Clark
2021-07-28 1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
@ 2021-07-28 1:06 ` Rob Clark
1 sibling, 0 replies; 7+ messages in thread
From: Rob Clark @ 2021-07-28 1:06 UTC (permalink / raw)
To: dri-devel
Cc: freedreno, Rob Clark, Christian König, Rob Clark, Sean Paul,
David Airlie, Daniel Vetter, Sumit Semwal,
open list:DRM DRIVER FOR MSM ADRENO GPU, open list,
open list:DMA BUFFER SHARING FRAMEWORK,
moderated list:DMA BUFFER SHARING FRAMEWORK
From: Rob Clark <robdclark@chromium.org>
For existing adrenos, there is one or more ringbuffer, depending on
whether preemption is supported. When preemption is supported, each
ringbuffer has it's own priority. A submitqueue (which maps to a
gl context or vk queue in userspace) is mapped to a specific ring-
buffer at creation time, based on the submitqueue's priority.
Each ringbuffer has it's own drm_gpu_scheduler. Each submitqueue
maps to a drm_sched_entity. And each submit maps to a drm_sched_job.
Closes: https://gitlab.freedesktop.org/drm/msm/-/issues/4
Signed-off-by: Rob Clark <robdclark@chromium.org>
Acked-by: Christian König <christian.koenig@amd.com>
---
drivers/gpu/drm/msm/Kconfig | 1 +
drivers/gpu/drm/msm/msm_gem.c | 35 ------
drivers/gpu/drm/msm/msm_gem.h | 26 ++++-
drivers/gpu/drm/msm/msm_gem_submit.c | 161 +++++++++++++-------------
drivers/gpu/drm/msm/msm_gpu.c | 13 +--
drivers/gpu/drm/msm/msm_gpu.h | 2 +
drivers/gpu/drm/msm/msm_rd.c | 6 +-
drivers/gpu/drm/msm/msm_ringbuffer.c | 66 +++++++++++
drivers/gpu/drm/msm/msm_ringbuffer.h | 12 ++
drivers/gpu/drm/msm/msm_submitqueue.c | 26 +++++
10 files changed, 217 insertions(+), 131 deletions(-)
diff --git a/drivers/gpu/drm/msm/Kconfig b/drivers/gpu/drm/msm/Kconfig
index 52536e7adb95..dc7f3e40850b 100644
--- a/drivers/gpu/drm/msm/Kconfig
+++ b/drivers/gpu/drm/msm/Kconfig
@@ -14,6 +14,7 @@ config DRM_MSM
select REGULATOR
select DRM_KMS_HELPER
select DRM_PANEL
+ select DRM_SCHED
select SHMEM
select TMPFS
select QCOM_SCM if ARCH_QCOM
diff --git a/drivers/gpu/drm/msm/msm_gem.c b/drivers/gpu/drm/msm/msm_gem.c
index 4e99c448b83a..a527a6b1d6ba 100644
--- a/drivers/gpu/drm/msm/msm_gem.c
+++ b/drivers/gpu/drm/msm/msm_gem.c
@@ -806,41 +806,6 @@ void msm_gem_vunmap(struct drm_gem_object *obj)
msm_obj->vaddr = NULL;
}
-/* must be called before _move_to_active().. */
-int msm_gem_sync_object(struct drm_gem_object *obj,
- struct msm_fence_context *fctx, bool exclusive)
-{
- struct dma_resv_list *fobj;
- struct dma_fence *fence;
- int i, ret;
-
- fobj = dma_resv_shared_list(obj->resv);
- if (!fobj || (fobj->shared_count == 0)) {
- fence = dma_resv_excl_fence(obj->resv);
- /* don't need to wait on our own fences, since ring is fifo */
- if (fence && (fence->context != fctx->context)) {
- ret = dma_fence_wait(fence, true);
- if (ret)
- return ret;
- }
- }
-
- if (!exclusive || !fobj)
- return 0;
-
- for (i = 0; i < fobj->shared_count; i++) {
- fence = rcu_dereference_protected(fobj->shared[i],
- dma_resv_held(obj->resv));
- if (fence->context != fctx->context) {
- ret = dma_fence_wait(fence, true);
- if (ret)
- return ret;
- }
- }
-
- return 0;
-}
-
void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu)
{
struct msm_gem_object *msm_obj = to_msm_bo(obj);
diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h
index e0579abda5b9..a48114058ff9 100644
--- a/drivers/gpu/drm/msm/msm_gem.h
+++ b/drivers/gpu/drm/msm/msm_gem.h
@@ -9,6 +9,7 @@
#include <linux/kref.h>
#include <linux/dma-resv.h>
+#include "drm/gpu_scheduler.h"
#include "msm_drv.h"
/* Make all GEM related WARN_ON()s ratelimited.. when things go wrong they
@@ -143,8 +144,6 @@ void *msm_gem_get_vaddr_active(struct drm_gem_object *obj);
void msm_gem_put_vaddr_locked(struct drm_gem_object *obj);
void msm_gem_put_vaddr(struct drm_gem_object *obj);
int msm_gem_madvise(struct drm_gem_object *obj, unsigned madv);
-int msm_gem_sync_object(struct drm_gem_object *obj,
- struct msm_fence_context *fctx, bool exclusive);
void msm_gem_active_get(struct drm_gem_object *obj, struct msm_gpu *gpu);
void msm_gem_active_put(struct drm_gem_object *obj);
int msm_gem_cpu_prep(struct drm_gem_object *obj, uint32_t op, ktime_t *timeout);
@@ -311,6 +310,7 @@ void msm_gem_vunmap(struct drm_gem_object *obj);
* make it easier to unwind when things go wrong, etc).
*/
struct msm_gem_submit {
+ struct drm_sched_job base;
struct kref ref;
struct drm_device *dev;
struct msm_gpu *gpu;
@@ -319,7 +319,22 @@ struct msm_gem_submit {
struct list_head bo_list;
struct ww_acquire_ctx ticket;
uint32_t seqno; /* Sequence number of the submit on the ring */
- struct dma_fence *fence;
+
+ /* Array of struct dma_fence * to block on before submitting this job.
+ */
+ struct xarray deps;
+ unsigned long last_dep;
+
+ /* Hw fence, which is created when the scheduler executes the job, and
+ * is signaled when the hw finishes (via seqno write from cmdstream)
+ */
+ struct dma_fence *hw_fence;
+
+ /* Userspace visible fence, which is signaled by the scheduler after
+ * the hw_fence is signaled.
+ */
+ struct dma_fence *user_fence;
+
int fence_id; /* key into queue->fence_idr */
struct msm_gpu_submitqueue *queue;
struct pid *pid; /* submitting process */
@@ -350,6 +365,11 @@ struct msm_gem_submit {
} bos[];
};
+static inline struct msm_gem_submit *to_msm_submit(struct drm_sched_job *job)
+{
+ return container_of(job, struct msm_gem_submit, base);
+}
+
void __msm_gem_submit_destroy(struct kref *kref);
static inline void msm_gem_submit_get(struct msm_gem_submit *submit)
diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
index f570155bc086..2b158433a6e5 100644
--- a/drivers/gpu/drm/msm/msm_gem_submit.c
+++ b/drivers/gpu/drm/msm/msm_gem_submit.c
@@ -33,6 +33,7 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
{
struct msm_gem_submit *submit;
uint64_t sz;
+ int ret;
sz = struct_size(submit, bos, nr_bos) +
((u64)nr_cmds * sizeof(submit->cmd[0]));
@@ -44,6 +45,14 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev,
if (!submit)
return ERR_PTR(-ENOMEM);
+ ret = drm_sched_job_init(&submit->base, &queue->entity, queue);
+ if (ret) {
+ kfree(submit);
+ return ERR_PTR(ret);
+ }
+
+ xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
+
kref_init(&submit->ref);
submit->dev = dev;
submit->aspace = queue->ctx->aspace;
@@ -63,6 +72,8 @@ void __msm_gem_submit_destroy(struct kref *kref)
{
struct msm_gem_submit *submit =
container_of(kref, struct msm_gem_submit, ref);
+ unsigned long index;
+ struct dma_fence *fence;
unsigned i;
if (submit->fence_id) {
@@ -71,7 +82,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
mutex_unlock(&submit->queue->lock);
}
- dma_fence_put(submit->fence);
+ xa_for_each (&submit->deps, index, fence) {
+ dma_fence_put(fence);
+ }
+
+ xa_destroy(&submit->deps);
+
+ dma_fence_put(submit->user_fence);
+ dma_fence_put(submit->hw_fence);
put_pid(submit->pid);
msm_submitqueue_put(submit->queue);
@@ -307,7 +325,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 msm_gem_object *msm_obj = submit->bos[i].obj;
+ struct drm_gem_object *obj = &submit->bos[i].obj->base;
bool write = submit->bos[i].flags & MSM_SUBMIT_BO_WRITE;
if (!write) {
@@ -316,8 +334,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
* strange place to call it. OTOH this is a
* convenient can-fail point to hook it in.
*/
- ret = dma_resv_reserve_shared(msm_obj->base.resv,
- 1);
+ ret = dma_resv_reserve_shared(obj->resv, 1);
if (ret)
return ret;
}
@@ -325,7 +342,7 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit)
if (no_implicit)
continue;
- ret = msm_gem_sync_object(&msm_obj->base, submit->ring->fctx,
+ ret = drm_gem_fence_array_add_implicit(&submit->deps, obj,
write);
if (ret)
break;
@@ -376,9 +393,9 @@ static void submit_attach_object_fences(struct msm_gem_submit *submit)
struct drm_gem_object *obj = &submit->bos[i].obj->base;
if (submit->bos[i].flags & MSM_SUBMIT_BO_WRITE)
- dma_resv_add_excl_fence(obj->resv, submit->fence);
+ dma_resv_add_excl_fence(obj->resv, submit->user_fence);
else if (submit->bos[i].flags & MSM_SUBMIT_BO_READ)
- dma_resv_add_shared_fence(obj->resv, submit->fence);
+ dma_resv_add_shared_fence(obj->resv, submit->user_fence);
}
}
@@ -517,12 +534,12 @@ struct msm_submit_post_dep {
struct dma_fence_chain *chain;
};
-static struct drm_syncobj **msm_wait_deps(struct drm_device *dev,
- struct drm_file *file,
- uint64_t in_syncobjs_addr,
- uint32_t nr_in_syncobjs,
- size_t syncobj_stride,
- struct msm_ringbuffer *ring)
+static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit,
+ struct drm_file *file,
+ uint64_t in_syncobjs_addr,
+ uint32_t nr_in_syncobjs,
+ size_t syncobj_stride,
+ struct msm_ringbuffer *ring)
{
struct drm_syncobj **syncobjs = NULL;
struct drm_msm_gem_submit_syncobj syncobj_desc = {0};
@@ -546,7 +563,7 @@ static struct drm_syncobj **msm_wait_deps(struct drm_device *dev,
}
if (syncobj_desc.point &&
- !drm_core_check_feature(dev, DRIVER_SYNCOBJ_TIMELINE)) {
+ !drm_core_check_feature(submit->dev, DRIVER_SYNCOBJ_TIMELINE)) {
ret = -EOPNOTSUPP;
break;
}
@@ -561,10 +578,7 @@ static struct drm_syncobj **msm_wait_deps(struct drm_device *dev,
if (ret)
break;
- if (!dma_fence_match_context(fence, ring->fctx->context))
- ret = dma_fence_wait(fence, true);
-
- dma_fence_put(fence);
+ ret = drm_gem_fence_array_add(&submit->deps, fence);
if (ret)
break;
@@ -741,47 +755,6 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
trace_msm_gpu_submit(pid_nr(pid), ring->id, submitid,
args->nr_bos, args->nr_cmds);
- if (args->flags & MSM_SUBMIT_FENCE_FD_IN) {
- struct dma_fence *in_fence;
-
- in_fence = sync_file_get_fence(args->fence_fd);
-
- if (!in_fence)
- return -EINVAL;
-
- /*
- * Wait if the fence is from a foreign context, or if the fence
- * array contains any fence from a foreign context.
- */
- ret = 0;
- if (!dma_fence_match_context(in_fence, ring->fctx->context))
- ret = dma_fence_wait(in_fence, true);
-
- dma_fence_put(in_fence);
- if (ret)
- return ret;
- }
-
- if (args->flags & MSM_SUBMIT_SYNCOBJ_IN) {
- syncobjs_to_reset = msm_wait_deps(dev, file,
- args->in_syncobjs,
- args->nr_in_syncobjs,
- args->syncobj_stride, ring);
- if (IS_ERR(syncobjs_to_reset))
- return PTR_ERR(syncobjs_to_reset);
- }
-
- if (args->flags & MSM_SUBMIT_SYNCOBJ_OUT) {
- post_deps = msm_parse_post_deps(dev, file,
- args->out_syncobjs,
- args->nr_out_syncobjs,
- args->syncobj_stride);
- if (IS_ERR(post_deps)) {
- ret = PTR_ERR(post_deps);
- goto out_post_unlock;
- }
- }
-
ret = mutex_lock_interruptible(&dev->struct_mutex);
if (ret)
goto out_post_unlock;
@@ -807,22 +780,50 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
if (args->flags & MSM_SUBMIT_SUDO)
submit->in_rb = true;
+ if (args->flags & MSM_SUBMIT_FENCE_FD_IN) {
+ struct dma_fence *in_fence;
+
+ in_fence = sync_file_get_fence(args->fence_fd);
+
+ if (!in_fence) {
+ ret = -EINVAL;
+ goto out_unlock;
+ }
+
+ ret = drm_gem_fence_array_add(&submit->deps, in_fence);
+ if (ret)
+ goto out_unlock;
+ }
+
+ if (args->flags & MSM_SUBMIT_SYNCOBJ_IN) {
+ syncobjs_to_reset = msm_parse_deps(submit, file,
+ args->in_syncobjs,
+ args->nr_in_syncobjs,
+ args->syncobj_stride, ring);
+ if (IS_ERR(syncobjs_to_reset)) {
+ ret = PTR_ERR(syncobjs_to_reset);
+ goto out_unlock;
+ }
+ }
+
+ if (args->flags & MSM_SUBMIT_SYNCOBJ_OUT) {
+ post_deps = msm_parse_post_deps(dev, file,
+ args->out_syncobjs,
+ args->nr_out_syncobjs,
+ args->syncobj_stride);
+ if (IS_ERR(post_deps)) {
+ ret = PTR_ERR(post_deps);
+ goto out_unlock;
+ }
+ }
+
ret = submit_lookup_objects(submit, args, file);
if (ret)
- goto out_pre_pm;
+ goto out;
ret = submit_lookup_cmds(submit, args, file);
if (ret)
- goto out_pre_pm;
-
- /*
- * Thanks to dev_pm_opp opp_table_lock interactions with mm->mmap_sem
- * in the resume path, we need to to rpm get before we lock objs.
- * Which unfortunately might involve powering up the GPU sooner than
- * is necessary. But at least in the explicit fencing case, we will
- * have already done all the fence waiting.
- */
- pm_runtime_get_sync(&gpu->pdev->dev);
+ goto out;
/* copy_*_user while holding a ww ticket upsets lockdep */
ww_acquire_init(&submit->ticket, &reservation_ww_class);
@@ -869,12 +870,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
submit->nr_cmds = i;
- submit->fence = msm_fence_alloc(ring->fctx);
- if (IS_ERR(submit->fence)) {
- ret = PTR_ERR(submit->fence);
- submit->fence = NULL;
- goto out;
- }
+ submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);
/*
* Allocate an id which can be used by WAIT_FENCE ioctl to map back
@@ -882,7 +878,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
*/
mutex_lock(&queue->lock);
submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
- submit->fence, 0, INT_MAX, GFP_KERNEL);
+ submit->user_fence, 0, INT_MAX, GFP_KERNEL);
mutex_unlock(&queue->lock);
if (submit->fence_id < 0) {
ret = submit->fence_id = 0;
@@ -891,7 +887,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
}
if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
- struct sync_file *sync_file = sync_file_create(submit->fence);
+ struct sync_file *sync_file = sync_file_create(submit->user_fence);
if (!sync_file) {
ret = -ENOMEM;
goto out;
@@ -902,18 +898,19 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
submit_attach_object_fences(submit);
- msm_gpu_submit(gpu, submit);
+ /* The scheduler owns a ref now: */
+ msm_gem_submit_get(submit);
+
+ drm_sched_entity_push_job(&submit->base, &queue->entity);
args->fence = submit->fence_id;
msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
msm_process_post_deps(post_deps, args->nr_out_syncobjs,
- submit->fence);
+ submit->user_fence);
out:
- pm_runtime_put(&gpu->pdev->dev);
-out_pre_pm:
submit_cleanup(submit, !!ret);
if (has_ww_ticket)
ww_acquire_fini(&submit->ticket);
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 5bfc4d24a956..8a3a592da3a4 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -176,8 +176,8 @@ static void update_fences(struct msm_gpu *gpu, struct msm_ringbuffer *ring,
break;
msm_update_fence(submit->ring->fctx,
- submit->fence->seqno);
- dma_fence_signal(submit->fence);
+ submit->hw_fence->seqno);
+ dma_fence_signal(submit->hw_fence);
}
spin_unlock_irqrestore(&ring->submit_lock, flags);
}
@@ -380,10 +380,6 @@ static void recover_worker(struct kthread_work *work)
put_task_struct(task);
}
- /* msm_rd_dump_submit() needs bo locked to dump: */
- for (i = 0; i < submit->nr_bos; i++)
- msm_gem_lock(&submit->bos[i].obj->base);
-
if (comm && cmd) {
DRM_DEV_ERROR(dev->dev, "%s: offending task: %s (%s)\n",
gpu->name, comm, cmd);
@@ -393,9 +389,6 @@ static void recover_worker(struct kthread_work *work)
} else {
msm_rd_dump_submit(priv->hangrd, submit, NULL);
}
-
- for (i = 0; i < submit->nr_bos; i++)
- msm_gem_unlock(&submit->bos[i].obj->base);
}
/* Record the crash state */
@@ -704,7 +697,7 @@ static void retire_submits(struct msm_gpu *gpu)
* been signalled, then later submits are not signalled
* either, so we are also done.
*/
- if (submit && dma_fence_is_signaled(submit->fence)) {
+ if (submit && dma_fence_is_signaled(submit->hw_fence)) {
retire_submit(gpu, ring, submit);
} else {
break;
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 579627252540..b912cacaecc0 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -267,6 +267,7 @@ struct msm_gpu_perfcntr {
* seqno, protected by submitqueue lock
* @lock: submitqueue lock
* @ref: reference count
+ * @entity: the submit job-queue
*/
struct msm_gpu_submitqueue {
int id;
@@ -278,6 +279,7 @@ struct msm_gpu_submitqueue {
struct idr fence_idr;
struct mutex lock;
struct kref ref;
+ struct drm_sched_entity entity;
};
struct msm_gpu_state_bo {
diff --git a/drivers/gpu/drm/msm/msm_rd.c b/drivers/gpu/drm/msm/msm_rd.c
index 659e5cc4b40a..b55398a34fa4 100644
--- a/drivers/gpu/drm/msm/msm_rd.c
+++ b/drivers/gpu/drm/msm/msm_rd.c
@@ -325,15 +325,19 @@ static void snapshot_buf(struct msm_rd_state *rd,
if (!(submit->bos[idx].flags & MSM_SUBMIT_BO_READ))
return;
+ msm_gem_lock(&obj->base);
buf = msm_gem_get_vaddr_active(&obj->base);
if (IS_ERR(buf))
- return;
+ goto out_unlock;
buf += offset;
rd_write_section(rd, RD_BUFFER_CONTENTS, buf, size);
msm_gem_put_vaddr_locked(&obj->base);
+
+out_unlock:
+ msm_gem_unlock(&obj->base);
}
/* called under struct_mutex */
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c
index 437cca57d005..5643f579ac46 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.c
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.c
@@ -7,10 +7,64 @@
#include "msm_ringbuffer.h"
#include "msm_gpu.h"
+/**
+ * The max # of jobs to write into the hw ringbuffer.
+ */
+static uint num_hw_submissions = 8;
+MODULE_PARM_DESC(num_hw_submissions, "The max number of HW submissions (default 8)");
+module_param(num_hw_submissions, uint, 0600);
+
+static struct dma_fence *msm_job_dependency(struct drm_sched_job *job,
+ struct drm_sched_entity *s_entity)
+{
+ struct msm_gem_submit *submit = to_msm_submit(job);
+
+ if (!xa_empty(&submit->deps))
+ return xa_erase(&submit->deps, submit->last_dep++);
+
+ return NULL;
+}
+
+static struct dma_fence *msm_job_run(struct drm_sched_job *job)
+{
+ struct msm_gem_submit *submit = to_msm_submit(job);
+ struct msm_gpu *gpu = submit->gpu;
+
+ submit->hw_fence = msm_fence_alloc(submit->ring->fctx);
+
+ pm_runtime_get_sync(&gpu->pdev->dev);
+
+ /* TODO move submit path over to using a per-ring lock.. */
+ mutex_lock(&gpu->dev->struct_mutex);
+
+ msm_gpu_submit(gpu, submit);
+
+ mutex_unlock(&gpu->dev->struct_mutex);
+
+ pm_runtime_put(&gpu->pdev->dev);
+
+ return dma_fence_get(submit->hw_fence);
+}
+
+static void msm_job_free(struct drm_sched_job *job)
+{
+ struct msm_gem_submit *submit = to_msm_submit(job);
+
+ drm_sched_job_cleanup(job);
+ msm_gem_submit_put(submit);
+}
+
+const struct drm_sched_backend_ops msm_sched_ops = {
+ .dependency = msm_job_dependency,
+ .run_job = msm_job_run,
+ .free_job = msm_job_free
+};
+
struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,
void *memptrs, uint64_t memptrs_iova)
{
struct msm_ringbuffer *ring;
+ long sched_timeout;
char name[32];
int ret;
@@ -45,6 +99,16 @@ struct msm_ringbuffer *msm_ringbuffer_new(struct msm_gpu *gpu, int id,
ring->memptrs = memptrs;
ring->memptrs_iova = memptrs_iova;
+ /* currently managing hangcheck ourselves: */
+ sched_timeout = MAX_SCHEDULE_TIMEOUT;
+
+ ret = drm_sched_init(&ring->sched, &msm_sched_ops,
+ num_hw_submissions, 0, sched_timeout,
+ NULL, to_msm_bo(ring->bo)->name);
+ if (ret) {
+ goto fail;
+ }
+
INIT_LIST_HEAD(&ring->submits);
spin_lock_init(&ring->submit_lock);
spin_lock_init(&ring->preempt_lock);
@@ -65,6 +129,8 @@ void msm_ringbuffer_destroy(struct msm_ringbuffer *ring)
if (IS_ERR_OR_NULL(ring))
return;
+ drm_sched_fini(&ring->sched);
+
msm_fence_context_free(ring->fctx);
msm_gem_kernel_put(ring->bo, ring->gpu->aspace);
diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.h b/drivers/gpu/drm/msm/msm_ringbuffer.h
index fe55d4a1aa16..d8c63df4e9ca 100644
--- a/drivers/gpu/drm/msm/msm_ringbuffer.h
+++ b/drivers/gpu/drm/msm/msm_ringbuffer.h
@@ -7,6 +7,7 @@
#ifndef __MSM_RINGBUFFER_H__
#define __MSM_RINGBUFFER_H__
+#include "drm/gpu_scheduler.h"
#include "msm_drv.h"
#define rbmemptr(ring, member) \
@@ -40,8 +41,19 @@ struct msm_ringbuffer {
struct drm_gem_object *bo;
uint32_t *start, *end, *cur, *next;
+ /*
+ * The job scheduler for this ring.
+ */
+ struct drm_gpu_scheduler sched;
+
/*
* List of in-flight submits on this ring. Protected by submit_lock.
+ *
+ * Currently just submits that are already written into the ring, not
+ * submits that are still in drm_gpu_scheduler's queues. At a later
+ * step we could probably move to letting drm_gpu_scheduler manage
+ * hangcheck detection and keep track of submit jobs that are in-
+ * flight.
*/
struct list_head submits;
spinlock_t submit_lock;
diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
index 66f8d0fb38b0..682ba2a7c0ec 100644
--- a/drivers/gpu/drm/msm/msm_submitqueue.c
+++ b/drivers/gpu/drm/msm/msm_submitqueue.c
@@ -14,6 +14,8 @@ void msm_submitqueue_destroy(struct kref *kref)
idr_destroy(&queue->fence_idr);
+ drm_sched_entity_destroy(&queue->entity);
+
msm_file_private_put(queue->ctx);
kfree(queue);
@@ -64,6 +66,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
{
struct msm_drm_private *priv = drm->dev_private;
struct msm_gpu_submitqueue *queue;
+ struct msm_ringbuffer *ring;
+ struct drm_gpu_scheduler *sched;
+ int ret;
if (!ctx)
return -ENODEV;
@@ -83,6 +88,27 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
queue->flags = flags;
queue->prio = prio;
+ ring = priv->gpu->rb[prio];
+ sched = &ring->sched;
+
+ /*
+ * TODO we can allow more priorities than we have ringbuffers by
+ * mapping:
+ *
+ * ring = prio / 3;
+ * ent_prio = DRM_SCHED_PRIORITY_MIN + (prio % 3);
+ *
+ * Probably avoid using DRM_SCHED_PRIORITY_KERNEL as that is
+ * treated specially in places.
+ */
+ ret = drm_sched_entity_init(&queue->entity,
+ DRM_SCHED_PRIORITY_NORMAL,
+ &sched, 1, NULL);
+ if (ret) {
+ kfree(queue);
+ return ret;
+ }
+
write_lock(&ctx->queuelock);
queue->ctx = msm_file_private_get(ctx);
--
2.31.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
2021-07-28 1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
@ 2021-11-10 15:28 ` Akhil P Oommen
2021-11-10 16:55 ` Rob Clark
0 siblings, 1 reply; 7+ messages in thread
From: Akhil P Oommen @ 2021-11-10 15:28 UTC (permalink / raw)
To: Rob Clark, dri-devel
Cc: Rob Clark, David Airlie, freedreno, open list,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, Sean Paul,
Christian König, open list:DMA BUFFER SHARING FRAMEWORK
On 7/28/2021 6:36 AM, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
>
> Previously the (non-fd) fence returned from submit ioctl was a raw
> seqno, which is scoped to the ring. But from UABI standpoint, the
> ioctls related to seqno fences all specify a submitqueue. We can
> take advantage of that to replace the seqno fences with a cyclic idr
> handle.
>
> This is in preperation for moving to drm scheduler, at which point
> the submit ioctl will return after queuing the submit job to the
> scheduler, but before the submit is written into the ring (and
> therefore before a ring seqno has been assigned). Which means we
> need to replace the dma_fence that userspace may need to wait on
> with a scheduler fence.
>
> Signed-off-by: Rob Clark <robdclark@chromium.org>
> Acked-by: Christian König <christian.koenig@amd.com>
> ---
> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
> drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
> drivers/gpu/drm/msm/msm_fence.h | 3 --
> drivers/gpu/drm/msm/msm_gem.h | 1 +
> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
> 7 files changed, 61 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 9b8fa2ad0d84..1594ae39d54f 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> ktime_t timeout = to_ktime(args->timeout);
> struct msm_gpu_submitqueue *queue;
> struct msm_gpu *gpu = priv->gpu;
> + struct dma_fence *fence;
> int ret;
>
> if (args->pad) {
> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> if (!queue)
> return -ENOENT;
>
> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
> - true);
> + /*
> + * Map submitqueue scoped "seqno" (which is actually an idr key)
> + * back to underlying dma-fence
> + *
> + * The fence is removed from the fence_idr when the submit is
> + * retired, so if the fence is not found it means there is nothing
> + * to wait for
> + */
> + ret = mutex_lock_interruptible(&queue->lock);
> + if (ret)
> + return ret;
> + fence = idr_find(&queue->fence_idr, args->fence);
> + if (fence)
> + fence = dma_fence_get_rcu(fence);
> + mutex_unlock(&queue->lock);
> +
> + if (!fence)
> + return 0;
>
> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> + if (ret == 0) {
> + ret = -ETIMEDOUT;
> + } else if (ret != -ERESTARTSYS) {
> + ret = 0;
> + }
> +
> + dma_fence_put(fence);
> msm_submitqueue_put(queue);
> +
> return ret;
> }
>
> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> index b92a9091a1e2..f2cece542c3f 100644
> --- a/drivers/gpu/drm/msm/msm_fence.c
> +++ b/drivers/gpu/drm/msm/msm_fence.c
> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
> strncpy(fctx->name, name, sizeof(fctx->name));
> fctx->context = dma_fence_context_alloc(1);
> fctx->fenceptr = fenceptr;
> - init_waitqueue_head(&fctx->event);
> spin_lock_init(&fctx->spinlock);
>
> return fctx;
> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
> (int32_t)(*fctx->fenceptr - fence) >= 0;
> }
>
> -/* legacy path for WAIT_FENCE ioctl: */
> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> - ktime_t *timeout, bool interruptible)
> -{
> - int ret;
> -
> - if (fence > fctx->last_fence) {
> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
> - fctx->name, fence, fctx->last_fence);
> - return -EINVAL;
Rob, we changed this pre-existing behaviour in this patch. Now, when
userspace tries to wait on a future fence, we don't return an error.
I just want to check if this was accidental or not?
-Akhil.
> - }
> -
> - if (!timeout) {
> - /* no-wait: */
> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
> - } else {
> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
> -
> - if (interruptible)
> - ret = wait_event_interruptible_timeout(fctx->event,
> - fence_completed(fctx, fence),
> - remaining_jiffies);
> - else
> - ret = wait_event_timeout(fctx->event,
> - fence_completed(fctx, fence),
> - remaining_jiffies);
> -
> - if (ret == 0) {
> - DBG("timeout waiting for fence: %u (completed: %u)",
> - fence, fctx->completed_fence);
> - ret = -ETIMEDOUT;
> - } else if (ret != -ERESTARTSYS) {
> - ret = 0;
> - }
> - }
> -
> - return ret;
> -}
> -
> /* called from workqueue */
> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
> {
> spin_lock(&fctx->spinlock);
> fctx->completed_fence = max(fence, fctx->completed_fence);
> spin_unlock(&fctx->spinlock);
> -
> - wake_up_all(&fctx->event);
> }
>
> struct msm_fence {
> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> index 6ab97062ff1a..4783db528bcc 100644
> --- a/drivers/gpu/drm/msm/msm_fence.h
> +++ b/drivers/gpu/drm/msm/msm_fence.h
> @@ -49,7 +49,6 @@ struct msm_fence_context {
> */
> volatile uint32_t *fenceptr;
>
> - wait_queue_head_t event;
> spinlock_t spinlock;
> };
>
> @@ -57,8 +56,6 @@ 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);
>
> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> - ktime_t *timeout, bool interruptible);
> 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.h b/drivers/gpu/drm/msm/msm_gem.h
> index da3af702a6c8..e0579abda5b9 100644
> --- a/drivers/gpu/drm/msm/msm_gem.h
> +++ b/drivers/gpu/drm/msm/msm_gem.h
> @@ -320,6 +320,7 @@ struct msm_gem_submit {
> struct ww_acquire_ctx ticket;
> uint32_t seqno; /* Sequence number of the submit on the ring */
> struct dma_fence *fence;
> + int fence_id; /* key into queue->fence_idr */
> struct msm_gpu_submitqueue *queue;
> struct pid *pid; /* submitting process */
> bool fault_dumped; /* Limit devcoredump dumping to one per submit */
> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> index 4f02fa3c78f9..f6f595aae2c5 100644
> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
> container_of(kref, struct msm_gem_submit, ref);
> unsigned i;
>
> + if (submit->fence_id) {
> + mutex_lock(&submit->queue->lock);
> + idr_remove(&submit->queue->fence_idr, submit->fence_id);
> + mutex_unlock(&submit->queue->lock);
> + }
> +
> dma_fence_put(submit->fence);
> +
> put_pid(submit->pid);
> msm_submitqueue_put(submit->queue);
>
> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> goto out;
> }
>
> + /*
> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back
> + * to the underlying fence.
> + */
> + mutex_lock(&queue->lock);
> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
> + submit->fence, 0, INT_MAX, GFP_KERNEL);
> + mutex_unlock(&queue->lock);
> + if (submit->fence_id < 0) {
> + ret = submit->fence_id = 0;
> + submit->fence_id = 0;
> + goto out;
> + }
> +
> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
> struct sync_file *sync_file = sync_file_create(submit->fence);
> if (!sync_file) {
> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>
> msm_gpu_submit(gpu, submit);
>
> - args->fence = submit->fence->seqno;
> + args->fence = submit->fence_id;
>
> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
> msm_process_post_deps(post_deps, args->nr_out_syncobjs,
> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> index 96efcb31e502..579627252540 100644
> --- a/drivers/gpu/drm/msm/msm_gpu.h
> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
> * which set of pgtables do submits jobs associated with the
> * submitqueue use)
> * @node: node in the context's list of submitqueues
> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence
> + * seqno, protected by submitqueue lock
> + * @lock: submitqueue lock
> * @ref: reference count
> */
> struct msm_gpu_submitqueue {
> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
> int faults;
> struct msm_file_private *ctx;
> struct list_head node;
> + struct idr fence_idr;
> + struct mutex lock;
> struct kref ref;
> };
>
> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> index 9e9fec61d629..66f8d0fb38b0 100644
> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
> struct msm_gpu_submitqueue *queue = container_of(kref,
> struct msm_gpu_submitqueue, ref);
>
> + idr_destroy(&queue->fence_idr);
> +
> msm_file_private_put(queue->ctx);
>
> kfree(queue);
> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
> if (id)
> *id = queue->id;
>
> + idr_init(&queue->fence_idr);
> + mutex_init(&queue->lock);
> +
> list_add_tail(&queue->node, &ctx->submitqueues);
>
> write_unlock(&ctx->queuelock);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
2021-11-10 15:28 ` Akhil P Oommen
@ 2021-11-10 16:55 ` Rob Clark
2021-11-11 15:53 ` Akhil P Oommen
0 siblings, 1 reply; 7+ messages in thread
From: Rob Clark @ 2021-11-10 16:55 UTC (permalink / raw)
To: Akhil P Oommen
Cc: dri-devel, Rob Clark, David Airlie, freedreno, open list,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, Sean Paul,
Christian König, open list:DMA BUFFER SHARING FRAMEWORK
On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> On 7/28/2021 6:36 AM, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > Previously the (non-fd) fence returned from submit ioctl was a raw
> > seqno, which is scoped to the ring. But from UABI standpoint, the
> > ioctls related to seqno fences all specify a submitqueue. We can
> > take advantage of that to replace the seqno fences with a cyclic idr
> > handle.
> >
> > This is in preperation for moving to drm scheduler, at which point
> > the submit ioctl will return after queuing the submit job to the
> > scheduler, but before the submit is written into the ring (and
> > therefore before a ring seqno has been assigned). Which means we
> > need to replace the dma_fence that userspace may need to wait on
> > with a scheduler fence.
> >
> > Signed-off-by: Rob Clark <robdclark@chromium.org>
> > Acked-by: Christian König <christian.koenig@amd.com>
> > ---
> > drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
> > drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
> > drivers/gpu/drm/msm/msm_fence.h | 3 --
> > drivers/gpu/drm/msm/msm_gem.h | 1 +
> > drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
> > drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
> > drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
> > 7 files changed, 61 insertions(+), 48 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 9b8fa2ad0d84..1594ae39d54f 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> > ktime_t timeout = to_ktime(args->timeout);
> > struct msm_gpu_submitqueue *queue;
> > struct msm_gpu *gpu = priv->gpu;
> > + struct dma_fence *fence;
> > int ret;
> >
> > if (args->pad) {
> > @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> > if (!queue)
> > return -ENOENT;
> >
> > - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
> > - true);
> > + /*
> > + * Map submitqueue scoped "seqno" (which is actually an idr key)
> > + * back to underlying dma-fence
> > + *
> > + * The fence is removed from the fence_idr when the submit is
> > + * retired, so if the fence is not found it means there is nothing
> > + * to wait for
> > + */
> > + ret = mutex_lock_interruptible(&queue->lock);
> > + if (ret)
> > + return ret;
> > + fence = idr_find(&queue->fence_idr, args->fence);
> > + if (fence)
> > + fence = dma_fence_get_rcu(fence);
> > + mutex_unlock(&queue->lock);
> > +
> > + if (!fence)
> > + return 0;
> >
> > + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> > + if (ret == 0) {
> > + ret = -ETIMEDOUT;
> > + } else if (ret != -ERESTARTSYS) {
> > + ret = 0;
> > + }
> > +
> > + dma_fence_put(fence);
> > msm_submitqueue_put(queue);
> > +
> > return ret;
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> > index b92a9091a1e2..f2cece542c3f 100644
> > --- a/drivers/gpu/drm/msm/msm_fence.c
> > +++ b/drivers/gpu/drm/msm/msm_fence.c
> > @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
> > strncpy(fctx->name, name, sizeof(fctx->name));
> > fctx->context = dma_fence_context_alloc(1);
> > fctx->fenceptr = fenceptr;
> > - init_waitqueue_head(&fctx->event);
> > spin_lock_init(&fctx->spinlock);
> >
> > return fctx;
> > @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
> > (int32_t)(*fctx->fenceptr - fence) >= 0;
> > }
> >
> > -/* legacy path for WAIT_FENCE ioctl: */
> > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> > - ktime_t *timeout, bool interruptible)
> > -{
> > - int ret;
> > -
> > - if (fence > fctx->last_fence) {
> > - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
> > - fctx->name, fence, fctx->last_fence);
> > - return -EINVAL;
>
> Rob, we changed this pre-existing behaviour in this patch. Now, when
> userspace tries to wait on a future fence, we don't return an error.
>
> I just want to check if this was accidental or not?
Hmm, perhaps we should do this to restore the previous behavior:
-------------
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 73e827641024..3dd6da56eae6 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct
drm_device *dev, void *data,
fence = dma_fence_get_rcu(fence);
mutex_unlock(&queue->lock);
- if (!fence)
- return 0;
+ if (!fence) {
+ struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx;
+ DRM_ERROR_RATELIMITED("%s: waiting on invalid fence:
%u (of %u)\n",
+ fctx->name, fence, fctx->last_fence);
+ return -EINVAL;
+ }
ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
if (ret == 0) {
-------------
BR,
-R
> -Akhil.
>
> > - }
> > -
> > - if (!timeout) {
> > - /* no-wait: */
> > - ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
> > - } else {
> > - unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
> > -
> > - if (interruptible)
> > - ret = wait_event_interruptible_timeout(fctx->event,
> > - fence_completed(fctx, fence),
> > - remaining_jiffies);
> > - else
> > - ret = wait_event_timeout(fctx->event,
> > - fence_completed(fctx, fence),
> > - remaining_jiffies);
> > -
> > - if (ret == 0) {
> > - DBG("timeout waiting for fence: %u (completed: %u)",
> > - fence, fctx->completed_fence);
> > - ret = -ETIMEDOUT;
> > - } else if (ret != -ERESTARTSYS) {
> > - ret = 0;
> > - }
> > - }
> > -
> > - return ret;
> > -}
> > -
> > /* called from workqueue */
> > void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
> > {
> > spin_lock(&fctx->spinlock);
> > fctx->completed_fence = max(fence, fctx->completed_fence);
> > spin_unlock(&fctx->spinlock);
> > -
> > - wake_up_all(&fctx->event);
> > }
> >
> > struct msm_fence {
> > diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> > index 6ab97062ff1a..4783db528bcc 100644
> > --- a/drivers/gpu/drm/msm/msm_fence.h
> > +++ b/drivers/gpu/drm/msm/msm_fence.h
> > @@ -49,7 +49,6 @@ struct msm_fence_context {
> > */
> > volatile uint32_t *fenceptr;
> >
> > - wait_queue_head_t event;
> > spinlock_t spinlock;
> > };
> >
> > @@ -57,8 +56,6 @@ 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);
> >
> > -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> > - ktime_t *timeout, bool interruptible);
> > 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.h b/drivers/gpu/drm/msm/msm_gem.h
> > index da3af702a6c8..e0579abda5b9 100644
> > --- a/drivers/gpu/drm/msm/msm_gem.h
> > +++ b/drivers/gpu/drm/msm/msm_gem.h
> > @@ -320,6 +320,7 @@ struct msm_gem_submit {
> > struct ww_acquire_ctx ticket;
> > uint32_t seqno; /* Sequence number of the submit on the ring */
> > struct dma_fence *fence;
> > + int fence_id; /* key into queue->fence_idr */
> > struct msm_gpu_submitqueue *queue;
> > struct pid *pid; /* submitting process */
> > bool fault_dumped; /* Limit devcoredump dumping to one per submit */
> > diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> > index 4f02fa3c78f9..f6f595aae2c5 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> > @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
> > container_of(kref, struct msm_gem_submit, ref);
> > unsigned i;
> >
> > + if (submit->fence_id) {
> > + mutex_lock(&submit->queue->lock);
> > + idr_remove(&submit->queue->fence_idr, submit->fence_id);
> > + mutex_unlock(&submit->queue->lock);
> > + }
> > +
> > dma_fence_put(submit->fence);
> > +
> > put_pid(submit->pid);
> > msm_submitqueue_put(submit->queue);
> >
> > @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> > goto out;
> > }
> >
> > + /*
> > + * Allocate an id which can be used by WAIT_FENCE ioctl to map back
> > + * to the underlying fence.
> > + */
> > + mutex_lock(&queue->lock);
> > + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
> > + submit->fence, 0, INT_MAX, GFP_KERNEL);
> > + mutex_unlock(&queue->lock);
> > + if (submit->fence_id < 0) {
> > + ret = submit->fence_id = 0;
> > + submit->fence_id = 0;
> > + goto out;
> > + }
> > +
> > if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
> > struct sync_file *sync_file = sync_file_create(submit->fence);
> > if (!sync_file) {
> > @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >
> > msm_gpu_submit(gpu, submit);
> >
> > - args->fence = submit->fence->seqno;
> > + args->fence = submit->fence_id;
> >
> > msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
> > msm_process_post_deps(post_deps, args->nr_out_syncobjs,
> > diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> > index 96efcb31e502..579627252540 100644
> > --- a/drivers/gpu/drm/msm/msm_gpu.h
> > +++ b/drivers/gpu/drm/msm/msm_gpu.h
> > @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
> > * which set of pgtables do submits jobs associated with the
> > * submitqueue use)
> > * @node: node in the context's list of submitqueues
> > + * @fence_idr: maps fence-id to dma_fence for userspace visible fence
> > + * seqno, protected by submitqueue lock
> > + * @lock: submitqueue lock
> > * @ref: reference count
> > */
> > struct msm_gpu_submitqueue {
> > @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
> > int faults;
> > struct msm_file_private *ctx;
> > struct list_head node;
> > + struct idr fence_idr;
> > + struct mutex lock;
> > struct kref ref;
> > };
> >
> > diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> > index 9e9fec61d629..66f8d0fb38b0 100644
> > --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> > +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> > @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
> > struct msm_gpu_submitqueue *queue = container_of(kref,
> > struct msm_gpu_submitqueue, ref);
> >
> > + idr_destroy(&queue->fence_idr);
> > +
> > msm_file_private_put(queue->ctx);
> >
> > kfree(queue);
> > @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
> > if (id)
> > *id = queue->id;
> >
> > + idr_init(&queue->fence_idr);
> > + mutex_init(&queue->lock);
> > +
> > list_add_tail(&queue->node, &ctx->submitqueues);
> >
> > write_unlock(&ctx->queuelock);
> >
>
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
2021-11-10 16:55 ` Rob Clark
@ 2021-11-11 15:53 ` Akhil P Oommen
2021-11-11 17:30 ` [Freedreno] " Rob Clark
0 siblings, 1 reply; 7+ messages in thread
From: Akhil P Oommen @ 2021-11-11 15:53 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, Rob Clark, David Airlie, freedreno, open list,
moderated list:DMA BUFFER SHARING FRAMEWORK,
open list:DRM DRIVER FOR MSM ADRENO GPU, Sean Paul,
Christian König, open list:DMA BUFFER SHARING FRAMEWORK
On 11/10/2021 10:25 PM, Rob Clark wrote:
> On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>>
>> On 7/28/2021 6:36 AM, Rob Clark wrote:
>>> From: Rob Clark <robdclark@chromium.org>
>>>
>>> Previously the (non-fd) fence returned from submit ioctl was a raw
>>> seqno, which is scoped to the ring. But from UABI standpoint, the
>>> ioctls related to seqno fences all specify a submitqueue. We can
>>> take advantage of that to replace the seqno fences with a cyclic idr
>>> handle.
>>>
>>> This is in preperation for moving to drm scheduler, at which point
>>> the submit ioctl will return after queuing the submit job to the
>>> scheduler, but before the submit is written into the ring (and
>>> therefore before a ring seqno has been assigned). Which means we
>>> need to replace the dma_fence that userspace may need to wait on
>>> with a scheduler fence.
>>>
>>> Signed-off-by: Rob Clark <robdclark@chromium.org>
>>> Acked-by: Christian König <christian.koenig@amd.com>
>>> ---
>>> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
>>> drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
>>> drivers/gpu/drm/msm/msm_fence.h | 3 --
>>> drivers/gpu/drm/msm/msm_gem.h | 1 +
>>> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
>>> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
>>> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
>>> 7 files changed, 61 insertions(+), 48 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
>>> index 9b8fa2ad0d84..1594ae39d54f 100644
>>> --- a/drivers/gpu/drm/msm/msm_drv.c
>>> +++ b/drivers/gpu/drm/msm/msm_drv.c
>>> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
>>> ktime_t timeout = to_ktime(args->timeout);
>>> struct msm_gpu_submitqueue *queue;
>>> struct msm_gpu *gpu = priv->gpu;
>>> + struct dma_fence *fence;
>>> int ret;
>>>
>>> if (args->pad) {
>>> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
>>> if (!queue)
>>> return -ENOENT;
>>>
>>> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
>>> - true);
>>> + /*
>>> + * Map submitqueue scoped "seqno" (which is actually an idr key)
>>> + * back to underlying dma-fence
>>> + *
>>> + * The fence is removed from the fence_idr when the submit is
>>> + * retired, so if the fence is not found it means there is nothing
>>> + * to wait for
>>> + */
>>> + ret = mutex_lock_interruptible(&queue->lock);
>>> + if (ret)
>>> + return ret;
>>> + fence = idr_find(&queue->fence_idr, args->fence);
>>> + if (fence)
>>> + fence = dma_fence_get_rcu(fence);
>>> + mutex_unlock(&queue->lock);
>>> +
>>> + if (!fence)
>>> + return 0;
>>>
>>> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
>>> + if (ret == 0) {
>>> + ret = -ETIMEDOUT;
>>> + } else if (ret != -ERESTARTSYS) {
>>> + ret = 0;
>>> + }
>>> +
>>> + dma_fence_put(fence);
>>> msm_submitqueue_put(queue);
>>> +
>>> return ret;
>>> }
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
>>> index b92a9091a1e2..f2cece542c3f 100644
>>> --- a/drivers/gpu/drm/msm/msm_fence.c
>>> +++ b/drivers/gpu/drm/msm/msm_fence.c
>>> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
>>> strncpy(fctx->name, name, sizeof(fctx->name));
>>> fctx->context = dma_fence_context_alloc(1);
>>> fctx->fenceptr = fenceptr;
>>> - init_waitqueue_head(&fctx->event);
>>> spin_lock_init(&fctx->spinlock);
>>>
>>> return fctx;
>>> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
>>> (int32_t)(*fctx->fenceptr - fence) >= 0;
>>> }
>>>
>>> -/* legacy path for WAIT_FENCE ioctl: */
>>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
>>> - ktime_t *timeout, bool interruptible)
>>> -{
>>> - int ret;
>>> -
>>> - if (fence > fctx->last_fence) {
>>> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
>>> - fctx->name, fence, fctx->last_fence);
>>> - return -EINVAL;
>>
>> Rob, we changed this pre-existing behaviour in this patch. Now, when
>> userspace tries to wait on a future fence, we don't return an error.
>>
>> I just want to check if this was accidental or not?
>
> Hmm, perhaps we should do this to restore the previous behavior:
>
> -------------
> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> index 73e827641024..3dd6da56eae6 100644
> --- a/drivers/gpu/drm/msm/msm_drv.c
> +++ b/drivers/gpu/drm/msm/msm_drv.c
> @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct
> drm_device *dev, void *data,
> fence = dma_fence_get_rcu(fence);
> mutex_unlock(&queue->lock);
>
> - if (!fence)
> - return 0;
> + if (!fence) {
> + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx;
> + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence:
> %u (of %u)\n",
> + fctx->name, fence, fctx->last_fence);
> + return -EINVAL;
> + }
With this, when userspace tries to wait on a fence which is already
retired, it gets -EINVAL instead of success. Will this break userspace?
-Akhil.
>
> ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> if (ret == 0) {
> -------------
>
> BR,
> -R
>
>> -Akhil.
>>
>>> - }
>>> -
>>> - if (!timeout) {
>>> - /* no-wait: */
>>> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
>>> - } else {
>>> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
>>> -
>>> - if (interruptible)
>>> - ret = wait_event_interruptible_timeout(fctx->event,
>>> - fence_completed(fctx, fence),
>>> - remaining_jiffies);
>>> - else
>>> - ret = wait_event_timeout(fctx->event,
>>> - fence_completed(fctx, fence),
>>> - remaining_jiffies);
>>> -
>>> - if (ret == 0) {
>>> - DBG("timeout waiting for fence: %u (completed: %u)",
>>> - fence, fctx->completed_fence);
>>> - ret = -ETIMEDOUT;
>>> - } else if (ret != -ERESTARTSYS) {
>>> - ret = 0;
>>> - }
>>> - }
>>> -
>>> - return ret;
>>> -}
>>> -
>>> /* called from workqueue */
>>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
>>> {
>>> spin_lock(&fctx->spinlock);
>>> fctx->completed_fence = max(fence, fctx->completed_fence);
>>> spin_unlock(&fctx->spinlock);
>>> -
>>> - wake_up_all(&fctx->event);
>>> }
>>>
>>> struct msm_fence {
>>> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
>>> index 6ab97062ff1a..4783db528bcc 100644
>>> --- a/drivers/gpu/drm/msm/msm_fence.h
>>> +++ b/drivers/gpu/drm/msm/msm_fence.h
>>> @@ -49,7 +49,6 @@ struct msm_fence_context {
>>> */
>>> volatile uint32_t *fenceptr;
>>>
>>> - wait_queue_head_t event;
>>> spinlock_t spinlock;
>>> };
>>>
>>> @@ -57,8 +56,6 @@ 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);
>>>
>>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
>>> - ktime_t *timeout, bool interruptible);
>>> 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.h b/drivers/gpu/drm/msm/msm_gem.h
>>> index da3af702a6c8..e0579abda5b9 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem.h
>>> +++ b/drivers/gpu/drm/msm/msm_gem.h
>>> @@ -320,6 +320,7 @@ struct msm_gem_submit {
>>> struct ww_acquire_ctx ticket;
>>> uint32_t seqno; /* Sequence number of the submit on the ring */
>>> struct dma_fence *fence;
>>> + int fence_id; /* key into queue->fence_idr */
>>> struct msm_gpu_submitqueue *queue;
>>> struct pid *pid; /* submitting process */
>>> bool fault_dumped; /* Limit devcoredump dumping to one per submit */
>>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> index 4f02fa3c78f9..f6f595aae2c5 100644
>>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
>>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
>>> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
>>> container_of(kref, struct msm_gem_submit, ref);
>>> unsigned i;
>>>
>>> + if (submit->fence_id) {
>>> + mutex_lock(&submit->queue->lock);
>>> + idr_remove(&submit->queue->fence_idr, submit->fence_id);
>>> + mutex_unlock(&submit->queue->lock);
>>> + }
>>> +
>>> dma_fence_put(submit->fence);
>>> +
>>> put_pid(submit->pid);
>>> msm_submitqueue_put(submit->queue);
>>>
>>> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>>> goto out;
>>> }
>>>
>>> + /*
>>> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back
>>> + * to the underlying fence.
>>> + */
>>> + mutex_lock(&queue->lock);
>>> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
>>> + submit->fence, 0, INT_MAX, GFP_KERNEL);
>>> + mutex_unlock(&queue->lock);
>>> + if (submit->fence_id < 0) {
>>> + ret = submit->fence_id = 0;
>>> + submit->fence_id = 0;
>>> + goto out;
>>> + }
>>> +
>>> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
>>> struct sync_file *sync_file = sync_file_create(submit->fence);
>>> if (!sync_file) {
>>> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
>>>
>>> msm_gpu_submit(gpu, submit);
>>>
>>> - args->fence = submit->fence->seqno;
>>> + args->fence = submit->fence_id;
>>>
>>> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
>>> msm_process_post_deps(post_deps, args->nr_out_syncobjs,
>>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
>>> index 96efcb31e502..579627252540 100644
>>> --- a/drivers/gpu/drm/msm/msm_gpu.h
>>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
>>> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
>>> * which set of pgtables do submits jobs associated with the
>>> * submitqueue use)
>>> * @node: node in the context's list of submitqueues
>>> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence
>>> + * seqno, protected by submitqueue lock
>>> + * @lock: submitqueue lock
>>> * @ref: reference count
>>> */
>>> struct msm_gpu_submitqueue {
>>> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
>>> int faults;
>>> struct msm_file_private *ctx;
>>> struct list_head node;
>>> + struct idr fence_idr;
>>> + struct mutex lock;
>>> struct kref ref;
>>> };
>>>
>>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
>>> index 9e9fec61d629..66f8d0fb38b0 100644
>>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
>>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
>>> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
>>> struct msm_gpu_submitqueue *queue = container_of(kref,
>>> struct msm_gpu_submitqueue, ref);
>>>
>>> + idr_destroy(&queue->fence_idr);
>>> +
>>> msm_file_private_put(queue->ctx);
>>>
>>> kfree(queue);
>>> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
>>> if (id)
>>> *id = queue->id;
>>>
>>> + idr_init(&queue->fence_idr);
>>> + mutex_init(&queue->lock);
>>> +
>>> list_add_tail(&queue->node, &ctx->submitqueues);
>>>
>>> write_unlock(&ctx->queuelock);
>>>
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Freedreno] [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr
2021-11-11 15:53 ` Akhil P Oommen
@ 2021-11-11 17:30 ` Rob Clark
0 siblings, 0 replies; 7+ messages in thread
From: Rob Clark @ 2021-11-11 17:30 UTC (permalink / raw)
To: Akhil P Oommen
Cc: Rob Clark, David Airlie, open list:DRM DRIVER FOR MSM ADRENO GPU,
open list, dri-devel, moderated list:DMA BUFFER SHARING FRAMEWORK,
Sean Paul, freedreno, Christian König,
open list:DMA BUFFER SHARING FRAMEWORK
On Thu, Nov 11, 2021 at 7:54 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
>
> On 11/10/2021 10:25 PM, Rob Clark wrote:
> > On Wed, Nov 10, 2021 at 7:28 AM Akhil P Oommen <akhilpo@codeaurora.org> wrote:
> >>
> >> On 7/28/2021 6:36 AM, Rob Clark wrote:
> >>> From: Rob Clark <robdclark@chromium.org>
> >>>
> >>> Previously the (non-fd) fence returned from submit ioctl was a raw
> >>> seqno, which is scoped to the ring. But from UABI standpoint, the
> >>> ioctls related to seqno fences all specify a submitqueue. We can
> >>> take advantage of that to replace the seqno fences with a cyclic idr
> >>> handle.
> >>>
> >>> This is in preperation for moving to drm scheduler, at which point
> >>> the submit ioctl will return after queuing the submit job to the
> >>> scheduler, but before the submit is written into the ring (and
> >>> therefore before a ring seqno has been assigned). Which means we
> >>> need to replace the dma_fence that userspace may need to wait on
> >>> with a scheduler fence.
> >>>
> >>> Signed-off-by: Rob Clark <robdclark@chromium.org>
> >>> Acked-by: Christian König <christian.koenig@amd.com>
> >>> ---
> >>> drivers/gpu/drm/msm/msm_drv.c | 30 +++++++++++++++++--
> >>> drivers/gpu/drm/msm/msm_fence.c | 42 ---------------------------
> >>> drivers/gpu/drm/msm/msm_fence.h | 3 --
> >>> drivers/gpu/drm/msm/msm_gem.h | 1 +
> >>> drivers/gpu/drm/msm/msm_gem_submit.c | 23 ++++++++++++++-
> >>> drivers/gpu/drm/msm/msm_gpu.h | 5 ++++
> >>> drivers/gpu/drm/msm/msm_submitqueue.c | 5 ++++
> >>> 7 files changed, 61 insertions(+), 48 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> >>> index 9b8fa2ad0d84..1594ae39d54f 100644
> >>> --- a/drivers/gpu/drm/msm/msm_drv.c
> >>> +++ b/drivers/gpu/drm/msm/msm_drv.c
> >>> @@ -911,6 +911,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> >>> ktime_t timeout = to_ktime(args->timeout);
> >>> struct msm_gpu_submitqueue *queue;
> >>> struct msm_gpu *gpu = priv->gpu;
> >>> + struct dma_fence *fence;
> >>> int ret;
> >>>
> >>> if (args->pad) {
> >>> @@ -925,10 +926,35 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data,
> >>> if (!queue)
> >>> return -ENOENT;
> >>>
> >>> - ret = msm_wait_fence(gpu->rb[queue->prio]->fctx, args->fence, &timeout,
> >>> - true);
> >>> + /*
> >>> + * Map submitqueue scoped "seqno" (which is actually an idr key)
> >>> + * back to underlying dma-fence
> >>> + *
> >>> + * The fence is removed from the fence_idr when the submit is
> >>> + * retired, so if the fence is not found it means there is nothing
> >>> + * to wait for
> >>> + */
> >>> + ret = mutex_lock_interruptible(&queue->lock);
> >>> + if (ret)
> >>> + return ret;
> >>> + fence = idr_find(&queue->fence_idr, args->fence);
> >>> + if (fence)
> >>> + fence = dma_fence_get_rcu(fence);
> >>> + mutex_unlock(&queue->lock);
> >>> +
> >>> + if (!fence)
> >>> + return 0;
> >>>
> >>> + ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> >>> + if (ret == 0) {
> >>> + ret = -ETIMEDOUT;
> >>> + } else if (ret != -ERESTARTSYS) {
> >>> + ret = 0;
> >>> + }
> >>> +
> >>> + dma_fence_put(fence);
> >>> msm_submitqueue_put(queue);
> >>> +
> >>> return ret;
> >>> }
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> >>> index b92a9091a1e2..f2cece542c3f 100644
> >>> --- a/drivers/gpu/drm/msm/msm_fence.c
> >>> +++ b/drivers/gpu/drm/msm/msm_fence.c
> >>> @@ -24,7 +24,6 @@ msm_fence_context_alloc(struct drm_device *dev, volatile uint32_t *fenceptr,
> >>> strncpy(fctx->name, name, sizeof(fctx->name));
> >>> fctx->context = dma_fence_context_alloc(1);
> >>> fctx->fenceptr = fenceptr;
> >>> - init_waitqueue_head(&fctx->event);
> >>> spin_lock_init(&fctx->spinlock);
> >>>
> >>> return fctx;
> >>> @@ -45,53 +44,12 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc
> >>> (int32_t)(*fctx->fenceptr - fence) >= 0;
> >>> }
> >>>
> >>> -/* legacy path for WAIT_FENCE ioctl: */
> >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> >>> - ktime_t *timeout, bool interruptible)
> >>> -{
> >>> - int ret;
> >>> -
> >>> - if (fence > fctx->last_fence) {
> >>> - DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n",
> >>> - fctx->name, fence, fctx->last_fence);
> >>> - return -EINVAL;
> >>
> >> Rob, we changed this pre-existing behaviour in this patch. Now, when
> >> userspace tries to wait on a future fence, we don't return an error.
> >>
> >> I just want to check if this was accidental or not?
> >
> > Hmm, perhaps we should do this to restore the previous behavior:
> >
> > -------------
> > diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
> > index 73e827641024..3dd6da56eae6 100644
> > --- a/drivers/gpu/drm/msm/msm_drv.c
> > +++ b/drivers/gpu/drm/msm/msm_drv.c
> > @@ -1000,8 +1000,12 @@ static int msm_ioctl_wait_fence(struct
> > drm_device *dev, void *data,
> > fence = dma_fence_get_rcu(fence);
> > mutex_unlock(&queue->lock);
> >
> > - if (!fence)
> > - return 0;
> > + if (!fence) {
> > + struct msm_fence_context *fctx = gpu->rb[queue->ring_nr]->fctx;
> > + DRM_ERROR_RATELIMITED("%s: waiting on invalid fence:
> > %u (of %u)\n",
> > + fctx->name, fence, fctx->last_fence);
> > + return -EINVAL;
> > + }
>
> With this, when userspace tries to wait on a fence which is already
> retired, it gets -EINVAL instead of success. Will this break userspace?
Oh, right, we definitely don't want that.. I guess that was the reason
for the original logic.
I have a different idea.. will send a patch in a bit.
BR,
-R
> -Akhil.
>
> >
> > ret = dma_fence_wait_timeout(fence, true, timeout_to_jiffies(&timeout));
> > if (ret == 0) {
> > -------------
> >
> > BR,
> > -R
> >
> >> -Akhil.
> >>
> >>> - }
> >>> -
> >>> - if (!timeout) {
> >>> - /* no-wait: */
> >>> - ret = fence_completed(fctx, fence) ? 0 : -EBUSY;
> >>> - } else {
> >>> - unsigned long remaining_jiffies = timeout_to_jiffies(timeout);
> >>> -
> >>> - if (interruptible)
> >>> - ret = wait_event_interruptible_timeout(fctx->event,
> >>> - fence_completed(fctx, fence),
> >>> - remaining_jiffies);
> >>> - else
> >>> - ret = wait_event_timeout(fctx->event,
> >>> - fence_completed(fctx, fence),
> >>> - remaining_jiffies);
> >>> -
> >>> - if (ret == 0) {
> >>> - DBG("timeout waiting for fence: %u (completed: %u)",
> >>> - fence, fctx->completed_fence);
> >>> - ret = -ETIMEDOUT;
> >>> - } else if (ret != -ERESTARTSYS) {
> >>> - ret = 0;
> >>> - }
> >>> - }
> >>> -
> >>> - return ret;
> >>> -}
> >>> -
> >>> /* called from workqueue */
> >>> void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence)
> >>> {
> >>> spin_lock(&fctx->spinlock);
> >>> fctx->completed_fence = max(fence, fctx->completed_fence);
> >>> spin_unlock(&fctx->spinlock);
> >>> -
> >>> - wake_up_all(&fctx->event);
> >>> }
> >>>
> >>> struct msm_fence {
> >>> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> >>> index 6ab97062ff1a..4783db528bcc 100644
> >>> --- a/drivers/gpu/drm/msm/msm_fence.h
> >>> +++ b/drivers/gpu/drm/msm/msm_fence.h
> >>> @@ -49,7 +49,6 @@ struct msm_fence_context {
> >>> */
> >>> volatile uint32_t *fenceptr;
> >>>
> >>> - wait_queue_head_t event;
> >>> spinlock_t spinlock;
> >>> };
> >>>
> >>> @@ -57,8 +56,6 @@ 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);
> >>>
> >>> -int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence,
> >>> - ktime_t *timeout, bool interruptible);
> >>> 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.h b/drivers/gpu/drm/msm/msm_gem.h
> >>> index da3af702a6c8..e0579abda5b9 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gem.h
> >>> +++ b/drivers/gpu/drm/msm/msm_gem.h
> >>> @@ -320,6 +320,7 @@ struct msm_gem_submit {
> >>> struct ww_acquire_ctx ticket;
> >>> uint32_t seqno; /* Sequence number of the submit on the ring */
> >>> struct dma_fence *fence;
> >>> + int fence_id; /* key into queue->fence_idr */
> >>> struct msm_gpu_submitqueue *queue;
> >>> struct pid *pid; /* submitting process */
> >>> bool fault_dumped; /* Limit devcoredump dumping to one per submit */
> >>> diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> index 4f02fa3c78f9..f6f595aae2c5 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> +++ b/drivers/gpu/drm/msm/msm_gem_submit.c
> >>> @@ -68,7 +68,14 @@ void __msm_gem_submit_destroy(struct kref *kref)
> >>> container_of(kref, struct msm_gem_submit, ref);
> >>> unsigned i;
> >>>
> >>> + if (submit->fence_id) {
> >>> + mutex_lock(&submit->queue->lock);
> >>> + idr_remove(&submit->queue->fence_idr, submit->fence_id);
> >>> + mutex_unlock(&submit->queue->lock);
> >>> + }
> >>> +
> >>> dma_fence_put(submit->fence);
> >>> +
> >>> put_pid(submit->pid);
> >>> msm_submitqueue_put(submit->queue);
> >>>
> >>> @@ -872,6 +879,20 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >>> goto out;
> >>> }
> >>>
> >>> + /*
> >>> + * Allocate an id which can be used by WAIT_FENCE ioctl to map back
> >>> + * to the underlying fence.
> >>> + */
> >>> + mutex_lock(&queue->lock);
> >>> + submit->fence_id = idr_alloc_cyclic(&queue->fence_idr,
> >>> + submit->fence, 0, INT_MAX, GFP_KERNEL);
> >>> + mutex_unlock(&queue->lock);
> >>> + if (submit->fence_id < 0) {
> >>> + ret = submit->fence_id = 0;
> >>> + submit->fence_id = 0;
> >>> + goto out;
> >>> + }
> >>> +
> >>> if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
> >>> struct sync_file *sync_file = sync_file_create(submit->fence);
> >>> if (!sync_file) {
> >>> @@ -886,7 +907,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,
> >>>
> >>> msm_gpu_submit(gpu, submit);
> >>>
> >>> - args->fence = submit->fence->seqno;
> >>> + args->fence = submit->fence_id;
> >>>
> >>> msm_reset_syncobjs(syncobjs_to_reset, args->nr_in_syncobjs);
> >>> msm_process_post_deps(post_deps, args->nr_out_syncobjs,
> >>> diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
> >>> index 96efcb31e502..579627252540 100644
> >>> --- a/drivers/gpu/drm/msm/msm_gpu.h
> >>> +++ b/drivers/gpu/drm/msm/msm_gpu.h
> >>> @@ -263,6 +263,9 @@ struct msm_gpu_perfcntr {
> >>> * which set of pgtables do submits jobs associated with the
> >>> * submitqueue use)
> >>> * @node: node in the context's list of submitqueues
> >>> + * @fence_idr: maps fence-id to dma_fence for userspace visible fence
> >>> + * seqno, protected by submitqueue lock
> >>> + * @lock: submitqueue lock
> >>> * @ref: reference count
> >>> */
> >>> struct msm_gpu_submitqueue {
> >>> @@ -272,6 +275,8 @@ struct msm_gpu_submitqueue {
> >>> int faults;
> >>> struct msm_file_private *ctx;
> >>> struct list_head node;
> >>> + struct idr fence_idr;
> >>> + struct mutex lock;
> >>> struct kref ref;
> >>> };
> >>>
> >>> diff --git a/drivers/gpu/drm/msm/msm_submitqueue.c b/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> index 9e9fec61d629..66f8d0fb38b0 100644
> >>> --- a/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> +++ b/drivers/gpu/drm/msm/msm_submitqueue.c
> >>> @@ -12,6 +12,8 @@ void msm_submitqueue_destroy(struct kref *kref)
> >>> struct msm_gpu_submitqueue *queue = container_of(kref,
> >>> struct msm_gpu_submitqueue, ref);
> >>>
> >>> + idr_destroy(&queue->fence_idr);
> >>> +
> >>> msm_file_private_put(queue->ctx);
> >>>
> >>> kfree(queue);
> >>> @@ -89,6 +91,9 @@ int msm_submitqueue_create(struct drm_device *drm, struct msm_file_private *ctx,
> >>> if (id)
> >>> *id = queue->id;
> >>>
> >>> + idr_init(&queue->fence_idr);
> >>> + mutex_init(&queue->lock);
> >>> +
> >>> list_add_tail(&queue->node, &ctx->submitqueues);
> >>>
> >>> write_unlock(&ctx->queuelock);
> >>>
> >>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-11-11 17:25 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-07-28 1:06 [PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups Rob Clark
2021-07-28 1:06 ` [PATCH v4 07/13] drm/msm: Track "seqno" fences by idr Rob Clark
2021-11-10 15:28 ` Akhil P Oommen
2021-11-10 16:55 ` Rob Clark
2021-11-11 15:53 ` Akhil P Oommen
2021-11-11 17:30 ` [Freedreno] " Rob Clark
2021-07-28 1:06 ` [PATCH v4 09/13] drm/msm: Conversion to drm scheduler 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 NNTP newsgroup(s).