Linaro-mm-sig

linaro-mm-sig@lists.linaro.org

6 participants
3123 discussions

[PATCH v4 03/18] drm/sched: Add dependency tracking

by Daniel Vetter

Instead of just a callback we can just glue in the gem helpers that panfrost, v3d and lima currently use. There's really not that many ways to skin this cat. On the naming bikeshed: The idea for using _await_ to denote adding dependencies to a job comes from i915, where that's used quite extensively all over the place, in lots of datastructures. v2/3: Rebased. Reviewed-by: Steven Price <steven.price(a)arm.com> (v1) Signed-off-by: Daniel Vetter <daniel.vetter(a)intel.com> Cc: David Airlie <airlied(a)linux.ie> Cc: Daniel Vetter <daniel(a)ffwll.ch> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Andrey Grodzovsky <andrey.grodzovsky(a)amd.com> Cc: Lee Jones <lee.jones(a)linaro.org> Cc: Nirmoy Das <nirmoy.aiemd(a)gmail.com> Cc: Boris Brezillon <boris.brezillon(a)collabora.com> Cc: Luben Tuikov <luben.tuikov(a)amd.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Jack Zhang <Jack.Zhang1(a)amd.com> Cc: linux-media(a)vger.kernel.org Cc: linaro-mm-sig(a)lists.linaro.org --- drivers/gpu/drm/scheduler/sched_entity.c | 18 +++- drivers/gpu/drm/scheduler/sched_main.c | 103 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 31 ++++++- 3 files changed, 146 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 89e3f6eaf519..381fbf462ea7 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -211,6 +211,19 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, job->sched->ops->free_job(job); } +static struct dma_fence * +drm_sched_job_dependency(struct drm_sched_job *job, + struct drm_sched_entity *entity) +{ + if (!xa_empty(&job->dependencies)) + return xa_erase(&job->dependencies, job->last_dependency++); + + if (job->sched->ops->dependency) + return job->sched->ops->dependency(job, entity); + + return NULL; +} + /** * drm_sched_entity_kill_jobs - Make sure all remaining jobs are killed * @@ -229,7 +242,7 @@ static void drm_sched_entity_kill_jobs(struct drm_sched_entity *entity) struct drm_sched_fence *s_fence = job->s_fence; /* Wait for all dependencies to avoid data corruptions */ - while ((f = job->sched->ops->dependency(job, entity))) + while ((f = drm_sched_job_dependency(job, entity))) dma_fence_wait(f, false); drm_sched_fence_scheduled(s_fence); @@ -419,7 +432,6 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) */ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) { - struct drm_gpu_scheduler *sched = entity->rq->sched; struct drm_sched_job *sched_job; sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue)); @@ -427,7 +439,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) return NULL; while ((entity->dependency = - sched->ops->dependency(sched_job, entity))) { + drm_sched_job_dependency(sched_job, entity))) { trace_drm_sched_job_wait_dep(sched_job, entity->dependency); if (drm_sched_entity_add_dependency_cb(entity)) diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 454cb6164bdc..84c30badb78e 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -603,6 +603,8 @@ int drm_sched_job_init(struct drm_sched_job *job, INIT_LIST_HEAD(&job->list); + xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC); + return 0; } EXPORT_SYMBOL(drm_sched_job_init); @@ -637,6 +639,98 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm); +/** + * drm_sched_job_await_fence - adds the fence as a job dependency + * @job: scheduler job to add the dependencies to + * @fence: the dma_fence to add to the list of dependencies. + * + * Note that @fence is consumed in both the success and error cases. + * + * Returns: + * 0 on success, or an error on failing to expand the array. + */ +int drm_sched_job_await_fence(struct drm_sched_job *job, + struct dma_fence *fence) +{ + struct dma_fence *entry; + unsigned long index; + u32 id = 0; + int ret; + + if (!fence) + return 0; + + /* Deduplicate if we already depend on a fence from the same context. + * This lets the size of the array of deps scale with the number of + * engines involved, rather than the number of BOs. + */ + xa_for_each(&job->dependencies, index, entry) { + if (entry->context != fence->context) + continue; + + if (dma_fence_is_later(fence, entry)) { + dma_fence_put(entry); + xa_store(&job->dependencies, index, fence, GFP_KERNEL); + } else { + dma_fence_put(fence); + } + return 0; + } + + ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b, GFP_KERNEL); + if (ret != 0) + dma_fence_put(fence); + + return ret; +} +EXPORT_SYMBOL(drm_sched_job_await_fence); + +/** + * drm_sched_job_await_implicit - adds implicit dependencies as job dependencies + * @job: scheduler job to add the dependencies to + * @obj: the gem object to add new dependencies from. + * @write: whether the job might write the object (so we need to depend on + * shared fences in the reservation object). + * + * This should be called after drm_gem_lock_reservations() on your array of + * GEM objects used in the job but before updating the reservations with your + * own fences. + * + * Returns: + * 0 on success, or an error on failing to expand the array. + */ +int drm_sched_job_await_implicit(struct drm_sched_job *job, + struct drm_gem_object *obj, + bool write) +{ + int ret; + struct dma_fence **fences; + unsigned int i, fence_count; + + if (!write) { + struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv); + + return drm_sched_job_await_fence(job, fence); + } + + ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences); + if (ret || !fence_count) + return ret; + + for (i = 0; i < fence_count; i++) { + ret = drm_sched_job_await_fence(job, fences[i]); + if (ret) + break; + } + + for (; i < fence_count; i++) + dma_fence_put(fences[i]); + kfree(fences); + return ret; +} +EXPORT_SYMBOL(drm_sched_job_await_implicit); + + /** * drm_sched_job_cleanup - clean up scheduler job resources * @job: scheduler job to clean up @@ -652,6 +746,9 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) { + struct dma_fence *fence; + unsigned long index; + if (kref_read(&job->s_fence->finished.refcount)) { /* drm_sched_job_arm() has been called */ dma_fence_put(&job->s_fence->finished); @@ -661,6 +758,12 @@ void drm_sched_job_cleanup(struct drm_sched_job *job) } job->s_fence = NULL; + + xa_for_each(&job->dependencies, index, fence) { + dma_fence_put(fence); + } + xa_destroy(&job->dependencies); + } EXPORT_SYMBOL(drm_sched_job_cleanup); diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 83afc3aa8e2f..74fb321dbc44 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -27,9 +27,12 @@ #include <drm/spsc_queue.h> #include <linux/dma-fence.h> #include <linux/completion.h> +#include <linux/xarray.h> #define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000) +struct drm_gem_object; + struct drm_gpu_scheduler; struct drm_sched_rq; @@ -198,6 +201,16 @@ struct drm_sched_job { enum drm_sched_priority s_priority; struct drm_sched_entity *entity; struct dma_fence_cb cb; + /** + * @dependencies: + * + * Contains the dependencies as struct dma_fence for this job, see + * drm_sched_job_await_fence() and drm_sched_job_await_implicit(). + */ + struct xarray dependencies; + + /** @last_dependency: tracks @dependencies as they signal */ + unsigned long last_dependency; }; static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job, @@ -220,9 +233,14 @@ enum drm_gpu_sched_stat { */ struct drm_sched_backend_ops { /** - * @dependency: Called when the scheduler is considering scheduling - * this job next, to get another struct dma_fence for this job to - * block on. Once it returns NULL, run_job() may be called. + * @dependency: + * + * Called when the scheduler is considering scheduling this job next, to + * get another struct dma_fence for this job to block on. Once it + * returns NULL, run_job() may be called. + * + * If a driver exclusively uses drm_sched_job_await_fence() and + * drm_sched_job_await_implicit() this can be ommitted and left as NULL. */ struct dma_fence *(*dependency)(struct drm_sched_job *sched_job, struct drm_sched_entity *s_entity); @@ -349,6 +367,13 @@ int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); void drm_sched_job_arm(struct drm_sched_job *job); +int drm_sched_job_await_fence(struct drm_sched_job *job, + struct dma_fence *fence); +int drm_sched_job_await_implicit(struct drm_sched_job *job, + struct drm_gem_object *obj, + bool write); + + void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); -- 2.32.0

