Re: [PATCH] drm: fix deadlock of syncobj v5

[zhoucm1 <zhoucm1@amd.com>]

On 2018年10月23日 17:01, Chris Wilson wrote:
> Quoting Chunming Zhou (2018-10-23 08:57:54)
>> v2:
>> add a mutex between sync_cb execution and free.
>> v3:
>> clearly separating the roles for pt_lock and cb_mutex (Chris)
>> v4:
>> the cb_mutex should be taken outside of the pt_lock around this if() block. (Chris)
>> v5:
>> fix a corner case
>>
>> Tested by syncobj_basic and syncobj_wait of igt.
>>
>> 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>
>> Cc: intel-gfx@lists.freedesktop.org
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/drm_syncobj.c | 55 +++++++++++++++++++----------------
>>   include/drm/drm_syncobj.h     |  8 +++--
>>   2 files changed, 36 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_syncobj.c b/drivers/gpu/drm/drm_syncobj.c
>> index 57bf6006394d..679a56791e34 100644
>> --- a/drivers/gpu/drm/drm_syncobj.c
>> +++ b/drivers/gpu/drm/drm_syncobj.c
>> @@ -125,23 +125,26 @@ static int drm_syncobj_fence_get_or_add_callback(struct drm_syncobj *syncobj,
>>          if (!ret)
>>                  return 1;
>>   
>> -       spin_lock(&syncobj->lock);
>> +       mutex_lock(&syncobj->cb_mutex);
>>          /* We've already tried once to get a fence and failed.  Now that we
>>           * have the lock, try one more time just to be sure we don't add a
>>           * callback when a fence has already been set.
>>           */
>> +       spin_lock(&syncobj->pt_lock);
>>          if (!list_empty(&syncobj->signal_pt_list)) {
>> -               spin_unlock(&syncobj->lock);
>> +               spin_unlock(&syncobj->pt_lock);
>>                  drm_syncobj_search_fence(syncobj, 0, 0, fence);
> Hmm, just thinking of other ways of tidying this up
>
> mutex_lock(cb_lock);
> spin_lock(pt_lock);
> *fence = drm_syncobj_find_signal_pt_for_point();
> spin_unlock(pt_list);
> if (*!fence)
> 	drm_syncobj_add_callback_locked(syncobj, cb, func);
> mutex_unlock(cb_lock);
>
> i.e. get rid of the early return and we can even drop the int return here
> as it is unimportant and unused.
Yes, do you need I send v6? or you make a separate patch as a improvment?

Thanks,
David Zhou
> -Chris

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel