<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span dir="ltr"><<a href="mailto:tapani.palli@intel.com" target="_blank">tapani.palli@intel.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">diff --git a/src/glsl/shader_cache.cpp b/src/glsl/shader_cache.cpp<br>
new file mode 100644<br>
index 0000000..4b5de45<br>
--- /dev/null<br>
+++ b/src/glsl/shader_cache.cpp<br>
@@ -0,0 +1,489 @@<br>
+/* -*- c++ -*- */<br>
+/*<br>
+ * Copyright © 2013 Intel Corporation<br>
+ *<br>
+ * Permission is hereby granted, free of charge, to any person obtaining a<br>
+ * copy of this software and associated documentation files (the "Software"),<br>
+ * to deal in the Software without restriction, including without limitation<br>
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,<br>
+ * and/or sell copies of the Software, and to permit persons to whom the<br>
+ * Software is furnished to do so, subject to the following conditions:<br>
+ *<br>
+ * The above copyright notice and this permission notice (including the next<br>
+ * paragraph) shall be included in all copies or substantial portions of the<br>
+ * Software.<br>
+ *<br>
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR<br>
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,<br>
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL<br>
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER<br>
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING<br>
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER<br>
+ * DEALINGS IN THE SOFTWARE.<br>
+ */<br>
+<br>
+#include "main/shaderobj.h"<br>
+#include "main/uniforms.h"<br>
+#include "main/macros.h"<br>
+#include "program/hash_table.h"<br>
+#include "ir_serialize.h"<br>
+#include "ir_deserializer.h"<br>
+#include "shader_cache_magic.h"<br>
+<br>
+/**<br>
+ * It is currently unknown if we need these to be available.<br>
+ * There can be calls in mesa/main (like glGetAttachedShaders) that use<br>
+ * the Shaders list.<br>
+ */<br>
+const bool STORE_UNLINKED_SHADERS = false;<br></blockquote><div><br></div><div>I think it really exaggerates our level of uncertainty to say that it is "currently unknown" whether we need unlinked shaders to be available. Speaking for myself at least, I'm quite convinced that we don't. AFAICT, OES_get_program_binary was purposefully architected so taht we don't need to store the unlinked shaders. GL has always maintained two independent collections of state about any given program: pre-link state and post-link state. There's a one-way flow of information from the pre-link state to the post-link state--once you've linked a program using glLinkShader(), there's no way to get the unlinked information back. This isn't a problem for most users of GL because most people don't try to modify the pre-link state after linking the program. However it's perfectly permissible for someone to create a program, attach shaders, link, and then detatch all the shaders. Since detatching the shaders affects pre-link state, the program would still work after this point (since the post-link information would not have changed since the link) but it would be impossible to query information about the individual shaders anymore.<br>
<br></div><div>A similar situation exists when the client uses OES_get_attached_shaders. glProgramBinaryOES() bypasses the pre-link state completely, and drops the compiled binary directly into the post-link state. From the OES_get_program_binary spec:<br>
<br> Note that ProgramBinaryOES disregards any shader objects attached to the<br> program object, as these shader objects are used only by LinkProgram.<br><br></div><div>And later:<br><br> 7. Can BindAttribLocation be called after ProgramBinaryOES to remap an<br>
attribute location used by the program binary?<br><br> RESOLVED: No. BindAttribLocation only affects the result of a subsequent<br> call to LinkProgram. LinkProgram operates on the attached shader objects <br>
and replaces any program binary loaded prior to LinkProgram. So there is no<br> mechanism to remap an attribute location after loading a program binary.<br><br> However, an application is free to remap an attribute location prior to<br>
retrieving the program binary. By calling BindAttribLocation followed by<br> LinkProgram, an application can remap the attribute location. If this is<br> followed by a call to GetProgramBinaryOES, the retrieved program binary will<br>
include the desired attribute location assignment.<br></div><div><br>So if the user creates a program and calls glProgramBinaryOES() followed by glGetAttachedShaders(), they will see no shaders, since the pre-link state is still in its initial configuration of having no shaders attached.<br>
<br></div>IMHO, trying to plan for the contingency case where we're wrong about this is just going to lead to confusion and make the code hard to maintain. I think we should drop this const, and remove the code that would have been executed if STORE_UNLINKED_SHADERS were true. In the unlikely event that it turns out we were wrong about this (or Khronos makes a change to the spec that makes it necessary to store unlinked shaders) we can always change the code in a future patch.<br>
<br>Also, incidentally, the way you've used STORE_UNLINKED_SHADERS wouldn't have worked anyway. You've defined it as a const bool (which is evaluated by the compiler) but later when you try to conditionally compile based on it, you use the preprocessor. Since the preprocessor operates on the code before the rest of the compiler, it doesn't know about the const declaration above--even if you changed it to true, #ifdef STORE_UNLINKED_SHADERS would still evaluate to false.<br>
<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+static void<br>
+write_header(gl_shader *shader, memory_writer &blob)<br>
+{<br>
+ GET_CURRENT_CONTEXT(ctx);<br>
+<br>
+ blob.write_string(mesa_get_shader_cache_magic());<br>
+ blob.write_string((const char *)ctx->Driver.GetString(ctx, GL_VENDOR));<br>
+ blob.write_string((const char *)ctx->Driver.GetString(ctx, GL_RENDERER));<br>
+ blob.write_uint32_t(shader->Version);<br>
+ blob.write_uint32_t(shader->Type);<br>
+ blob.write_uint8_t(shader->IsES);<br>
+<br>
+ /* post-link data */<br>
+ blob.write_uint32_t(shader->num_samplers);<br>
+ blob.write_uint32_t(shader->active_samplers);<br>
+ blob.write_uint32_t(shader->shadow_samplers);<br>
+ blob.write_uint32_t(shader->num_uniform_components);<br>
+ blob.write_uint32_t(shader->num_combined_uniform_components);<br>
+<br>
+ uint8_t uses_builtins = shader->uses_builtin_functions;<br>
+ blob.write_uint8_t(uses_builtins);<br>
+<br>
+ blob.write(&shader->Geom, sizeof(shader->Geom));<br></blockquote><div><br></div><div>It seems a little strange to see geometry-shader-specific information being dumped into the binary, since OES_get_program_binary is a GLES extension, and GLES doesn't support geometry shaders. I assume you are doing this because you also want this code to work for ARB_get_program_binary? If so it would be nice to have a comment explaining that.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ for (unsigned i = 0; i < MAX_SAMPLERS; i++)<br>
+ blob.write_uint8_t(shader->SamplerUnits[i]);<br>
+<br>
+ for (unsigned i = 0; i < MAX_SAMPLERS; i++) {<br>
+ int32_t target = shader->SamplerTargets[i];<br>
+ blob.write_int32_t(target);<br>
+ }<br>
+}<br>
+<br>
+<br>
+<br>
+<br>
+static void<br>
+serialize_uniform_storage(gl_uniform_storage *uni, memory_writer &blob)<br></blockquote><div><br></div><div>I don't think this is right. The ARB_get_program_binary spec says:<br><br> 8. How does ProgramBinary interact with uniform values, including<br>
shader-specified defaults?<br> <br> RESOLVED: All uniforms specified in the binary are reset to their shader-<br> specified defaults, or to zero if they aren't specified, when the program<br> binary is loaded. The spec language has been updated to specify this<br>
behavior.<br><br></div><div>The OES_get_program_binary spec doesn't mention uniforms at all, but I believe this is not intended to indicate a behavioural difference--it's just a consequence of the fact that ARB_get_program_binary was written later, so they had a chance to clarify things. In fact, issue #7 in ARB_get_program_binary specifically says:<br>
<br> 7. How does this spec differ from the OES_get_program_binary and why?<br><br> ...<br><br> There are other areas where language was clarified, but this is meant to<br> be consistent with the intent of the original specification and not to<br>
alter previously established behavior.<br><br></div><div>So I believe we shouldn't be serializing uniform values.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+static void<br>
+read_hash_table(struct string_to_uint_map *hash, memory_map *map)<br>
+{<br>
+ uint32_t size = map->read_uint32_t();<br>
+<br>
+ for (unsigned i = 0; i < size; i++) {<br></blockquote><div><br></div><div>Security issue: we need to bail out of this loop if we try to read beyond the end of the binary. Otherwise a bogus size could cause us to read from garbage memory for a very long time.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ char *key = map->read_string();<br>
+ uint32_t value = map->read_uint32_t();<br>
+<br>
+ /* put() adds +1 bias on the value (see hash_table.h), this<br>
+ * is taken care here when reading<br>
+ */<br>
+ hash->put(value-1, key);<br>
+ }<br>
+}<br>
+<br>
+<br>
+static void<br>
+read_uniform_storage(void *mem_ctx, gl_uniform_storage *uni, memory_map &map)<br></blockquote><div><br></div><div>As with serialize_uniform_storage(), I believe this is not necessary.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+/**<br>
+ * Deserialize gl_shader_program structure<br>
+ */<br>
+extern "C" int<br>
+mesa_program_deserialize(struct gl_shader_program *prog,<br>
+ const GLvoid *blob, size_t size)<br>
+{<br>
+ memory_map map;<br>
+ map.map((const void*)blob, size);<br>
+<br>
+ prog->Type = map.read_uint32_t();<br>
+ prog->NumShaders = map.read_uint32_t();<br>
+ prog->LinkStatus = map.read_uint8_t();<br>
+ prog->Version = map.read_uint32_t();<br>
+ prog->IsES = map.read_uint8_t();<br>
+ prog->NumUserUniformStorage = map.read_uint32_t();<br>
+ prog->UniformLocationBaseScale = map.read_uint32_t();<br>
+ prog->LastClipDistanceArraySize = map.read_uint32_t();<br>
+ prog->FragDepthLayout = (gl_frag_depth_layout) map.read_uint8_t();<br>
+<br>
+ prog->UniformStorage = NULL;<br>
+ prog->Label = NULL;<br>
+<br>
+ /* these already allocated by _mesa_init_shader_program */<br>
+ read_hash_table(prog->AttributeBindings, &map);<br>
+ read_hash_table(prog->FragDataBindings, &map);<br>
+ read_hash_table(prog->FragDataIndexBindings, &map);<br>
+<br>
+ prog->UniformHash = new string_to_uint_map;<br>
+ read_hash_table(prog->UniformHash, &map);<br>
+<br>
+ map.read(&prog->Geom, sizeof(prog->Geom));<br>
+ map.read(&prog->Vert, sizeof(prog->Vert));<br>
+<br>
+ /* just zero for now */<br>
+ prog->LinkedTransformFeedback.Outputs = NULL;<br>
+ prog->LinkedTransformFeedback.Varyings = NULL;<br>
+ prog->LinkedTransformFeedback.NumVarying = 0;<br>
+ prog->LinkedTransformFeedback.NumOutputs = 0;<br></blockquote><div><br></div><div>Did you forget to finish writing this code? We need to serialize this stuff, since it's part of post-link state.<br></div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* uniform storage */<br>
+ prog->UniformStorage = rzalloc_array(prog, struct gl_uniform_storage,<br>
+ prog->NumUserUniformStorage);<br>
+<br>
+ for (unsigned i = 0; i < prog->NumUserUniformStorage; i++)<br>
+ read_uniform_storage(prog, &prog->UniformStorage[i], map);<br>
+<br>
+ GET_CURRENT_CONTEXT(ctx);<br>
+<br>
+#if (STORE_UNLINKED_SHADERS)<br>
+ /* Shaders array (unlinked */<br>
+ prog->Shaders = (struct gl_shader **)<br>
+ _mesa_realloc(prog->Shaders, 0,<br>
+ (prog->NumShaders) * sizeof(struct gl_shader *));<br>
+<br>
+ for (unsigned i = 0; i < prog->NumShaders; i++) {<br>
+<br>
+ struct gl_shader *sha = read_shader(prog, map);<br>
+<br>
+ if (sha) {<br>
+ prog->Shaders[i] = NULL; /* alloc did not initialize */<br>
+ _mesa_reference_shader(ctx, &prog->Shaders[i], sha);<br>
+ CACHE_DEBUG("%s: read unlinked shader, index %d (%p) size %d\n",<br>
+ __func__, i, sha, shader_size);<br>
+ }<br>
+ }<br>
+#else<br>
+ prog->Shaders = NULL;<br>
+ prog->NumShaders = 0;<br></blockquote><div><br></div><div>This is not correct. Shader objects are part of pre-link state. That means that glProgramBinaryOES() should ignore them and leave them unchanged. It shouldn't throw out any attached shaders.<br>
<br></div><div>Incidentally, it occurs to me that maybe we should do a refactor at the start of this series that splits gl_shader_program into two parts:<br><br></div><div>struct gl_shader_program {<br></div><div> struct {<br>
...<br></div><div> } PreLink;<br></div><div> struct {<br> ...<br></div><div> } PostLink;<br>};<br><br></div><div>That way future maintainers won't have to rely on deep understanding of the GL spec to keep track of which program state is pre-link and which is post-link. They can just look in mtypes.h. <br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+#endif<br>
+<br>
+ /* init list, cache can contain only some shader types */<br>
+ for (unsigned i = 0; i < MESA_SHADER_TYPES; i++)<br>
+ prog->_LinkedShaders[i] = NULL;<br>
+<br>
+ /* read _LinkedShaders */<br>
+ while(map.position() < size) {<br>
+ uint32_t index = map.read_uint32_t();<br>
+<br>
+ struct gl_shader *sha = read_shader(prog, map);<br>
+<br>
+ /* note, for 'automatic cache' in case of failure we would<br>
+ * need to fallback to compiling/linking the shaders in the<br>
+ * prog->Shaders list<br>
+ */<br>
+ if (!sha) {<br>
+ CACHE_DEBUG("failed to read shader (index %d)\n", index);<br>
+ return -1;<br>
+ }<br>
+<br>
+ resolve_uniform_types(prog, sha);<br>
+<br>
+ _mesa_reference_shader(ctx, &prog->_LinkedShaders[index], sha);<br>
+ CACHE_DEBUG("%s: read a linked shader, index %d (%p)\n",<br>
+ __func__, index, sha);<br>
+ }<br>
+<br>
+ return 0;<br>
+}<br></blockquote></div></div></div>