4 years, 3 months

[PATCH v4 00/13] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)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

4 years, 3 months

[PATCH v3 00/13] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)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. 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 | 39 --- drivers/gpu/drm/msm/msm_fence.h | 2 - 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(+), 387 deletions(-) -- 2.31.1

4 years, 3 months

[PATCH v2 00/12] drm/msm: drm scheduler conversion and cleanups

by Rob Clark

From: Rob Clark <robdclark(a)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}) Rob Clark (12): 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 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 | 39 --- drivers/gpu/drm/msm/msm_fence.h | 2 - drivers/gpu/drm/msm/msm_gem.c | 93 +----- drivers/gpu/drm/msm/msm_gem.h | 39 ++- drivers/gpu/drm/msm/msm_gem_submit.c | 316 ++++++++++++-------- 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, 503 insertions(+), 363 deletions(-) -- 2.31.1

4 years, 3 months

[PATCH 0/2] drm/msm: Reduce fence signal latency

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> A couple tweaks to reduce fence signal latency. Rob Clark (2): drm/msm: Let fences read directly from memptrs drm/msm: Signal fences sooner drivers/gpu/drm/msm/msm_fence.c | 11 +++++-- drivers/gpu/drm/msm/msm_fence.h | 41 +++++++++++++++++++++++--- drivers/gpu/drm/msm/msm_gpu.c | 44 ++++++++++++++++------------ drivers/gpu/drm/msm/msm_ringbuffer.c | 2 +- 4 files changed, 73 insertions(+), 25 deletions(-) -- 2.31.1

