Re: [Mesa-dev] [v2 6/6] mesa: OES_get_program_binary functionality

[Tapani Pälli]

On 11/01/2013 12:38 PM, Erik Faye-Lund wrote:
On Fri, Nov 1, 2013 at 11:35 AM, Tapani Pälli <tapani.pa...@intel.com> wrote:
On 11/01/2013 12:21 PM, Erik Faye-Lund wrote:
On Fri, Nov 1, 2013 at 10:16 AM, Tapani Pälli <tapani.pa...@intel.com>
wrote:
Signed-off-by: Tapani Pälli <tapani.pa...@intel.com>
---
   src/mesa/main/shaderapi.c | 46
+++++++++++++++++++++++++++++++++++++++-------
   1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
index 7da860d..67aee6b 100644
--- a/src/mesa/main/shaderapi.c
+++ b/src/mesa/main/shaderapi.c
@@ -57,6 +57,8 @@
   #include "../glsl/ir.h"
   #include "../glsl/ir_uniform.h"
   #include "../glsl/program.h"
+#include "../glsl/ir_cache.h"
+#include "git_sha1.h"

   /** Define this to enable shader substitution (see below) */
   #define SHADER_SUBST 0
@@ -212,7 +214,6 @@ attach_shader(struct gl_context *ctx, GLuint program,
GLuint shader)
      struct gl_shader_program *shProg;
      struct gl_shader *sh;
      GLuint i, n;
-
      const bool same_type_disallowed = _mesa_is_gles(ctx);

      shProg = _mesa_lookup_shader_program_err(ctx, program,
"glAttachShader");
@@ -1630,8 +1631,26 @@ _mesa_GetProgramBinary(GLuint program, GLsizei
bufSize, GLsizei *length,
      if (length != NULL)
         *length = 0;

-   (void) binaryFormat;
-   (void) binary;
+   size_t size = 0;
+   char *data = _mesa_program_serialize(shProg, &size, MESA_GIT_SHA1);
+
+   /* we have more data that can fit to user given buffer */
+   if (size > bufSize) {
+      _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);
+      if (data)
+         free(data);
+      return;
+   }
+
+   if (data) {
+      memcpy(binary, data, size);
+      free(data);
+   }
+
+   if (length != NULL)
+      *length = size;
+
+   *binaryFormat = 0;
   }

   void GLAPIENTRY
@@ -1645,10 +1664,23 @@ _mesa_ProgramBinary(GLuint program, GLenum
binaryFormat,
      if (!shProg)
         return;

-   (void) binaryFormat;
-   (void) binary;
-   (void) length;
-   _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);
+   if (length <= 0)
+      return;
+
+   /* free possible existing data and initialize structure */
+   _mesa_free_shader_program_data(ctx, shProg);
+   _mesa_init_shader_program(ctx, shProg);
+
+   /* fill structure from a binary blob */
+   if (_mesa_program_deserialize(shProg, binary, length, MESA_GIT_SHA1))
{

Won't using the git-sha1 as a compatibility-criteria cause issues for
developers with local changes? I'm not so worried about this for
OES_get_program_binary itself, but once the shader-cache is in place
it sounds like a potential source of difficult to track down
misbehavior...

I agree it might be too aggressive criteria but it is hard to come up with
better and as simple.
That's not my objection. My objection is that this might give
headaches for people with local modifications to the glsl-compiler.
Local modifications does not affect the git-sha1.

For the automatic shader cache this headache could be helped a bit with 
a environment variable or drirc setting that can be used during 
development. On the other hand an automatic cache must work in a 
transparent way so it should be always able to recover when it fails, so 
one should only see it as 'slower than usual' (since 
recompilation/relink required) sort of behaviour. The WIP of the 
automatic cache I sent some time earlier also marked (renamed) these 
'problematic' cached shaders so that they can be detected on further 
runs and cache can ignore those.

I agree that it might become problematic, on the other hand it is also 
easy to just wipe ~/.cache/mesa and disable cache. Not sure if Nvidia or 
Imagination try to handles these cases with their cache implementations.

// Tapani

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev