[Mesa-dev] [PATCH 2/2] r600g: use SIMPLE_FLOAT for blending to avoid NaNs in RTs

[Ilia Mirkin]

On Mon, Nov 6, 2017 at 8:48 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Mon, Nov 6, 2017 at 6:21 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> On 06.11.2017 05:22, Ilia Mirkin wrote:
>>>
>>> Radeonsi also sets this flag.
>>>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=103544
>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>> ---
>>>
>>> This needs testing with the fbo-float-nan piglit that was recently added.
>>> Just
>>> guessing that this is the right flag to set here.
>>
>>
>> Assuming that the test passes:
>
> Well, the test can't pass or fail. The behavior is undefined. But Dave
> ran it last night to see what would happen. It appears that with the
> current code, if you have a float RB with a infinity or nan in it, and
> then you set the dst blend factor to GL_ZERO, then you'll still end up
> with a NaN in the result.
>
> With this change, you'll end up with what most people would expect
> with a GL_ZERO dst blend factor. Unless your src blend factor is e.g.
> GL_DST_ALPHA, the dst op is multiplied. So if e.g. the RB has
> (0,0,0,infinity), and shader output is (0,0,0,infinity), then src *
> GL_DST_ALPHA + dst * GL_ZERO = (nan, nan, nan, nan). However src *
> GL_SRC_ALPHA + dst + GL_ZERO = (0, 0, 0, infinity).

Ugh. I managed to confuse myself. With the patch and src = dst =
(0,0,0,infinity):

src * GL_DST_ALPHA + dst * GL_ZERO = (nan, nan, nan, nan).
src * GL_SRC_ALPHA + dst * GL_ZERO = (nan, nan, nan, infinity)

>
> Perhaps there's another flag which controls 0 * inf = 0 vs NaN thing
> for blending, but it wasn't immediately apparent from the docs. NVIDIA
> blob drivers configure the hw for 0 * inf = 0.
>
>>
>> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Let me know if this stands, given the above info.
>
>>
>>
>>
>>>
>>>   src/gallium/drivers/r600/evergreen_state.c | 1 +
>>>   src/gallium/drivers/r600/r600_state.c      | 1 +
>>>   2 files changed, 2 insertions(+)
>>>
>>> diff --git a/src/gallium/drivers/r600/evergreen_state.c
>>> b/src/gallium/drivers/r600/evergreen_state.c
>>> index 96eb35a9818..131778dea9f 100644
>>> --- a/src/gallium/drivers/r600/evergreen_state.c
>>> +++ b/src/gallium/drivers/r600/evergreen_state.c
>>> @@ -1211,6 +1211,7 @@ static void
>>> evergreen_set_color_surface_common(struct r600_context *rctx,
>>>                 S_028C70_COMP_SWAP(swap) |
>>>                 S_028C70_BLEND_CLAMP(blend_clamp) |
>>>                 S_028C70_BLEND_BYPASS(blend_bypass) |
>>> +               S_028C70_SIMPLE_FLOAT(1) |
>>>                 S_028C70_NUMBER_TYPE(ntype) |
>>>                 S_028C70_ENDIAN(endian);
>>>   diff --git a/src/gallium/drivers/r600/r600_state.c
>>> b/src/gallium/drivers/r600/r600_state.c
>>> index c21e8dabb1f..0c331537460 100644
>>> --- a/src/gallium/drivers/r600/r600_state.c
>>> +++ b/src/gallium/drivers/r600/r600_state.c
>>> @@ -898,6 +898,7 @@ static void r600_init_color_surface(struct
>>> r600_context *rctx,
>>>                 S_0280A0_COMP_SWAP(swap) |
>>>                 S_0280A0_BLEND_BYPASS(blend_bypass) |
>>>                 S_0280A0_BLEND_CLAMP(blend_clamp) |
>>> +               S_0280A0_SIMPLE_FLOAT(1) |
>>>                 S_0280A0_NUMBER_TYPE(ntype) |
>>>                 S_0280A0_ENDIAN(endian);
>>>
>>
>>
>>
>> --
>> Lerne, wie die Welt wirklich ist,
>> Aber vergiss niemals, wie sie sein sollte.