[Mesa-dev] [PATCH] [RFC v2] mesa/glthread: Call unmarshal_batch directly in glthread_finish when batch queue is empty.

[Nicolai Hähnle]

On 29.03.2017 18:11, Bartosz Tomczyk wrote:
> This avoids costly thread synchronisation. With this fix games that previously regressed with mesa_glthread=true like xonotic or grid autosport.
> Could someone test if games that benefit from glthread didn't regress?

Please make sure the commit message is wrapped to 75 characters.

The approach seems like a good idea: if the current thread is going to 
wait anyway, we might as well do any pending work locally to avoid 
context switch overhead. Would be nice to see some benchmarks, but this 
should mostly be a win -- the only reason I could imagine why it might 
not be is cache effects, and those could go either way.


> ---
>  src/mesa/main/glthread.c | 49 +++++++++++++++++++++++++++++++++---------------
>  1 file changed, 34 insertions(+), 15 deletions(-)
>
> diff --git a/src/mesa/main/glthread.c b/src/mesa/main/glthread.c
> index 06115b916d..eef7202f01 100644
> --- a/src/mesa/main/glthread.c
> +++ b/src/mesa/main/glthread.c
> @@ -194,18 +194,11 @@ _mesa_glthread_restore_dispatch(struct gl_context *ctx)
>     }
>  }
>
> -void
> -_mesa_glthread_flush_batch(struct gl_context *ctx)
> +static void
> +_mesa_glthread_flush_batch_no_lock(struct gl_context *ctx)

A better and more idiomatic name for this function would be 
_mesa_glthread_flush_batch_locked.


>  {
>     struct glthread_state *glthread = ctx->GLThread;
> -   struct glthread_batch *batch;
> -
> -   if (!glthread)
> -      return;
> -
> -   batch = glthread->batch;
> -   if (!batch->used)
> -      return;
> +   struct glthread_batch *batch = glthread->batch;
>
>     /* Immediately reallocate a new batch, since the next marshalled call would
>      * just do it.
> @@ -223,10 +216,26 @@ _mesa_glthread_flush_batch(struct gl_context *ctx)
>        return;
>     }
>
> -   pthread_mutex_lock(&glthread->mutex);
>     *glthread->batch_queue_tail = batch;
>     glthread->batch_queue_tail = &batch->next;
>     pthread_cond_broadcast(&glthread->new_work);
> +
> +}
> +void
> +_mesa_glthread_flush_batch(struct gl_context *ctx)
> +{
> +   struct glthread_state *glthread = ctx->GLThread;
> +   struct glthread_batch *batch;
> +
> +   if (!glthread)
> +      return;
> +
> +   batch = glthread->batch;
> +   if (!batch->used)
> +      return;
> +
> +   pthread_mutex_lock(&glthread->mutex);
> +   _mesa_glthread_flush_batch_no_lock(ctx);
>     pthread_mutex_unlock(&glthread->mutex);
>  }
>
> @@ -252,12 +261,22 @@ _mesa_glthread_finish(struct gl_context *ctx)
>     if (pthread_self() == glthread->thread)
>        return;
>
> -   _mesa_glthread_flush_batch(ctx);
> -
>     pthread_mutex_lock(&glthread->mutex);
>
> -   while (glthread->batch_queue || glthread->busy)
> -      pthread_cond_wait(&glthread->work_done, &glthread->mutex);
> +   if (!(glthread->batch_queue || glthread->busy))
> +   {
> +      if (glthread->batch && glthread->batch->used)
> +      {
> +         glthread_unmarshal_batch(ctx, glthread->batch);

You _must_ reset the api dispatch afterwards; otherwise, your change 
here effectively disables glthread forever. To be on the safe side, I 
think you need to save the current dispatch in a temp variable and then 
reset after unmarshalling.

Cheers,
Nicolai


> +      }
> +      glthread_allocate_batch(ctx);
> +   }
> +   else
> +   {
> +      _mesa_glthread_flush_batch_no_lock(ctx);
> +      while (glthread->batch_queue || glthread->busy)
> +         pthread_cond_wait(&glthread->work_done, &glthread->mutex);
> +   }
>
>     pthread_mutex_unlock(&glthread->mutex);
>  }
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.