pan/cs: Don't leak builder resources

cs_finish() is doing two things:

1. wrapping up the CS to prepare for its execution
2. freeing the temporary instrs array and maybe_ctx allocations

Mixing those two things lead to confusion and leaks, so let's split
those into cs_end() and cs_builder_fini(), and make sure panvk/panfrost
call both when appropriate.

Fixes: 50d2396b7e ("pan/cs: add helpers to emit contiguous csf code blocks")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Acked-by: Erik Faye-Lund <erik.faye-lund@collabora.com>
Part-of: <https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/39003>

src/gallium/drivers/panfrost/pan_csf.c

@@ -201,7 +201,8 @@ csf_oom_handler_init(struct panfrost_context *ctx)
    }
 
    assert(cs_is_valid(&b));
-   cs_finish(&b);
+   cs_end(&b);
+   cs_builder_fini(&b);
    ctx->csf.tiler_oom_handler.cs_bo = cs_bo;
    ctx->csf.tiler_oom_handler.length = handler.length * sizeof(uint64_t);
    ctx->csf.tiler_oom_handler.save_bo = reg_save_bo;
@@ -224,7 +225,10 @@ fail:
 void
 GENX(csf_cleanup_batch)(struct panfrost_batch *batch)
 {
-   free(batch->csf.cs.builder);
+   if (batch->csf.cs.builder) {
+      cs_builder_fini(batch->csf.cs.builder);
+      free(batch->csf.cs.builder);
+   }
 
    panfrost_pool_cleanup(&batch->csf.cs_chunk_pool);
 }
@@ -362,10 +366,9 @@ csf_emit_batch_end(struct panfrost_batch *batch)
    cs_wait_slot(b, 0);
 
    /* Finish the command stream */
-   if (!cs_is_valid(batch->csf.cs.builder))
+   if (cs_is_valid(batch->csf.cs.builder))
-      return -1;
+      cs_end(batch->csf.cs.builder);
 
-   cs_finish(batch->csf.cs.builder);
    return 0;
 }
 
@@ -1602,11 +1605,13 @@ GENX(csf_init_context)(struct panfrost_context *ctx)
    };
 
    assert(cs_is_valid(&b));
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t cs_start = cs_root_chunk_gpu_addr(&b);
    uint32_t cs_size = cs_root_chunk_size(&b);
 
+   cs_builder_fini(&b);
+
    csf_prepare_qsubmit(ctx, &qsubmit, 0, cs_start, cs_size, &sync, 1);
    csf_prepare_gsubmit(ctx, &gsubmit, &qsubmit, 1);
    ret = csf_submit_gsubmit(ctx, &gsubmit);

src/panfrost/genxml/cs_builder.h

