drm/msm: wait_fence fixes
Subsystems (5)
cover drm/msm: wait_fence fixes
From: Rob Clark <[email protected]>
A couple of wait_fence related fixes.
Rob Clark (2):
drm/msm: Fix wait_fence submitqueue leak
drm/msm: Restore error return on invalid fence
drivers/gpu/drm/msm/msm_drv.c | 49 ++++++++++++++++++---------- drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 ++ 3 files changed, 36 insertions(+), 17 deletions(-)
Signature · 1 line
2.31.1
PATCH 1/2 drm/msm: Fix wait_fence submitqueue leak
From: Rob Clark <[email protected]>
We weren't dropping the submitqueue reference in all paths. In particular, when the fence has already been signalled. Split out a helper to simplify handling this in the various different return paths.
--- drivers/gpu/drm/msm/msm_drv.c | 49 +++++++++++++++++++++-------------- 1 file changed, 29 insertions(+), 20 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +29 -20
@@ -961,29 +961,12 @@ static int msm_ioctl_gem_info(struct drm_device *dev, void *data,return ret; } -static int msm_ioctl_wait_fence (struct drm_device *dev,void *data, -struct drm_file *file) +static int wait_fence (struct msm_gpu_submitqueue *queue,uint32_t fence_id, +ktime_t timeout) { -struct msm_drm_private *priv = dev->dev_private; -struct drm_msm_wait_fence *args = 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) { -DRM_ERROR ("invalid pad: %08x\n" , args->pad); -return -EINVAL ; - } - -if (!gpu) -return 0 ; - - queue =msm_submitqueue_get (file->driver_priv, args->queueid); -if (!queue) -return -ENOENT ; -/* * Map submitqueue scoped "seqno" (which is actually an idr key) * back to underlying dma-fence
@@ -995,7 +978,7 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, ret =mutex_lock_interruptible (&queue->lock);if (ret)return ret; - fence =idr_find (&queue->fence_idr, args->fence); + fence =idr_find (&queue->fence_idr, fence_id);if (fence) fence =dma_fence_get_rcu (fence);mutex_unlock (&queue->lock);
@@ -1011,6 +994,32 @@ static int msm_ioctl_wait_fence(struct drm_device *dev, void *data, }dma_fence_put (fence); + +return ret; +} + +static int msm_ioctl_wait_fence (struct drm_device *dev,void *data, +struct drm_file *file) +{ +struct msm_drm_private *priv = dev->dev_private; +struct drm_msm_wait_fence *args = data; +struct msm_gpu_submitqueue *queue; +int ret; + +if (args->pad) { +DRM_ERROR ("invalid pad: %08x\n" , args->pad); +return -EINVAL ; + } + +if (!priv->gpu) +return 0 ; + + queue =msm_submitqueue_get (file->driver_priv, args->queueid); +if (!queue) +return -ENOENT ; + + ret =wait_fence (queue, args->fence,to_ktime (args->timeout)); +msm_submitqueue_put (queue);return ret;
Signature · 1 line
2.31.1
PATCH 2/2 drm/msm: Restore error return on invalid fence
In reply to Rob ClarkFrom: Rob Clark <[email protected]>
When converting to use an idr to map userspace fence seqno values back to a dma_fence, we lost the error return when userspace passes seqno that is larger than the last submitted fence. Restore this check.
---
Note: I will rebase "drm/msm: Handle fence rollover" on top of this, to simplify backporting this patch to stable kernels
drivers/gpu/drm/msm/msm_drv.c | 6 ++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 +++ 3 files changed, 10 insertions(+)
drivers/gpu/drm/msm/msm_drv.c +6 -0
@@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id,struct dma_fence *fence;int ret; +if (fence_id > queue->last_fence) { +DRM_ERROR_RATELIMITED ("waiting on invalid fence: %u (of %u)\n" , + fence_id, queue->last_fence); +return -EINVAL ; + } +/* * Map submitqueue scoped "seqno" (which is actually an idr key) * back to underlying dma-fence
drivers/gpu/drm/msm/msm_gem_submit.c +1 -0
@@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,drm_sched_entity_push_job (&submit->base, queue->entity); args->fence = submit->fence_id; + queue->last_fence = submit->fence_id;msm_reset_syncobjs (syncobjs_to_reset, args->nr_in_syncobjs); msm_process_post_deps(post_deps, args->nr_out_syncobjs,
drivers/gpu/drm/msm/msm_gpu.h +3 -0
@@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio,* @ring_nr: the ringbuffer used by this submitqueue, which is determined * by the submitqueue's priority * @faults: the number of GPU hangs associated with this submitqueue +* @last_fence: the sequence number of the last allocated fence (for error +* checking) * @ctx: the per-drm_file context associated with the submitqueue (ie. * which set of pgtables do submits jobs associated with the * submitqueue use)
@@ -391,6 +393,7 @@ struct msm_gpu_submitqueue {u32 flags;u32 ring_nr;int faults; +uint32_t last_fence;struct msm_file_private *ctx;struct list_head node;struct idr fence_idr;
Signature · 1 line
2.31.1
Re: [PATCH 2/2] drm/msm: Restore error return on invalid fence
Quoted 23 lines · Rob Clark
From: Rob Clark <[email protected]>
When converting to use an idr to map userspace fence seqno values back to a
dma_fence, we lost the error return when userspace passes seqno that is larger than the last submitted fence. Restore this check.Reported-by Akhil P Oommen <[email protected]>Fixes a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr")Signed-off-by Rob Clark <[email protected]>---
Note: I will rebase "drm/msm: Handle fence rollover" on top of this, to simplify backporting this patch to stable kernelsdrivers/gpu/drm/msm/msm_drv.c | 6 ++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 +++ 3 files changed, 10 insertions(+)drivers/gpu/drm/msm/msm_drv.c +0
-0
drivers/gpu/drm/msm/msm_drv.c static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id, · line 970
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0@@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id,struct dma_fence *fence;int ret; +if (fence_id > queue->last_fence) {
But fence_id can wrap around and then this check won't be valid.
-Akhil.
Quoted 42 lines
drivers/gpu/drm/msm/msm_drv.c +5
-0+DRM_ERROR_RATELIMITED ("waiting on invalid fence: %u (of %u)\n" , + fence_id, queue->last_fence); +return -EINVAL ; + } +/* * Map submitqueue scoped "seqno" (which is actually an idr key) * back to underlying dma-fence drivers/gpu/drm/msm/msm_gem_submit.c +1
-0@@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,drm_sched_entity_push_job (&submit->base, queue->entity); args->fence = submit->fence_id; + queue->last_fence = submit->fence_id;msm_reset_syncobjs (syncobjs_to_reset, args->nr_in_syncobjs); msm_process_post_deps(post_deps, args->nr_out_syncobjs,drivers/gpu/drm/msm/msm_gpu.h +3
-0@@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio,* @ring_nr: the ringbuffer used by this submitqueue, which is determined * by the submitqueue's priority * @faults: the number of GPU hangs associated with this submitqueue +* @last_fence: the sequence number of the last allocated fence (for error +* checking) * @ctx: the per-drm_file context associated with the submitqueue (ie. * which set of pgtables do submits jobs associated with the * submitqueue use) @@ -391,6 +393,7 @@ struct msm_gpu_submitqueue {u32 flags;u32 ring_nr;int faults; +uint32_t last_fence;struct msm_file_private *ctx;struct list_head node;struct idr fence_idr;
Re: [PATCH 2/2] drm/msm: Restore error return on invalid fence
Quoted 28 lines · Akhil P Oommen
Quoted 26 lines · Rob Clark
From: Rob Clark <[email protected]>
When converting to use an idr to map userspace fence seqno values back to a
dma_fence, we lost the error return when userspace passes seqno that is larger than the last submitted fence. Restore this check.Reported-by Akhil P Oommen <[email protected]>Fixes a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr")Signed-off-by Rob Clark <[email protected]>---
Note: I will rebase "drm/msm: Handle fence rollover" on top of this, to simplify backporting this patch to stable kernelsdrivers/gpu/drm/msm/msm_drv.c | 6 ++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 +++ 3 files changed, 10 insertions(+)drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id,struct dma_fence *fence;int ret;
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id, · line 970
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +1
-0+if (fence_id > queue->last_fence) {But
fence_idcan wrap around and then this check won't be valid.
that is correct, but see my note about rebasing "drm/msm: Handle fence rollover" on top of this patch, so this patch could be more easily cherry-picked to stable/lts branches
BR,
-R
Quoted 45 lines
-Akhil.
Quoted 42 lines
drivers/gpu/drm/msm/msm_drv.c +5
-0+DRM_ERROR_RATELIMITED ("waiting on invalid fence: %u (of %u)\n" , + fence_id, queue->last_fence); +return -EINVAL ; + } +/* * Map submitqueue scoped "seqno" (which is actually an idr key) * back to underlying dma-fence drivers/gpu/drm/msm/msm_gem_submit.c +1
-0@@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,drm_sched_entity_push_job (&submit->base, queue->entity); args->fence = submit->fence_id; + queue->last_fence = submit->fence_id;msm_reset_syncobjs (syncobjs_to_reset, args->nr_in_syncobjs); msm_process_post_deps(post_deps, args->nr_out_syncobjs,drivers/gpu/drm/msm/msm_gpu.h +3
-0@@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio,* @ring_nr: the ringbuffer used by this submitqueue, which is determined * by the submitqueue's priority * @faults: the number of GPU hangs associated with this submitqueue +* @last_fence: the sequence number of the last allocated fence (for error +* checking) * @ctx: the per-drm_file context associated with the submitqueue (ie. * which set of pgtables do submits jobs associated with the * submitqueue use) @@ -391,6 +393,7 @@ struct msm_gpu_submitqueue {u32 flags;u32 ring_nr;int faults; +uint32_t last_fence;struct msm_file_private *ctx;struct list_head node;struct idr fence_idr;
Akhil P Oommen
Reviewed-byRe: [PATCH 2/2] drm/msm: Restore error return on invalid fence
Quoted 33 lines · Rob Clark
Quoted 28 lines · Akhil P Oommen
Quoted 26 lines · Rob Clark
From: Rob Clark <[email protected]>
When converting to use an idr to map userspace fence seqno values back to a
dma_fence, we lost the error return when userspace passes seqno that is larger than the last submitted fence. Restore this check.Reported-by Akhil P Oommen <[email protected]>Fixes a61acbbe9cf8 ("drm/msm: Track "seqno" fences by idr")Signed-off-by Rob Clark <[email protected]>---
Note: I will rebase "drm/msm: Handle fence rollover" on top of this, to simplify backporting this patch to stable kernelsdrivers/gpu/drm/msm/msm_drv.c | 6 ++++++ drivers/gpu/drm/msm/msm_gem_submit.c | 1 + drivers/gpu/drm/msm/msm_gpu.h | 3 +++ 3 files changed, 10 insertions(+)drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -967,6 +967,12 @@ static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id,struct dma_fence *fence;int ret;Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c static int wait_fence(struct msm_gpu_submitqueue *queue, uint32_t fence_id, · line 970
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +1
-0+if (fence_id > queue->last_fence) {But
fence_idcan wrap around and then this check won't be valid.
Quoted 6 lines
that is correct, but see my note about rebasing "drm/msm: Handle fence rollover" on top of this patch, so this patch could be more easily cherry-picked to stable/lts branches
BR,
-R
Missed that. Thanks.
-Akhil.
Quoted 46 lines
Quoted 46 lines
-Akhil.
Quoted 42 lines
drivers/gpu/drm/msm/msm_drv.c +5
-0+DRM_ERROR_RATELIMITED ("waiting on invalid fence: %u (of %u)\n" , + fence_id, queue->last_fence); +return -EINVAL ; + } +/* * Map submitqueue scoped "seqno" (which is actually an idr key) * back to underlying dma-fence drivers/gpu/drm/msm/msm_gem_submit.c +1
-0@@ -911,6 +911,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,drm_sched_entity_push_job (&submit->base, queue->entity); args->fence = submit->fence_id; + queue->last_fence = submit->fence_id;msm_reset_syncobjs (syncobjs_to_reset, args->nr_in_syncobjs); msm_process_post_deps(post_deps, args->nr_out_syncobjs,drivers/gpu/drm/msm/msm_gpu.h +3
-0@@ -376,6 +376,8 @@ static inline int msm_gpu_convert_priority(struct msm_gpu *gpu, int prio,* @ring_nr: the ringbuffer used by this submitqueue, which is determined * by the submitqueue's priority * @faults: the number of GPU hangs associated with this submitqueue +* @last_fence: the sequence number of the last allocated fence (for error +* checking) * @ctx: the per-drm_file context associated with the submitqueue (ie. * which set of pgtables do submits jobs associated with the * submitqueue use) @@ -391,6 +393,7 @@ struct msm_gpu_submitqueue {u32 flags;u32 ring_nr;int faults; +uint32_t last_fence;struct msm_file_private *ctx;struct list_head node;struct idr fence_idr;