Bug 29774

42719df0b866a00ea4a7739e82e1639c9943fcfd is the first bad commit
commit 42719df0b866a00ea4a7739e82e1639c9943fcfd
Author: Keith Whitwell <keithw@vmware.com>
Date:   Sun Aug 22 14:14:55 2010 +0100

    glx/xlib: configurable strict/non-strict buffer size invalidate
    
    Introduce a new configuration option XMESA_STRICT_INVALIDATE to switch
    between swapbuffers-based and glViewport-based buffer invalidation.
    
    Default strict invalidate to false, ie glViewport-based invalidation,
    aka ST_MANAGER_BROKEN_INVALIDATE.
    
    This means we will not call XGetGeometry after every swapbuffers,
    which allows swapbuffers to remain asynchronous.  For apps running at
    100fps with synchronous swapping, a 10% boost is typical.  For gears,
    I see closer to 20% speedup.
    
    Note that the work of copying data on swapbuffers doesn't disappear -
    this change just allows the X server to execute the PutImage
    asynchronously without us effectively blocked until its completion.
    
    This applies even to llvmpipe's threaded rasterization as the
    swapbuffers operation was a large part of the serial component of an
    llvmpipe frame.
    
    The downside of this is correctness - applications which don't call
    glViewport on window resizes will get incorrect rendering, unless
    XMESA_STRICT_INVALIDATE is set.
    
    The ultimate solution would be to have per-frame but asynchronous
    invalidation.  Xcb almost looks as if it could provide this, but the
    API doesn't seem to quite be there.

:040000 040000 c252970f9b71af088aa1a466f1cc0abe36fa1660 a024a9242fb8039240612478a89dc3b56ed7f212 M	src
bisect run success

piglit texgen passes with XMESA_STRICT_INVALIDATE=1.

piglit fp-generic dph and fp-generic kil-swizzle have also regressed. These two tests also pass with XMESA_STRICT_INVALIDATE=1.

glean polygonOffset has also regressed but passes with XMESA_STRICT_INVALIDATE=1.

The piglit tests texgen, fp-generic dph, and fp-generic kil-swizzle fail on llvmpipe as well.

Created attachment 38390 [details] [review]
proposed fix

The problem is the SwapBuffers is swapping the front/back color buffers/textures (in xmesa_swap_st_framebuffer()) but the state tracker code gets no notification.  When we do the subsequent glReadPixels() we're reading from the wrong texture.

The st_context_notify_invalid_framebuffer() callback would seem to be good candidate for this, but it's a per-context thing and we don't have a rendering context in SwapBuffers().

My proposed solution is to move/promote the 'int32_t revalidate' flag from the st_framebuffer struct to the st_framebuffer_iface struct.  It can be set in the winsys code upon SwapBuffers and the state tracker can check it during state validation.

This seems to do the trick, but we still have no guarantee that the state tracker will revalidate its context state after SwapBuffers is called.  To do this, we might need a new callback like st_framebuffer_iface::validate(struct st_framebuffer_iface *stfbi).  The implementation of this callback would check if the current context (or any/all contexts) is (are) bound to the given fb and if so, set some dirty bit in the context.

Created attachment 38518 [details] [review]
call notify_invalid_framebuffer after swapping

As noted by Brian, st/glx fails to notify st/mesa to validate the framebuffer again after it swaps the front/back pipe_resources of a drawable.  It should be easier to fix this bug by calling st_context_iface::notify_invalid_framebuffer on the context the framebuffer is bound to.

The reason that notify_invalid_framebuffer is per-context instead of per-framebuffer is that, when the framebuffer is not bound to any context, there is no context, and no need, to notify.  It should not be a limit to GLX as GLX assures that when a framebuffer is bound to some context, the context is the current context.

I've committed my patch as 03224f492dc9cee179ff9ed961be0443a3669dd1.  It fixes the mentioned regressions.  I am closing this bug.

mesa: 8fbe968a62f845da2a1491c398acf0b2140d2372 (master)
mesa: d169a67ad18b8f57210d991a77c86e012551642c (7.9)

Verified fixed.

texgen, fp-generic dph, fp-generic kil-swizzle, and polygonOffset pass on softpipe.