[Mesa-dev] [PATCH] llvmpipe: handle offset_clamp

[Roland Scheidegger]

Am 27.06.2013 09:41, schrieb Jose Fonseca:
> ----- Original Message -----
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> This was just ignored (unless for some reason like unfilled polys draw was
>> handling this).
> 
> Patch looks good.
> 
>> I'm not convinced of that code, putting the float for the clamp in the key
>> isn't really a good idea. 
> 
> Indeed.
> 
>> Then again the other floats for depth bias are
>> already in there too anyway (should probably have a jit_context for the
>> setup function), so this is just a quick fix.
>> Also, the "minimum resolvable depth difference" used isn't really right as it
>> should be calculated according to the z values of the current primitive
>> and not be a constant (of course, this only makes a difference for float
>> depth buffers), at least for d3d10, so depth biasing is still not quite
>> right.
>> ---
>>  src/gallium/drivers/llvmpipe/lp_state_setup.c   |   20 +++++++++++++++++++-
>>  src/gallium/drivers/llvmpipe/lp_state_setup.h   |    3 ++-
>>  src/gallium/drivers/llvmpipe/lp_state_surface.c |    2 ++
>>  3 files changed, 23 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/drivers/llvmpipe/lp_state_setup.c
>> b/src/gallium/drivers/llvmpipe/lp_state_setup.c
>> index ed68b98..2988bed 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_state_setup.c
>> +++ b/src/gallium/drivers/llvmpipe/lp_state_setup.c
>> @@ -244,6 +244,7 @@ lp_do_offset_tri(struct gallivm_state *gallivm,
>>  {
>>     LLVMBuilderRef b = gallivm->builder;
>>     struct lp_build_context bld;
>> +   struct lp_build_context flt_scalar_bld;
>>     LLVMValueRef zoffset, mult;
>>     LLVMValueRef z0_new, z1_new, z2_new;
>>     LLVMValueRef dzdxdzdy, dzdx, dzdy, dzxyz20, dyzzx01, dyzzx01_dzxyz20,
>>     dzx01_dyz20;
>> @@ -298,6 +299,18 @@ lp_do_offset_tri(struct gallivm_state *gallivm,
>>                             lp_build_const_float(gallivm,
>>                             key->pgon_offset_units),
>>                             mult, "zoffset");
>>  
>> +   lp_build_context_init(&flt_scalar_bld, gallivm, lp_type_float_vec(32,
>> 32));
>> +   if (key->pgon_offset_clamp > 0) {
>> +      zoffset = lp_build_min(&flt_scalar_bld,
>> +                             lp_build_const_float(gallivm,
>> key->pgon_offset_clamp),
>> +                             zoffset);
>> +   }
>> +   else if (key->pgon_offset_clamp < 0) {
>> +      zoffset = lp_build_max(&flt_scalar_bld,
>> +                             lp_build_const_float(gallivm,
>> key->pgon_offset_clamp),
>> +                             zoffset);
>> +   }
>> +
>>     /* yuck */
>>     shuffles[0] = twoi;
>>     shuffles[1] = lp_build_const_int32(gallivm, 6);
>> @@ -312,6 +325,10 @@ lp_do_offset_tri(struct gallivm_state *gallivm,
>>     zoffset = vec4f_from_scalar(gallivm, zoffset, "");
>>  
>>     /* clamp and do offset */
>> +   /*
>> +    * XXX I suspect the clamp (is that even right to always clamp to fixed
>> 0.0/1.0?)
>> +    * should really be per fragment?
>> +    */
> 
> yes. this doesn't look right. Should probably be a FIXME too.
Ok. Draw actually has a similar comment though I believe it is a bit
wrong there (because it is saying offset should be per-fragment too
which I think isn't true at all, just the clamp though I'm unsure where
this comes from in the first place).

Roland


> 
>>     z0z1z2 = lp_build_clamp(&bld, LLVMBuildFAdd(b, z0z1z2, zoffset, ""),
>>     bld.zero, bld.one);
>>  
>>     /* insert into args->a0.z, a1.z, a2.z:
>> @@ -810,7 +827,7 @@ lp_make_setup_variant_key(struct llvmpipe_context *lp,
>>     key->pixel_center_half = lp->rasterizer->half_pixel_center;
>>     key->twoside = lp->rasterizer->light_twoside;
>>     key->size = Offset(struct lp_setup_variant_key,
>> -		      inputs[key->num_inputs]);
>> +                      inputs[key->num_inputs]);
>>  
>>     key->color_slot  = lp->color_slot [0];
>>     key->bcolor_slot = lp->bcolor_slot[0];
>> @@ -823,6 +840,7 @@ lp_make_setup_variant_key(struct llvmpipe_context *lp,
>>  
>>     key->pgon_offset_units = (float) (lp->rasterizer->offset_units *
>>     lp->mrd);
>>     key->pgon_offset_scale = lp->rasterizer->offset_scale;
>> +   key->pgon_offset_clamp = lp->rasterizer->offset_clamp;
>>     key->pad = 0;
>>     memcpy(key->inputs, fs->inputs, key->num_inputs * sizeof key->inputs[0]);
>>     for (i = 0; i < key->num_inputs; i++) {
>> diff --git a/src/gallium/drivers/llvmpipe/lp_state_setup.h
>> b/src/gallium/drivers/llvmpipe/lp_state_setup.h
>> index 73d40a5..c2a2c7f 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_state_setup.h
>> +++ b/src/gallium/drivers/llvmpipe/lp_state_setup.h
>> @@ -14,7 +14,7 @@ struct lp_setup_variant_list_item
>>  };
>>  
>>  
>> -struct lp_setup_variant_key {
>> +struct lp_setup_variant_key {
>>     unsigned size:16;
>>     unsigned num_inputs:8;
>>     int color_slot:8;
>> @@ -29,6 +29,7 @@ struct lp_setup_variant_key {
>>  
>>     float pgon_offset_units;
>>     float pgon_offset_scale;
>> +   float pgon_offset_clamp;
>>     struct lp_shader_input inputs[PIPE_MAX_SHADER_INPUTS];
>>  };
>>  
>> diff --git a/src/gallium/drivers/llvmpipe/lp_state_surface.c
>> b/src/gallium/drivers/llvmpipe/lp_state_surface.c
>> index 375ceb2..e6aac31 100644
>> --- a/src/gallium/drivers/llvmpipe/lp_state_surface.c
>> +++ b/src/gallium/drivers/llvmpipe/lp_state_surface.c
>> @@ -65,6 +65,8 @@ llvmpipe_set_framebuffer_state(struct pipe_context *pipe,
>>        }
>>  
>>        /* Tell draw module how deep the Z/depth buffer is */
>> +      /* FIXME: mrd constant isn't right should use a value derived from
>> +       * current primitive not a constant (for float depth buffers) */
>>        if (lp->framebuffer.zsbuf) {
>>           int depth_bits;
>>           double mrd;
>> --
>> 1.7.9.5
>>
> 
> Jose
>