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

[Roland Scheidegger]

Am 27.06.2013 09:48, schrieb 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.
Ok I think I'm just going to nuke it instead. Given how this currently
works (scene is set to active, query binned) this looks really
impossible. (I am not sure if it actually makes a lot of sense to bin
this since the value won't really be more exact than a single
os_time_get_nano() value at the end of the scene anyway but as long as
the scene is set to active this still couldn't happen.)

> 
>> 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.
Ok.

> 
>> +       * 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
>