[PATCH xserver 3/4] animcur: Run the timer from the device, not the screen
Aaron Plattner
aplattner at nvidia.com
Mon Jan 8 20:39:03 UTC 2018
Nothing like deploying code in the wild to find bugs. :(
The user on the forum reported a crash after this patch and I reproduced it locally:
Thread 1 "Xorg" received signal SIGSEGV, Segmentation fault.
0x000055604b0b8acd in dixGetPrivateAddr (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:123
123 ../include/privates.h: No such file or directory.
(gdb) bt
#0 0x000055604b0b8acd in dixGetPrivateAddr (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:123
#1 0x000055604b0b8b5d in dixLookupPrivate (privates=0x3e8, key=0x55604b40ace0 <AnimCurScreenPrivateKeyRec>) at ../include/privates.h:165
#2 0x000055604b0b8d79 in AnimCurTimerNotify (timer=0x55604d751a30, now=45483869, arg=0x55604d31cb70) at animcur.c:134
#3 0x000055604b15fc70 in DoTimer ()
#4 0x000055604b15fcd7 in DoTimers ()
#5 0x000055604b15ffa7 in WaitForSomething ()
#6 0x000055604af90ec1 in Dispatch () at dispatch.c:422
#7 0x000055604af9e7dd in dix_main (argc=14, argv=0x7ffd9f1c4c78, envp=0x7ffd9f1c4cf0) at main.c:287
#8 0x00007f64f5282f4a in __libc_start_main () at /usr/lib/libc.so.6
#9 0x000055604af82a8a in _start ()
#2 0x000055604b0b8d79 in AnimCurTimerNotify (timer=0x55604d751a30, now=45483869, arg=0x55604d31cb70) at animcur.c:134
134 in animcur.c
(gdb) p pScreen
$8 = (ScreenPtr) 0x0
I'm not sure how this is happening, yet.
-- Aaron
On 11/06/2017 12:19 PM, Adam Jackson wrote:
> This is very slightly more efficient since the callback now doesn't need
> to walk every input device, instead we know exactly which device's
> cursor is being updated. AnimCurTimerNotify() gets outdented nicely as a
> result. A more important side effect is that we can stop using the
> TimerAbsolute mode and just pass in the relative delay.
>
> In AnimCurSetCursorPosition, we no longer need to rearm the timer with
> the new screen; it is enough to update the device's state. In
> AnimCurDisplayCursor we need to notice when we're switching from
> animated cursor to regular and cancel the existing timer.
>
> Signed-off-by: Adam Jackson <ajax at redhat.com>
> ---
> render/animcur.c | 87 +++++++++++++++++++-------------------------------------
> 1 file changed, 29 insertions(+), 58 deletions(-)
>
> diff --git a/render/animcur.c b/render/animcur.c
> index 05dfc640aa..e59a7c3c40 100644
> --- a/render/animcur.c
> +++ b/render/animcur.c
> @@ -55,6 +55,7 @@ typedef struct _AnimCurElt {
> typedef struct _AnimCur {
> int nelt; /* number of elements in the elts array */
> AnimCurElt *elts; /* actually allocated right after the structure */
> + OsTimerPtr timer;
> } AnimCurRec, *AnimCurPtr;
>
> typedef struct _AnimScrPriv {
> @@ -65,8 +66,6 @@ typedef struct _AnimScrPriv {
> RealizeCursorProcPtr RealizeCursor;
> UnrealizeCursorProcPtr UnrealizeCursor;
> RecolorCursorProcPtr RecolorCursor;
> - OsTimerPtr timer;
> - Bool timer_set;
> } AnimCurScreenRec, *AnimCurScreenPtr;
>
> static unsigned char empty[4];
> @@ -131,49 +130,27 @@ AnimCurCursorLimits(DeviceIntPtr pDev,
> static CARD32
> AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void *arg)
> {
> - ScreenPtr pScreen = arg;
> + DeviceIntPtr dev = arg;
> + ScreenPtr pScreen = dev->spriteInfo->anim.pScreen;
> AnimCurScreenPtr as = GetAnimCurScreen(pScreen);
> - DeviceIntPtr dev;
> - Bool activeDevice = FALSE;
> - CARD32 soonest = ~0; /* earliest time to wakeup again */
> -
> - for (dev = inputInfo.devices; dev; dev = dev->next) {
> - if (IsPointerDevice(dev) && pScreen == dev->spriteInfo->anim.pScreen) {
> - if (!activeDevice)
> - activeDevice = TRUE;
> -
> - if ((INT32) (now - dev->spriteInfo->anim.time) >= 0) {
> - AnimCurPtr ac = GetAnimCur(dev->spriteInfo->anim.pCursor);
> - int elt = (dev->spriteInfo->anim.elt + 1) % ac->nelt;
> - DisplayCursorProcPtr DisplayCursor;
> -
> - /*
> - * Not a simple Unwrap/Wrap as this
> - * isn't called along the DisplayCursor
> - * wrapper chain.
> - */
> - DisplayCursor = pScreen->DisplayCursor;
> - pScreen->DisplayCursor = as->DisplayCursor;
> - (void) (*pScreen->DisplayCursor) (dev,
> - pScreen,
> - ac->elts[elt].pCursor);
> - as->DisplayCursor = pScreen->DisplayCursor;
> - pScreen->DisplayCursor = DisplayCursor;
> -
> - dev->spriteInfo->anim.elt = elt;
> - dev->spriteInfo->anim.time = now + ac->elts[elt].delay;
> - }
>
> - if (soonest > dev->spriteInfo->anim.time)
> - soonest = dev->spriteInfo->anim.time;
> - }
> - }
> + AnimCurPtr ac = GetAnimCur(dev->spriteInfo->anim.pCursor);
> + int elt = (dev->spriteInfo->anim.elt + 1) % ac->nelt;
> + DisplayCursorProcPtr DisplayCursor = pScreen->DisplayCursor;
>
> - if (activeDevice)
> - return soonest - now;
> + /*
> + * Not a simple Unwrap/Wrap as this isn't called along the DisplayCursor
> + * wrapper chain.
> + */
> + pScreen->DisplayCursor = as->DisplayCursor;
> + (void) (*pScreen->DisplayCursor) (dev, pScreen, ac->elts[elt].pCursor);
> + as->DisplayCursor = pScreen->DisplayCursor;
> + pScreen->DisplayCursor = DisplayCursor;
>
> - as->timer_set = FALSE;
> - return 0;
> + dev->spriteInfo->anim.elt = elt;
> + dev->spriteInfo->anim.time = now + ac->elts[elt].delay;
> +
> + return ac->elts[elt].delay;
> }
>
> static Bool
> @@ -199,17 +176,19 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCursor)
> pDev->spriteInfo->anim.pCursor = pCursor;
> pDev->spriteInfo->anim.pScreen = pScreen;
>
> - if (!as->timer_set) {
> - TimerSet(as->timer, TimerAbsolute, pDev->spriteInfo->anim.time,
> - AnimCurTimerNotify, pScreen);
> - as->timer_set = TRUE;
> - }
> + ac->timer = TimerSet(ac->timer, 0, ac->elts[0].delay,
> + AnimCurTimerNotify, pDev);
> }
> }
> else
> ret = TRUE;
> }
> else {
> + CursorPtr old = pDev->spriteInfo->anim.pCursor;
> +
> + if (old && IsAnimCur(old))
> + TimerCancel(GetAnimCur(old)->timer);
> +
> pDev->spriteInfo->anim.pCursor = 0;
> pDev->spriteInfo->anim.pScreen = 0;
> ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor);
> @@ -228,12 +207,6 @@ AnimCurSetCursorPosition(DeviceIntPtr pDev,
> Unwrap(as, pScreen, SetCursorPosition);
> if (pDev->spriteInfo->anim.pCursor) {
> pDev->spriteInfo->anim.pScreen = pScreen;
> -
> - if (!as->timer_set) {
> - TimerSet(as->timer, TimerAbsolute, pDev->spriteInfo->anim.time,
> - AnimCurTimerNotify, pScreen);
> - as->timer_set = TRUE;
> - }
> }
> ret = (*pScreen->SetCursorPosition) (pDev, pScreen, x, y, generateEvent);
> Wrap(as, pScreen, SetCursorPosition, AnimCurSetCursorPosition);
> @@ -308,11 +281,6 @@ AnimCurInit(ScreenPtr pScreen)
> return FALSE;
>
> as = GetAnimCurScreen(pScreen);
> - as->timer = TimerSet(NULL, TimerAbsolute, 0, AnimCurTimerNotify, pScreen);
> - if (!as->timer) {
> - return FALSE;
> - }
> - as->timer_set = FALSE;
>
> Wrap(as, pScreen, CloseScreen, AnimCurCloseScreen);
>
> @@ -360,10 +328,14 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int ncursor,
>
> pCursor->id = cid;
>
> + ac = GetAnimCur(pCursor);
> + ac->timer = TimerSet(NULL, 0, 0, AnimCurTimerNotify, NULL);
> +
> /* security creation/labeling check */
> rc = XaceHook(XACE_RESOURCE_ACCESS, client, cid, RT_CURSOR, pCursor,
> RT_NONE, NULL, DixCreateAccess);
> if (rc != Success) {
> + TimerFree(ac->timer);
> dixFiniPrivates(pCursor, PRIVATE_CURSOR);
> free(pCursor);
> return rc;
> @@ -373,7 +345,6 @@ AnimCursorCreate(CursorPtr *cursors, CARD32 *deltas, int ncursor,
> * Fill in the AnimCurRec
> */
> animCursorBits.refcnt++;
> - ac = GetAnimCur(pCursor);
> ac->nelt = ncursor;
> ac->elts = (AnimCurElt *) (ac + 1);
>
>
More information about the xorg-devel
mailing list