diff --git a/src/gallium/drivers/panfrost/pan_cmdstream.c b/src/gallium/drivers/panfrost/pan_cmdstream.c index 473cf4c7b64..cc9a8521f30 100644 --- a/src/gallium/drivers/panfrost/pan_cmdstream.c +++ b/src/gallium/drivers/panfrost/pan_cmdstream.c @@ -1321,28 +1321,6 @@ panfrost_upload_sysvals(struct panfrost_batch *batch, void *ptr_cpu, } } -static const void * -panfrost_map_constant_buffer_cpu(struct panfrost_context *ctx, - struct panfrost_constant_buffer *buf, - unsigned index) -{ - struct pipe_constant_buffer *cb = &buf->cb[index]; - struct panfrost_resource *rsrc = pan_resource(cb->buffer); - - if (rsrc) { - if (panfrost_bo_mmap(rsrc->bo)) - return NULL; - - panfrost_flush_writer(ctx, rsrc, "CPU constant buffer mapping"); - panfrost_bo_wait(rsrc->bo, INT64_MAX, false); - - return rsrc->bo->ptr.cpu + cb->buffer_offset; - } else if (cb->user_buffer) { - return cb->user_buffer + cb->buffer_offset; - } else - unreachable("No constant buffer"); -} - /* Emit a single UBO record. On Valhall, UBOs are dumb buffers and are * implemented with buffer descriptors in the resource table, sized in terms of * bytes. On Bifrost and older, UBOs have special uniform buffer data @@ -1504,13 +1482,33 @@ panfrost_emit_const_buf(struct panfrost_batch *batch, sysval_comp < ARRAY_SIZE(batch->num_wg_sysval)) batch->num_wg_sysval[sysval_comp] = ptr; } - /* Map the UBO, this should be cheap. For some buffers this may - * read from write-combine memory which is slow, though :-( + + /* Grab the mapped memory. We only do this path for sysvals & user + * buffers, which are already CPU mapped. We do not use this path for + * "real" UBOs for a few reasons. First, real UBOs are generally mapped + * write-combine, so reading them here is very expensive. Second, real + * UBOs may be written from the GPU, which would require a full stall to + * get the results fro m the GPU. Third, it may happen that *this* batch + * is writing the UBO which would require us to split the batch *and* + * stall, which we lack the batch tracking primitives to do correctly. + * + * The "proper" way to push true UBOs is on-device. Either we would + * dispatch a small compute kernel to run this logic at the start of the + * draw, or we would wire up nir_opt_preamble to compute kernels to the + * same effect. We will likely do this for Vulkan. + * + * For now, use the straightforward correct implementation. */ - const void *mapped_ubo = - (src.ubo == sysval_ubo) - ? sysvals - : panfrost_map_constant_buffer_cpu(ctx, buf, src.ubo); + const void *mapped_ubo; + if (src.ubo == sysval_ubo) { + mapped_ubo = sysvals; + } else { + struct pipe_constant_buffer *cb = &buf->cb[src.ubo]; + assert(!cb->buffer && cb->user_buffer && + "only user buffers use this path"); + + mapped_ubo = cb->user_buffer + cb->buffer_offset; + } if (!mapped_ubo) return 0; diff --git a/src/panfrost/ci/panfrost-g52-fails.txt b/src/panfrost/ci/panfrost-g52-fails.txt index 23f7338a82b..41b15fa7c3a 100644 --- a/src/panfrost/ci/panfrost-g52-fails.txt +++ b/src/panfrost/ci/panfrost-g52-fails.txt @@ -74,8 +74,6 @@ spec@arb_texture_rg@fbo-blending-formats@GL_R8,Fail spec@arb_texture_rg@fbo-blending-formats@GL_RG8,Fail spec@arb_texture_rg@fbo-blending-formats@GL_RG,Fail spec@arb_transform_feedback_instanced@draw-auto instanced,Fail -spec@arb_uniform_buffer_object@rendering-dsa-offset,Fail -spec@arb_uniform_buffer_object@rendering-offset,Fail spec@egl 1.4@eglterminate then unbind context,Fail spec@egl_chromium_sync_control@conformance@eglGetSyncValuesCHROMIUM_msc_and_sbc_test,Fail spec@egl_chromium_sync_control@conformance,Fail diff --git a/src/panfrost/ci/panfrost-g57-fails.txt b/src/panfrost/ci/panfrost-g57-fails.txt index f9ea598e8f5..e2b5e98de42 100644 --- a/src/panfrost/ci/panfrost-g57-fails.txt +++ b/src/panfrost/ci/panfrost-g57-fails.txt @@ -66,8 +66,6 @@ spec@arb_texture_rg@fbo-blending-formats@GL_R8,Fail spec@arb_texture_rg@fbo-blending-formats@GL_RG8,Fail spec@arb_texture_rg@fbo-blending-formats@GL_RG,Fail spec@arb_transform_feedback_instanced@draw-auto instanced,Fail -spec@arb_uniform_buffer_object@rendering-dsa-offset,Fail -spec@arb_uniform_buffer_object@rendering-offset,Fail spec@egl 1.4@eglterminate then unbind context,Fail spec@egl_chromium_sync_control@conformance@eglGetSyncValuesCHROMIUM_msc_and_sbc_test,Fail spec@egl_chromium_sync_control@conformance,Fail diff --git a/src/panfrost/compiler/bi_opt_push_ubo.c b/src/panfrost/compiler/bi_opt_push_ubo.c index 9a4f3c527a7..bf9c7b247df 100644 --- a/src/panfrost/compiler/bi_opt_push_ubo.c +++ b/src/panfrost/compiler/bi_opt_push_ubo.c @@ -36,12 +36,16 @@ bi_is_ubo(bi_instr *ins) (ins->seg == BI_SEG_UBO); } +/* For now, we only allow pushing UBO 0. This matches the Gallium convention + * where UBO 0 is mapped on the CPU but other UBOs are not. When we switch to + * pushing UBOs with a compute kernel (or CSF instructions), we can relax this. + */ static bool -bi_is_direct_aligned_ubo(bi_instr *ins) +bi_is_pushable_ubo(bi_instr *ins) { return bi_is_ubo(ins) && (ins->src[0].type == BI_INDEX_CONSTANT) && (ins->src[1].type == BI_INDEX_CONSTANT) && - ((ins->src[0].value & 0x3) == 0); + ((ins->src[0].value & 0x3) == 0) && (ins->src[1].value == 0); } /* Represents use data for a single UBO */ @@ -69,7 +73,7 @@ bi_analyze_ranges(bi_context *ctx) res.blocks = calloc(res.nr_blocks, sizeof(struct bi_ubo_block)); bi_foreach_instr_global(ctx, ins) { - if (!bi_is_direct_aligned_ubo(ins)) + if (!bi_is_pushable_ubo(ins)) continue; unsigned ubo = pan_res_handle_get_index(ins->src[1].value); @@ -130,6 +134,9 @@ bi_pick_ubo(struct panfrost_ubo_push *push, struct bi_ubo_analysis *analysis) void bi_opt_push_ubo(bi_context *ctx) { + /* We only push from the "default" UBO 0 */ + assert(ctx->nir->info.first_ubo_is_default_ubo && "precondition"); + struct bi_ubo_analysis analysis = bi_analyze_ranges(ctx); bi_pick_ubo(ctx->info.push, &analysis); @@ -142,7 +149,7 @@ bi_opt_push_ubo(bi_context *ctx) unsigned ubo = pan_res_handle_get_index(ins->src[1].value); unsigned offset = ins->src[0].value; - if (!bi_is_direct_aligned_ubo(ins)) { + if (!bi_is_pushable_ubo(ins)) { /* The load can't be pushed, so this UBO needs to be * uploaded conventionally */ if (ins->src[1].type == BI_INDEX_CONSTANT) diff --git a/src/panfrost/midgard/mir_promote_uniforms.c b/src/panfrost/midgard/mir_promote_uniforms.c index b8ab5877ecd..38b06db74e3 100644 --- a/src/panfrost/midgard/mir_promote_uniforms.c +++ b/src/panfrost/midgard/mir_promote_uniforms.c @@ -42,11 +42,15 @@ mir_is_ubo(midgard_instruction *ins) return (ins->type == TAG_LOAD_STORE_4) && (OP_IS_UBO_READ(ins->op)); } +/* We only allow pushing UBO 0. This matches the Gallium convention + * where UBO 0 is mapped on the CPU but other UBOs are not. + */ static bool -mir_is_direct_aligned_ubo(midgard_instruction *ins) +mir_is_pushable_ubo(midgard_instruction *ins) { return mir_is_ubo(ins) && !(ins->constants.u32[0] & 0xF) && - (ins->src[1] == ~0) && (ins->src[2] == ~0); + (ins->src[1] == ~0) && (ins->src[2] == ~0) && + midgard_unpack_ubo_index_imm(ins->load_store) == 0; } /* Represents use data for a single UBO */ @@ -74,7 +78,7 @@ mir_analyze_ranges(compiler_context *ctx) res.blocks = calloc(res.nr_blocks, sizeof(struct mir_ubo_block)); mir_foreach_instr_global(ctx, ins) { - if (!mir_is_direct_aligned_ubo(ins)) + if (!mir_is_pushable_ubo(ins)) continue; unsigned ubo = midgard_unpack_ubo_index_imm(ins->load_store); @@ -272,6 +276,9 @@ midgard_promote_uniforms(compiler_context *ctx) return; } + /* We only push from the "default" UBO 0 */ + assert(ctx->nir->info.first_ubo_is_default_ubo && "precondition"); + struct mir_ubo_analysis analysis = mir_analyze_ranges(ctx); unsigned work_count = mir_work_heuristic(ctx, &analysis); @@ -293,7 +300,7 @@ midgard_promote_uniforms(compiler_context *ctx) unsigned ubo = midgard_unpack_ubo_index_imm(ins->load_store); unsigned qword = ins->constants.u32[0] / 16; - if (!mir_is_direct_aligned_ubo(ins)) { + if (!mir_is_pushable_ubo(ins)) { if (ins->src[1] == ~0) ctx->ubo_mask |= BITSET_BIT(ubo); else