[Mesa-dev] [PATCH 2/2] radeonsi/gfx9: fix gl_ViewportIndex

[Nicolai Hähnle]

On 02.05.2017 16:25, Marek Olšák wrote:
> From: Marek Olšák <marek.olsak at amd.com>
>
> Cc: 17.1 <mesa-stable at lists.freedesktop.org>
> ---
>  src/gallium/drivers/radeonsi/si_shader.c        | 43 +++++++++++++++++++++----
>  src/gallium/drivers/radeonsi/si_state_shaders.c | 13 ++++++--
>  2 files changed, 48 insertions(+), 8 deletions(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index aa5ba8d..f19073d 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -2374,22 +2374,22 @@ handle_semantic:
>  		pos_args[0].out[3] = base->one;  /* W */
>  	}
>
>  	/* Write the misc vector (point size, edgeflag, layer, viewport). */
>  	if (shader->selector->info.writes_psize ||
>  	    shader->selector->info.writes_edgeflag ||
>  	    shader->selector->info.writes_viewport_index ||
>  	    shader->selector->info.writes_layer) {
>  		pos_args[1].enabled_channels = shader->selector->info.writes_psize |
>  					       (shader->selector->info.writes_edgeflag << 1) |
> -					       (shader->selector->info.writes_layer << 2) |
> -					       (shader->selector->info.writes_viewport_index << 3);
> +					       (shader->selector->info.writes_layer << 2);
> +
>  		pos_args[1].valid_mask = 0; /* EXEC mask */
>  		pos_args[1].done = 0; /* last export? */
>  		pos_args[1].target = V_008DFC_SQ_EXP_POS + 1;
>  		pos_args[1].compr = 0; /* COMPR flag */
>  		pos_args[1].out[0] = base->zero; /* X */
>  		pos_args[1].out[1] = base->zero; /* Y */
>  		pos_args[1].out[2] = base->zero; /* Z */
>  		pos_args[1].out[3] = base->zero; /* W */
>
>  		if (shader->selector->info.writes_psize)
> @@ -2404,25 +2404,56 @@ handle_semantic:
>  			edgeflag_value = lp_build_min(&bld_base->int_bld,
>  						      edgeflag_value,
>  						      ctx->i32_1);
>
>  			/* The LLVM intrinsic expects a float. */
>  			pos_args[1].out[1] = LLVMBuildBitCast(ctx->gallivm.builder,
>  							  edgeflag_value,
>  							  ctx->f32, "");
>  		}
>
> -		if (shader->selector->info.writes_layer)
> -			pos_args[1].out[2] = layer_value;
> +		if (ctx->screen->b.chip_class >= GFX9) {
> +			/* GFX9 has the layer in out.z[10:0] and the viewport
> +			 * index in out.z[19:16].
> +			 */
> +			if (shader->selector->info.writes_layer) {
> +				LLVMValueRef v = layer_value;
> +
> +				v = bitcast(bld_base, TGSI_TYPE_UNSIGNED, v);
> +				v = LLVMBuildAnd(ctx->gallivm.builder, v,
> +						 LLVMConstInt(ctx->i32, 0x7ff, 0), "");
> +				pos_args[1].out[2] = bitcast(bld_base, TGSI_TYPE_FLOAT, v);
> +			}
> +
> +			if (shader->selector->info.writes_viewport_index) {
> +				LLVMValueRef v = viewport_index_value;
> +
> +				v = bitcast(bld_base, TGSI_TYPE_UNSIGNED, v);
> +				v = LLVMBuildAnd(ctx->gallivm.builder, v,
> +						 LLVMConstInt(ctx->i32, 0xf, 0), "");

I think the ANDs are redundant in both cases. At least for gl_Layer, the 
spec explicitly says that an out-of-bounds value leads to undefined results.

Apart from that, both patches:

Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>


> +				v = LLVMBuildShl(ctx->gallivm.builder, v,
> +						 LLVMConstInt(ctx->i32, 16, 0), "");
> +				v = LLVMBuildOr(ctx->gallivm.builder, v,
> +						bitcast(bld_base, TGSI_TYPE_UNSIGNED,
> +						        pos_args[1].out[2]), "");
> +				pos_args[1].out[2] = bitcast(bld_base, TGSI_TYPE_FLOAT, v);
> +				pos_args[1].enabled_channels |= 1 << 2;
> +			}
> +		} else {
> +			if (shader->selector->info.writes_layer)
> +				pos_args[1].out[2] = layer_value;
>
> -		if (shader->selector->info.writes_viewport_index)
> -			pos_args[1].out[3] = viewport_index_value;
> +			if (shader->selector->info.writes_viewport_index) {
> +				pos_args[1].out[3] = viewport_index_value;
> +				pos_args[1].enabled_channels |= 1 << 3;
> +			}
> +		}
>  	}
>
>  	for (i = 0; i < 4; i++)
>  		if (pos_args[i].out[0])
>  			shader->info.nr_pos_exports++;
>
>  	pos_idx = 0;
>  	for (i = 0; i < 4; i++) {
>  		if (!pos_args[i].out[0])
>  			continue;
> diff --git a/src/gallium/drivers/radeonsi/si_state_shaders.c b/src/gallium/drivers/radeonsi/si_state_shaders.c
> index a5260f5..8c09ff1 100644
> --- a/src/gallium/drivers/radeonsi/si_state_shaders.c
> +++ b/src/gallium/drivers/radeonsi/si_state_shaders.c
> @@ -850,22 +850,31 @@ static void si_shader_vs(struct si_screen *sscreen, struct si_shader *shader,
>  		return;
>
>  	/* We always write VGT_GS_MODE in the VS state, because every switch
>  	 * between different shader pipelines involving a different GS or no
>  	 * GS at all involves a switch of the VS (different GS use different
>  	 * copy shaders). On the other hand, when the API switches from a GS to
>  	 * no GS and then back to the same GS used originally, the GS state is
>  	 * not sent again.
>  	 */
>  	if (!gs) {
> -		si_pm4_set_reg(pm4, R_028A40_VGT_GS_MODE,
> -			       S_028A40_MODE(enable_prim_id ? V_028A40_GS_SCENARIO_A : 0));
> +		unsigned mode = 0;
> +
> +		/* PrimID needs GS scenario A.
> +		 * GFX9 also needs it when ViewportIndex is enabled.
> +		 */
> +		if (enable_prim_id ||
> +		    (sscreen->b.chip_class >= GFX9 &&
> +		     shader->selector->info.writes_viewport_index))
> +			mode = V_028A40_GS_SCENARIO_A;
> +
> +		si_pm4_set_reg(pm4, R_028A40_VGT_GS_MODE, S_028A40_MODE(mode));
>  		si_pm4_set_reg(pm4, R_028A84_VGT_PRIMITIVEID_EN, enable_prim_id);
>  	} else {
>  		si_pm4_set_reg(pm4, R_028A40_VGT_GS_MODE, si_vgt_gs_mode(gs));
>  		si_pm4_set_reg(pm4, R_028A84_VGT_PRIMITIVEID_EN, 0);
>  	}
>
>  	va = shader->bo->gpu_address;
>  	si_pm4_add_bo(pm4, shader->bo, RADEON_USAGE_READ, RADEON_PRIO_SHADER_BINARY);
>
>  	if (gs) {
>


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.