drm/msm/sde: avoid spinlock recursion in sde fence
The sde fence driver signals the fence with spinlock
acquired because it wants to protect the linked list
containing all fence objects. The dma fence_array
driver always calls the fence_put once fence is signaled
in the caller context itself. This fence_put call may
be translated into fence_release if no other client
has registered for the current fence object. The release
call will again trigger the sde_fence_release and tries
to acquired the same spinlock to protect the list. This
change separates the "timeline + fence" lock and list lock
to avoid holding it during fence_signal call. Just refer
below callstack for recursion case.
spin_bug+0x90/0xb4
do_raw_spin_lock+0xcc/0x1bc
_raw_spin_lock_irqsave+0x34/0x44
sde_fence_release+0x30/0x104
fence_release+0x54/0xf8
fence_array_release+0x80/0xb4
fence_release+0x54/0xf8
fence_array_cb_func+0x68/0x80
fence_signal_locked+0x80/0x170
sde_fence_signal+0x1ac/0x2a8
sde_crtc_complete_commit+0x64/0xb8
sde_kms_complete_commit+0x74/0xd0
complete_commit+0x324/0x404
_msm_drm_commit_work_cb+0x24/0x5c
kthread_worker_fn+0xc4/0x1e8
kthread+0xfc/0x110
ret_from_fork+0x10/0x40
Change-Id: I87ab33214e5828dda0b89e8d5c339f54917cfc0f
Signed-off-by: Dhaval Patel <pdhaval@codeaurora.org>
diff --git a/drivers/gpu/drm/msm/sde/sde_fence.c b/drivers/gpu/drm/msm/sde/sde_fence.c
index 5f257bb..826fe14 100644
--- a/drivers/gpu/drm/msm/sde/sde_fence.c
+++ b/drivers/gpu/drm/msm/sde/sde_fence.c
@@ -139,10 +139,9 @@
struct sde_fence *f = to_sde_fence(fence);
struct sde_fence *fc, *next;
struct sde_fence_context *ctx = f->ctx;
- unsigned long flags;
bool release_kref = false;
- spin_lock_irqsave(&ctx->lock, flags);
+ spin_lock(&ctx->list_lock);
list_for_each_entry_safe(fc, next, &ctx->fence_list_head,
fence_list) {
/* fence release called before signal */
@@ -152,7 +151,7 @@
break;
}
}
- spin_unlock_irqrestore(&ctx->lock, flags);
+ spin_unlock(&ctx->list_lock);
/* keep kput outside spin_lock because it may release ctx */
if (release_kref)
@@ -198,7 +197,6 @@
struct sync_file *sync_file;
signed int fd = -EINVAL;
struct sde_fence_context *ctx = fence_ctx;
- unsigned long flags;
if (!ctx) {
SDE_ERROR("invalid context\n");
@@ -234,12 +232,12 @@
fd_install(fd, sync_file->file);
- spin_lock_irqsave(&ctx->lock, flags);
+ spin_lock(&ctx->list_lock);
sde_fence->ctx = fence_ctx;
sde_fence->fd = fd;
list_add_tail(&sde_fence->fence_list, &ctx->fence_list_head);
kref_get(&ctx->kref);
- spin_unlock_irqrestore(&ctx->lock, flags);
+ spin_unlock(&ctx->list_lock);
exit:
return fd;
}
@@ -260,6 +258,7 @@
ctx->context = fence_context_alloc(1);
spin_lock_init(&ctx->lock);
+ spin_lock_init(&ctx->list_lock);
INIT_LIST_HEAD(&ctx->fence_list_head);
return 0;
@@ -333,7 +332,8 @@
{
unsigned long flags;
struct sde_fence *fc, *next;
- uint32_t count = 0;
+ bool is_signaled = false;
+ struct list_head local_list_head;
if (!ctx) {
SDE_ERROR("invalid ctx, %pK\n", ctx);
@@ -342,37 +342,45 @@
return;
}
+ INIT_LIST_HEAD(&local_list_head);
+
spin_lock_irqsave(&ctx->lock, flags);
if ((int)(ctx->done_count - ctx->commit_count) < 0) {
++ctx->done_count;
+ SDE_DEBUG("fence_signal:done count:%d commit count:%d\n",
+ ctx->commit_count, ctx->done_count);
} else {
SDE_ERROR("extra signal attempt! done count:%d commit:%d\n",
ctx->done_count, ctx->commit_count);
- goto end;
+ spin_unlock_irqrestore(&ctx->lock, flags);
+ return;
}
+ spin_unlock_irqrestore(&ctx->lock, flags);
+ spin_lock(&ctx->list_lock);
if (list_empty(&ctx->fence_list_head)) {
SDE_DEBUG("nothing to trigger!-no get_prop call\n");
- goto end;
+ spin_unlock(&ctx->list_lock);
+ return;
}
- SDE_DEBUG("fence_signal:done count:%d commit count:%d\n",
- ctx->commit_count, ctx->done_count);
+ list_for_each_entry_safe(fc, next, &ctx->fence_list_head, fence_list)
+ list_move(&fc->fence_list, &local_list_head);
+ spin_unlock(&ctx->list_lock);
- list_for_each_entry_safe(fc, next, &ctx->fence_list_head,
- fence_list) {
- if (fence_is_signaled_locked(&fc->base)) {
- list_del_init(&fc->fence_list);
- count++;
+ list_for_each_entry_safe(fc, next, &local_list_head, fence_list) {
+ spin_lock_irqsave(&ctx->lock, flags);
+ is_signaled = fence_signal_locked(&fc->base);
+ spin_unlock_irqrestore(&ctx->lock, flags);
+
+ if (is_signaled) {
+ kref_put(&ctx->kref, sde_fence_destroy);
+ } else {
+ spin_lock(&ctx->list_lock);
+ list_move(&fc->fence_list, &ctx->fence_list_head);
+ spin_unlock(&ctx->list_lock);
}
}
SDE_EVT32(ctx->drm_id, ctx->done_count);
-
-end:
- spin_unlock_irqrestore(&ctx->lock, flags);
-
- /* keep this outside spin_lock because same ctx may be released */
- for (; count > 0; count--)
- kref_put(&ctx->kref, sde_fence_destroy);
}
diff --git a/drivers/gpu/drm/msm/sde/sde_fence.h b/drivers/gpu/drm/msm/sde/sde_fence.h
index f3f8b35..207f29c 100644
--- a/drivers/gpu/drm/msm/sde/sde_fence.h
+++ b/drivers/gpu/drm/msm/sde/sde_fence.h
@@ -29,7 +29,8 @@
* @done_count: Number of completed commits since bootup
* @drm_id: ID number of owning DRM Object
* @ref: kref counter on timeline
- * @lock: spinlock for timeline and fence counter protection
+ * @lock: spinlock for fence counter protection
+ * @list_lock: spinlock for timeline protection
* @context: fence context
* @list_head: fence list to hold all the fence created on this context
* @name: name of fence context/timeline
@@ -40,6 +41,7 @@
uint32_t drm_id;
struct kref kref;
spinlock_t lock;
+ spinlock_t list_lock;
u64 context;
struct list_head fence_list_head;
char name[SDE_FENCE_NAME_SIZE];