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

[Timothy Arceri]

On 24/02/17 21:02, Marek Olšák wrote:
> 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.

Seems SVN didn't used to worry about collisions either.

https://arstechnica.com/security/2017/02/watershed-sha1-collision-just-broke-the-webkit-repository-others-may-follow/

>
> Marek
>