Re: [Intel-gfx] [PATCH] drm: fix deadlock of syncobj v5

2018-10-23 Thread Koenig, Christian 
Am 23.10.18 um 11:11 schrieb Chris Wilson:
> Quoting zhoucm1 (2018-10-23 10:09:01)
>>
>> 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 
 Cc: Daniel Vetter 
 Cc: Chris Wilson 
 Cc: Christian König 
 Cc: intel-gfx@lists.freedesktop.org
 Reviewed-by: Chris Wilson 
 ---
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(>lock);
 +   mutex_lock(>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(>pt_lock);
   if (!list_empty(>signal_pt_list)) {
 -   spin_unlock(>lock);
 +   spin_unlock(>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?
> Send it in reply, we still have some time before the shards catch up
> with the ml ;)

I'm idle anyway because I've locked myself out of the AMD VPN accidentally.

So just send me a ping when the v6 is ready to be committed and I can 
push it to drm-misc-next.

Christian.

> -Chris

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: fix deadlock of syncobj v5

2018-10-23 Thread Chris Wilson 
Quoting zhoucm1 (2018-10-23 10:09:01)
> 
> 
> 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 
> >> Cc: Daniel Vetter 
> >> Cc: Chris Wilson 
> >> Cc: Christian König 
> >> Cc: intel-gfx@lists.freedesktop.org
> >> Reviewed-by: Chris Wilson 
> >> ---
> >>   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(>lock);
> >> +   mutex_lock(>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(>pt_lock);
> >>  if (!list_empty(>signal_pt_list)) {
> >> -   spin_unlock(>lock);
> >> +   spin_unlock(>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?

Send it in reply, we still have some time before the shards catch up
with the ml ;)
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm: fix deadlock of syncobj v5

2018-10-23 Thread Chris Wilson 
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 
> Cc: Daniel Vetter 
> Cc: Chris Wilson 
> Cc: Christian König 
> Cc: intel-gfx@lists.freedesktop.org
> Reviewed-by: Chris Wilson 
> ---
>  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(>lock);
> +   mutex_lock(>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(>pt_lock);
> if (!list_empty(>signal_pt_list)) {
> -   spin_unlock(>lock);
> +   spin_unlock(>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.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm: fix deadlock of syncobj v5

2018-10-23 Thread Chunming Zhou 
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 
Cc: Daniel Vetter 
Cc: Chris Wilson 
Cc: Christian König 
Cc: intel-gfx@lists.freedesktop.org
Reviewed-by: Chris Wilson 
---
 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(>lock);
+   mutex_lock(>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(>pt_lock);
if (!list_empty(>signal_pt_list)) {
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
drm_syncobj_search_fence(syncobj, 0, 0, fence);
-   if (*fence)
+   if (*fence) {
+   mutex_unlock(>cb_mutex);
return 1;
-   spin_lock(>lock);
+   }
} else {
+   spin_unlock(>pt_lock);
*fence = NULL;
drm_syncobj_add_callback_locked(syncobj, cb, func);
ret = 0;
}
-   spin_unlock(>lock);
+   mutex_unlock(>cb_mutex);
 
return ret;
 }
@@ -150,43 +153,43 @@ void drm_syncobj_add_callback(struct drm_syncobj *syncobj,
  struct drm_syncobj_cb *cb,
  drm_syncobj_func_t func)
 {
-   spin_lock(>lock);
+   mutex_lock(>cb_mutex);
drm_syncobj_add_callback_locked(syncobj, cb, func);
-   spin_unlock(>lock);
+   mutex_unlock(>cb_mutex);
 }
 
 void drm_syncobj_remove_callback(struct drm_syncobj *syncobj,
 struct drm_syncobj_cb *cb)
 {
-   spin_lock(>lock);
+   mutex_lock(>cb_mutex);
list_del_init(>node);
-   spin_unlock(>lock);
+   mutex_unlock(>cb_mutex);
 }
 
 static void drm_syncobj_init(struct drm_syncobj *syncobj)
 {
-   spin_lock(>lock);
+   spin_lock(>pt_lock);
syncobj->timeline_context = dma_fence_context_alloc(1);
syncobj->timeline = 0;
syncobj->signal_point = 0;
init_waitqueue_head(>wq);
 
INIT_LIST_HEAD(>signal_pt_list);
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
 }
 
 static void drm_syncobj_fini(struct drm_syncobj *syncobj)
 {
struct drm_syncobj_signal_pt *signal_pt = NULL, *tmp;
 
-   spin_lock(>lock);
+   spin_lock(>pt_lock);
list_for_each_entry_safe(signal_pt, tmp,
 >signal_pt_list, list) {
list_del(_pt->list);
dma_fence_put(_pt->fence_array->base);
kfree(signal_pt);
}
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
 }
 
 static struct dma_fence
@@ -249,14 +252,14 @@ static int drm_syncobj_create_signal_pt(struct 
drm_syncobj *syncobj,
fences[num_fences++] = dma_fence_get(fence);
/* timeline syncobj must take this dependency */
if (syncobj->type == DRM_SYNCOBJ_TYPE_TIMELINE) {
-   spin_lock(>lock);
+   spin_lock(>pt_lock);
if (!list_empty(>signal_pt_list)) {
tail_pt = list_last_entry(>signal_pt_list,
  struct drm_syncobj_signal_pt, 
list);
fences[num_fences++] =
dma_fence_get(_pt->fence_array->base);
}
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
}
signal_pt->fence_array = dma_fence_array_create(num_fences, fences,

syncobj->timeline_context,
@@ -266,16 +269,16 @@ static int drm_syncobj_create_signal_pt(struct 
drm_syncobj *syncobj,
goto fail;
}
 
-   spin_lock(>lock);
+   spin_lock(>pt_lock);
if (syncobj->signal_point >= point) {
DRM_WARN("A later signal is ready!");
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
goto exist;
}
signal_pt->value = point;
list_add_tail(_pt->list, >signal_pt_list);
syncobj->signal_point = point;
-   spin_unlock(>lock);
+   spin_unlock(>pt_lock);
wake_up_all(>wq);
 
return 0;