On 01/15/2014 06:13 PM, Paul Berry wrote:
On 2 January 2014 03:58, Tapani Pälli <tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>> wrote:
Signed-off-by: Tapani Pälli <tapani.pa...@intel.com
<mailto:tapani.pa...@intel.com>>
---
src/mesa/main/shaderapi.c | 44
++++++++++++++++++++++++++++++++++++++------
1 file changed, 38 insertions(+), 6 deletions(-)
+ char *data = mesa_program_serialize(shProg, &size);
+
+ /* 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);
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
assert(data !=NULL);
Also, it's safe to call free() on a NULL pointer. According to the C
standard, freeing a NULL pointer does nothing.
sure, will fix
+ return;
+ }
+
+ if (data) {
Similarly, this if-statement is unnecessary.
it is required for memcpy but not for free
+ memcpy(binary, data, size);
+ free(data);
+ }
+
+ if (length != NULL)
+ *length = size;
+
+ *binaryFormat = 0;
}
void GLAPIENTRY
@@ -1647,10 +1666,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;
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:
OK, I'll make a common error handling for this and case you mention below ..
If ProgramBinaryOES failed, any information about a previous link
or load of
that program object is lost. Thus, a failed load does not restore
the old
state of <program>.
+
+ /* 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_error(ctx, GL_INVALID_VALUE, "glProgramBinary(binary
incompatible)");
This seems wrong to me. From the OES_get_program_binary spec:
... <binaryFormat> and <binary> must be
those returned by a previous call to GetProgramBinaryOES, and
<length> must
be the length of the program binary as returned by
GetProgramBinaryOES or
GetProgramiv with <pname> PROGRAM_BINARY_LENGTH_OES. The program
binary
will fail to load if these conditions are not met.
...
A program object's program binary is replaced by calls to
LinkProgram or
ProgramBinaryOES. Either command sets the program object's
LINK_STATUS to
TRUE or FALSE, as queried with GetProgramiv, to reflect success or
failure.
Either command also updates its information log, queried with
GetProgramInfoLog, to provide details about warnings or errors.
I believe this means that if deserialization fails, it should not be a
GL error. It should simply be treated as a link failure.
+ return;
+ }
+
+ /* driver specific link, optimizations and what not */
+ ctx->Driver.LinkShader(ctx, shProg);
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.
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.
So what's in the blob is:
* some validation data
* gl_shader_program uniform and variable hashes
* shader type + misc data for gl_shader structure
* mesa generated ir (gl_shader->ir) for each shader stage
Additionally the plan would be to add:
* brw_shader -> precompile vs, fs, gs (program, prog_data structure (+
param & pull_param?), cache keys
* i965 driver generated ir (brw_shader->ir) if required
+
+ _mesa_ValidateProgram(program);
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.
OK I see, then some own sanity checks could be done here instead.
// Tapani
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
