[Mesa-dev] [PATCH] i965: Fix missing BRW_NEW_*_PROG_DATA flagging caused by cache reuse.

[Pohjolainen, Topi]

On Wed, Oct 28, 2015 at 01:58:41AM -0700, Kenneth Graunke wrote:
> Consider the case of two nearly identical GLSL fragment shaders:
> 
>    out vec4 color;
>    void main() { color = vec4(1); }
> 
> and
> 
>    layout(early_fragment_tests) in;
>    out vec4 color;
>    void main() { color = vec4(1); }
> 
> These shaders compile to the exact same assembly, but have distinct
> values for brw_wm_prog_data::early_fragment_tests.
> 
> Since these are two independent GLSL shaders, they have different
> program keys - notably, brw_wm_prog_key::program_string_id differs.
> 
> When uploading the second, brw_upload_cache will find an existing copy
> of the assembly in the cache BO, which means matching_data will be
> non-NULL.  Although we create a second cache item (with the new key
> and prog_data), we set item->offset to the existing copy and avoid
> re-uploading duplicate assembly.
> 
> However, brw_search_cache() would only flag BRW_NEW_*_PROG_DATA if
> item->offset differed from the supplied offset.  With reuse, both
> programs have the same offset, but prog_data changed.  We have to
> flag it, but failed to.
> 
> To fix this, we simply need to check if the aux (prog_data) pointer
> changed.  If either the assembly or the prog_data differs, flag it.
> 
> This fixes a regression since 1bba29ed403e735ba0bf04ed8aa2e571884f,
> where Topi fixed brw_upload_cache() to actually reuse identical
> assembly.  Prior to that, reuse basically never happened due to bugs.
> Unfortunately, this code apparently wasn't prepared to handle reuse!
> 
> Fixes GPU hangs in Dolphin on Broadwell.
> 
> Huge thanks to Pierre Bourdon and Ilia Mirkin for debugging this
> and helping track down the real issue.

And thanks for the quick fix Ken!

Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>

> 
> Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> Cc: "11.0" <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92623
> Tested-by: Pierre Bourdon <delroth at gmail.com>
> Signed-off-by: Kenneth Graunke <kenneth at whitecape.org>
> ---
>  src/mesa/drivers/dri/i965/brw_state.h       | 2 +-
>  src/mesa/drivers/dri/i965/brw_state_cache.c | 7 ++++---
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_state.h b/src/mesa/drivers/dri/i965/brw_state.h
> index dc2b941..6fc9c14 100644
> --- a/src/mesa/drivers/dri/i965/brw_state.h
> +++ b/src/mesa/drivers/dri/i965/brw_state.h
> @@ -220,7 +220,7 @@ bool brw_search_cache(struct brw_cache *cache,
>  		      enum brw_cache_id cache_id,
>  		      const void *key,
>  		      GLuint key_size,
> -		      uint32_t *inout_offset, void *out_aux);
> +		      uint32_t *inout_offset, void *inout_aux);
>  void brw_state_cache_check_size( struct brw_context *brw );
>  
>  void brw_init_caches( struct brw_context *brw );
> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c b/src/mesa/drivers/dri/i965/brw_state_cache.c
> index 2fbcd14..f9a1918 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
> @@ -137,7 +137,7 @@ bool
>  brw_search_cache(struct brw_cache *cache,
>                   enum brw_cache_id cache_id,
>                   const void *key, GLuint key_size,
> -                 uint32_t *inout_offset, void *out_aux)
> +                 uint32_t *inout_offset, void *inout_aux)
>  {
>     struct brw_context *brw = cache->brw;
>     struct brw_cache_item *item;
> @@ -155,11 +155,12 @@ brw_search_cache(struct brw_cache *cache,
>     if (item == NULL)
>        return false;
>  
> -   *(void **)out_aux = ((char *)item->key + item->key_size);
> +   void *aux = ((char *) item->key) + item->key_size;
>  
> -   if (item->offset != *inout_offset) {
> +   if (item->offset != *inout_offset || aux != *((void **) inout_aux)) {
>        brw->ctx.NewDriverState |= (1 << cache_id);
>        *inout_offset = item->offset;
> +      *((void **) inout_aux) = aux;
>     }
>  
>     return true;
> -- 
> 2.6.2
>