<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body bgcolor="#FFFFFF" text="#000000">
<div class="moz-cite-prefix">On 01/15/2014 06:13 PM, Paul Berry
wrote:<br>
</div>
<blockquote
cite="mid:CA+yLL66znuNooFANUZ-6=A=O34BvEFeL+QrYd-B4j4SjG9taGg@mail.gmail.com"
type="cite">
<meta http-equiv="Content-Type" content="text/html;
charset=ISO-8859-1">
<div dir="ltr">On 2 January 2014 03:58, Tapani Pälli <span
dir="ltr"><<a moz-do-not-send="true"
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">Signed-off-by: Tapani
Pälli <<a moz-do-not-send="true"
href="mailto:tapani.palli@intel.com">tapani.palli@intel.com</a>><br>
---<br>
src/mesa/main/shaderapi.c | 44
++++++++++++++++++++++++++++++++++++++------<br>
1 file changed, 38 insertions(+), 6 deletions(-)<br>
<br>
+ char *data = mesa_program_serialize(shProg,
&size);<br>
+<br>
+ /* we have more data that can fit to user given buffer
*/<br>
+ if (size > bufSize) {<br>
+ _mesa_error(ctx, GL_INVALID_OPERATION,
__FUNCTION__);<br>
+ if (data)<br>
+ free(data);<br>
</blockquote>
<div><br>
</div>
<div>Why would we ever expect mesa_program_serialize to set
size to a nonzero value but return NULL? It seems like
this could only happen if there's a bug, in which case
this really ought to be<br>
<br>
</div>
<div>assert(data !=NULL);<br>
<br>
</div>
<div>Also, it's safe to call free() on a NULL pointer.
According to the C standard, freeing a NULL pointer does
nothing.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
sure, will fix<br>
<br>
<blockquote
cite="mid:CA+yLL66znuNooFANUZ-6=A=O34BvEFeL+QrYd-B4j4SjG9taGg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ return;<br>
+ }<br>
+<br>
+ if (data) {<br>
</blockquote>
<div><br>
</div>
<div>Similarly, this if-statement is unnecessary.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
it is required for memcpy but not for free<br>
<br>
<blockquote
cite="mid:CA+yLL66znuNooFANUZ-6=A=O34BvEFeL+QrYd-B4j4SjG9taGg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex">
+ memcpy(binary, data, size);<br>
+ free(data);<br>
+ }<br>
+<br>
+ if (length != NULL)<br>
+ *length = size;<br>
+<br>
+ *binaryFormat = 0;<br>
}<br>
<br>
void GLAPIENTRY<br>
@@ -1647,10 +1666,23 @@ _mesa_ProgramBinary(GLuint
program, GLenum binaryFormat,<br>
if (!shProg)<br>
return;<br>
<br>
- (void) binaryFormat;<br>
- (void) binary;<br>
- (void) length;<br>
- _mesa_error(ctx, GL_INVALID_OPERATION, __FUNCTION__);<br>
+ if (length <= 0)<br>
+ return;<br>
</blockquote>
<div><br>
</div>
<div>In the case of an invalid length, we need to make sure
to set the program's LinkStatus to false. Also, the
information log needs to be cleared, in accordance with
this text from OES_get_program_binary:<br>
<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK, I'll make a common error handling for this and case you mention
below ..<br>
<br>
<blockquote
cite="mid:CA+yLL66znuNooFANUZ-6=A=O34BvEFeL+QrYd-B4j4SjG9taGg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<div> If ProgramBinaryOES failed, any information about a
previous link or load of<br>
that program object is lost. Thus, a failed load does
not restore the old<br>
state of <program>.<br>
<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>
+ /* free possible existing data and initialize
structure */<br>
+ _mesa_free_shader_program_data(ctx, shProg);<br>
+ _mesa_init_shader_program(ctx, shProg);<br>
+<br>
+ /* fill structure from a binary blob */<br>
+ if (mesa_program_deserialize(shProg, binary, length))
{<br>
+ _mesa_error(ctx, GL_INVALID_VALUE,
"glProgramBinary(binary incompatible)");<br>
</blockquote>
<div><br>
</div>
<div>This seems wrong to me. From the
OES_get_program_binary spec:<br>
<br>
... <binaryFormat> and <binary> must be<br>
those returned by a previous call to
GetProgramBinaryOES, and <length> must<br>
be the length of the program binary as returned by
GetProgramBinaryOES or<br>
GetProgramiv with <pname>
PROGRAM_BINARY_LENGTH_OES. The program binary<br>
will fail to load if these conditions are not met.<br>
<br>
...<br>
<br>
A program object's program binary is replaced by calls
to LinkProgram or<br>
ProgramBinaryOES. Either command sets the program
object's LINK_STATUS to<br>
TRUE or FALSE, as queried with GetProgramiv, to
reflect success or failure.<br>
Either command also updates its information log,
queried with<br>
GetProgramInfoLog, to provide details about warnings
or errors.<br>
<br>
</div>
<div>I believe this means that if deserialization fails, it
should not be a GL error. It should simply be treated as
a link failure.<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">
+ return;<br>
+ }<br>
+<br>
+ /* driver specific link, optimizations and what not */<br>
+ ctx->Driver.LinkShader(ctx, shProg);<br>
</blockquote>
<div><br>
</div>
<div>Now I'm confused. I thought a major part of the
purpose of this extension was that it would store the
post-link program, so that not only does glProgramBinary()
avoid the runtime penalty of parsing and compiling, it
also avoids the runtime penalty of link-time
optimizations. Calling ctx->Driver.LinkShader seems
like it defeats that purpose. It seems like what we ought
to be doing is store the data that
ctx->Driver.LinkShader *produces* in the binary blob,
so that once it's loaded there's no further linking
necessary. If there is a small amount of driver-specific
hook necessary, that should be placed in a new
ctx->Driver function rather than incurring the overhead
of another link.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
This is the driver specific parts which is unfortunately not yet
there. I'm working on adding data stored in 'state_cache' (program,
prog_data and keys) from i965 driver within the blob so that I could
avoid the call to the driver. It could be that driver generated IR
and possibly some other structures need caching or recreation as
well to skip the whole call (now experiencing some gpu hangs when
restoring only the program and prog_data parts), another hook might
be needed to cover this. I'm currently studying the i965 driver to
understand what is required.<br>
<br>
So what's in the blob is:<br>
<br>
* some validation data<br>
* gl_shader_program uniform and variable hashes<br>
* shader type + misc data for gl_shader structure<br>
* mesa generated ir (gl_shader->ir) for each shader stage<br>
<br>
Additionally the plan would be to add:<br>
<br>
* brw_shader -> precompile vs, fs, gs (program, prog_data
structure (+ param & pull_param?), cache keys<br>
* i965 driver generated ir (brw_shader->ir) if required<br>
<br>
<br>
<blockquote
cite="mid:CA+yLL66znuNooFANUZ-6=A=O34BvEFeL+QrYd-B4j4SjG9taGg@mail.gmail.com"
type="cite">
<div dir="ltr">
<div class="gmail_extra">
<div class="gmail_quote">
<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>
+ _mesa_ValidateProgram(program);<br>
</blockquote>
<div><br>
</div>
<div>I don't think this is correct. ValidateProgram()
doesn't mean "check that the program is well-formed". It
means "check whether the program can execute given the
current GL state". A lot of GL apps compile all their
shaders during startup, long before they've established
the correct GL state for running those programs. It's
reasonable to assume that apps using this extension will
make their calls to glProgramBinary() during startup as
well. So calling ValidateProgram() from glProgramBinary()
will just put unexpected bogus warnings into the info
log. Also, I don't see anything in either the
OES_get_program_binary or ARB_get_program_binary spec
saying we should do this.<br>
</div>
</div>
</div>
</div>
</blockquote>
<br>
OK I see, then some own sanity checks could be done here instead.<br>
<br>
// Tapani<br>
</body>
</html>