[Mesa-dev] [PATCH] llvmpipe: fix a bug in opaque optimization

[Jose Fonseca]

----- Original Message -----
> From: Roland Scheidegger <sroland at vmware.com>
> 
> If there are queries active the opaque optimization reseting the bin needs to
> be disabled.
> (Not really tested since the bug was discovered by code inspection not
> an actual test failure.)
> ---
>  src/gallium/drivers/llvmpipe/lp_query.c     |    2 ++
>  src/gallium/drivers/llvmpipe/lp_scene.h     |    2 ++
>  src/gallium/drivers/llvmpipe/lp_setup.c     |    4 ++++
>  src/gallium/drivers/llvmpipe/lp_setup_tri.c |   28
>  +++++++++++++--------------
>  4 files changed, 22 insertions(+), 14 deletions(-)
> 
> diff --git a/src/gallium/drivers/llvmpipe/lp_query.c
> b/src/gallium/drivers/llvmpipe/lp_query.c
> index ac6d09d..7bd090d 100644
> --- a/src/gallium/drivers/llvmpipe/lp_query.c
> +++ b/src/gallium/drivers/llvmpipe/lp_query.c
> @@ -134,6 +134,8 @@ llvmpipe_get_query_result(struct pipe_context *pipe,
>           if (pq->end[i] > *result) {
>              *result = pq->end[i];
>           }
> +         /* XXX if this would happen then querying this multiple times
> +          * would return a different result each time... */
>           if (*result == 0)
>              *result = os_time_get_nano();
>        }

This chunk seems unrelated to the rest of the change and should be in a separate change.  Furthermore seems easily fixable -- just store the value on the put the value pq->count[i] itself.

It also seems that if-statement should be after the loop.

> diff --git a/src/gallium/drivers/llvmpipe/lp_scene.h
> b/src/gallium/drivers/llvmpipe/lp_scene.h
> index 59cce7d..5501d40 100644
> --- a/src/gallium/drivers/llvmpipe/lp_scene.h
> +++ b/src/gallium/drivers/llvmpipe/lp_scene.h
> @@ -132,6 +132,8 @@ struct lp_scene {
>     /* The queries still active at end of scene */
>     struct llvmpipe_query *active_queries[LP_MAX_ACTIVE_BINNED_QUERIES];
>     unsigned num_active_queries;
> +   /* If queries were either active or there were begin/end query commands
> */
> +   boolean had_queries;
>  
>     /* Framebuffer mappings - valid only between begin_rasterization()
>      * and end_rasterization().
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c
> b/src/gallium/drivers/llvmpipe/lp_setup.c
> index 49aead2..49b61c3 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> @@ -237,6 +237,8 @@ begin_binning( struct lp_setup_context *setup )
>     setup->clear.zsmask = 0;
>     setup->clear.zsvalue = 0;
>  
> +   scene->had_queries = !!setup->active_binned_queries;
> +
>     LP_DBG(DEBUG_SETUP, "%s done\n", __FUNCTION__);
>     return TRUE;
>  }
> @@ -1237,6 +1239,7 @@ lp_setup_begin_query(struct lp_setup_context *setup,
>              return;
>           }
>        }
> +      setup->scene->had_queries |= TRUE;
>     }
>  }
>  
> @@ -1272,6 +1275,7 @@ lp_setup_end_query(struct lp_setup_context *setup,
> struct llvmpipe_query *pq)
>                 goto fail;
>              }
>           }
> +         setup->scene->had_queries |= TRUE;
>        }
>     }
>     else {
> diff --git a/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> index 6dc136c..dbfad46 100644
> --- a/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> +++ b/src/gallium/drivers/llvmpipe/lp_setup_tri.c
> @@ -204,22 +204,22 @@ lp_setup_whole_tile(struct lp_setup_context *setup,
>     LP_COUNT(nr_fully_covered_64);
>  
>     /* if variant is opaque and scissor doesn't effect the tile */
> -   /*
> -    * Need to disable this optimization for layered rendering and cannot use
> -    * setup->layer_slot here to determine it, because it could incorrectly
> -    * reset the tile if a previous shader used layer_slot but not this one
> -    * (or maybe even "undo" clears). So determine this from presence of
> layers
> -    * instead (in which case layer_slot will have no effect).
> -    */
> -   if (inputs->opaque && scene->fb_max_layer == 0) {
> -      if (!scene->fb.zsbuf) {
> +   if (inputs->opaque) {
> +      /* Several things prevent this optimization from working:

Could you please reformat the comment as an itemized list -- it would make it easier to read.

> +       * For layered rendering we can't determine if this covers the same
> layer
> +       * as previous rendering (or in case of clears those actually always
> cover
> +       * all layers so optimization is impossible). Need to use fb_max_layer
> and
> +       * not setup->layer_slot to determine this since even if there's
> currently
> +       * no slot assigned previous rendering could have used one.
> +       * If there were any Begin/End query commands in the scene then those
> +       * would get removed which would be very wrong. Furthermore, if
> queries
> +       * were just active we also can't do the optimization since to get
> +       * accurate query results we unfortunately need to execute the
> rendering
> +       * commands.
> +       */
> +      if (!scene->fb.zsbuf && scene->fb_max_layer == 0 &&
> !scene->had_queries) {
>           /*
>            * All previous rendering will be overwritten so reset the bin.
> -          * XXX This is wrong wrt to all queries arriving here (timestamp,
> -          * occlusion, ps invocations). Not counting stuff might be ok but
> it
> -          * will kill the begin/end query commands too which is definitely
> -          * wrong (and at this point we don't even know if there were any
> -          * such commands here).
>            */
>           lp_scene_bin_reset( scene, tx, ty );
>        }
> --
> 1.7.9.5
> 

Otherwise looks great.

Jose