Re: [Mesa-dev] [PATCH v2 18/32] i965: add initial implementation of on disk shader cache

[Jason Ekstrand]

I've got a meta-comment and then a couple more inline comments below before
I sign-off for the week-end.

Most of my comments on this patch have focussed towards two goals:

 1) As close to perfect paridy between read_program_data and
write_program_data as we can manage.  They should be almost identical
except for the direction the data flows.

 2) Reduced redundancy.  There are several places where we pass values or
read/write values that are already found in brw_stage_prog_data.  Let's
just use the ones in brw_stage_prog_data.  If we can't trust that, then
we're toast anyway.

Hopefully that will make the rest of my comments a bit more understandable.

On Fri, Oct 20, 2017 at 4:30 PM, Jason Ekstrand 
wrote:

> On Fri, Oct 20, 2017 at 4:05 PM, Jason Ekstrand 
> wrote:
>
>> On Wed, Oct 18, 2017 at 10:32 PM, Jordan Justen <
>> jordan.l.jus...@intel.com> wrote:
>>
>>> From: Timothy Arceri 
>>>
>>> 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.
>>>
>>> v2:
>>>  * Squash 'i965: add image param shader cache support'
>>>  * Squash 'i965: add shader cache support for pull param pointers'
>>>  * Sustantially simplified by a rework on top of Jason's 2975e4c56a7a.
>>>  * Rename load_program_data to read_program_data. (Jason)
>>>
>>> [jordan.l.jus...@intel.com: *_cached_program =>
>>> brw_disk_cache_*_program]
>>> [jordan.l.jus...@intel.com: brw_shader_cache.c => brw_disk_cache.c]
>>> [jordan.l.jus...@intel.com: don't map to write program when LLC is
>>> present]
>>> [jordan.l.jus...@intel.com: set program_written_to_cache on read from
>>> cache]
>>> [jordan.l.jus...@intel.com: only try cache when status is
>>> linking_skipped]
>>> [jordan.l.jus...@intel.com: rework based on uniforms rework
>>> 2975e4c56a7a]
>>> Signed-off-by: Jordan Justen 
>>> ---
>>>  src/mesa/drivers/dri/i965/Makefile.sources |   1 +
>>>  src/mesa/drivers/dri/i965/brw_disk_cache.c | 357
>>> +
>>>  src/mesa/drivers/dri/i965/brw_state.h  |   5 +
>>>  src/mesa/drivers/dri/i965/meson.build  |   1 +
>>>  4 files changed, 364 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 053d89b81e..2980cdb3c5 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.c \
>>> 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 00..6fe39a7997
>>> --- /dev/null
>>> +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>>> @@ -0,0 +1,357 @@
>>> +/*
>>> + * 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/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 size_t
>>> +key_size(gl_shader_stage stage)
>>> +{
>>> +   switch (stage) {
>>> +   case MESA_SHADER_VERTEX:
>>> +  return sizeof(struct brw_vs_prog_key);
>>> +   case 

Re: [Mesa-dev] [PATCH v2 18/32] i965: add initial implementation of on disk shader cache

[Jason Ekstrand]

On Fri, Oct 20, 2017 at 4:05 PM, Jason Ekstrand 
wrote:

> On Wed, Oct 18, 2017 at 10:32 PM, Jordan Justen  > wrote:
>
>> From: Timothy Arceri 
>>
>> 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.
>>
>> v2:
>>  * Squash 'i965: add image param shader cache support'
>>  * Squash 'i965: add shader cache support for pull param pointers'
>>  * Sustantially simplified by a rework on top of Jason's 2975e4c56a7a.
>>  * Rename load_program_data to read_program_data. (Jason)
>>
>> [jordan.l.jus...@intel.com: *_cached_program => brw_disk_cache_*_program]
>> [jordan.l.jus...@intel.com: brw_shader_cache.c => brw_disk_cache.c]
>> [jordan.l.jus...@intel.com: don't map to write program when LLC is
>> present]
>> [jordan.l.jus...@intel.com: set program_written_to_cache on read from
>> cache]
>> [jordan.l.jus...@intel.com: only try cache when status is
>> linking_skipped]
>> [jordan.l.jus...@intel.com: rework based on uniforms rework 2975e4c56a7a]
>> Signed-off-by: Jordan Justen 
>> ---
>>  src/mesa/drivers/dri/i965/Makefile.sources |   1 +
>>  src/mesa/drivers/dri/i965/brw_disk_cache.c | 357
>> +
>>  src/mesa/drivers/dri/i965/brw_state.h  |   5 +
>>  src/mesa/drivers/dri/i965/meson.build  |   1 +
>>  4 files changed, 364 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 053d89b81e..2980cdb3c5 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.c \
>> 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 00..6fe39a7997
>> --- /dev/null
>> +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
>> @@ -0,0 +1,357 @@
>> +/*
>> + * 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/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 size_t
>> +key_size(gl_shader_stage stage)
>> +{
>> +   switch (stage) {
>> +   case MESA_SHADER_VERTEX:
>> +  return sizeof(struct brw_vs_prog_key);
>> +   case MESA_SHADER_TESS_CTRL:
>> +  return sizeof(struct brw_tcs_prog_key);
>> +   case MESA_SHADER_TESS_EVAL:
>> +  return sizeof(struct brw_tes_prog_key);
>> +   case MESA_SHADER_GEOMETRY:
>> +  return sizeof(struct brw_gs_prog_key);
>> +   case MESA_SHADER_FRAGMENT:
>> +  return sizeof(struct brw_wm_prog_key);
>> +   case MESA_SHADER_COMPUTE:
>> +  return sizeof(struct brw_cs_prog_key);
>> +   default:
>> +  unreachable("Unsupported stage!");
>> +   }
>> +}
>>
>
> It might be worth putting this in brw_program.h and maybe adding one for
> prog_data as well.
>
>
>> +
>> +static void
>> +gen_shader_sha1(struct brw_context *brw, struct gl_program *prog,
>> +gl_shader_stage stage, void *key, unsigned char
>> *out_sha1)
>> +{
>> +   char sha1_buf[41];
>> +   unsigned char sha1[20];
>> +   char manifest[256];
>> +   int 

Re: [Mesa-dev] [PATCH v2 18/32] i965: add initial implementation of on disk shader cache

[Jason Ekstrand]

On Wed, Oct 18, 2017 at 10:32 PM, Jordan Justen 
wrote:

> From: Timothy Arceri 
>
> 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.
>
> v2:
>  * Squash 'i965: add image param shader cache support'
>  * Squash 'i965: add shader cache support for pull param pointers'
>  * Sustantially simplified by a rework on top of Jason's 2975e4c56a7a.
>  * Rename load_program_data to read_program_data. (Jason)
>
> [jordan.l.jus...@intel.com: *_cached_program => brw_disk_cache_*_program]
> [jordan.l.jus...@intel.com: brw_shader_cache.c => brw_disk_cache.c]
> [jordan.l.jus...@intel.com: don't map to write program when LLC is
> present]
> [jordan.l.jus...@intel.com: set program_written_to_cache on read from
> cache]
> [jordan.l.jus...@intel.com: only try cache when status is linking_skipped]
> [jordan.l.jus...@intel.com: rework based on uniforms rework 2975e4c56a7a]
> Signed-off-by: Jordan Justen 
> ---
>  src/mesa/drivers/dri/i965/Makefile.sources |   1 +
>  src/mesa/drivers/dri/i965/brw_disk_cache.c | 357
> +
>  src/mesa/drivers/dri/i965/brw_state.h  |   5 +
>  src/mesa/drivers/dri/i965/meson.build  |   1 +
>  4 files changed, 364 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 053d89b81e..2980cdb3c5 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.c \
> 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 00..6fe39a7997
> --- /dev/null
> +++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
> @@ -0,0 +1,357 @@
> +/*
> + * 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/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 size_t
> +key_size(gl_shader_stage stage)
> +{
> +   switch (stage) {
> +   case MESA_SHADER_VERTEX:
> +  return sizeof(struct brw_vs_prog_key);
> +   case MESA_SHADER_TESS_CTRL:
> +  return sizeof(struct brw_tcs_prog_key);
> +   case MESA_SHADER_TESS_EVAL:
> +  return sizeof(struct brw_tes_prog_key);
> +   case MESA_SHADER_GEOMETRY:
> +  return sizeof(struct brw_gs_prog_key);
> +   case MESA_SHADER_FRAGMENT:
> +  return sizeof(struct brw_wm_prog_key);
> +   case MESA_SHADER_COMPUTE:
> +  return sizeof(struct brw_cs_prog_key);
> +   default:
> +  unreachable("Unsupported stage!");
> +   }
> +}
>

It might be worth putting this in brw_program.h and maybe adding one for
prog_data as well.


> +
> +static void
> +gen_shader_sha1(struct brw_context *brw, struct gl_program *prog,
> +gl_shader_stage stage, void *key, unsigned char *out_sha1)
> +{
> +   char sha1_buf[41];
> +   unsigned char sha1[20];
> +   char manifest[256];
> +   int offset = 0;
> +
> +   _mesa_sha1_format(sha1_buf, prog->sh.data->sha1);
> +   offset += snprintf(manifest, sizeof(manifest), "program: %s\n",
> sha1_buf);
> +
> +   _mesa_sha1_compute(key, key_size(stage), sha1);
> +   

[Mesa-dev] [PATCH v2 18/32] i965: add initial implementation of on disk shader cache

[Jordan Justen]

From: Timothy Arceri 

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.

v2:
 * Squash 'i965: add image param shader cache support'
 * Squash 'i965: add shader cache support for pull param pointers'
 * Sustantially simplified by a rework on top of Jason's 2975e4c56a7a.
 * Rename load_program_data to read_program_data. (Jason)

[jordan.l.jus...@intel.com: *_cached_program => brw_disk_cache_*_program]
[jordan.l.jus...@intel.com: brw_shader_cache.c => brw_disk_cache.c]
[jordan.l.jus...@intel.com: don't map to write program when LLC is present]
[jordan.l.jus...@intel.com: set program_written_to_cache on read from cache]
[jordan.l.jus...@intel.com: only try cache when status is linking_skipped]
[jordan.l.jus...@intel.com: rework based on uniforms rework 2975e4c56a7a]
Signed-off-by: Jordan Justen 
---
 src/mesa/drivers/dri/i965/Makefile.sources |   1 +
 src/mesa/drivers/dri/i965/brw_disk_cache.c | 357 +
 src/mesa/drivers/dri/i965/brw_state.h  |   5 +
 src/mesa/drivers/dri/i965/meson.build  |   1 +
 4 files changed, 364 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 053d89b81e..2980cdb3c5 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.c \
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 00..6fe39a7997
--- /dev/null
+++ b/src/mesa/drivers/dri/i965/brw_disk_cache.c
@@ -0,0 +1,357 @@
+/*
+ * 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/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 size_t
+key_size(gl_shader_stage stage)
+{
+   switch (stage) {
+   case MESA_SHADER_VERTEX:
+  return sizeof(struct brw_vs_prog_key);
+   case MESA_SHADER_TESS_CTRL:
+  return sizeof(struct brw_tcs_prog_key);
+   case MESA_SHADER_TESS_EVAL:
+  return sizeof(struct brw_tes_prog_key);
+   case MESA_SHADER_GEOMETRY:
+  return sizeof(struct brw_gs_prog_key);
+   case MESA_SHADER_FRAGMENT:
+  return sizeof(struct brw_wm_prog_key);
+   case MESA_SHADER_COMPUTE:
+  return sizeof(struct brw_cs_prog_key);
+   default:
+  unreachable("Unsupported stage!");
+   }
+}
+
+static void
+gen_shader_sha1(struct brw_context *brw, struct gl_program *prog,
+gl_shader_stage stage, void *key, unsigned char *out_sha1)
+{
+   char sha1_buf[41];
+   unsigned char sha1[20];
+   char manifest[256];
+   int offset = 0;
+
+   _mesa_sha1_format(sha1_buf, prog->sh.data->sha1);
+   offset += snprintf(manifest, sizeof(manifest), "program: %s\n", sha1_buf);
+
+   _mesa_sha1_compute(key, key_size(stage), sha1);
+   _mesa_sha1_format(sha1_buf, sha1);
+   offset += snprintf(manifest + offset, sizeof(manifest) - offset,
+  "%s_key: %s\n", _mesa_shader_stage_to_abbrev(stage),
+  sha1_buf);
+
+   _mesa_sha1_compute(manifest, strlen(manifest), out_sha1);
+}
+
+static void
+read_program_data(struct gl_program *glprog, struct blob_reader *binary,
+  struct brw_stage_prog_data *prog_data,
+  struct