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

[Jose Fonseca]

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

Roland can confirm but I believe TXL is handled separately in exec_tex().

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


Otherwise looks good to me too. Though I can't really comment on nouveau changes.

Jose