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

[Roland Scheidegger]

Am 26.06.2018 um 09:59 schrieb Gert Wollny:
> Am Montag, den 25.06.2018, 23:47 +0200 schrieb Roland Scheidegger:
>>
>> Alright albeit you have logic to handle incoming z offsets, whereas
>> that should always be 0.
> I'm preparing for future standards that allow for these z-offsets ;) 
> 
>> To be honest I'm actually kind of surprised the hw would honor texel
>> offsets for array coordinates (the term "texel offset" wouldn't even
>> apply) - this is done after denormalization and coord wrapping,
>> neither of which apply to the array coord.
> I was also kind of expecting that setting *ARRAY as texture format
> would take care of the rest, but apparently it doesn't.
The mysteries of hw :-).
I'm wondering what the blob did.

> 
> 
>>
>> Would be surprising if the hw did trunc instead of round (or
>> +0.5/floor), are you sure that's really (always) the case? I'm
>> wondering if there could be some (bogus) dependency on other
>> texture/sampler state (similar to the gather issues when selecting
>> the wrong texels for int textures).
> I honestly don't know. I mostly to took the approach with a TDD
> perspective: have some tests and make them pass, hopefully without
> breaking anything. I was also surprised that the array index supports
> floating point input, but maybe the hardware just handles the 2D array
> texture like a 3D texture and one can 
> 
>> At a quick glance radeonsi doesn't seem to do anything like this, and
>> generally I think the sampler hw shares most of the bugs, at least
>> for early gcn chips... (albeit I don't know if the tests would pass
>> there neither).
> I have no access to any GNC hardware, so I can't comment on what may or
> may not be the case. I have Barts and Cedar cards (both VLIW5), and for
> these two the patches work.
> 
>> But anyway, if the hw really does trunc and not round for array slice
>> selection then that is the right thing to do indeed (a directed test
>> should easily reveal the switch-over points).
>>
>>>
>>>> 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.
>>>
>>>
>>
> [...]
> 
>> Although TRUNCATE_COORD would apply to all coords, which seems like a
>> bad idea (regardless what the hell those z_filter/truncate_coord
>> values actually do...). (Among other things, it would mean with point
>> filtering, you could actually get different texels selected depending
>> if you declared a texture as 2d or as 2d array with 1 layer - not
>> good.)
> Fair enough, but this is weighting one inaccuracy against another one:
>  
> In point mode you get either trunc or this wired round to n.6 and then
> trunc. The difference is that for this rounding mode you get
> [i, i + 0.984375] -> i  and (i + 0.984375, i+1] -> i+1
> and for trunc you get [i, i + 1) -> i. Considering how arrays are used,
> getting the slice right seems to me more important than being off by
> less then a 1/10 of a pixel when apllying the texture.
Well I'd say considering how array textures are used, you're crazy if
you use random floats and rely on exact rounding in your app :-).
Incorrect texel selection however has been shown to be a problem in the
past (in particular because it's possible to get it wrong and end up
with coords shifted by half a texel if you're not careful, which makes
the app rely on exact implementation there - yes it's basically an app bug).



> 
> You are right that not all tests that are in the GLES CTS are required
> to pass and I will have to correct the commit messsage when I re-
> submit, because some related tests still fail, but they are not part of
> the master-android3 test set, so I would guess they are kind of nice-
> to-have but not really expected to pass (regardless on how this TRUNC
> value is set).
> 
>> Regardless the "bogus" test, that is simply based on GL not actually
>> requiring any specific precision in that area, therefore it's not
>> reasonable to expect 100% accurate (to float) results. That is, I
>> think it would be well allowed to round 1.49999 to 2 there. So imho
>> there's no value in trying to fix this just to allow the test to
>> pass.
> In a way you are probably also right with this, but there is of course
> another incentive: Currently, we are working on getting virgl pass
> these tests, and they pass on an Intel host, on an radeonsi host, and
> on an Nvidia host, so it would be nice to get the same results on r600,
>  to make sure that whatever problem arises we know that it is a bug in
> virgl and not in the host driver.
> 
> Best, 
> Gert
> 

Alright, I suppose as long as it doesn't break anything... I'm really no
proponent though of adding workarounds just for tests, if the failures
don't actually indicate a real bug.

Roland