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

[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...


> -      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?


> 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.


-- 
Earthling Michel Dänzer           |                   http://www.amd.com
Libre software enthusiast         |          Debian, X and DRI developer