[Mesa-dev] [PATCH] gallium: fix tgsi SAMPLE_L opcode to use separate source for explicit lod

[Roland Scheidegger]

Am 12.02.2013 08:06, schrieb Michel Dänzer:
> On Mon, 2013-02-11 at 20:47 +0100, sroland at vmware.com wrote: 
>> From: Roland Scheidegger <sroland at vmware.com>
>>
>> It looks like using coord.w as explicit lod value is a mistake, most likely
>> because some dx10 docs had it specified that way. Seems this was changed though:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/hh447229%28v=vs.85%29.aspx
>> - let's just hope it doesn't depend on runtime build version or something.
>> Not only would this need translation (so go against the stated goal these
>> opcodes should be close to dx10 semantics) but it would prevent usage of this
>> opcode with cube arrays, which is apparently possible:
>> http://msdn.microsoft.com/en-us/library/windows/desktop/bb509699%28v=vs.85%29.aspx
>> (Note not only does this show cube arrays using explicit lod, but also the
>> confusion with this opcode: it lists an explicit lod parameter value, but then
>> states last component of location is used as lod).
>> (For "true" hw drivers, only nv50 had code to handle it, and it appears the
>> code was already right for the new semantics, though fix up the seemingly
>> wrong c/d arguments while there.)
>> ---
>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_info.c   |    5 +----
>>  src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c    |    2 +-
>>  src/gallium/auxiliary/tgsi/tgsi_exec.c             |    2 +-
>>  src/gallium/auxiliary/tgsi/tgsi_info.c             |    2 +-
>>  src/gallium/auxiliary/tgsi/tgsi_opcode_tmp.h       |    2 +-
>>  src/gallium/docs/source/tgsi.rst                   |   12 ++++++------
>>  .../drivers/nv50/codegen/nv50_ir_from_tgsi.cpp     |    2 +-
>>  .../state_trackers/d3d1x/gd3d1x/sm4_to_tgsi.cpp    |    9 ++-------
>>  8 files changed, 14 insertions(+), 22 deletions(-)
>>
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_info.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_info.c
>> index cb6564a..3a19fe2 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_info.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_info.c
>> @@ -241,13 +241,10 @@ analyse_sample(struct analysis_context *ctx,
>>        tex_info->sampler_unit = inst->Src[2].Register.Index;
>>  
>>        if (modifier == LP_BLD_TEX_MODIFIER_EXPLICIT_DERIV ||
>> +          modifier == LP_BLD_TEX_MODIFIER_EXPLICIT_LOD ||
>>            modifier == LP_BLD_TEX_MODIFIER_LOD_BIAS || shadow) {
>>           /* We don't track insts with additional regs, although we could */
>>           indirect = TRUE;
>> -      }  else {
>> -         if (modifier == LP_BLD_TEX_MODIFIER_EXPLICIT_LOD) {
>> -            readmask |= TGSI_WRITEMASK_W;
>> -         }
>>        }
>>  
>>        for (chan = 0; chan < 4; ++chan) {
>> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> index 52a60dd..f816103 100644
>> --- a/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> +++ b/src/gallium/auxiliary/gallivm/lp_bld_tgsi_soa.c
>> @@ -1430,7 +1430,7 @@ emit_sample(struct lp_build_tgsi_soa_context *bld,
>>     else if (modifier == LP_BLD_TEX_MODIFIER_EXPLICIT_LOD) {
>>        /* lod bias comes from src 3.r but explicit lod from 0.a */
> 
> This comment would need to be updated...
Right, I missed that.

> 
> 
>> -      explicit_lod = lp_build_emit_fetch( &bld->bld_base, inst, 0, 3 );
>> +      explicit_lod = lp_build_emit_fetch( &bld->bld_base, inst, 3, 0 );
>>     }
>>     else if (modifier == LP_BLD_TEX_MODIFIER_LOD_ZERO) {
>>        lod_bias = NULL;
>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.c b/src/gallium/auxiliary/tgsi/tgsi_exec.c
>> index 6da7d42..03f1942 100644
>> --- a/src/gallium/auxiliary/tgsi/tgsi_exec.c
>> +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.c
>> @@ -2154,7 +2154,7 @@ exec_sample(struct tgsi_exec_machine *mach,
>>           control = tgsi_sampler_lod_bias;
>>        }
>>        else if (modifier == TEX_MODIFIER_EXPLICIT_LOD) {
>> -         FETCH(&c1, 0, TGSI_CHAN_W);
>> +         FETCH(&c1, 3, TGSI_CHAN_X);
>>           lod = &c1;
>>           control = tgsi_sampler_lod_explicit;
>>        }
> 
> ... but don't these changes break TXL?
No, this code isn't used for the "old" sampling opcodes at all - I
didn't unify exec_sample and exec_tex because of too many such differences.

> 
> 
>> diff --git a/src/gallium/docs/source/tgsi.rst b/src/gallium/docs/source/tgsi.rst
>> index 31b6796..dd4c773 100644
>> --- a/src/gallium/docs/source/tgsi.rst
>> +++ b/src/gallium/docs/source/tgsi.rst
>> @@ -1383,9 +1383,10 @@ instructions. If in doubt double check Direct3D documentation.
>>  
>>  .. opcode:: SAMPLE_I_MS - Just like SAMPLE_I but allows fetch data from
>>                 multi-sampled surfaces.
>> +               SAMPLE_I_MS dst, address, sampler_view, sample
>>  
>>  .. opcode:: SAMPLE_B - Just like the SAMPLE instruction with the
>> -               exception that an additiona bias is applied to the
>> +               exception that an additional bias is applied to the
>>                 level of detail computed as part of the instruction
>>                 execution.
>>                 SAMPLE_B dst, address, sampler_view, sampler, lod_bias
>> @@ -1394,7 +1395,7 @@ instructions. If in doubt double check Direct3D documentation.
>>  
>>  .. opcode:: SAMPLE_C - Similar to the SAMPLE instruction but it
>>                 performs a comparison filter. The operands to SAMPLE_C
>> -               are identical to SAMPLE, except that tere is an additional
>> +               are identical to SAMPLE, except that there is an additional
>>                 float32 operand, reference value, which must be a register
>>                 with single-component, or a scalar literal.
>>                 SAMPLE_C makes the hardware use the current samplers
> 
> These cleanups should be in a separate change.
Ok.

Roland