[Mesa-stable] [Mesa-dev] [PATCH 2/5] r600: use DX10_CLAMP bit in shader setup

Andres Gomez agomez at igalia.com
Tue Nov 28 14:05:09 UTC 2017


Nicolai, this looks like a good candidate to nominate for inclusion in
all the stable queues.

What do you think?

On Thu, 2017-11-09 at 20:00 +0100, sroland at vmware.com wrote:
> From: Roland Scheidegger <sroland at vmware.com>
> 
> The docs are not very concise in what this really does, however both
> Alex Deucher and Nicolai Hähnle suggested this only really affects instructions
> using the CLAMP output modifier, and I've confirmed that with the newly
> changed piglit isinf_and_isnan test.
> So, with this bit set, if an instruction has the CLAMP modifier bit (which
> clamps to [0,1]) set, then NaNs will be converted to zero, otherwise the result
> will be NaN.
> D3D10 would require this, glsl doesn't have modifiers (with mesa
> clamp(x,0,1) would get converted to such a modifier) coupled with a
> whatever-floats-your-boat specified NaN behavior, but the clamp behavior
> should probably always be used (this also matches what a decomposition into
> min(1.0, max(x, 0.0)) would do, if min/max also adhere to the ieee spec of
> picking the non-nan result).
> Some apps may in fact rely on this, as this prevents misrenderings in
> This War of Mine since using ieee muls
> (ce7a045feeef8cad155f1c9aa07f166e146e3d00), without having to use clamped
> rcp opcode, which would also fix this bug there.
> radeonsi also seems to set this bit nowadays if I see that righ (albeit the
> llvm amdgpu code comment now says "Make clamp modifier on NaN input returns 0"
> instead of "Do not clamp NAN to 0" since it was changed, which also looks
> a bit misleading).
> 
> v2: set it in all shader stages.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103544
> ---
>  src/gallium/drivers/r600/evergreen_state.c | 6 ++++++
>  src/gallium/drivers/r600/r600_state.c      | 9 +++++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/src/gallium/drivers/r600/evergreen_state.c b/src/gallium/drivers/r600/evergreen_state.c
> index 96eb35a981..ef323bf4f6 100644
> --- a/src/gallium/drivers/r600/evergreen_state.c
> +++ b/src/gallium/drivers/r600/evergreen_state.c
> @@ -3235,6 +3235,7 @@ void evergreen_update_ps_state(struct pipe_context *ctx, struct r600_pipe_shader
>  	r600_store_value(cb, /* R_028844_SQ_PGM_RESOURCES_PS */
>  			 S_028844_NUM_GPRS(rshader->bc.ngpr) |
>  			 S_028844_PRIME_CACHE_ON_DRAW(1) |
> +			 S_028844_DX10_CLAMP(1) |
>  			 S_028844_STACK_SIZE(rshader->bc.nstack));
>  	/* After that, the NOP relocation packet must be emitted (shader->bo, RADEON_USAGE_READ). */
>  
> @@ -3255,6 +3256,7 @@ void evergreen_update_es_state(struct pipe_context *ctx, struct r600_pipe_shader
>  
>  	r600_store_context_reg(cb, R_028890_SQ_PGM_RESOURCES_ES,
>  			       S_028890_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_028890_DX10_CLAMP(1) |
>  			       S_028890_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_02888C_SQ_PGM_START_ES,
>  			       shader->bo->gpu_address >> 8);
> @@ -3317,6 +3319,7 @@ void evergreen_update_gs_state(struct pipe_context *ctx, struct r600_pipe_shader
>  
>  	r600_store_context_reg(cb, R_028878_SQ_PGM_RESOURCES_GS,
>  			       S_028878_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_028878_DX10_CLAMP(1) |
>  			       S_028878_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_028874_SQ_PGM_START_GS,
>  			       shader->bo->gpu_address >> 8);
> @@ -3357,6 +3360,7 @@ void evergreen_update_vs_state(struct pipe_context *ctx, struct r600_pipe_shader
>  			       S_0286C4_VS_EXPORT_COUNT(nparams - 1));
>  	r600_store_context_reg(cb, R_028860_SQ_PGM_RESOURCES_VS,
>  			       S_028860_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_028860_DX10_CLAMP(1) |
>  			       S_028860_STACK_SIZE(rshader->bc.nstack));
>  	if (rshader->vs_position_window_space) {
>  		r600_store_context_reg(cb, R_028818_PA_CL_VTE_CNTL,
> @@ -3391,6 +3395,7 @@ void evergreen_update_hs_state(struct pipe_context *ctx, struct r600_pipe_shader
>  	r600_init_command_buffer(cb, 32);
>  	r600_store_context_reg(cb, R_0288BC_SQ_PGM_RESOURCES_HS,
>  			       S_0288BC_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_0288BC_DX10_CLAMP(1) |
>  			       S_0288BC_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_0288B8_SQ_PGM_START_HS,
>  			       shader->bo->gpu_address >> 8);
> @@ -3404,6 +3409,7 @@ void evergreen_update_ls_state(struct pipe_context *ctx, struct r600_pipe_shader
>  	r600_init_command_buffer(cb, 32);
>  	r600_store_context_reg(cb, R_0288D4_SQ_PGM_RESOURCES_LS,
>  			       S_0288D4_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_0288D4_DX10_CLAMP(1) |
>  			       S_0288D4_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_0288D0_SQ_PGM_START_LS,
>  			       shader->bo->gpu_address >> 8);
> diff --git a/src/gallium/drivers/r600/r600_state.c b/src/gallium/drivers/r600/r600_state.c
> index c21e8dabb1..db3d6db70b 100644
> --- a/src/gallium/drivers/r600/r600_state.c
> +++ b/src/gallium/drivers/r600/r600_state.c
> @@ -2548,6 +2548,12 @@ void r600_update_ps_state(struct pipe_context *ctx, struct r600_pipe_shader *sha
>  	r600_store_context_reg_seq(cb, R_028850_SQ_PGM_RESOURCES_PS, 2);
>  	r600_store_value(cb, /* R_028850_SQ_PGM_RESOURCES_PS*/
>  			 S_028850_NUM_GPRS(rshader->bc.ngpr) |
> +	/*
> +	 * docs are misleading about the dx10_clamp bit. This only affects
> +	 * instructions using CLAMP dst modifier, in which case they will
> +	 * return 0 with this set for a NaN (otherwise NaN).
> +	 */
> +			 S_028850_DX10_CLAMP(1) |
>  			 S_028850_STACK_SIZE(rshader->bc.nstack) |
>  			 S_028850_UNCACHED_FIRST_INST(ufi));
>  	r600_store_value(cb, exports_ps); /* R_028854_SQ_PGM_EXPORTS_PS */
> @@ -2597,6 +2603,7 @@ void r600_update_vs_state(struct pipe_context *ctx, struct r600_pipe_shader *sha
>  			       S_0286C4_VS_EXPORT_COUNT(nparams - 1));
>  	r600_store_context_reg(cb, R_028868_SQ_PGM_RESOURCES_VS,
>  			       S_028868_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_028868_DX10_CLAMP(1) |
>  			       S_028868_STACK_SIZE(rshader->bc.nstack));
>  	if (rshader->vs_position_window_space) {
>  		r600_store_context_reg(cb, R_028818_PA_CL_VTE_CNTL,
> @@ -2681,6 +2688,7 @@ void r600_update_gs_state(struct pipe_context *ctx, struct r600_pipe_shader *sha
>  
>  	r600_store_context_reg(cb, R_02887C_SQ_PGM_RESOURCES_GS,
>  			       S_02887C_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_02887C_DX10_CLAMP(1) |
>  			       S_02887C_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_02886C_SQ_PGM_START_GS, 0);
>  	/* After that, the NOP relocation packet must be emitted (shader->bo, RADEON_USAGE_READ). */
> @@ -2695,6 +2703,7 @@ void r600_update_es_state(struct pipe_context *ctx, struct r600_pipe_shader *sha
>  
>  	r600_store_context_reg(cb, R_028890_SQ_PGM_RESOURCES_ES,
>  			       S_028890_NUM_GPRS(rshader->bc.ngpr) |
> +			       S_028890_DX10_CLAMP(1) |
>  			       S_028890_STACK_SIZE(rshader->bc.nstack));
>  	r600_store_context_reg(cb, R_028880_SQ_PGM_START_ES, 0);
>  	/* After that, the NOP relocation packet must be emitted (shader->bo, RADEON_USAGE_READ). */
-- 
Br,

Andres


More information about the mesa-stable mailing list