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

[Ben Widawsky]

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

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