[Mesa-dev] [wip 9/9] mesa: OES_get_program_binary extension functionality

[Tapani Pälli]

On 01/15/2014 06:13 PM, Paul Berry wrote:
> On 2 January 2014 03:58, Tapani Pälli <tapani.palli at intel.com 
> <mailto:tapani.palli at intel.com>> wrote:
>
>     Signed-off-by: Tapani Pälli <tapani.palli at intel.com
>     <mailto:tapani.palli at 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20140116/ef2e06f3/attachment.html>