[Mesa-dev] [PATCH 0/2] r600: Fix array texture slice index evaluation

[Gert Wollny]

Am Montag, den 25.06.2018, 17:36 +0200 schrieb Roland Scheidegger:
> I didn't actually get the original email for some reason, so can't
> comment inline as I'm just looking it up at patchwork...
> But the array offset stuff (the first patch) looks completely bogus
> to me, array textures do not support offsets for the array index, at
> least not in any shader language I know of.
It is legal to derive the texture array slice index from float value
and the formula given in the standard is "floor(z + 0.5)". 

The normal rounding mode doesn't handle this, probably because it
actually does "round-nearest-even to n.6 and drop fraction when point
sampling", so it doesn't really round to integers, but to some fixed
point format that then might get truncated. (It is not written what is
done if the TEX_Z_FILTER_NONE is set, but I assume it must be trunc
just like with TEX_Z_FILTER_POINT). When looking at the test results I
figured out that the slice selection must be off, and voila,  half of
the tests are fixed by adding 0.5 to the z-coordinate, be it by
injecting the code into the shader that adds this value to the
coordinate or by putting this value into the offsets. The first
approach is obviously cheaper, because it doesn't add any instruction.
I don't think that it is relevant whether the shader language supports
this, because in the end this gets translated to byte code that needs
to communicate the intent to the hardware.

> I'm not really sure about the 2nd patch, what exactly is the
> difference with the ordinary rounding and the new one? Is there just
> a difference for values exactly between 2 integers (e.g. 1.5 etc.)?
> In this case I would suspect the driver is allowed to pick either
> value and the test is bogus.
The remaining tests fail for an inaccuracy in the rounding mode (These
tests explicitly check rounding by varying the slice index from 1.49999
to 1.50001). The default rounding mode that rounds to n.6 fixed point
can not capturere this accuracy, and therefore these tests fail. Using
trunc, which for positive values is equal to floor, fixes this, and
given that the standard is explicitely written to use floor, I don't
think that the tests are bogous. One could probably achieve the same in
the shader, but again, programming the sampler like this saves
instructions in the shader. 

> If that's not the case, it looks like it actually needs to be one
> patch?
> You're adding the 0.5 offset in one but adjust the sampler state
> which probably needs to match in another?
Well, it is two patches because that's how I went forward fixing these
tests (like described above), but it would probably okay to squash
them. I'll have to revise the first patch anyway.  

best, 
Gert

> Roland
> 
> Am 25.06.2018 um 07:54 schrieb Dave Airlie:
> > /home/airlied/devel/piglit/bin/textureGather fs nonconst r 0 float
> > 2DArray repeat
> > 
> > amongst others appears to regress with these two.
> > 
> > Dave.
> > 
> > 
> > On 22 June 2018 at 19:37, Gert Wollny <gert.wollny at collabora.com>
> > wrote:
> > > these two patches correct the offets and the rounding modes for
> > > the
> > > index evaluation when accessing texture arrays. The patches were
> > > tested
> > > with the gles3 test suite where they fix a number of tests
> > > related to 2D
> > > texture arrays and didn't show any regressions.
> > > 
> > > Best,
> > > Gert
> > > 
> > > Gert Wollny (2):
> > >   r600: correct texture offset for array index lookup
> > >   r600: set rounding mode for texture array layer selection
> > > 
> > >  src/gallium/drivers/r600/evergreen_state.c | 21 ++++++++++
> > >  src/gallium/drivers/r600/r600_shader.c     | 64
> > > +++++++++++++++++++++++++++++-
> > >  2 files changed, 84 insertions(+), 1 deletion(-)
> > > 
> > > --
> > > 2.16.4
> > > 
> > 
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fli
> > sts.freedesktop.org%2Fmailman%2Flistinfo%2Fmesa-
> > dev&data=02%7C01%7Csroland%40vmware.com%7C038ae632c8354209810c08d5d
> > a601219%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C1%7C1%7C63665502853319
> > 4684&sdata=gvQB0DNGvaNwBN5EHlJNrXED4biq3xC4rFqNEYqS3xs%3D&reserved=
> > 0
> > 
> 
>