On Fri, Jun 12, 2009 at 1:56 PM, Allen Akin<a...@pobox.com> wrote:
> On Fri, Jun 12, 2009 at 10:25:42AM +1000, Dave Airlie wrote:
> | On Fri, Jun 12, 2009 at 10:01 AM, Allen Akin<a...@pobox.com> wrote:
> | > On the other hand, if there's no mechanism for implicitly flushing the
> | > GL command stream on window teardown, then whatever problems this error
> | > is designed to address can happen every time a window is closed. I
> | > would expect to find something in the spec that says "You must execute
> | > (SwapBuffers|Flush|Finish...) before destroying a bound window or
> | > such-and-such bad things can happen." Trivial test programs would have
> | > been failing since day one.
> |
> | Well usually when the window is torndown, we exit straight away afterwards,
> | normally you don't keep going...
>
> I don't think you have to keep going -- all you have to do is destroy
> the window when there are still GL commands that haven't been flushed.
> It looks like the same (or very similar) problem to me; what do you do
> with those commands that have been queued up for a now-nonexistent
> window?
>
> | ... however the glean test case does another
> | test which opens a new window and rendering context and calls MakeCurrent
> | on it, thereby triggering the above failure case. esp after it has
> | done rendering
> | on the previous context and then blown away the window without flushing or
> | swapbuffers.
>
> I didn't trace the test, so I'm not fully confident about this, but it
> looks to me like this is the sequence of commands:
>
> Create window.
> Create rendering contexts.
> Make an RC current.
> Clear the buffer.
> Query the clear color.
> ReadPixels() to get the contents of the buffer.
> <repeat>
> Destroy rendering contexts.
> (one RC is still current, so its destruction is postponed)
> Destroy window.
> ...
> A subsequent test does a MakeCurrent, which triggers the error.
>
> The ReadPixels() does an implicit flush. Since it's the last operation
> before the MakeCurrent, and it's synchronous, there should be no other
> commands remaining in the GL stream at the time of the MakeCurrent. If
> that's correct, it explains why this problem has never been detected
> before, and it suggests that there might still be an implementation bug
> to be tracked down. What's left in the queue, or is it marked nonempty
> even though it's really empty?
>
Okay I can also fix this using the attached patch.
So the issue is makeCurrent sets up 3 contexts, one null, one direct,
one indirect,
it finishes with the render call on the indirect followed by the
readpixels, then destroys
the drawable, for the next test it creates a new direct drawable and
calls makeCurrent
on it.
So we can fix the test to do a glFlush or we can fix the X server to
note the flushing
on ReadPixels also (which this patch does).
Dave.
> I'm persuaded that the test should be changed, though. It's not
> reasonable to depend on the flushing behavior of ReadPixels to meet the
> requirements of the spec, since a minor modification of the test could
> cause things to start failing mysteriously.
>
> | > What happens when one X client destroys a window that another one is
> | > using for GL rendering? The destruction of the window has to be
> | > postponed until it's no longer bound to an RC, or the GL command queue
> | > has to be redirected to a black hole, or GL rendering has to be
> | > terminated by error somehow. Or something else.
> |
> | If the window is destroyed the app normally gets a SIGPIPE and dies soon
> | after.
>
> Really? That surprises me. For normal X apps, I would think it would
> just get an error delivered via the X protocol the next time it attempts
> to use the window ID. The GL case seems messier.
>
> Allen
>
diff --git a/glx/singlepix.c b/glx/singlepix.c
index 7b2cb4c..a0a6a79 100644
--- a/glx/singlepix.c
+++ b/glx/singlepix.c
@@ -91,6 +91,7 @@ int __glXDisp_ReadPixels(__GLXclientState *cl, GLbyte *pc)
__GLX_SEND_HEADER();
__GLX_SEND_VOID_ARRAY(compsize);
}
+ __GLX_NOTE_FLUSHED_CMDS(cx);
return Success;
}
------------------------------------------------------------------------------
Crystal Reports - New Free Runtime and 30 Day Trial
Check out the new simplified licensing option that enables unlimited
royalty-free distribution of the report engine for externally facing
server and web deployment.
http://p.sf.net/sfu/businessobjects
--
_______________________________________________
Dri-devel mailing list
Dri-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/dri-devel
