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

[Marek Olšák]

On Fri, Feb 24, 2017 at 3:18 AM, Timothy Arceri <tarceri at itsqueeze.com> wrote:
>
>
> 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?

I'll let others comment on this. If nobody comments, checking only the
key can stay.

Marek