[Mesa-dev] [PATCH v2 1/2] radeonsi: Don't leave gaps between position exports from vertex shader
Tom Stellard
tom at stellard.net
Tue Aug 13 19:43:11 PDT 2013
On Tue, Aug 13, 2013 at 07:39:10PM +0200, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer at amd.com>
>
> If the vertex shader exports clip distances but not point size, use
> position exports 1/2 instead of 2/3 for the clip distances. Fixes
> geometry corruption in that case.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=66974
>
> Cc: mesa-stable at lists.freedesktop.org
> Signed-off-by: Michel Dänzer <michel.daenzer at amd.com>
I took a look through the LLVM calls in these patches and they look OK
to me.
Reviewed-by: Tom Stellard <thomas.stellard at amd.com>
> ---
>
> v2: No need to export unused position vectors, just export to consecutive
> position export slots.
>
> src/gallium/drivers/radeonsi/radeonsi_shader.c | 135 +++++++++++++++----------
> src/gallium/drivers/radeonsi/radeonsi_shader.h | 1 +
> src/gallium/drivers/radeonsi/si_state_draw.c | 6 +-
> 3 files changed, 83 insertions(+), 59 deletions(-)
>
> diff --git a/src/gallium/drivers/radeonsi/radeonsi_shader.c b/src/gallium/drivers/radeonsi/radeonsi_shader.c
> index fee6262..dd9581d 100644
> --- a/src/gallium/drivers/radeonsi/radeonsi_shader.c
> +++ b/src/gallium/drivers/radeonsi/radeonsi_shader.c
> @@ -562,12 +562,11 @@ static void si_alpha_test(struct lp_build_tgsi_context *bld_base,
> }
>
> static void si_llvm_emit_clipvertex(struct lp_build_tgsi_context * bld_base,
> - unsigned index)
> + LLVMValueRef (*pos)[9], unsigned index)
> {
> struct si_shader_context *si_shader_ctx = si_shader_context(bld_base);
> struct lp_build_context *base = &bld_base->base;
> struct lp_build_context *uint = &si_shader_ctx->radeon_bld.soa.bld_base.uint_bld;
> - LLVMValueRef args[9];
> unsigned reg_index;
> unsigned chan;
> unsigned const_chan;
> @@ -582,6 +581,8 @@ static void si_llvm_emit_clipvertex(struct lp_build_tgsi_context * bld_base,
> }
>
> for (reg_index = 0; reg_index < 2; reg_index ++) {
> + LLVMValueRef *args = pos[2 + reg_index];
> +
> args[5] =
> args[6] =
> args[7] =
> @@ -612,10 +613,6 @@ static void si_llvm_emit_clipvertex(struct lp_build_tgsi_context * bld_base,
> args[3] = lp_build_const_int32(base->gallivm,
> V_008DFC_SQ_EXP_POS + 2 + reg_index);
> args[4] = uint->zero;
> - lp_build_intrinsic(base->gallivm->builder,
> - "llvm.SI.export",
> - LLVMVoidTypeInContext(base->gallivm->context),
> - args, 9);
> }
> }
>
> @@ -630,17 +627,18 @@ static void si_llvm_emit_epilogue(struct lp_build_tgsi_context * bld_base)
> struct tgsi_parse_context *parse = &si_shader_ctx->parse;
> LLVMValueRef args[9];
> LLVMValueRef last_args[9] = { 0 };
> + LLVMValueRef pos_args[4][9] = { { 0 } };
> unsigned semantic_name;
> unsigned color_count = 0;
> unsigned param_count = 0;
> int depth_index = -1, stencil_index = -1;
> + int i;
>
> while (!tgsi_parse_end_of_tokens(parse)) {
> struct tgsi_full_declaration *d =
> &parse->FullToken.FullDeclaration;
> unsigned target;
> unsigned index;
> - int i;
>
> tgsi_parse_token(parse);
>
> @@ -716,7 +714,7 @@ handle_semantic:
> target = V_008DFC_SQ_EXP_POS + 2 + d->Semantic.Index;
> break;
> case TGSI_SEMANTIC_CLIPVERTEX:
> - si_llvm_emit_clipvertex(bld_base, index);
> + si_llvm_emit_clipvertex(bld_base, pos_args, index);
> shader->clip_dist_write = 0xFF;
> continue;
> case TGSI_SEMANTIC_FOG:
> @@ -734,9 +732,13 @@ handle_semantic:
>
> si_llvm_init_export_args(bld_base, d, index, target, args);
>
> - if (si_shader_ctx->type == TGSI_PROCESSOR_VERTEX ?
> - (semantic_name == TGSI_SEMANTIC_POSITION) :
> - (semantic_name == TGSI_SEMANTIC_COLOR)) {
> + if (si_shader_ctx->type == TGSI_PROCESSOR_VERTEX &&
> + target >= V_008DFC_SQ_EXP_POS &&
> + target <= (V_008DFC_SQ_EXP_POS + 3)) {
> + memcpy(pos_args[target - V_008DFC_SQ_EXP_POS],
> + args, sizeof(args));
> + } else if (si_shader_ctx->type == TGSI_PROCESSOR_FRAGMENT &&
> + semantic_name == TGSI_SEMANTIC_COLOR) {
> if (last_args[0]) {
> lp_build_intrinsic(base->gallivm->builder,
> "llvm.SI.export",
> @@ -806,66 +808,87 @@ handle_semantic:
> memcpy(last_args, args, sizeof(args));
> }
>
> - if (!last_args[0]) {
> - assert(si_shader_ctx->type == TGSI_PROCESSOR_FRAGMENT);
> -
> - /* Specify which components to enable */
> - last_args[0] = lp_build_const_int32(base->gallivm, 0x0);
> -
> - /* Specify the target we are exporting */
> - last_args[3] = lp_build_const_int32(base->gallivm, V_008DFC_SQ_EXP_MRT);
> -
> - /* Set COMPR flag to zero to export data as 32-bit */
> - last_args[4] = uint->zero;
> -
> - /* dummy bits */
> - last_args[5]= uint->zero;
> - last_args[6]= uint->zero;
> - last_args[7]= uint->zero;
> - last_args[8]= uint->zero;
> -
> - si_shader_ctx->shader->spi_shader_col_format |=
> - V_028714_SPI_SHADER_32_ABGR;
> - si_shader_ctx->shader->cb_shader_mask |= S_02823C_OUTPUT0_ENABLE(0xf);
> - }
> -
> - /* Specify whether the EXEC mask represents the valid mask */
> - last_args[1] = lp_build_const_int32(base->gallivm,
> - si_shader_ctx->type == TGSI_PROCESSOR_FRAGMENT);
> + if (si_shader_ctx->type == TGSI_PROCESSOR_VERTEX) {
> + unsigned pos_idx = 0;
>
> - if (shader->fs_write_all && shader->nr_cbufs > 1) {
> - int i;
> + for (i = 0; i < 4; i++)
> + if (pos_args[i][0])
> + shader->nr_pos_exports++;
>
> - /* Specify that this is not yet the last export */
> - last_args[2] = lp_build_const_int32(base->gallivm, 0);
> + for (i = 0; i < 4; i++) {
> + if (!pos_args[i][0])
> + continue;
>
> - for (i = 1; i < shader->nr_cbufs; i++) {
> /* Specify the target we are exporting */
> - last_args[3] = lp_build_const_int32(base->gallivm,
> - V_008DFC_SQ_EXP_MRT + i);
> + pos_args[i][3] = lp_build_const_int32(base->gallivm, V_008DFC_SQ_EXP_POS + pos_idx++);
> +
> + if (pos_idx == shader->nr_pos_exports)
> + /* Specify that this is the last export */
> + pos_args[i][2] = uint->one;
>
> lp_build_intrinsic(base->gallivm->builder,
> "llvm.SI.export",
> LLVMVoidTypeInContext(base->gallivm->context),
> - last_args, 9);
> + pos_args[i], 9);
> + }
> + } else {
> + if (!last_args[0]) {
> + /* Specify which components to enable */
> + last_args[0] = lp_build_const_int32(base->gallivm, 0x0);
> +
> + /* Specify the target we are exporting */
> + last_args[3] = lp_build_const_int32(base->gallivm, V_008DFC_SQ_EXP_MRT);
> +
> + /* Set COMPR flag to zero to export data as 32-bit */
> + last_args[4] = uint->zero;
> +
> + /* dummy bits */
> + last_args[5]= uint->zero;
> + last_args[6]= uint->zero;
> + last_args[7]= uint->zero;
> + last_args[8]= uint->zero;
>
> si_shader_ctx->shader->spi_shader_col_format |=
> - si_shader_ctx->shader->spi_shader_col_format << 4;
> - si_shader_ctx->shader->cb_shader_mask |=
> - si_shader_ctx->shader->cb_shader_mask << 4;
> + V_028714_SPI_SHADER_32_ABGR;
> + si_shader_ctx->shader->cb_shader_mask |= S_02823C_OUTPUT0_ENABLE(0xf);
> }
>
> - last_args[3] = lp_build_const_int32(base->gallivm, V_008DFC_SQ_EXP_MRT);
> - }
> + /* Specify whether the EXEC mask represents the valid mask */
> + last_args[1] = uint->one;
> +
> + if (shader->fs_write_all && shader->nr_cbufs > 1) {
> + int i;
> +
> + /* Specify that this is not yet the last export */
> + last_args[2] = lp_build_const_int32(base->gallivm, 0);
> +
> + for (i = 1; i < shader->nr_cbufs; i++) {
> + /* Specify the target we are exporting */
> + last_args[3] = lp_build_const_int32(base->gallivm,
> + V_008DFC_SQ_EXP_MRT + i);
> +
> + lp_build_intrinsic(base->gallivm->builder,
> + "llvm.SI.export",
> + LLVMVoidTypeInContext(base->gallivm->context),
> + last_args, 9);
> +
> + si_shader_ctx->shader->spi_shader_col_format |=
> + si_shader_ctx->shader->spi_shader_col_format << 4;
> + si_shader_ctx->shader->cb_shader_mask |=
> + si_shader_ctx->shader->cb_shader_mask << 4;
> + }
>
> - /* Specify that this is the last export */
> - last_args[2] = lp_build_const_int32(base->gallivm, 1);
> + last_args[3] = lp_build_const_int32(base->gallivm, V_008DFC_SQ_EXP_MRT);
> + }
>
> - lp_build_intrinsic(base->gallivm->builder,
> - "llvm.SI.export",
> - LLVMVoidTypeInContext(base->gallivm->context),
> - last_args, 9);
> + /* Specify that this is the last export */
> + last_args[2] = lp_build_const_int32(base->gallivm, 1);
>
> + lp_build_intrinsic(base->gallivm->builder,
> + "llvm.SI.export",
> + LLVMVoidTypeInContext(base->gallivm->context),
> + last_args, 9);
> + }
> /* XXX: Look up what this function does */
> /* ctx->shader->output[i].spi_sid = r600_spi_sid(&ctx->shader->output[i]);*/
> }
> diff --git a/src/gallium/drivers/radeonsi/radeonsi_shader.h b/src/gallium/drivers/radeonsi/radeonsi_shader.h
> index 60a48f4..f28a0ea 100644
> --- a/src/gallium/drivers/radeonsi/radeonsi_shader.h
> +++ b/src/gallium/drivers/radeonsi/radeonsi_shader.h
> @@ -113,6 +113,7 @@ struct si_shader {
> bool vs_out_misc_write;
> bool vs_out_point_size;
> unsigned nr_cbufs;
> + unsigned nr_pos_exports;
> unsigned clip_dist_write;
> };
>
> diff --git a/src/gallium/drivers/radeonsi/si_state_draw.c b/src/gallium/drivers/radeonsi/si_state_draw.c
> index 0d1bd81..7d037d0 100644
> --- a/src/gallium/drivers/radeonsi/si_state_draw.c
> +++ b/src/gallium/drivers/radeonsi/si_state_draw.c
> @@ -74,13 +74,13 @@ static void si_pipe_shader_vs(struct pipe_context *ctx, struct si_pipe_shader *s
>
> si_pm4_set_reg(pm4, R_02870C_SPI_SHADER_POS_FORMAT,
> S_02870C_POS0_EXPORT_FORMAT(V_02870C_SPI_SHADER_4COMP) |
> - S_02870C_POS1_EXPORT_FORMAT(shader->shader.vs_out_misc_write ?
> + S_02870C_POS1_EXPORT_FORMAT(shader->shader.nr_pos_exports > 1 ?
> V_02870C_SPI_SHADER_4COMP :
> V_02870C_SPI_SHADER_NONE) |
> - S_02870C_POS2_EXPORT_FORMAT((shader->shader.clip_dist_write & 0x0F) ?
> + S_02870C_POS2_EXPORT_FORMAT(shader->shader.nr_pos_exports > 2 ?
> V_02870C_SPI_SHADER_4COMP :
> V_02870C_SPI_SHADER_NONE) |
> - S_02870C_POS3_EXPORT_FORMAT((shader->shader.clip_dist_write & 0xF0) ?
> + S_02870C_POS3_EXPORT_FORMAT(shader->shader.nr_pos_exports > 3 ?
> V_02870C_SPI_SHADER_4COMP :
> V_02870C_SPI_SHADER_NONE));
>
> --
> 1.8.4.rc2
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list