@@ -248,6 +248,7 @@ cs_builder_init(struct cs_builder *b, const struct cs_builder_conf *conf,
                 struct cs_buffer root_buffer)
 {
    memset(b, 0, sizeof(*b));
+   util_dynarray_init(&b->blocks.instrs, NULL);
    b->conf = *conf;
    b->root_chunk.buffer = root_buffer;
    b->cur_chunk.buffer = root_buffer;
@@ -263,6 +264,13 @@ cs_builder_init(struct cs_builder *b, const struct cs_builder_conf *conf,
    util_dynarray_init(&b->blocks.instrs, NULL);
 }
 
+static inline void
+cs_builder_fini(struct cs_builder *b)
+{
+   util_dynarray_fini(&b->blocks.instrs);
+   ralloc_free(b->maybe_ctx);
+}
+
 static inline bool
 cs_is_valid(struct cs_builder *b)
 {
@@ -285,7 +293,7 @@ cs_root_chunk_gpu_addr(struct cs_builder *b)
 static inline uint32_t
 cs_root_chunk_size(struct cs_builder *b)
 {
-   /* Make sure cs_finish() was called. */
+   /* Make sure cs_end() was called. */
    struct cs_chunk empty_chunk;
    memset(&empty_chunk, 0, sizeof(empty_chunk));
    assert(!memcmp(&b->cur_chunk, &empty_chunk, sizeof(b->cur_chunk)));
@@ -295,7 +303,7 @@ cs_root_chunk_size(struct cs_builder *b)
 
 /*
  * Wrap the current queue. External users shouldn't call this function
- * directly, they should call cs_finish() when they are done building
+ * directly, they should call cs_end() when they are done building
  * the command stream, which will in turn call cs_wrap_queue().
  *
  * Internally, this is also used to finalize internal CS chunks when
@@ -753,7 +761,7 @@ cs_alloc_ins(struct cs_builder *b)
  * it for submission.
  */
 static inline void
-cs_finish(struct cs_builder *b)
+cs_end(struct cs_builder *b)
 {
    if (!cs_is_valid(b))
       return;
@@ -763,9 +771,6 @@ cs_finish(struct cs_builder *b)
 
    /* This prevents adding instructions after that point. */
    memset(&b->cur_chunk, 0, sizeof(b->cur_chunk));
-
-   util_dynarray_fini(&b->blocks.instrs);
-   ralloc_free(b->maybe_ctx);
 }
 
 /*
@@ -1458,7 +1463,7 @@ cs_maybe_end(struct cs_builder *b, struct cs_maybe_state *state,
         __state != NULL; cs_maybe_end(__b, __state, __maybe),                  \
         __state = NULL)
 
-/* Must be called before cs_finish */
+/* Must be called before cs_end */
 static inline void
 cs_patch_maybe(struct cs_builder *b, struct cs_maybe *maybe)
 {

src/panfrost/genxml/test/cs_builder.cpp

@@ -37,13 +37,14 @@ CsBuilderTest::CsBuilderTest()
 
 CsBuilderTest::~CsBuilderTest()
 {
+   cs_builder_fini(&b);
    delete output;
 }
 
 TEST_F(CsBuilderTest, basic)
 {
    cs_move32_to(&b, cs_reg32(&b, 42), 0xdeadbeef);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected[] = {
       0x022a0000deadbeef, /* MOVE32 r42, #0xdeadbeef */
@@ -60,7 +61,7 @@ TEST_F(CsBuilderTest, maybe_no_patch)
    cs_maybe(&b, &maybe) {
       cs_move32_to(&b, cs_reg32(&b, 42), 0xdeadbeef);
    }
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected[] = {
       0x022a0000abad1dea, /* MOVE32 r42, #0xabad1dea */
@@ -78,7 +79,7 @@ TEST_F(CsBuilderTest, maybe_patch)
       cs_move32_to(&b, cs_reg32(&b, 42), 0xdeadbeef);
    }
    cs_patch_maybe(&b, maybe);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x022a0000abad1dea, /* MOVE32 r42, #0xabad1dea */
@@ -101,7 +102,7 @@ TEST_F(CsBuilderTest, maybe_inner_block)
       cs_move32_to(&b, cs_reg32(&b, 42), 0xabcdef01);
    }
    cs_patch_maybe(&b, maybe);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x022a0000abad1dea, /* MOVE32 r42, #0xabad1dea */
@@ -128,7 +129,7 @@ TEST_F(CsBuilderTest, maybe_early_patch)
 
       cs_patch_maybe(&b, maybe);
    }
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x022a0000abad1dea, /* MOVE32 r42, #0xabad1dea */
@@ -154,7 +155,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_unrelated_inside)
       cs_break(&b);
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x14000a0000010000, /* LOAD_MULTIPLE r0, addr, #0x0 */
@@ -182,7 +183,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_load_only_inside_if)
       cs_break(&b);
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x1600000050000001, /* BRANCH ge, r0, #1 */
@@ -215,7 +216,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_load_only_continue_inside_if)
       cs_break(&b);
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x1000000000000000, /* ADD32 r0, r0, #0x0 */
@@ -247,7 +248,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_load_only_break_inside_if)
       }
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x1000000000000000, /* ADD32 r0, r0, #0x0 */
@@ -281,7 +282,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_load_same_inside)
       }
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x14000a0000010000, /* LOAD_MULTIPLE r0, addr, #0x0 */
@@ -316,7 +317,7 @@ TEST_F(CsBuilderTest, loop_ls_tracker_load_same_inside_use_as_cond)
       }
    }
    cs_add32(&b, r0, r0, 0xab);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x14000a0000010000, /* LOAD_MULTIPLE r0, addr, #0x0 */
@@ -354,7 +355,7 @@ TEST_F(CsBuilderTest, maybe_flush_outer_load)
    /* This should also flush the load to reg */
    cs_add32(&b, reg2, reg1, 0);
    cs_patch_maybe(&b, maybe);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       0x1403000000010000, /* LOAD_MULTIPLE r3, [d0] */
