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

[Anuj Phogat]

On Thu, Jun 25, 2015 at 5:45 AM, Topi Pohjolainen
<topi.pohjolainen at intel.com> wrote:
> Items in the program cache consist of three things: key, the data
> representing the instructions and auxiliary data representing
> uniform storage. The data consisting of instructions is stored into
> a drm buffer object while the key and the auxiliary data reside in
> malloced section. Now the cache uploading is equipped with a check
> that iterates over existing items and seeks to find a another item
> using identical instruction data than the one being just uploaded.
> If such is found there is no need to add another section into the
> drm buffer object holding identical copy of the existing one. The
> item just being uploaded should instead simply point to the same
> offset in the underlying drm buffer object.
>
> Unfortunately the check for the matching instruction data is
> coupled with a check for matching auxiliary data also. This
> effectively prevents the cache from ever containing two items
> that could share a section in the drm buffer object.
>
> The constraint for the instruction data and auxiliary data to
> match is, fortunately, unnecessary strong. When items are stored
> into the cache they will anyway contain their own copy of the
> auxiliary data (even if they matched - which they in real world
> never will). The only thing the items would be sharing is the
> instruction data and hence we should only check for that to match
> and nothing else.
>
> No piglit regression in jenkins.
>
> CC: Kenneth Graunke <kenneth at whitecape.org>
> Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> ---
>  src/mesa/drivers/dri/i965/brw_state_cache.c | 52 +++++++++++------------------
>  1 file changed, 20 insertions(+), 32 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_state_cache.c b/src/mesa/drivers/dri/i965/brw_state_cache.c
> index d42b4b4..a0ec280 100644
> --- a/src/mesa/drivers/dri/i965/brw_state_cache.c
> +++ b/src/mesa/drivers/dri/i965/brw_state_cache.c
> @@ -200,36 +200,23 @@ brw_cache_new_bo(struct brw_cache *cache, uint32_t new_size)
>  }
>
>  /**
> - * Attempts to find an item in the cache with identical data and aux
> - * data to use
> + * Attempts to find an item in the cache with identical data.
>   */
> -static bool
> -brw_try_upload_using_copy(struct brw_cache *cache,
> -                         struct brw_cache_item *result_item,
> -                         const void *data,
> -                         const void *aux)
> +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)
>  {
> -   struct brw_context *brw = cache->brw;
> +   const struct brw_context *brw = cache->brw;
>     int i;
> -   struct brw_cache_item *item;
> +   const 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 != 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) {
> +        if (item->cache_id != cache_id || item->size != data_size)
>             continue;
> -        }
>
>           if (!brw->has_llc)
>              drm_intel_bo_map(cache->bo, false);
> @@ -239,13 +226,11 @@ brw_try_upload_using_copy(struct brw_cache *cache,
>          if (ret)
>             continue;
>
> -        result_item->offset = item->offset;
> -
> -        return true;
> +        return item;
>        }
>     }
>
> -   return false;
> +   return NULL;
>  }
>
>  static uint32_t
> @@ -294,6 +279,8 @@ 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;
>
> @@ -305,14 +292,15 @@ brw_upload_cache(struct brw_cache *cache,
>     hash = hash_key(item);
>     item->hash = hash;
>
> -   /* 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 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 (!brw_try_upload_using_copy(cache, item, data, aux)) {
> +   if (matching_data) {
> +      item->offset = matching_data->offset;
> +   } else {
>        item->offset = brw_alloc_item_data(cache, data_size);
>
>        /* Copy data to the buffer */
> --
> 1.9.3
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Idea behind the patch looks good to me. Maybe Ken would provide more useful
comments here.