[Mesa-dev] [PATCH] RFC: llvmpipe map scene buffers outside thread. (v2)

[Roland Scheidegger]

Am 09.11.2015 um 05:19 schrieb Dave Airlie:
> From: Dave Airlie <airlied at redhat.com>
> 
> There might be a reason we do this inside the thread, but I'm not aware of it
> yet, move stuff around and see if this jogs anyone's memory.
> 
> Doing this outside the thread at least with front buffer rendering avoids
> problems with XGetImage failing in the thread and deadlocking, now things
> crash, which is a lot nicer from a piglit point of view.
> 
> v2: map outside rast mutex, so if we fail XGetImage the fail paths
> don't deadlock
> ---
>  src/gallium/drivers/llvmpipe/lp_scene.c | 21 ++++++++++++++++-----
>  src/gallium/drivers/llvmpipe/lp_scene.h |  5 +++--
>  src/gallium/drivers/llvmpipe/lp_setup.c |  1 +
>  3 files changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_scene.c b/src/gallium/drivers/llvmpipe/lp_scene.c
> index 2441b3c..1a6fe5c 100644
> --- a/src/gallium/drivers/llvmpipe/lp_scene.c
> +++ b/src/gallium/drivers/llvmpipe/lp_scene.c
> @@ -147,7 +147,7 @@ lp_scene_bin_reset(struct lp_scene *scene, unsigned x, unsigned y)
>  
>  
>  void
> -lp_scene_begin_rasterization(struct lp_scene *scene)
> +lp_scene_map_buffers(struct lp_scene *scene)
>  {
>     const struct pipe_framebuffer_state *fb = &scene->fb;
>     int i;
> @@ -200,16 +200,20 @@ lp_scene_begin_rasterization(struct lp_scene *scene)
>     }
>  }
>  
> -
> +void
> +lp_scene_begin_rasterization(struct lp_scene *scene)
> +{
> +   scene->started = true;
> +}
>  
>  
>  /**
>   * Free all the temporary data in a scene.
>   */
> -void
> -lp_scene_end_rasterization(struct lp_scene *scene )
> +static void
> +lp_scene_unmap_buffers(struct lp_scene *scene )
>  {
> -   int i, j;
> +   int i;
>  
>     /* Unmap color buffers */
>     for (i = 0; i < scene->fb.nr_cbufs; i++) {
> @@ -232,7 +236,14 @@ lp_scene_end_rasterization(struct lp_scene *scene )
>                                zsbuf->u.tex.first_layer);
>        scene->zsbuf.map = NULL;
>     }
> +}
>  
> +void
> +lp_scene_end_rasterization(struct lp_scene *scene )
> +{
> +   int i, j;
> +   lp_scene_unmap_buffers(scene);
> +   scene->started = false;
>     /* Reset all command lists:
>      */
>     for (i = 0; i < scene->tiles_x; i++) {
> diff --git a/src/gallium/drivers/llvmpipe/lp_scene.h b/src/gallium/drivers/llvmpipe/lp_scene.h
> index b1464bb..7ed38c9 100644
> --- a/src/gallium/drivers/llvmpipe/lp_scene.h
> +++ b/src/gallium/drivers/llvmpipe/lp_scene.h
> @@ -178,6 +178,7 @@ struct lp_scene {
>  
>     struct cmd_bin tile[TILES_X][TILES_Y];
>     struct data_block_list data;
> +   boolean started;
Is this just for debug? Doesn't look like it's ever read.

>  };
>  
>  
> @@ -405,8 +406,8 @@ lp_scene_begin_rasterization(struct lp_scene *scene);
>  void
>  lp_scene_end_rasterization(struct lp_scene *scene );
>  
> -
> -
> +void
> +lp_scene_map_buffers(struct lp_scene *scene);
>  
>  
>  #endif /* LP_BIN_H */
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c b/src/gallium/drivers/llvmpipe/lp_setup.c
> index 1778b13..481dfb1 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -163,6 +163,7 @@ lp_setup_rasterize_scene( struct lp_setup_context *setup )
>  
>     if (setup->last_fence)
>        setup->last_fence->issued = TRUE;
> +   lp_scene_map_buffers(scene);
>  
>     pipe_mutex_lock(screen->rast_mutex);
>  
> 

At some point map/unmap was actually done per-tile (hence inside
rasterization) because of the swizzled rendering (so, only affected
tiles were swizzled if they weren't already) - that's at least my
recollection...
I suspect it doesn't need to be done inside the thread nowadays. Of
course, we don't really do a "real" map/unmap for anything except winsys
buffers (and btw, we only ever unmap winsys buffers for rendering, but
not for texturing, so there's bugs in this area).
Also, because we don't actually do any work when rasterization is
running (see the comment in lp_setup_rasterize_scene()) there's probably
quite some things which don't really matter if they are done inside
rasterize thread or not anyway... Albeit that's something which should
eventually be fixed...

Roland