[Mesa-dev] [PATCH 06/27] i965: add initial implementation of on disk shader cache

[Timothy Arceri]

On 14/09/17 09:53, Matt Turner wrote:
> On 08/19, Jordan Justen wrote:
>> From: Timothy Arceri <timothy.arceri at collabora.com>
>>
>> This uses the recently-added disk_cache.c to write out the final
>> linked binary for vertex and fragment shader programs.
>>
>> This is based off the initial implementation done by Carl Worth.
>>
>> [jordan.l.justen at intel.com: *_cached_program => brw_disk_cache_*_program]
>> [jordan.l.justen at intel.com: brw_shader_cache.c => brw_disk_cache.c]
>> [jordan.l.justen at intel.com: don't map to write program when LLC is 
>> present]
>> [jordan.l.justen at intel.com: set program_written_to_cache on read from 
>> cache]
>> [jordan.l.justen at intel.com: only try cache when status is 
>> linking_skipped]
>> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
>> ---
>> src/mesa/drivers/dri/i965/Makefile.sources |   1 +
>> src/mesa/drivers/dri/i965/brw_disk_cache.c | 395 
>> +++++++++++++++++++++++++++++
>> src/mesa/drivers/dri/i965/brw_state.h      |   5 +
>> 3 files changed, 401 insertions(+)
>> create mode 100644 src/mesa/drivers/dri/i965/brw_disk_cache.c
>>
>> diff --git a/src/mesa/drivers/dri/i965/Makefile.sources 
>> b/src/mesa/drivers/dri/i965/Makefile.sources
>> index 425c883de8..6e21010bae 100644
>> --- a/src/mesa/drivers/dri/i965/Makefile.sources
>> +++ b/src/mesa/drivers/dri/i965/Makefile.sources
>> @@ -14,6 +14,7 @@ i965_FILES = \
>>     brw_cs.h \
>>     brw_curbe.c \
>>     brw_defines.h \
>> +    brw_disk_cache.cpp \
>>     brw_draw.c \
>>     brw_draw.h \
>>     brw_draw_upload.c \
>> diff --git a/src/mesa/drivers/dri/i965/brw_disk_cache.c 
>> b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> new file mode 100644
>> index 0000000000..b56e561e14
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> @@ -0,0 +1,395 @@
>> +/*
>> + * Copyright © 2014 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial portions 
>> of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT 
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + */
>> +
>> +#include "compiler/glsl/blob.h"
>> +#include "compiler/glsl/ir_uniform.h"
>> +#include "compiler/glsl/shader_cache.h"
>> +#include "main/mtypes.h"
>> +#include "util/disk_cache.h"
>> +#include "util/macros.h"
>> +#include "util/mesa-sha1.h"
>> +
>> +#include "brw_context.h"
>> +#include "brw_state.h"
>> +#include "brw_vs.h"
>> +#include "brw_wm.h"
>> +
>> +static uint64_t
>> +ptr_to_uint64_t(void *ptr)
>> +{
>> +   uint64_t ptr_int = (uint64_t) ptr;
>> +#if __i386__
>> +   ptr_int &= 0xFFFFFFFF;
>> +#endif
>> +   return ptr_int;
>> +}
> 
> This function can be as simple as
> 
> static uint64_t
> ptr_to_uint64_t(void *ptr)
> {
>    return (uintptr_t) ptr;
> }
> 
> But I have a question. We don't expect to be able to load a shader with
> a 64-bit Mesa that was generated on a 32-bit Mesa (or vice versa)
> because we're using the build id to determine compatibility.
> 
> Do we need to write out all pointers as 64-bits, or should we really
> just be writing out uintptr_t? Maybe having the binary format the same
> keeps something simple?

People insisted this be stored as 64-bit even though I argued that it 
was a bad idea to share cache items between the Mesa builds, that simple 
fact that we will never test this path in our automated tests is as good 
a reason as any not to do it. Currently the disk_cache util uses ptr 
size as part of the drivers key so we should never share cache items 
between builds currently.  I changed it to always store as 64bit here 
anyway in order to move the conversation along and try to get the series 
reviewed. I think at this point we could drop it.