@@ -386,7 +387,7 @@ TEST_F(CsBuilderTest, maybe_flush_inner_load)
    /* This should not flush the load to reg */
    cs_add32(&b, reg2, reg1, 0);
    cs_patch_maybe(&b, maybe);
-   cs_finish(&b);
+   cs_end(&b);
 
    uint64_t expected_patched[] = {
       /* inside maybe block */

src/panfrost/vulkan/csf/panvk_vX_cmd_buffer.c

@@ -187,7 +187,7 @@ finish_cs(struct panvk_cmd_buffer *cmdbuf, uint32_t subqueue)
    panvk_per_arch(panvk_instr_end_work)(
       subqueue, cmdbuf, PANVK_INSTR_WORK_TYPE_CMDBUF, &instr_info_cmdbuf);
 
-   cs_finish(&cmdbuf->state.cs[subqueue].builder);
+   cs_end(&cmdbuf->state.cs[subqueue].builder);
 }
 
 static void
@@ -929,6 +929,9 @@ panvk_reset_cmdbuf(struct vk_command_buffer *vk_cmdbuf,
       u_trace_init(ut, &dev->utrace.utctx);
    }
 
+   for (uint32_t i = 0; i < ARRAY_SIZE(cmdbuf->state.cs); i++)
+      cs_builder_fini(&cmdbuf->state.cs[i].builder);
+
    memset(&cmdbuf->state, 0, sizeof(cmdbuf->state));
    init_cs_builders(cmdbuf);
 }
@@ -945,6 +948,9 @@ panvk_destroy_cmdbuf(struct vk_command_buffer *vk_cmdbuf)
    for (uint32_t i = 0; i < ARRAY_SIZE(cmdbuf->utrace.uts); i++)
       u_trace_fini(&cmdbuf->utrace.uts[i]);
 
+   for (uint32_t i = 0; i < ARRAY_SIZE(cmdbuf->state.cs); i++)
+      cs_builder_fini(&cmdbuf->state.cs[i].builder);
+
    panvk_pool_cleanup(&cmdbuf->cs_pool);
    panvk_pool_cleanup(&cmdbuf->desc_pool);
    panvk_pool_cleanup(&cmdbuf->tls_pool);

src/panfrost/vulkan/csf/panvk_vX_cmd_draw.c

@@ -109,7 +109,8 @@ generate_fn_set_fbds_provoking_vertex(struct panvk_device *dev,
    }
 
    assert(cs_is_valid(&b));
-   cs_finish(&b);
+   cs_end(&b);
+   cs_builder_fini(&b);
 
    *dump_region_size = function.dump_size;
 

src/panfrost/vulkan/csf/panvk_vX_exception_handler.c

@@ -300,7 +300,8 @@ generate_tiler_oom_handler(struct panvk_device *dev,
    }
 
    assert(cs_is_valid(&b));
-   cs_finish(&b);
+   cs_end(&b);
+   cs_builder_fini(&b);
    *dump_region_size = handler.dump_size;
 
    return handler.length * sizeof(uint64_t);

src/panfrost/vulkan/csf/panvk_vX_gpu_queue.c

@@ -458,7 +458,7 @@ init_subqueue(struct panvk_gpu_queue *queue, enum panvk_subqueue_id subqueue)
       break;
    }
 
-   cs_finish(&b);
+   cs_end(&b);
 
    assert(cs_is_valid(&b));
 
@@ -480,6 +480,8 @@ init_subqueue(struct panvk_gpu_queue *queue, enum panvk_subqueue_id subqueue)
       .queue_submits = DRM_PANTHOR_OBJ_ARRAY(1, &qsubmit),
    };
 
+   cs_builder_fini(&b);
+
    int ret = pan_kmod_ioctl(dev->drm_fd, DRM_IOCTL_PANTHOR_GROUP_SUBMIT,
                             &gsubmit);
    if (ret)

src/panfrost/vulkan/csf/panvk_vX_utrace.c

@@ -271,5 +271,6 @@ panvk_per_arch(utrace_clone_finish_builder)(struct cs_builder *b)
                    cs_defer(SB_IMM_MASK, SB_ID(IMM_FLUSH)));
    cs_wait_slot(b, SB_ID(IMM_FLUSH));
 
-   cs_finish(b);
+   cs_end(b);
+   cs_builder_fini(b);
 }