[Mesa-dev] [PATCH] draw: implement vertex color clamping
Keith Whitwell
keithw at vmware.com
Wed Mar 30 05:45:50 PDT 2011
On Wed, 2011-03-30 at 14:33 +0200, Marek Olšák wrote:
> From: Luca Barbieri <luca at luca-barbieri.com>
>
> Disclaimer:
> I will not push this code if this patch does not get any attention,
> because I cannot say if it is 100% correct (the code is not mine).
> However last time I checked, it passed all the related tests.
> Also, the SSE and PPC paths are disabled with this code.
> -Marek
>
> Squashed commit of the following:
>
> commit 737c0c6b7d591ac0fc969a7590e1691eeef0ce5e
> Author: Luca Barbieri <luca at luca-barbieri.com>
> Date: Fri Aug 27 02:13:57 2010 +0200
>
> draw: disable SSE and PPC paths (use LLVM instead)
>
> These paths don't support vertex clamping, and are anyway
> obsoleted by LLVM.
>
> If you want to re-enable them, add vertex clamping and test that it
> works with the ARB_color_buffer_float piglit tests.
>
> commit fed3486a7ca0683b403913604a26ee49a3ef48c7
> Author: Luca Barbieri <luca at luca-barbieri.com>
> Date: Thu Aug 26 18:27:38 2010 +0200
>
> draw_llvm: respect vertex color clamp
>
> commit ef0efe9f3d1d0f9b40ebab78940491d2154277a9
> Author: Luca Barbieri <luca at luca-barbieri.com>
> Date: Thu Aug 26 18:26:43 2010 +0200
>
> draw: respect vertex clamping in interpreter path
> ---
> src/gallium/auxiliary/draw/draw_llvm.c | 35 ++++++++++++++++++++++++++--
> src/gallium/auxiliary/draw/draw_llvm.h | 1 +
> src/gallium/auxiliary/draw/draw_vs.c | 7 +++++
> src/gallium/auxiliary/draw/draw_vs_exec.c | 22 ++++++++++++++----
> 4 files changed, 57 insertions(+), 8 deletions(-)
>
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.c b/src/gallium/auxiliary/draw/draw_llvm.c
> index a5217c1..27c5f3b 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.c
> +++ b/src/gallium/auxiliary/draw/draw_llvm.c
> @@ -438,7 +438,8 @@ generate_vs(struct draw_llvm *llvm,
> const LLVMValueRef (*inputs)[NUM_CHANNELS],
> LLVMValueRef system_values_array,
> LLVMValueRef context_ptr,
> - struct lp_build_sampler_soa *draw_sampler)
> + struct lp_build_sampler_soa *draw_sampler,
> + boolean clamp_vertex_color)
> {
> const struct tgsi_token *tokens = llvm->draw->vs.vertex_shader->state.tokens;
> struct lp_type vs_type;
> @@ -474,6 +475,30 @@ generate_vs(struct draw_llvm *llvm,
> outputs,
> sampler,
> &llvm->draw->vs.vertex_shader->info);
> +
> + if(clamp_vertex_color)
> + {
> + LLVMValueRef out;
> + unsigned chan, attrib;
> + struct lp_build_context bld;
> + struct tgsi_shader_info* info = &llvm->draw->vs.vertex_shader->info;
> + lp_build_context_init(&bld, llvm->gallivm, vs_type);
> +
> + for (attrib = 0; attrib < info->num_outputs; ++attrib) {
> + for(chan = 0; chan < NUM_CHANNELS; ++chan) {
> + if(outputs[attrib][chan]) {
> + switch (info->output_semantic_name[attrib]) {
> + case TGSI_SEMANTIC_COLOR:
> + case TGSI_SEMANTIC_BCOLOR:
> + out = LLVMBuildLoad(builder, outputs[attrib][chan], "");
> + out = lp_build_clamp(&bld, out, bld.zero, bld.one);
> + LLVMBuildStore(builder, out, outputs[attrib][chan]);
> + break;
> + }
> + }
> + }
> + }
> + }
> }
>
> #if DEBUG_STORE
> @@ -1235,7 +1260,8 @@ draw_llvm_generate(struct draw_llvm *llvm, struct draw_llvm_variant *variant)
> ptr_aos,
> system_values_array,
> context_ptr,
> - sampler);
> + sampler,
> + variant->key.clamp_vertex_color);
>
> /* store original positions in clip before further manipulation */
> store_clip(gallivm, io, outputs);
> @@ -1446,7 +1472,8 @@ draw_llvm_generate_elts(struct draw_llvm *llvm, struct draw_llvm_variant *varian
> ptr_aos,
> system_values_array,
> context_ptr,
> - sampler);
> + sampler,
> + variant->key.clamp_vertex_color);
>
> /* store original positions in clip before further manipulation */
> store_clip(gallivm, io, outputs);
> @@ -1524,6 +1551,8 @@ draw_llvm_make_variant_key(struct draw_llvm *llvm, char *store)
>
> key = (struct draw_llvm_variant_key *)store;
>
> + key->clamp_vertex_color = llvm->draw->rasterizer->clamp_vertex_color; /**/
> +
> /* Presumably all variants of the shader should have the same
> * number of vertex elements - ie the number of shader inputs.
> */
> diff --git a/src/gallium/auxiliary/draw/draw_llvm.h b/src/gallium/auxiliary/draw/draw_llvm.h
> index e8623e7..643a9ef 100644
> --- a/src/gallium/auxiliary/draw/draw_llvm.h
> +++ b/src/gallium/auxiliary/draw/draw_llvm.h
> @@ -162,6 +162,7 @@ struct draw_llvm_variant_key
> {
> unsigned nr_vertex_elements:8;
> unsigned nr_samplers:8;
> + unsigned clamp_vertex_color:8;
> unsigned clip_xy:1;
> unsigned clip_z:1;
> unsigned clip_user:1;
Why are there 8 bits for this?
I'd suggest 1 bit is sufficient, and that you should take one bit from
"pad" to make space for it.
> diff --git a/src/gallium/auxiliary/draw/draw_vs.c b/src/gallium/auxiliary/draw/draw_vs.c
> index 7caad6f..1763dbc 100644
> --- a/src/gallium/auxiliary/draw/draw_vs.c
> +++ b/src/gallium/auxiliary/draw/draw_vs.c
> @@ -104,11 +104,18 @@ draw_create_vertex_shader(struct draw_context *draw,
> }
>
> if (!draw->pt.middle.llvm) {
> +#if 0
> +/* these paths don't support vertex clamping
> + * TODO: either add it, or remove them completely
> + * use LLVM instead if you want performance
> + * use exec instead if you want debugging/more correctness
> + */
> #if defined(PIPE_ARCH_X86)
> vs = draw_create_vs_sse( draw, shader );
> #elif defined(PIPE_ARCH_PPC)
> vs = draw_create_vs_ppc( draw, shader );
> #endif
> +#endif
Please god, let's just kill those paths.
> }
> #if HAVE_LLVM
> else {
> diff --git a/src/gallium/auxiliary/draw/draw_vs_exec.c b/src/gallium/auxiliary/draw/draw_vs_exec.c
> index c41d7c4..d9c4209 100644
> --- a/src/gallium/auxiliary/draw/draw_vs_exec.c
> +++ b/src/gallium/auxiliary/draw/draw_vs_exec.c
> @@ -95,6 +95,7 @@ vs_exec_run_linear( struct draw_vertex_shader *shader,
> struct tgsi_exec_machine *machine = evs->machine;
> unsigned int i, j;
> unsigned slot;
> + boolean clamp_vertex_color = shader->draw->rasterizer->clamp_vertex_color;
>
> tgsi_exec_set_constant_buffers(machine, PIPE_MAX_CONSTANT_BUFFERS,
> constants, const_size);
> @@ -151,11 +152,22 @@ vs_exec_run_linear( struct draw_vertex_shader *shader,
> */
> for (j = 0; j < max_vertices; j++) {
> for (slot = 0; slot < shader->info.num_outputs; slot++) {
> - output[slot][0] = machine->Outputs[slot].xyzw[0].f[j];
> - output[slot][1] = machine->Outputs[slot].xyzw[1].f[j];
> - output[slot][2] = machine->Outputs[slot].xyzw[2].f[j];
> - output[slot][3] = machine->Outputs[slot].xyzw[3].f[j];
> -
> + unsigned name = shader->info.output_semantic_name[slot];
> + if(clamp_vertex_color &&
> + (name == TGSI_SEMANTIC_COLOR || name == TGSI_SEMANTIC_BCOLOR))
> + {
> + output[slot][0] = CLAMP(machine->Outputs[slot].xyzw[0].f[j], 0.0f, 1.0f);
> + output[slot][1] = CLAMP(machine->Outputs[slot].xyzw[1].f[j], 0.0f, 1.0f);
> + output[slot][2] = CLAMP(machine->Outputs[slot].xyzw[2].f[j], 0.0f, 1.0f);
> + output[slot][3] = CLAMP(machine->Outputs[slot].xyzw[3].f[j], 0.0f, 1.0f);
> + }
> + else
> + {
> + output[slot][0] = machine->Outputs[slot].xyzw[0].f[j];
> + output[slot][1] = machine->Outputs[slot].xyzw[1].f[j];
> + output[slot][2] = machine->Outputs[slot].xyzw[2].f[j];
> + output[slot][3] = machine->Outputs[slot].xyzw[3].f[j];
> + }
> }
>
> #if 0
Otherwise, it looks good to me.
Keith
More information about the mesa-dev
mailing list