[Mesa-dev] [PATCH 1/2] radeonsi: add support for an on-disk shader cache

[Timothy Arceri]

On 24/02/17 08:49, Timothy Arceri wrote:
>
>
> On 24/02/17 05:12, Marek Olšák wrote:
>> On Thu, Feb 23, 2017 at 3:09 AM, Timothy Arceri
>> <tarceri at itsqueeze.com> wrote:
>>> From: kdj0c <kdj0c at djinvi.net>
>>>
>>> V2 (Timothy Arceri):
>>> - when loading from disk cache also binary insert into memory cache.
>>> - check that the binary loaded from disk is the correct size. If not
>>>   delete the cache item and skip loading from cache.
>>> ---
>>>  src/gallium/drivers/radeonsi/si_state_shaders.c | 69
>>> ++++++++++++++++++++++---
>>>  1 file changed, 62 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c
>>> b/src/gallium/drivers/radeonsi/si_state_shaders.c
>>> index f615aa8..71556f9 100644
>>> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c
>>> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
>>> @@ -36,6 +36,9 @@
>>>  #include "util/u_memory.h"
>>>  #include "util/u_prim.h"
>>>
>>> +#include "util/disk_cache.h"
>>> +#include "util/mesa-sha1.h"
>>> +
>>>  /* SHADER_CACHE */
>>>
>>>  /**
>>> @@ -182,10 +185,12 @@ static bool si_load_shader_binary(struct
>>> si_shader *shader, void *binary)
>>>   */
>>>  static bool si_shader_cache_insert_shader(struct si_screen *sscreen,
>>>                                           void *tgsi_binary,
>>> -                                         struct si_shader *shader)
>>> +                                         struct si_shader *shader,
>>> +                                         bool insert_into_disk_cache)
>>>  {
>>>         void *hw_binary;
>>>         struct hash_entry *entry;
>>> +       uint8_t key[CACHE_KEY_SIZE];
>>>
>>>         entry = _mesa_hash_table_search(sscreen->shader_cache,
>>> tgsi_binary);
>>>         if (entry)
>>> @@ -201,6 +206,12 @@ static bool si_shader_cache_insert_shader(struct
>>> si_screen *sscreen,
>>>                 return false;
>>>         }
>>>
>>> +       if (sscreen->b.disk_shader_cache && insert_into_disk_cache) {
>>> +               _mesa_sha1_compute(tgsi_binary, *((uint32_t
>>> *)tgsi_binary), key);
>>
>> What happens if we randomly get a sha1 collision?
>
> You should stop playing your game which will be rendering incorrectly
> and by a lotto ticket.
>
>> Shouldn't we store the whole key as well?
>
> Sure I can add that, its cheap to check here anyway. Although the other
> cache stages rely on a collision being improbable.


For some reason I thought the key was simpler than it is. It seems 
excessive to store and compare the tgsi again. I don't think git even 
worries about the possibility of a collision and we will be dealing with 
much smaller amounts of cache items then commits in a git repository.

Thoughts?

>
>>
>>> +               disk_cache_put(sscreen->b.disk_shader_cache, key,
>>> hw_binary,
>>> +                              *((uint32_t *) hw_binary));
>>> +       }
>>> +
>>>         return true;
>>>  }
>>>
>>> @@ -210,12 +221,56 @@ static bool si_shader_cache_load_shader(struct
>>> si_screen *sscreen,
>>>  {
>>>         struct hash_entry *entry =
>>>                 _mesa_hash_table_search(sscreen->shader_cache,
>>> tgsi_binary);
>>> -       if (!entry)
>>> -               return false;
>>> +       if (!entry) {
>>> +               if (sscreen->b.disk_shader_cache) {
>>> +                       unsigned char sha1[CACHE_KEY_SIZE];
>>> +                       size_t tg_size = *((uint32_t *) tgsi_binary);
>>> +
>>> +                       _mesa_sha1_compute(tgsi_binary, tg_size, sha1);
>>> +
>>> +                       size_t binary_size;
>>> +                       uint8_t *buffer =
>>> +
>>> disk_cache_get(sscreen->b.disk_shader_cache,
>>> +                                              sha1, &binary_size);
>>> +                       if (!buffer)
>>> +                               return false;
>>>
>>> -       if (!si_load_shader_binary(shader, entry->data))
>>> -               return false;
>>> +                       uint32_t stored_binary_size;
>>
>> It looks like you don't need this variable.
>
>
> Right, I'll tidy that up thanks.
>
>>
>> Marek
>>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev