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 <[email protected]> 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 _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
