[Mesa-dev] Mesa (master): draw: fix flat shading and screen-space linear interpolation in clipper

[Jose Fonseca]

----- Original Message -----
> On Wed, Jul 11, 2012 at 1:09 PM, Jose Fonseca <jfonseca at vmware.com>
> wrote:
> >
> >
> > ----- Original Message -----
> >>
> >>
> >> ----- Original Message -----
> >> > On Tue, Jul 10, 2012 at 3:30 PM, Jose Fonseca
> >> > <jfonseca at vmware.com>
> >> > wrote:
> >> > > Yep. The interfaces are busted.
> >> > >
> >> > > Without native integers we get
> >> > >
> >> > >    DCL SV[0], INSTANCEID
> >> > >    ...
> >> > >    ARL ADDR[0].x, SV[0].xxxx
> >> > >
> >> > > and with integers we get
> >> > >
> >> > >    DCL SV[0], INSTANCEID
> >> > >    ...
> >> > >    UARL ADDR[0].x, SV[0].xxxx
> >> > >
> >> > > Olivier's fix is incorrect. It works on the above cases by
> >> > > guessing
> >> > > the type, but if we have:
> >> > >
> >> > >    DCL SV[0], INSTANCEID
> >> > >    ...
> >> > >    MOV TEMP[0].x, SV[0].xxxx
> >> > >    ARL ADDR[0].x, TEMP[0].xxxx
> >> > >
> >> > > and
> >> > >
> >> > >    DCL SV[0], INSTANCEID
> >> > >    ...
> >> > >    MOV TEMP[0].x, SV[0].xxxx
> >> > >    UARL ADDR[0].x, TEMP[0].xxxx
> >> > >
> >> > > it is impossible to guess -- the change is merely replacing a
> >> > > bug
> >> > > with another.
> >> > >
> >> > >
> >> > > For the record, the problem also happens without LLVM:
> >> > >
> >> > >   DRAW_USE_LLVM=0 draw-instanced -auto
> >> > >
> >> > >
> >> > >
> >> > > Anyway, AFAICT, all hardware out there that really supports
> >> > > INSTANCEID/VERTEXID also supports native integers, so this is
> >> > > a
> >> > > problem specific to the draw module.
> >> > >
> >> > > But given that draw module can support anything, this is
> >> > > actually
> >> > > self inflicted! In short, INSTANCEID/VERTEXID without integers
> >> > > is
> >> > > an historic artifact, that should not exist going forward.
> >> > >
> >> > > The right fix is merely making sure that
> >> > > PIPE_SHADER_CAP_INTEGERS
> >> > > is accurately advertised as 1 by the draw module (just like
> >> > > Stephane secretly did in
> >> > > 45fc069600ddbfe07a0a0cd5280161a8c7c55dd0
> >> > > :)
> >> >
> >> > I just wanted to tell you Stephane's change cannot work and it
> >> > even
> >> > has no effect at the moment. The native integer support is
> >> > global
> >> > in
> >> > core Mesa. It's because integer uniforms are converted to floats
> >> > based
> >> > on the global NativeInteger flag for all shader stages and that
> >> > can't
> >> > be fixed easily, because uniforms can be shared between shaders.
> >> > Basically, all drivers must advertise integer support either for
> >> > all
> >> > shader stages or none.
> >>
> >> I see..
> >>
> >> > Given that, I only see two possible outcomes:
> >> >
> >> > 1) Disable INSTANCEID support in DX9-level drivers using Draw
> >> > (that's
> >> > only i915g AFAIK) and only support INSTANCEID with integers.
> >>
> >> > 2) Let drivers set the type of system values in Draw, so that
> >> > Draw
> >> > doesn't have to guess what it should be.
> >>
> >> My issue with 2) with this is that TGSI semantics depend on an
> >> external state. I think this is ugly and confusing.
> >>
> >> If Stephane's OK with 1), I'd prefer taking it.
> >
> > My current plan is to:
> > - make it clear that INSTANCEID/VERTEXID always means integer
> > - require PIPE_SHADER_CAP_INTEGERS to be advertise in the vertex
> > shader stage in order to advertise INSTANCEID/VERTEXID in Mesa
> > statetracker
> > - given that Mesa assumes integer, insert a I2F when loading
> > INSTANCEID/VERTEXID (this meets the new semantics while avoiding a
> > big re-architecture)
> 
> The first two points sound good, but why I2F? 

When NativeIntegers is FALSE, Mesa generates this VS:

  DCL SV[0], INSTANCEID
  ...
  ARL ADDR[0].x, SV[0].xxxx

If INSTANCEID is an integer this is wrong, as it is reading a float where there's an integer. (e.g., for INSTANCEID =1 ARRD[0] would have round(0.0000000001) zero).

But, if I modify mesa statetracker to insert the U2F as necessary evertyhing will work as expected

  DCL SV[0], INSTANCEID
  ...
  U2F TEMP[0].x, SV[0].xxxx
  ARL ADDR[0].x, TEMP[0].xxxx

This is essencially what Olivier patch did, but without guessing. Mesa state tracker can known everything it needs to do the "right thing", i.e., INSTANCEID is integer.

> Note that softpipe
> fully
> supports integers while llvmpipe doesn't, and I2F after loading
> INSTANCEID would very likely break softpipe.

softpipe is not effected here, because NativeIntegers will be TRUE, which causes UARL to be used, which is fine, no change needed.

llvmpipe will not be affected either, as it will start advertising NativeIntegers on all stages.

The only affected case are hw drivers which use draw module, but no hw native integer support, like i915.

Jose