[Mesa-dev] [PATCH 2/5] winsys/radeon: remove cs_queue_empty

[Marek Olšák]

This causes a deadlock and X freeze for some users. See the bug:
https://bugs.freedesktop.org/show_bug.cgi?id=70123

Do you have any idea what's wrong there?

Marek

On Sat, Sep 21, 2013 at 4:41 PM, Christian König
<deathsimple at vodafone.de> wrote:
> From: Christian König <christian.koenig at amd.com>
>
> Waiting for an empty queue is nonsense and can lead to deadlocks if we have
> multiple waiters or another thread that continuously sends down new commands.
>
> Just post the cs to the queue and immediately wait for it to finish.
>
> This is a candidate for the stable branch.
>
> Signed-off-by: Christian König <christian.koenig at amd.com>
> ---
>  src/gallium/winsys/radeon/drm/radeon_drm_cs.c     | 11 +++--------
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.c |  6 ------
>  src/gallium/winsys/radeon/drm/radeon_drm_winsys.h |  5 -----
>  3 files changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_cs.c b/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
> index 38a9209..d530011 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_cs.c
> @@ -560,17 +560,12 @@ static void radeon_drm_cs_flush(struct radeon_winsys_cs *rcs, unsigned flags, ui
>              break;
>          }
>
> -        if (cs->ws->thread && (flags & RADEON_FLUSH_ASYNC)) {
> +        if (cs->ws->thread) {
>              cs->flush_started = 1;
>              radeon_drm_ws_queue_cs(cs->ws, cs);
> +            if (!(flags & RADEON_FLUSH_ASYNC))
> +                radeon_drm_cs_sync_flush(rcs);
>          } else {
> -            pipe_mutex_lock(cs->ws->cs_stack_lock);
> -            if (cs->ws->thread) {
> -                while (p_atomic_read(&cs->ws->ncs)) {
> -                    pipe_condvar_wait(cs->ws->cs_queue_empty, cs->ws->cs_stack_lock);
> -                }
> -            }
> -            pipe_mutex_unlock(cs->ws->cs_stack_lock);
>              radeon_drm_cs_emit_ioctl_oneshot(cs, cs->cst);
>          }
>      } else {
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> index 0a3b932..27bf348 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.c
> @@ -431,7 +431,6 @@ static void radeon_winsys_destroy(struct radeon_winsys *rws)
>          pipe_thread_wait(ws->thread);
>      }
>      pipe_semaphore_destroy(&ws->cs_queued);
> -    pipe_condvar_destroy(ws->cs_queue_empty);
>
>      pipe_mutex_destroy(ws->hyperz_owner_mutex);
>      pipe_mutex_destroy(ws->cmask_owner_mutex);
> @@ -567,9 +566,6 @@ next:
>              }
>              ws->cs_stack[p_atomic_read(&ws->ncs) - 1] = NULL;
>              empty_stack = p_atomic_dec_zero(&ws->ncs);
> -            if (empty_stack) {
> -                pipe_condvar_signal(ws->cs_queue_empty);
> -            }
>              pipe_mutex_unlock(ws->cs_stack_lock);
>
>              pipe_semaphore_signal(&cs->flush_completed);
> @@ -585,7 +581,6 @@ next:
>          ws->cs_stack[i] = NULL;
>      }
>      p_atomic_set(&ws->ncs, 0);
> -    pipe_condvar_signal(ws->cs_queue_empty);
>      pipe_mutex_unlock(ws->cs_stack_lock);
>      return NULL;
>  }
> @@ -651,7 +646,6 @@ struct radeon_winsys *radeon_drm_winsys_create(int fd)
>
>      p_atomic_set(&ws->ncs, 0);
>      pipe_semaphore_init(&ws->cs_queued, 0);
> -    pipe_condvar_init(ws->cs_queue_empty);
>      if (ws->num_cpus > 1 && debug_get_option_thread())
>          ws->thread = pipe_thread_create(radeon_drm_cs_emit_ioctl, ws);
>
> diff --git a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> index 682479e..ed90194 100644
> --- a/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> +++ b/src/gallium/winsys/radeon/drm/radeon_drm_winsys.h
> @@ -67,11 +67,6 @@ struct radeon_drm_winsys {
>      /* rings submission thread */
>      pipe_mutex cs_stack_lock;
>      pipe_semaphore cs_queued;
> -    /* we cannot use semaphore for empty queue because maintaining an even
> -     * number of call to semaphore_wait and semaphore_signal is, to say the
> -     * least, tricky
> -     */
> -    pipe_condvar cs_queue_empty;
>      pipe_thread thread;
>      int kill_thread;
>      int ncs;
> --
> 1.8.1.2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev