| Message ID | 20181021111424.5271-1-david1.zhou@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: fix deadlock of syncobj v2 | expand |
Ping... Btw: The patch is tested by syncobj_basic and syncobj_wait of IGT. > -----Original Message----- > From: Chunming Zhou <david1.zhou@amd.com> > Sent: Sunday, October 21, 2018 7:14 PM > To: dri-devel@lists.freedesktop.org > Cc: Zhou, David(ChunMing) <David1.Zhou@amd.com>; Daniel Vetter > <daniel@ffwll.ch>; Chris Wilson <chris@chris-wilson.co.uk>; Koenig, Christian > <Christian.Koenig@amd.com> > Subject: [PATCH] drm: fix deadlock of syncobj v2 > > v2: > add a mutex between sync_cb execution and free. > > Signed-off-by: Chunming Zhou <david1.zhou@amd.com> > Cc: Daniel Vetter <daniel@ffwll.ch> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_syncobj.c | 11 +++++++++-- > include/drm/drm_syncobj.h | 4 ++++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_syncobj.c > b/drivers/gpu/drm/drm_syncobj.c index 57bf6006394d..c025a0b93565 > 100644 > --- a/drivers/gpu/drm/drm_syncobj.c > +++ b/drivers/gpu/drm/drm_syncobj.c > @@ -158,9 +158,11 @@ void drm_syncobj_add_callback(struct drm_syncobj > *syncobj, void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, > struct drm_syncobj_cb *cb) > { > + mutex_lock(&syncobj->mutex); > spin_lock(&syncobj->lock); > list_del_init(&cb->node); > spin_unlock(&syncobj->lock); > + mutex_unlock(&syncobj->mutex); > } > > static void drm_syncobj_init(struct drm_syncobj *syncobj) @@ -344,13 > +346,17 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, > drm_syncobj_create_signal_pt(syncobj, fence, pt_value); > if (fence) { > struct drm_syncobj_cb *cur, *tmp; > + LIST_HEAD(cb_list); > > + mutex_lock(&syncobj->mutex); > spin_lock(&syncobj->lock); > - list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) > { > + list_splice_init(&syncobj->cb_list, &cb_list); > + spin_unlock(&syncobj->lock); > + list_for_each_entry_safe(cur, tmp, &cb_list, node) { > list_del_init(&cur->node); > cur->func(syncobj, cur); > } > - spin_unlock(&syncobj->lock); > + mutex_unlock(&syncobj->mutex); > } > } > EXPORT_SYMBOL(drm_syncobj_replace_fence); > @@ -501,6 +507,7 @@ int drm_syncobj_create(struct drm_syncobj > **out_syncobj, uint32_t flags, > kref_init(&syncobj->refcount); > INIT_LIST_HEAD(&syncobj->cb_list); > spin_lock_init(&syncobj->lock); > + mutex_init(&syncobj->mutex); > if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE) > syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE; > else > diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index > 5e8c5c027e09..3d3c8c181bd2 100644 > --- a/include/drm/drm_syncobj.h > +++ b/include/drm/drm_syncobj.h > @@ -78,6 +78,10 @@ struct drm_syncobj { > * @lock: Protects syncobj list and write-locks &fence. > */ > spinlock_t lock; > + /** > + * @mutex: mutex between syncobj cb execution and free. > + */ > + struct mutex mutex; > /** > * @file: A file backing for this syncobj. > */ > -- > 2.17.1
Quoting Chunming Zhou (2018-10-21 12:14:24) > v2: > add a mutex between sync_cb execution and free. The result would appear to be that syncobj->lock is relegated to protecting the pt_list and the mutex would only be needed for the syncobj->cb_list. The patch looks correct for resolving the deadlock and avoiding introducing any new fails, but I do wonder if spinlock_t pt_lock; struct mutex cb_mutex; and clearly separating the roles would not be a better approach. -Chris
On 2018年10月22日 16:34, Chris Wilson wrote: > Quoting Chunming Zhou (2018-10-21 12:14:24) >> v2: >> add a mutex between sync_cb execution and free. > The result would appear to be that syncobj->lock is relegated to > protecting the pt_list and the mutex would only be needed for the > syncobj->cb_list. > > The patch looks correct for resolving the deadlock and avoiding > introducing any new fails, but I do wonder if > > spinlock_t pt_lock; > struct mutex cb_mutex; > > and clearly separating the roles would not be a better approach. good idea, thanks, -David > -Chris
diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c index 57bf6006394d..c025a0b93565 100644 --- a/drivers/gpu/drm/drm_syncobj.c +++ b/drivers/gpu/drm/drm_syncobj.c @@ -158,9 +158,11 @@ void drm_syncobj_add_callback(struct drm_syncobj *syncobj, void drm_syncobj_remove_callback(struct drm_syncobj *syncobj, struct drm_syncobj_cb *cb) { + mutex_lock(&syncobj->mutex); spin_lock(&syncobj->lock); list_del_init(&cb->node); spin_unlock(&syncobj->lock); + mutex_unlock(&syncobj->mutex); } static void drm_syncobj_init(struct drm_syncobj *syncobj) @@ -344,13 +346,17 @@ void drm_syncobj_replace_fence(struct drm_syncobj *syncobj, drm_syncobj_create_signal_pt(syncobj, fence, pt_value); if (fence) { struct drm_syncobj_cb *cur, *tmp; + LIST_HEAD(cb_list); + mutex_lock(&syncobj->mutex); spin_lock(&syncobj->lock); - list_for_each_entry_safe(cur, tmp, &syncobj->cb_list, node) { + list_splice_init(&syncobj->cb_list, &cb_list); + spin_unlock(&syncobj->lock); + list_for_each_entry_safe(cur, tmp, &cb_list, node) { list_del_init(&cur->node); cur->func(syncobj, cur); } - spin_unlock(&syncobj->lock); + mutex_unlock(&syncobj->mutex); } } EXPORT_SYMBOL(drm_syncobj_replace_fence); @@ -501,6 +507,7 @@ int drm_syncobj_create(struct drm_syncobj **out_syncobj, uint32_t flags, kref_init(&syncobj->refcount); INIT_LIST_HEAD(&syncobj->cb_list); spin_lock_init(&syncobj->lock); + mutex_init(&syncobj->mutex); if (flags & DRM_SYNCOBJ_CREATE_TYPE_TIMELINE) syncobj->type = DRM_SYNCOBJ_TYPE_TIMELINE; else diff --git a/include/drm/drm_syncobj.h b/include/drm/drm_syncobj.h index 5e8c5c027e09..3d3c8c181bd2 100644 --- a/include/drm/drm_syncobj.h +++ b/include/drm/drm_syncobj.h @@ -78,6 +78,10 @@ struct drm_syncobj { * @lock: Protects syncobj list and write-locks &fence. */ spinlock_t lock; + /** + * @mutex: mutex between syncobj cb execution and free. + */ + struct mutex mutex; /** * @file: A file backing for this syncobj. */
v2: add a mutex between sync_cb execution and free. Signed-off-by: Chunming Zhou <david1.zhou@amd.com> Cc: Daniel Vetter <daniel@ffwll.ch> Cc: Chris Wilson <chris@chris-wilson.co.uk> Cc: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_syncobj.c | 11 +++++++++-- include/drm/drm_syncobj.h | 4 ++++ 2 files changed, 13 insertions(+), 2 deletions(-)