From 69c864b0b92da981b169cf879f7718e3d2c458c0 Mon Sep 17 00:00:00 2001 From: Boris Brezillon Date: Sat, 17 Oct 2020 12:08:17 +0200 Subject: [PATCH] panfrost: Make {midgard,bifrost}_compile_shader_nir() return a program object Letting the caller zero-initialize the program object is error prone, not to mention that resources attached to the program might not be freed by the caller. Let's simplify that by letting the compiler allocate the panfrost_program object. Those objects should be freed with ralloc_free(). Signed-off-by: Boris Brezillon Reviewed-by: Alyssa Rosenzweig Part-of: --- src/gallium/drivers/panfrost/pan_assemble.c | 33 ++++++++++--------- .../drivers/panfrost/pan_blend_shaders.c | 19 ++++++----- src/panfrost/bifrost/bi_pack.c | 2 -- src/panfrost/bifrost/bifrost_compile.c | 10 ++++-- src/panfrost/bifrost/bifrost_compile.h | 5 +-- src/panfrost/bifrost/cmdline.c | 8 ++--- src/panfrost/bifrost/test/bi_submit.c | 6 ++-- src/panfrost/bifrost/test/bi_test_pack.c | 5 +-- src/panfrost/bifrost/test/bit.h | 2 +- src/panfrost/lib/pan_blit.c | 24 +++++++------- src/panfrost/midgard/midgard_compile.c | 10 +++--- src/panfrost/midgard/midgard_compile.h | 4 +-- 12 files changed, 70 insertions(+), 58 deletions(-) diff --git a/src/gallium/drivers/panfrost/pan_assemble.c b/src/gallium/drivers/panfrost/pan_assemble.c index 6808a180a01..6b0466367b9 100644 --- a/src/gallium/drivers/panfrost/pan_assemble.c +++ b/src/gallium/drivers/panfrost/pan_assemble.c @@ -241,7 +241,6 @@ panfrost_shader_compile(struct panfrost_context *ctx, s->info.stage = stage; /* Call out to Midgard compiler given the above NIR */ - panfrost_program program = {0}; struct panfrost_compile_inputs inputs = { .gpu_id = dev->gpu_id, .shaderdb = !!(dev->debug & PAN_DBG_PRECOMPILE), @@ -249,19 +248,21 @@ panfrost_shader_compile(struct panfrost_context *ctx, memcpy(inputs.rt_formats, state->rt_formats, sizeof(inputs.rt_formats)); + panfrost_program *program; + if (dev->quirks & IS_BIFROST) - bifrost_compile_shader_nir(s, &program, &inputs); + program = bifrost_compile_shader_nir(NULL, s, &inputs); else - midgard_compile_shader_nir(s, &program, &inputs); + program = midgard_compile_shader_nir(NULL, s, &inputs); /* Prepare the compiled binary for upload */ mali_ptr shader = 0; unsigned attribute_count = 0, varying_count = 0; - int size = program.compiled.size; + int size = program->compiled.size; if (size) { state->bo = panfrost_bo_create(dev, size, PAN_BO_EXECUTE); - memcpy(state->bo->cpu, program.compiled.data, size); + memcpy(state->bo->cpu, program->compiled.data, size); shader = state->bo->gpu; } @@ -271,15 +272,13 @@ panfrost_shader_compile(struct panfrost_context *ctx, /* If size = 0, we tag as "end-of-shader" */ if (size) - shader |= program.first_tag; + shader |= program->first_tag; else shader = 0x1; } - util_dynarray_fini(&program.compiled); - - state->sysval_count = program.sysval_count; - memcpy(state->sysval, program.sysvals, sizeof(state->sysval[0]) * state->sysval_count); + state->sysval_count = program->sysval_count; + memcpy(state->sysval, program->sysvals, sizeof(state->sysval[0]) * state->sysval_count); bool vertex_id = s->info.system_values_read & (1 << SYSTEM_VALUE_VERTEX_ID); bool instance_id = s->info.system_values_read & (1 << SYSTEM_VALUE_INSTANCE_ID); @@ -303,11 +302,11 @@ panfrost_shader_compile(struct panfrost_context *ctx, break; case MESA_SHADER_FRAGMENT: for (unsigned i = 0; i < ARRAY_SIZE(state->blend_ret_addrs); i++) { - if (!program.blend_ret_offsets[i]) + if (!program->blend_ret_offsets[i]) continue; state->blend_ret_addrs[i] = (state->bo->gpu & UINT32_MAX) + - program.blend_ret_offsets[i]; + program->blend_ret_offsets[i]; assert(!(state->blend_ret_addrs[i] & 0x7)); } varying_count = util_bitcount64(s->info.inputs_read); @@ -340,7 +339,7 @@ panfrost_shader_compile(struct panfrost_context *ctx, state->can_discard = s->info.fs.uses_discard; state->helper_invocations = s->info.fs.needs_helper_invocations; - state->stack_size = program.tls_size; + state->stack_size = program->tls_size; state->reads_frag_coord = s->info.inputs_read & (1 << VARYING_SLOT_POS); state->reads_point_coord = s->info.inputs_read & (1 << VARYING_SLOT_PNTC); @@ -352,12 +351,12 @@ panfrost_shader_compile(struct panfrost_context *ctx, /* Separate as primary uniform count is truncated. Sysvals are prefix * uniforms */ - state->uniform_count = MIN2(s->num_uniforms + program.sysval_count, program.uniform_cutoff); - state->work_reg_count = program.work_register_count; + state->uniform_count = MIN2(s->num_uniforms + program->sysval_count, program->uniform_cutoff); + state->work_reg_count = program->work_register_count; if (dev->quirks & IS_BIFROST) for (unsigned i = 0; i < ARRAY_SIZE(state->blend_types); i++) - state->blend_types[i] = bifrost_blend_type_from_nir(program.blend_types[i]); + state->blend_types[i] = bifrost_blend_type_from_nir(program->blend_types[i]); /* Record the varying mapping for the command stream's bookkeeping */ @@ -395,6 +394,8 @@ panfrost_shader_compile(struct panfrost_context *ctx, if (stage != MESA_SHADER_FRAGMENT) pan_upload_shader_descriptor(ctx, state); + ralloc_free(program); + /* In both clone and tgsi_to_nir paths, the shader is ralloc'd against * a NULL context */ ralloc_free(s); diff --git a/src/gallium/drivers/panfrost/pan_blend_shaders.c b/src/gallium/drivers/panfrost/pan_blend_shaders.c index 470f9479567..24c0d6f7e5f 100644 --- a/src/gallium/drivers/panfrost/pan_blend_shaders.c +++ b/src/gallium/drivers/panfrost/pan_blend_shaders.c @@ -311,8 +311,6 @@ panfrost_compile_blend_shader(struct panfrost_blend_shader *shader, return; /* Compile or recompile the NIR shader */ - panfrost_program program; - struct panfrost_compile_inputs inputs = { .gpu_id = dev->gpu_id, .is_blend = true, @@ -323,19 +321,22 @@ panfrost_compile_blend_shader(struct panfrost_blend_shader *shader, if (constants) memcpy(inputs.blend.constants, constants, sizeof(inputs.blend.constants)); + panfrost_program *program; + if (dev->quirks & IS_BIFROST) { inputs.blend.bifrost_blend_desc = bifrost_get_blend_desc(shader->key.format, shader->key.rt); - bifrost_compile_shader_nir(shader->nir, &program, &inputs); + program = bifrost_compile_shader_nir(NULL, shader->nir, &inputs); } else { - midgard_compile_shader_nir(shader->nir, &program, &inputs); + program = midgard_compile_shader_nir(NULL, shader->nir, &inputs); } /* Allow us to patch later */ - shader->first_tag = program.first_tag; - shader->size = program.compiled.size; + shader->first_tag = program->first_tag; + shader->size = program->compiled.size; shader->buffer = reralloc_size(shader, shader->buffer, shader->size); - memcpy(shader->buffer, program.compiled.data, shader->size); - util_dynarray_fini(&program.compiled); - shader->work_count = program.work_register_count; + memcpy(shader->buffer, program->compiled.data, shader->size); + shader->work_count = program->work_register_count; + + ralloc_free(program); } diff --git a/src/panfrost/bifrost/bi_pack.c b/src/panfrost/bifrost/bi_pack.c index 0c9dd39459d..ed3c4bd59c9 100644 --- a/src/panfrost/bifrost/bi_pack.c +++ b/src/panfrost/bifrost/bi_pack.c @@ -1111,8 +1111,6 @@ bi_pack(bi_context *ctx, struct util_dynarray *emission) { bool tdd = bi_terminate_discarded_threads(ctx); - util_dynarray_init(emission, NULL); - bi_foreach_block(ctx, _block) { bi_block *block = (bi_block *) _block; diff --git a/src/panfrost/bifrost/bifrost_compile.c b/src/panfrost/bifrost/bifrost_compile.c index a09afd291a5..fc71e5c5786 100644 --- a/src/panfrost/bifrost/bifrost_compile.c +++ b/src/panfrost/bifrost/bifrost_compile.c @@ -1870,10 +1870,12 @@ bi_optimize_nir(nir_shader *nir) NIR_PASS(progress, nir, nir_convert_from_ssa, true); } -void -bifrost_compile_shader_nir(nir_shader *nir, panfrost_program *program, +panfrost_program * +bifrost_compile_shader_nir(void *mem_ctx, nir_shader *nir, const struct panfrost_compile_inputs *inputs) { + panfrost_program *program = rzalloc(mem_ctx, panfrost_program); + bifrost_debug = debug_get_option_bifrost_debug(); bi_context *ctx = rzalloc(NULL, bi_context); @@ -1955,6 +1957,8 @@ bifrost_compile_shader_nir(nir_shader *nir, panfrost_program *program, bi_register_allocate(ctx); if (bifrost_debug & BIFROST_DBG_SHADERS) bi_print_shader(ctx, stdout); + + util_dynarray_init(&program->compiled, NULL); bi_pack(ctx, &program->compiled); memcpy(program->blend_ret_offsets, ctx->blend_ret_offsets, sizeof(program->blend_ret_offsets)); @@ -1963,4 +1967,6 @@ bifrost_compile_shader_nir(nir_shader *nir, panfrost_program *program, disassemble_bifrost(stdout, program->compiled.data, program->compiled.size, true); ralloc_free(ctx); + + return program; } diff --git a/src/panfrost/bifrost/bifrost_compile.h b/src/panfrost/bifrost/bifrost_compile.h index e93d4713c9e..942f20228c2 100644 --- a/src/panfrost/bifrost/bifrost_compile.h +++ b/src/panfrost/bifrost/bifrost_compile.h @@ -28,8 +28,9 @@ #include "util/u_dynarray.h" #include "panfrost/util/pan_ir.h" -void bifrost_compile_shader_nir(nir_shader *nir, panfrost_program *program, - const struct panfrost_compile_inputs *inputs); +panfrost_program * +bifrost_compile_shader_nir(void *mem_ctx, nir_shader *nir, + const struct panfrost_compile_inputs *inputs); static const nir_shader_compiler_options bifrost_nir_options = { .lower_scmp = true, diff --git a/src/panfrost/bifrost/cmdline.c b/src/panfrost/bifrost/cmdline.c index a21f646491f..4c717c506b2 100644 --- a/src/panfrost/bifrost/cmdline.c +++ b/src/panfrost/bifrost/cmdline.c @@ -32,7 +32,7 @@ #include "bifrost_compile.h" #include "test/bit.h" -static panfrost_program +static panfrost_program * compile_shader(char **argv, bool vertex_only) { struct gl_shader_program *prog; @@ -53,7 +53,7 @@ compile_shader(char **argv, bool vertex_only) prog = standalone_compile_shader(&options, 2, argv, &local_ctx); prog->_LinkedShaders[MESA_SHADER_FRAGMENT]->Program->info.stage = MESA_SHADER_FRAGMENT; - panfrost_program compiled; + panfrost_program *compiled; for (unsigned i = 0; i < 2; ++i) { nir[i] = glsl_to_nir(&local_ctx, prog, shader_types[i], &bifrost_nir_options); NIR_PASS_V(nir[i], nir_lower_global_vars_to_local); @@ -71,7 +71,7 @@ compile_shader(char **argv, bool vertex_only) .gpu_id = 0x7212, /* Mali G52 */ }; - bifrost_compile_shader_nir(nir[i], &compiled, &inputs); + compiled = bifrost_compile_shader_nir(NULL, nir[i], &inputs); if (vertex_only) return compiled; @@ -161,7 +161,7 @@ run(const char *filename) }, }; - bit_vertex(dev, prog, NULL, 0, NULL, 0, NULL, 0, BIT_DEBUG_ALL); + bit_vertex(dev, &prog, NULL, 0, NULL, 0, NULL, 0, BIT_DEBUG_ALL); free(code); } diff --git a/src/panfrost/bifrost/test/bi_submit.c b/src/panfrost/bifrost/test/bi_submit.c index cfa2aa30be6..fd015995cd8 100644 --- a/src/panfrost/bifrost/test/bi_submit.c +++ b/src/panfrost/bifrost/test/bi_submit.c @@ -125,12 +125,12 @@ bit_sanity_check(struct panfrost_device *dev) /* Constructs a vertex job */ bool -bit_vertex(struct panfrost_device *dev, panfrost_program prog, +bit_vertex(struct panfrost_device *dev, panfrost_program *prog, uint32_t *iubo, size_t sz_ubo, uint32_t *iattr, size_t sz_attr, uint32_t *expected, size_t sz_expected, enum bit_debug debug) { - struct panfrost_bo *shader = bit_bo_create(dev, prog.compiled.size); + struct panfrost_bo *shader = bit_bo_create(dev, prog->compiled.size); struct panfrost_bo *shader_desc = bit_bo_create(dev, 4096); struct panfrost_bo *ubo = bit_bo_create(dev, 4096); struct panfrost_bo *var = bit_bo_create(dev, 4096); @@ -183,7 +183,7 @@ bit_vertex(struct panfrost_device *dev, panfrost_program prog, cfg.preload.uniform_count = (sz_ubo / 16); } - memcpy(shader->cpu, prog.compiled.data, prog.compiled.size); + memcpy(shader->cpu, prog->compiled.data, prog->compiled.size); struct mali_compute_job_packed job; diff --git a/src/panfrost/bifrost/test/bi_test_pack.c b/src/panfrost/bifrost/test/bi_test_pack.c index fba8b78cbdb..e95577989d9 100644 --- a/src/panfrost/bifrost/test/bi_test_pack.c +++ b/src/panfrost/bifrost/test/bi_test_pack.c @@ -143,10 +143,11 @@ bit_test_single(struct panfrost_device *dev, clauses[2]->message_type = BIFROST_MESSAGE_ATTRIBUTE; clauses[3]->message_type = BIFROST_MESSAGE_STORE; - panfrost_program prog; + panfrost_program prog = { 0 }; + util_dynarray_init(&prog.compiled, NULL); bi_pack(ctx, &prog.compiled); - bool succ = bit_vertex(dev, prog, input, 16, NULL, 0, + bool succ = bit_vertex(dev, &prog, input, 16, NULL, 0, s.r, 16, debug); if (debug >= BIT_DEBUG_ALL || (!succ && debug >= BIT_DEBUG_FAIL)) { diff --git a/src/panfrost/bifrost/test/bit.h b/src/panfrost/bifrost/test/bit.h index 9f6c94ca2b9..787c8bbcb42 100644 --- a/src/panfrost/bifrost/test/bit.h +++ b/src/panfrost/bifrost/test/bit.h @@ -45,7 +45,7 @@ enum bit_debug { }; bool -bit_vertex(struct panfrost_device *dev, panfrost_program prog, +bit_vertex(struct panfrost_device *dev, panfrost_program *prog, uint32_t *iubo, size_t sz_ubo, uint32_t *iattr, size_t sz_attr, uint32_t *expected, size_t sz_expected, enum bit_debug debug); diff --git a/src/panfrost/lib/pan_blit.c b/src/panfrost/lib/pan_blit.c index 7bbc65693de..60bb93dee1a 100644 --- a/src/panfrost/lib/pan_blit.c +++ b/src/panfrost/lib/pan_blit.c @@ -43,8 +43,8 @@ * This is primarily designed as a fallback for preloads but could be extended * for other clears/blits if needed in the future. */ -static void -panfrost_build_blit_shader(panfrost_program *program, unsigned gpu_id, gl_frag_result loc, nir_alu_type T, bool ms) +static panfrost_program * +panfrost_build_blit_shader(unsigned gpu_id, gl_frag_result loc, nir_alu_type T, bool ms) { bool is_colour = loc >= FRAG_RESULT_DATA0; @@ -102,8 +102,10 @@ panfrost_build_blit_shader(panfrost_program *program, unsigned gpu_id, gl_frag_r .gpu_id = gpu_id, }; - midgard_compile_shader_nir(shader, program, &inputs); + panfrost_program *program = midgard_compile_shader_nir(NULL, shader, &inputs); + ralloc_free(shader); + return program; } /* Compile and upload all possible blit shaders ahead-of-time to reduce draw @@ -158,16 +160,16 @@ panfrost_init_blit_shaders(struct panfrost_device *dev) if (!(shader_descs[i].types & (1 << T))) continue; - panfrost_program program; - panfrost_build_blit_shader(&program, dev->gpu_id, loc, - nir_types[T], ms); + panfrost_program *program = + panfrost_build_blit_shader(dev->gpu_id, loc, + nir_types[T], ms); - assert(offset + program.compiled.size < total_size); - memcpy(dev->blit_shaders.bo->cpu + offset, program.compiled.data, program.compiled.size); + assert(offset + program->compiled.size < total_size); + memcpy(dev->blit_shaders.bo->cpu + offset, program->compiled.data, program->compiled.size); - dev->blit_shaders.loads[loc][T][ms] = (dev->blit_shaders.bo->gpu + offset) | program.first_tag; - offset += ALIGN_POT(program.compiled.size, 64); - util_dynarray_fini(&program.compiled); + dev->blit_shaders.loads[loc][T][ms] = (dev->blit_shaders.bo->gpu + offset) | program->first_tag; + offset += ALIGN_POT(program->compiled.size, 64); + ralloc_free(program); } } } diff --git a/src/panfrost/midgard/midgard_compile.c b/src/panfrost/midgard/midgard_compile.c index 7f67fb35537..acbf296be85 100644 --- a/src/panfrost/midgard/midgard_compile.c +++ b/src/panfrost/midgard/midgard_compile.c @@ -2942,10 +2942,12 @@ mir_add_writeout_loops(compiler_context *ctx) } } -int -midgard_compile_shader_nir(nir_shader *nir, panfrost_program *program, +panfrost_program * +midgard_compile_shader_nir(void *mem_ctx, nir_shader *nir, const struct panfrost_compile_inputs *inputs) { + panfrost_program *program = rzalloc(mem_ctx, panfrost_program); + struct util_dynarray *compiled = &program->compiled; midgard_debug = debug_get_option_midgard_debug(); @@ -3041,7 +3043,7 @@ midgard_compile_shader_nir(nir_shader *nir, panfrost_program *program, break; /* TODO: Multi-function shaders */ } - util_dynarray_init(compiled, NULL); + util_dynarray_init(compiled, program); /* Per-block lowering before opts */ @@ -3187,5 +3189,5 @@ midgard_compile_shader_nir(nir_shader *nir, panfrost_program *program, ralloc_free(ctx); - return 0; + return program; } diff --git a/src/panfrost/midgard/midgard_compile.h b/src/panfrost/midgard/midgard_compile.h index 93ac3a825db..c50ece351b1 100644 --- a/src/panfrost/midgard/midgard_compile.h +++ b/src/panfrost/midgard/midgard_compile.h @@ -28,8 +28,8 @@ #include "util/u_dynarray.h" #include "panfrost/util/pan_ir.h" -int -midgard_compile_shader_nir(nir_shader *nir, panfrost_program *program, +panfrost_program * +midgard_compile_shader_nir(void *mem_ctx, nir_shader *nir, const struct panfrost_compile_inputs *inputs); /* NIR options are shared between the standalone compiler and the online