4 years, 3 months

[PATCH] dma-buf: WARN on dmabuf release with pending attachments

by Charan Teja Reddy

It is expected from the clients to follow the below steps on an imported dmabuf fd: a) dmabuf = dma_buf_get(fd) // Get the dmabuf from fd b) dma_buf_attach(dmabuf); // Clients attach to the dmabuf o Here the kernel does some slab allocations, say for dma_buf_attachment and may be some other slab allocation in the dmabuf->ops->attach(). c) Client may need to do dma_buf_map_attachment(). d) Accordingly dma_buf_unmap_attachment() should be called. e) dma_buf_detach () // Clients detach to the dmabuf. o Here the slab allocations made in b) are freed. f) dma_buf_put(dmabuf) // Can free the dmabuf if it is the last reference. Now say an erroneous client failed at step c) above thus it directly called dma_buf_put(), step f) above. Considering that it may be the last reference to the dmabuf, buffer will be freed with pending attachments left to the dmabuf which can show up as the 'memory leak'. This should at least be reported as the WARN(). Signed-off-by: Charan Teja Reddy <charante(a)codeaurora.org> --- drivers/dma-buf/dma-buf.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index 511fe0d..733c8b1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -79,6 +79,7 @@ static void dma_buf_release(struct dentry *dentry) if (dmabuf->resv == (struct dma_resv *)&dmabuf[1]) dma_resv_fini(dmabuf->resv); + WARN_ON(!list_empty(&dmabuf->attachments)); module_put(dmabuf->owner); kfree(dmabuf->name); kfree(dmabuf); -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by The Linux Foundation

4 years, 3 months

[PATCH] drm/msm: Add fence->wait() op

by Rob Clark

