[PATCH] hw/xfree86: Fix block handler wrapping in xf86Rotate

[Keith Packard]

xf86Rotate, it was delaying unwrapping the BlockHandler until after
calling xf86RotateRedisplay. If there was a software cursor on the
screen, the redisplay operation would cause cursor to be removed from
the frame buffer and the misprite block handler to be inserted into
the block handler chain with the misprite screen private saved block
handler now set to xf86RotateBlockHandler.

When xf86RotateRedisplay returned, xf86RotateBlockHandler would then
set screen->BlockHandler to its saved value, call down and then reset
screen->BlockHandler to xf86RotateBlockHandler. miSpriteBlockHandler
would never be called after that, which meant that the software cursor
will now disappear from the screen whenever rendering overlapped and
would only reappear when the cursor was moved.

To correct this, all that is needed is to move the restoration of
screen->BlockHandler to the top of xf86RotateBlockHandler, before the
call to xf86RotateRedisplay.

I've also added to this patch a check to see if the rotate block
handler is still required and only re-insert itself if that is the
case.

There's also an enormous comment added to scrnintstr.h which details
the required conventions for screen function wrapping.

Signed-off-by: Keith Packard <keithp at keithp.com>
---
 hw/xfree86/modes/xf86Rotate.c | 18 +++++++--
 include/scrnintstr.h          | 90 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 104 insertions(+), 4 deletions(-)

diff --git a/hw/xfree86/modes/xf86Rotate.c b/hw/xfree86/modes/xf86Rotate.c
index 0ddd840..1627e61 100644
--- a/hw/xfree86/modes/xf86Rotate.c
+++ b/hw/xfree86/modes/xf86Rotate.c
@@ -234,12 +234,22 @@ xf86RotateBlockHandler(ScreenPtr pScreen,
     ScrnInfoPtr pScrn = xf86ScreenToScrn(pScreen);
     xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(pScrn);
 
-    xf86RotateRedisplay(pScreen);
+    /* Unwrap before redisplay in case the software
+     * cursor layer wants to add its block handler to the
+     * chain
+     */
     pScreen->BlockHandler = xf86_config->BlockHandler;
+
+    xf86RotateRedisplay(pScreen);
+
     (*pScreen->BlockHandler) (pScreen, pTimeout, pReadmask);
-    /* cannot avoid re-wrapping until all wrapping is audited */
-    xf86_config->BlockHandler = pScreen->BlockHandler;
-    pScreen->BlockHandler = xf86RotateBlockHandler;
+
+    /* Re-wrap if we still need this hook */
+    if (xf86_config->rotation_damage != NULL) {
+        xf86_config->BlockHandler = pScreen->BlockHandler;
+        pScreen->BlockHandler = xf86RotateBlockHandler;
+    } else
+        xf86_config->BlockHandler = NULL;
 }
 
 void
diff --git a/include/scrnintstr.h b/include/scrnintstr.h
index 6acdadd..cc6e596 100644
--- a/include/scrnintstr.h
+++ b/include/scrnintstr.h
@@ -358,6 +358,96 @@ typedef WindowPtr (*XYToWindowProcPtr)(ScreenPtr pScreen,
 
 typedef int (*NameWindowPixmapProcPtr)(WindowPtr, PixmapPtr, CARD32);
 
+/* Wrapping Screen procedures
+
+   There are a few modules in the X server which dynamically add and
+    remove themselves from various screen procedure call chains.
+
+    For example, the BlockHandler is dynamically modified by:
+
+     * xf86Rotate
+     * miSprite
+     * composite
+     * render (for animated cursors)
+
+    Correctly manipulating this chain is complicated by the fact that
+    the chain is constructed through a sequence of screen private
+    structures, each holding the next screen->proc pointer.
+
+    To add a module to a screen->proc chain is fairly simple; just save
+    the current screen->proc value in the module screen private
+    and store the module's function in the screen->proc location.
+
+    Removing a screen proc is a bit trickier. It seems like all you
+    need to do is set the screen->proc pointer back to the value saved
+    in your screen private. However, if some other module has come
+    along and wrapped on top of you, then the right place to store the
+    previous screen->proc value is actually in the wrapping module's
+    screen private structure(!). Of course, you have no idea what
+    other module may have wrapped on top, nor could you poke inside
+    its screen private in any case.
+
+    To make this work, we restrict the unwrapping process to happen
+    during the invocation of the screen proc itself, and then we
+    require the screen proc to take some care when manipulating the
+    screen proc functions pointers.
+
+    The requirements are:
+
+     1) The screen proc must set the screen->proc pointer back to the
+        value saved in its screen private before calling outside its
+        module.
+
+     2a) If the screen proc wants to be remove itself from the chain,
+         it must not manipulate screen->proc pointer again before
+         returning.
+
+     2b) If the screen proc wants to remain in the chain, it must:
+
+       2b.1) Re-fetch the screen->proc pointer and store that in
+             its screen private. This ensures that any changes
+             to the chain will be preserved.
+
+       2b.2) Set screen->proc back to itself
+
+    One key requirement here is that these steps must wrap not just
+    any invocation of the nested screen->proc value, but must nest
+    essentially any calls outside the current module. This ensures
+    that other modules can reliably manipulate screen->proc wrapping
+    using these same rules.
+
+    For example, the animated cursor code in render has two macros,
+    Wrap and Unwrap.
+
+        #define Unwrap(as,s,elt)    ((s)->elt = (as)->elt)
+
+    Unwrap takes the screen private (as), the screen (s) and the
+    member name (elt), and restores screen->proc to that saved in the
+    screen private.
+
+        #define Wrap(as,s,elt,func) (((as)->elt = (s)->elt), (s)->elt = func)
+
+    Wrap takes the screen private (as), the screen (s), the member
+    name (elt) and the wrapping function (func). It saves the
+    current screen->proc value in the screen private, and then sets the
+    screen->proc to the local wrapping function.
+
+    Within each of these functions, there's a pretty simple pattern:
+
+        Unwrap(as, pScreen, UnrealizeCursor);
+
+        // Do local stuff, including possibly calling down through
+        // pScreen->UnrealizeCursor
+
+        Wrap(as, pScreen, UnrealizeCursor, AnimCurUnrealizeCursor);
+
+    The wrapping block handler is a bit different; it does the Unwrap,
+    the local operations and then only re-Wraps if the hook is still
+    required. Unwrap occurrs at the top of each function, just after
+    entry, and Wrap occurrs at the bottom of each function, just
+    before returning.
+ */
+
 typedef struct _Screen {
     int myNum;                  /* index of this instance in Screens[] */
     ATOM id;
-- 
1.9.2