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

[Nicolai Hähnle]

On 06.11.2017 15:40, Ilia Mirkin wrote:
> 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.

Fine by me.

Cheers,
Nicolai


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


-- 
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.