From: Rob Clark <robdclark(a)chromium.org> Somehow we had neither ->wait() nor dma_fence_signal() calls, and no one noticed. Oops. Note that this removes the !timeout case, which has not been used in a long time. Signed-off-by: Rob Clark <robdclark(a)chromium.org> --- drivers/gpu/drm/msm/msm_fence.c | 59 +++++++++++++++++++-------------- 1 file changed, 34 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c index cd59a5918038..8ee96b90ded6 100644 --- a/drivers/gpu/drm/msm/msm_fence.c +++ b/drivers/gpu/drm/msm/msm_fence.c @@ -38,11 +38,10 @@ static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t fenc return (int32_t)(fctx->completed_fence - 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) +static signed long wait_fence(struct msm_fence_context *fctx, uint32_t fence, + signed long remaining_jiffies, bool interruptible) { - int ret; + signed long ret; if (fence > fctx->last_fence) { DRM_ERROR_RATELIMITED("%s: waiting on invalid fence: %u (of %u)\n", @@ -50,33 +49,34 @@ int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, return -EINVAL; } - if (!timeout) { - /* no-wait: */ - ret = fence_completed(fctx, fence) ? 0 : -EBUSY; + if (interruptible) { + ret = wait_event_interruptible_timeout(fctx->event, + fence_completed(fctx, fence), + remaining_jiffies); } 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; - } + 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; } +/* legacy path for WAIT_FENCE ioctl: */ +int msm_wait_fence(struct msm_fence_context *fctx, uint32_t fence, + ktime_t *timeout, bool interruptible) +{ + return wait_fence(fctx, fence, timeout_to_jiffies(timeout), interruptible); +} + /* called from workqueue */ void msm_update_fence(struct msm_fence_context *fctx, uint32_t fence) { @@ -114,10 +114,19 @@ static bool msm_fence_signaled(struct dma_fence *fence) return fence_completed(f->fctx, f->base.seqno); } +static signed long msm_fence_wait(struct dma_fence *fence, bool intr, + signed long timeout) +{ + struct msm_fence *f = to_msm_fence(fence); + + return wait_fence(f->fctx, fence->seqno, timeout, intr); +} + static const struct dma_fence_ops msm_fence_ops = { .get_driver_name = msm_fence_get_driver_name, .get_timeline_name = msm_fence_get_timeline_name, .signaled = msm_fence_signaled, + .wait = msm_fence_wait, }; struct dma_fence * -- 2.31.1

4 years, 3 months

Re: [Linaro-mm-sig] [PATCH v5 0/2] Add p2p via dmabuf to habanalabs

by Greg KH

On Sun, Jul 11, 2021 at 05:05:59PM +0300, Oded Gabbay wrote: > Hi, > This is v5 of this patch-set following again a long email thread. > > It contains fixes to the implementation according to the review that Jason > did on v4. Jason, I appreciate your feedback. If you can take another look > to see I didn't miss anything that would be great. > > The details of the fixes are in the changelog in the commit message of > the second patch. > > There was one issue with your proposal to set the orig_nents to 0. I did > that, but I also had to restore it to nents before calling sg_free_table > because that function uses orig_nents to iterate. Reviewed-by: Greg Kroah-Hartman <gregkh(a)linuxfoundation.org>

4 years, 3 months

Re: [Linaro-mm-sig] [PATCH v8 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Wed, Jul 21, 2021 at 2:44 PM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > On 21/7/21 6:29 pm, Daniel Vetter wrote: > > On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi > > <desmondcheongzx(a)gmail.com> wrote: > >> On 21/7/21 2:24 am, Daniel Vetter wrote: > >>> On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote: > >>>> Hi, > >>>> > >>>> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this. > >>>> > >>>> Overall, this series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > >>>> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > >>>> > >>>> The series is broken up into five patches: > >>>> > >>>> 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > >>>> > >>>> 2. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info(). > >>>> > >>>> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > >>>> > >>>> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes. > >>>> > >>>> 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > >>>> > >>>> v7 -> v8: > >>>> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI. > >>>> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI. > >>>> > >>>> v6 -> v7: > >>>> - Modify code alignment as suggested by the intel-gfx CI. > >>>> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > >>>> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. > >>>> > >>>> v5 -> v6: > >>>> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > >>>> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > >>>> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > >>>> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI. > >>>> > >>>> v4 -> v5: > >>>> - Add a new patch to the series that moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > >>>> - Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > >>>> > >>>> v3 -> v4: > >>>> - Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > >>>> - Inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > >>>> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter. > >>>> - Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > >>>> > >>>> v2 -> v3: > >>>> - Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > >>>> - Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > >>>> > >>>> v1 -> v2: > >>>> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. > >>> > >>> Apologies for the delay, I missed your series. Maybe just ping next time > >>> around there's silence. > >>> > >>> Looks all great, merged to drm-misc-next. Given how complex this was I'm > >>> vary of just pushing this to -fixes without some solid testing. > >>> > >> > >> Hi Daniel, > >> > >> Thanks for merging, more testing definitely sounds good to me. > >> > >>> One thing I noticed is that drm_is_current_master could just use the > >>> spinlock, since it's only doing a read access. Care to type up that patch? > >>> > >> > >> I thought about this too, but I'm not sure if that's the best solution. > >> > >> drm_is_current_master calls drm_lease_owner which then walks up the tree > >> of master lessors. The spinlock protects the master of the current drm > >> file, but subsequent lessors aren't protected without holding the > >> device's master mutex. > > > > But this isn't a fpriv->master pointer, but a master->lessor pointer. > > Which should never ever be able to change (we'd have tons of uaf bugs > > around drm_lease_owner otherwise). So I don't think there's anything > > that dev->master_lock protects here that fpriv->master_lookup_lock > > doesn't protect already? > > > > Or am I missing something? > > > The comment in the struct drm_master says it's protected by > > mode_config.idr_mutex, but that only applies to the idrs and lists I > > think. > > > > Ah you're right, I also completely forgot that lessees hold a reference > to their lessor so nothing will be freed as long as the spinlock is > held. I'll prepare that patch then, thanks for pointing it out. btw since we now looked at all this in detail, can you perhaps do a patch to update the kerneldoc for all the lease fields in struct drm_master? I think moving them to the inline style and then adding comments for each field how locking/lifetime rules work would be really good. Since right now it's all fresh from for us. -Daniel > >>> Also, do you plan to look into that idea we've discussed to flush pending > >>> access when we revoke a master or a lease? I think that would be really > >>> nice improvement here. > >>> -Daniel > >>> > >> > >> Yup, now that the potential UAFs are addressed (hopefully), I'll take a > >> closer look and propose a patch for this. > > > > Thanks a lot. > > -Daniel > > > >> > >> Best wishes, > >> Desmond > >> > >>>> > >>>> Desmond Cheong Zhi Xi (5): > >>>> drm: avoid circular locks in drm_mode_getconnector > >>>> drm: avoid blocking in drm_clients_info's rcu section > >>>> drm: add a locked version of drm_is_current_master > >>>> drm: serialize drm_file.master with a new spinlock > >>>> drm: protect drm_master pointers in drm_lease.c > >>>> > >>>> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++--------- > >>>> drivers/gpu/drm/drm_connector.c | 5 +- > >>>> drivers/gpu/drm/drm_debugfs.c | 3 +- > >>>> drivers/gpu/drm/drm_file.c | 1 + > >>>> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++------- > >>>> include/drm/drm_auth.h | 1 + > >>>> include/drm/drm_file.h | 18 +++++-- > >>>> 7 files changed, 152 insertions(+), 50 deletions(-) > >>>> > >>>> -- > >>>> 2.25.1 > >>>> > >>> > >> > > > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 3 months

Re: [Linaro-mm-sig] [PATCH v8 0/5] drm: address potential UAF bugs with drm_master ptrs

by Daniel Vetter

On Wed, Jul 21, 2021 at 6:12 AM Desmond Cheong Zhi Xi <desmondcheongzx(a)gmail.com> wrote: > On 21/7/21 2:24 am, Daniel Vetter wrote: > > On Mon, Jul 12, 2021 at 12:35:03PM +0800, Desmond Cheong Zhi Xi wrote: > >> Hi, > >> > >> In the previous thread on this series we decided to remove a patch that was violating a lockdep requirement in drm_lease. In addition to this change, I took a closer look at the CI logs for the Basic Acceptance Tests and noticed that another regression was introduced. The new patch 2 is a response to this. > >> > >> Overall, this series addresses potential use-after-free errors when dereferencing pointers to struct drm_master. These were identified after one such bug was caught by Syzbot in drm_getunique(): > >> https://syzkaller.appspot.com/bug?id=148d2f1dfac64af52ffd27b661981a540724f8… > >> > >> The series is broken up into five patches: > >> > >> 1. Move a call to drm_is_current_master() out from a section locked by &dev->mode_config.mutex in drm_mode_getconnector(). This patch does not apply to stable. > >> > >> 2. Move a call to drm_is_current_master() out from the RCU read-side critical section in drm_clients_info(). > >> > >> 3. Implement a locked version of drm_is_current_master() function that's used within drm_auth.c. > >> > >> 4. Serialize drm_file.master by introducing a new spinlock that's held whenever the value of drm_file.master changes. > >> > >> 5. Identify areas in drm_lease.c where pointers to struct drm_master are dereferenced, and ensure that the master pointers are not freed during use. > >> > >> v7 -> v8: > >> - Remove the patch that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. This patch violated an existing lockdep requirement as reported by the intel-gfx CI. > >> - Added a new patch that moves a call to drm_is_current_master out from the RCU critical section in drm_clients_info. This was reported by the intel-gfx CI. > >> > >> v6 -> v7: > >> - Modify code alignment as suggested by the intel-gfx CI. > >> - Add a new patch to the series that adds a new lock to serialize drm_file.master, in response to the lockdep splat by the intel-gfx CI. > >> - Update drm_file_get_master to use the new drm_file.master_lock instead of drm_device.master_mutex, in response to the lockdep splat by the intel-gfx CI. > >> > >> v5 -> v6: > >> - Add a new patch to the series that moves the call to _drm_lease_held out from the section locked by &dev->mode_config.idr_mutex in __drm_mode_object_find. > >> - Clarify the kerneldoc for dereferencing drm_file.master, as suggested by Daniel Vetter. > >> - Refactor error paths with goto labels so that each function only has a single drm_master_put(), as suggested by Emil Velikov. > >> - Modify comparisons to NULL into "!master", as suggested by the intel-gfx CI. > >> > >> v4 -> v5: > >> - Add a new patch to the series that moves the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. > >> - Additionally, added a missing semicolon to the patch, caught by the intel-gfx CI. > >> > >> v3 -> v4: > >> - Move the call to drm_is_current_master in drm_mode_getconnector out from the section locked by &dev->mode_config.mutex. As suggested by Daniel Vetter. This avoids a circular lock lock dependency as reported here https://patchwork.freedesktop.org/patch/440406/ > >> - Inside drm_is_current_master, instead of grabbing &fpriv->master->dev->master_mutex, we grab &fpriv->minor->dev->master_mutex to avoid dereferencing a null ptr if fpriv->master is not set. > >> - Modify kerneldoc formatting for drm_file.master, as suggested by Daniel Vetter. > >> - Additionally, add a file_priv->master NULL check inside drm_file_get_master, and handle the NULL result accordingly in drm_lease.c. As suggested by Daniel Vetter. > >> > >> v2 -> v3: > >> - Move the definition of drm_is_current_master and the _locked version higher up in drm_auth.c to avoid needing a forward declaration of drm_is_current_master_locked. As suggested by Daniel Vetter. > >> - Instead of leaking drm_device.master_mutex into drm_lease.c to protect drm_master pointers, add a new drm_file_get_master() function that returns drm_file->master while increasing its reference count, to prevent drm_file->master from being freed. As suggested by Daniel Vetter. > >> > >> v1 -> v2: > >> - Move the lock and assignment before the DRM_DEBUG_LEASE in drm_mode_get_lease_ioctl, as suggested by Emil Velikov. > > > > Apologies for the delay, I missed your series. Maybe just ping next time > > around there's silence. > > > > Looks all great, merged to drm-misc-next. Given how complex this was I'm > > vary of just pushing this to -fixes without some solid testing. > > > > Hi Daniel, > > Thanks for merging, more testing definitely sounds good to me. > > > One thing I noticed is that drm_is_current_master could just use the > > spinlock, since it's only doing a read access. Care to type up that patch? > > > > I thought about this too, but I'm not sure if that's the best solution. > > drm_is_current_master calls drm_lease_owner which then walks up the tree > of master lessors. The spinlock protects the master of the current drm > file, but subsequent lessors aren't protected without holding the > device's master mutex. But this isn't a fpriv->master pointer, but a master->lessor pointer. Which should never ever be able to change (we'd have tons of uaf bugs around drm_lease_owner otherwise). So I don't think there's anything that dev->master_lock protects here that fpriv->master_lookup_lock doesn't protect already? Or am I missing something? The comment in the struct drm_master says it's protected by mode_config.idr_mutex, but that only applies to the idrs and lists I think. > > Also, do you plan to look into that idea we've discussed to flush pending > > access when we revoke a master or a lease? I think that would be really > > nice improvement here. > > -Daniel > > > > Yup, now that the potential UAFs are addressed (hopefully), I'll take a > closer look and propose a patch for this. Thanks a lot. -Daniel > > Best wishes, > Desmond > > >> > >> Desmond Cheong Zhi Xi (5): > >> drm: avoid circular locks in drm_mode_getconnector > >> drm: avoid blocking in drm_clients_info's rcu section > >> drm: add a locked version of drm_is_current_master > >> drm: serialize drm_file.master with a new spinlock > >> drm: protect drm_master pointers in drm_lease.c > >> > >> drivers/gpu/drm/drm_auth.c | 93 ++++++++++++++++++++++++--------- > >> drivers/gpu/drm/drm_connector.c | 5 +- > >> drivers/gpu/drm/drm_debugfs.c | 3 +- > >> drivers/gpu/drm/drm_file.c | 1 + > >> drivers/gpu/drm/drm_lease.c | 81 +++++++++++++++++++++------- > >> include/drm/drm_auth.h | 1 + > >> include/drm/drm_file.h | 18 +++++-- > >> 7 files changed, 152 insertions(+), 50 deletions(-) > >> > >> -- > >> 2.25.1 > >> > > > -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

4 years, 3 months

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

Linaro-mm-sig