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

[Christian König]

radeon_drm_cs_sync_flush doesn't seems to be save when it's called from 
multiple threads but thought that this might be intentional.

Maybe we are running into that issue now that we are calling it more often.

Changing that should be trivial, going to attach a possible fix to the 
bugreport in a minute.

Christian.

Am 06.10.2013 18:06, schrieb 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