[Mesa-dev] [PATCH v2 16/17] i965: Add ARB_get_program_binary support using nir_serialization

[Emil Velikov]

Hi Jordan,

There's a small question below and a few nitpicks.
Latter can be quite subjective, so please don't bother with them if
you don't like the approach.

On 20 November 2017 at 22:27, Jordan Justen <jordan.l.justen at intel.com> wrote:

> +void
> +brw_program_binary_init(unsigned device_id)
> +{
> +   const struct build_id_note *note =
> +      build_id_find_nhdr_for_addr(brw_program_binary_init);
> +   assert(note);
> +
> +   /**
> +    * With Mesa's megadrivers, taking the sha1 of i965_dri.so may not be
> +    * unique. Therefore, we make a sha1 of the "i965" string and the sha1
> +    * build id from i965_dri.so.
> +    */
> +   struct mesa_sha1 ctx;
> +   _mesa_sha1_init(&ctx);
> +   char renderer[10];
> +   assert(device_id < 0x10000);
> +   int len = snprintf(renderer, sizeof(renderer), "i965_%04x", device_id);
> +   assert(len == sizeof(renderer) - 1);
> +   _mesa_sha1_update(&ctx, renderer, len);
> +   _mesa_sha1_update(&ctx, build_id_data(note), build_id_length(note));
> +   _mesa_sha1_final(&ctx, driver_sha1);

This function seems identical to the hunk in brw_disk_cache.c, correct?
Modulo the ENABLE_SHADER_CACHE guard, which we can drop since i965 is
built on !Windows - aka the cache is always available.


> +static bool
> +read_program_payload(struct gl_context *ctx, struct blob_reader *blob,
> +                     GLenum binary_format, struct gl_shader_program *sh_prog)
> +{
> +   bool deserialized;
> +
> +   deserialized = deserialize_glsl_program(blob, ctx, sh_prog);
> +
> +   if (!deserialized)
> +      return false;
> +
> +   unsigned int stage;
> +   for (stage = 0; stage < ARRAY_SIZE(sh_prog->_LinkedShaders); stage++) {
> +      struct gl_linked_shader *shader = sh_prog->_LinkedShaders[stage];
> +      if (!shader)
> +         continue;
> +
> +      brw_program_deserialize_nir(ctx, shader->Program, stage);
> +   }
> +
> +   return deserialized;
Nit: one could drop the temporary variable deserialized, making the
function shorted and easier to read.

> +}
> +
> +void
> +brw_get_program_binary_length(struct gl_context *ctx,
> +                              struct gl_shader_program *sh_prog,
> +                              GLint *length)
> +{
> +   struct blob blob;
> +   blob_init_fixed(&blob, NULL, SIZE_MAX);
> +   write_program_payload(ctx, &blob, sh_prog);
> +   *length = get_program_binary_header_size() + blob.size;
Nit: use blob_finish to pair the blob_init_fixed calls for consistency
with rest of Mesa?
Yes, calling blob_finish is a noop in that case.

> +extern void
> +brw_program_binary(struct gl_context *ctx, struct gl_shader_program *sh_prog,
> +                   GLenum binary_format, const GLvoid *binary, GLsizei length)
> +{
> +   void *extracted_payload = NULL;
> +   unsigned header_size = get_program_binary_header_size();
> +   const void *payload = get_program_binary_payload(binary_format, driver_sha1,
> +                                                    binary, length);
> +   bool payload_ok = false;
> +
> +   if (payload != NULL) {
> +      struct blob_reader blob;
> +      blob_reader_init(&blob, payload, length - header_size);
> +      payload_ok = read_program_payload(ctx, &blob, binary_format, sh_prog);
> +   }
> +
> +   if (payload_ok) {
> +      /* Reset uniforms to initial values as required by extension spec. */
> +      struct gl_shader_program_data *data = sh_prog->data;
> +      unsigned size =
> +         sizeof(union gl_constant_value) * data->NumUniformDataSlots;
> +      memcpy(data->UniformDataSlots, data->UniformDataDefaults, size);
> +
> +      sh_prog->data->LinkStatus = linking_success;
> +   } else {
> +      sh_prog->data->LinkStatus = linking_failure;
> +   }
> +
> +   if (extracted_payload)
> +      ralloc_free(extracted_payload);
> +}

Nit: the following approach will make the function a bit easier to read.

   if (payload == NULL)
      return;

   struct blob_reader blob;
   blob_reader_init(&blob, payload, length - header_size);

   if (!read_program_payload(ctx, &blob, binary_format, sh_prog))
      sh_prog->data->LinkStatus = linking_failure;
      return;
   }

   /* Reset uniforms to initial values as required by extension spec. */
   struct gl_shader_program_data *data = sh_prog->data;
   unsigned size = sizeof(union gl_constant_value) * data->NumUniformDataSlots;
   memcpy(data->UniformDataSlots, data->UniformDataDefaults, size);

   sh_prog->data->LinkStatus = linking_success;
}


HTH
Emil