[PATCH] wined3d: Unbind the FBOs before glDispatchCompute

[Fabian Maurer]

Fixes Bug 43406

Unbinding the FBO before executing the compute shader
makes sure that it doesn't read garbage from it.
This only an issue on open source drivers, since the
proprietary drivers play safe and synchronize.
But since this is undefined behavior, wine should avoid relying on that.

Many thanks to Matias N. Goldberg and Philip Rebohle
for investigating and solving the bug!

Signed-off-by: Fabian Maurer <dark.shadow4(a)web.de>
---
 dlls/wined3d/drawprim.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/dlls/wined3d/drawprim.c b/dlls/wined3d/drawprim.c
index 404623c9ac..f39c9961e9 100644
--- a/dlls/wined3d/drawprim.c
+++ b/dlls/wined3d/drawprim.c
@@ -858,6 +858,10 @@ void dispatch_compute(struct wined3d_device *device, const struct wined3d_state
         return;
     }
 
+    /* Unbind the FBOs to avoid have undefined behavior when reading data from it */
+    GL_EXTCALL(glBindFramebuffer(GL_READ_FRAMEBUFFER, 0));
+    GL_EXTCALL(glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0));
+
     if (parameters->indirect)
     {
         const struct wined3d_indirect_dispatch_parameters *indirect = ¶meters->u.indirect;
@@ -875,6 +879,10 @@ void dispatch_compute(struct wined3d_device *device, const struct wined3d_state
     }
     checkGLcall("dispatch compute");
 
+    /* Now rebind the FBOs to restore the original state */
+    GL_EXTCALL(glBindFramebuffer(GL_READ_FRAMEBUFFER, context->fbo_read_binding));
+    GL_EXTCALL(glBindFramebuffer(GL_DRAW_FRAMEBUFFER, context->fbo_draw_binding));
+
     GL_EXTCALL(glMemoryBarrier(GL_ALL_BARRIER_BITS));
     checkGLcall("glMemoryBarrier");
 
-- 
2.15.0

[Henri Verbeet]

On 25 November 2017 at 20:58, Fabian Maurer <dark.shadow4(a)web.de> wrote:
> @@ -858,6 +858,10 @@ void dispatch_compute(struct wined3d_device *device, const struct wined3d_state
>          return;
>      }
>
> +    /* Unbind the FBOs to avoid have undefined behavior when reading data from it */
> +    GL_EXTCALL(glBindFramebuffer(GL_READ_FRAMEBUFFER, 0));
> +    GL_EXTCALL(glBindFramebuffer(GL_DRAW_FRAMEBUFFER, 0));
> +
>      if (parameters->indirect)
>      {
>          const struct wined3d_indirect_dispatch_parameters *indirect = ¶meters->u.indirect;
> @@ -875,6 +879,10 @@ void dispatch_compute(struct wined3d_device *device, const struct wined3d_state
>      }
>      checkGLcall("dispatch compute");
>
> +    /* Now rebind the FBOs to restore the original state */
> +    GL_EXTCALL(glBindFramebuffer(GL_READ_FRAMEBUFFER, context->fbo_read_binding));
> +    GL_EXTCALL(glBindFramebuffer(GL_DRAW_FRAMEBUFFER, context->fbo_draw_binding));
> +
>      GL_EXTCALL(glMemoryBarrier(GL_ALL_BARRIER_BITS));
>      checkGLcall("glMemoryBarrier");

I think I'd feel better about this if context_apply_compute_state()
did a context_bind_fbo() followed by a context_invalidate_state()
instead.

[Fabian Maurer]

On Freitag, 1. Dezember 2017 13:49:59 CET Henri Verbeet wrote:
> I think I'd feel better about this if context_apply_compute_state()
> did a context_bind_fbo() followed by a context_invalidate_state()
> instead.

Alright, adding
> context_bind_fbo(context, GL_FRAMEBUFFER, 0);
> context_invalidate_state(context, STATE_FRAMEBUFFER);
> in "context_apply_compute_state" makes it render properly, too.
> But don't we need to re-bind the old framebuffer(s) afterwards?

Regards,
Fabian Maurer

[Henri Verbeet]

On 1 December 2017 at 20:25, Fabian Maurer <dark.shadow4(a)web.de> wrote:
> On Freitag, 1. Dezember 2017 13:49:59 CET Henri Verbeet wrote:
> Alright, adding
> > context_bind_fbo(context, GL_FRAMEBUFFER, 0);
> > context_invalidate_state(context, STATE_FRAMEBUFFER);
>
> in "context_apply_compute_state" makes it render properly, too.
>
> But don't we need to re-bind the old framebuffer(s) afterwards?

Not necessarily. The correct framebuffer needs to be bound for
subsequent draws, but that may very well be a different framebuffer
than the one currently bound. Invalidating STATE_FRAMEBUFFER ensures
that context_apply_draw_state() will bind the correct framebuffer even
if the bound render targets didn't change since the previous draw.

[Fabian Maurer]

> Not necessarily. The correct framebuffer needs to be bound for
> subsequent draws, but that may very well be a different framebuffer
> than the one currently bound. Invalidating STATE_FRAMEBUFFER ensures
> that context_apply_draw_state() will bind the correct framebuffer even
> if the bound render targets didn't change since the previous draw.
> No, the invalidated state is going to be applied again by wined3d
> "state manager", if needed.

Thanks for explaining, that's a clever setup.
I'll send an updated patch.

Regards,
Fabian Maurer

[Józef Kucia]

On Fri, Dec 1, 2017 at 5:55 PM, Fabian Maurer <dark.shadow4(a)web.de> wrote:
> > context_invalidate_state(context, STATE_FRAMEBUFFER);
>
> in "context_apply_compute_state" makes it render properly, too.
>
> But don't we need to re-bind the old framebuffer(s) afterwards?

No, the invalidated state is going to be applied again by wined3d
"state manager", if needed.