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
>