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

[sroland at vmware.com]

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();
       }
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:
+       * 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