[Mesa-dev] [PATCH] Revert "i965: Stop aux data compare preventing program binary re-use"

[Pohjolainen, Topi]

On Wed, Aug 26, 2015 at 03:46:05PM -0700, Ben Widawsky wrote:
> This reverts commit 1bba29ed403e735ba0bf04ed8aa2e571884fcaaf
> Author: Topi Pohjolainen <topi.pohjolainen at intel.com>
> Date:   Thu Jun 25 14:00:41 2015 +0300
> 
>     i965: Stop aux data compare preventing program binary re-use
> 
> This fixes an intermittent failure in
> piglit.spec.arb_pixel_buffer_object.texsubimage pbo.sklm64 (maybe other
> platforms as well, but it is harder to reproduce). I can usually hit the failure
> within 10 runs of the test. This is a very hairy commit to debug. I'll let Topi
> handle it, or else we should go with the revert. I am open to either. I got
> lucky that Jenkins caught this on a run.
> 
> Here was the script I used for bisect:
> 
> i=0
> while [ $i -lt 40 ] ; do
>    ./bin/texsubimage pbo -auto -fbo > /dev/null 2>&1
>    [[ $? != 0 ]] && echo fail && exit 1
>    ((i++))
> done
> 
> exit 0

Should I use different piglit version than the current master? I'm asking
because I get this with both Mesa master and my patch reverted.

testrunner at skl-y:~/topi/piglit$ ./bin/texsubimage pbo -auto -fbo
Using test set: Core formats
texsubimage failed
  target: GL_TEXTURE_2D
  internal format: GL_COMPRESSED_RGB_S3TC_DXT1_EXT
  region: 68, 12  32 x 48
texsubimage failed
  target: GL_TEXTURE_2D
  internal format: GL_COMPRESSED_RGBA_S3TC_DXT1_EXT
  region: 0, 28  116 x 20
texsubimage failed
  target: GL_TEXTURE_2D
  internal format: GL_COMPRESSED_RGBA_S3TC_DXT3_EXT
  region: 16, 4  60 x 36
texsubimage failed
  target: GL_TEXTURE_2D
  internal format: GL_COMPRESSED_RGBA_S3TC_DXT5_EXT
  region: 8, 0  104 x 60
Mesa: User error: GL_INVALID_OPERATION in glTexSubImage2D(out of bounds PBO access)
PIGLIT: {"result": "fail" }


Could you include the console output of the failure you get?

> 
> Cc: <mesa-stable at lists.freedesktop.org>
> Cc: Kenneth Graunke <kenneth at whitecape.org>
> Cc: Topi Pohjolainen <topi.pohjolainen at intel.com>
> Reported-by: Mark Janes <mark.a.janes at intel.com> (jenkins)
> Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> ---
>  src/mesa/drivers/dri/i965/brw_state_cache.c | 52 ++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 20 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c b/src/mesa/drivers/dri/i965/brw_state_cache.c
> index fbc0419..e50d6a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
> @@ -200,23 +200,36 @@ brw_cache_new_bo(struct brw_cache *cache, uint32_t new_size)
>  }
>  
>  /**
> - * Attempts to find an item in the cache with identical data.
> + * Attempts to find an item in the cache with identical data and aux
> + * data to use
>   */
> -static const struct brw_cache_item *
> -brw_lookup_prog(const struct brw_cache *cache,
> -                enum brw_cache_id cache_id,
> -                const void *data, unsigned data_size)
> +static bool
> +brw_try_upload_using_copy(struct brw_cache *cache,
> +			  struct brw_cache_item *result_item,
> +			  const void *data,
> +			  const void *aux)
>  {
> -   const struct brw_context *brw = cache->brw;
> +   struct brw_context *brw = cache->brw;
>     unsigned i;
> -   const struct brw_cache_item *item;
> +   struct brw_cache_item *item;
>  
>     for (i = 0; i < cache->size; i++) {
>        for (item = cache->items[i]; item; item = item->next) {
> +	 const void *item_aux = item->key + item->key_size;
>  	 int ret;
>  
> -	 if (item->cache_id != cache_id || item->size != data_size)
> +	 if (item->cache_id != result_item->cache_id ||
> +	     item->size != result_item->size ||
> +	     item->aux_size != result_item->aux_size) {
> +	    continue;
> +	 }
> +
> +         if (cache->aux_compare[result_item->cache_id]) {
> +            if (!cache->aux_compare[result_item->cache_id](item_aux, aux))
> +               continue;
> +         } else if (memcmp(item_aux, aux, item->aux_size) != 0) {
>  	    continue;
> +	 }
>  
>           if (!brw->has_llc)
>              drm_intel_bo_map(cache->bo, false);
> @@ -226,11 +239,13 @@ brw_lookup_prog(const struct brw_cache *cache,
>  	 if (ret)
>  	    continue;
>  
> -	 return item;
> +	 result_item->offset = item->offset;
> +
> +	 return true;
>        }
>     }
>  
> -   return NULL;
> +   return false;
>  }
>  
>  static uint32_t
> @@ -279,8 +294,6 @@ brw_upload_cache(struct brw_cache *cache,
>  {
>     struct brw_context *brw = cache->brw;
>     struct brw_cache_item *item = CALLOC_STRUCT(brw_cache_item);
> -   const struct brw_cache_item *matching_data =
> -      brw_lookup_prog(cache, cache_id, data, data_size);
>     GLuint hash;
>     void *tmp;
>  
> @@ -292,15 +305,14 @@ brw_upload_cache(struct brw_cache *cache,
>     hash = hash_key(item);
>     item->hash = hash;
>  
> -   /* If we can find a matching prog in the cache already, then reuse the
> -    * existing stuff without creating new copy into the underlying buffer
> -    * object. This is notably useful for programs generating shaders at
> -    * runtime, where multiple shaders may compile to the same thing in our
> -    * backend.
> +   /* If we can find a matching prog/prog_data combo in the cache
> +    * already, then reuse the existing stuff.  This will mean not
> +    * flagging CACHE_NEW_* when transitioning between the two
> +    * equivalent hash keys.  This is notably useful for programs
> +    * generating shaders at runtime, where multiple shaders may
> +    * compile to the thing in our backend.
>      */
> -   if (matching_data) {
> -      item->offset = matching_data->offset;
> -   } else {
> +   if (!brw_try_upload_using_copy(cache, item, data, aux)) {
>        item->offset = brw_alloc_item_data(cache, data_size);
>  
>        /* Copy data to the buffer */
> -- 
> 2.5.0
>