On 01/09/2018 10:08 AM, Aaron Plattner wrote: > On 01/09/2018 08:51 AM, Adam Jackson wrote: >> We weren't cancelling the old timer when changing cursors, making things >> go all crashy. Logically we could always cancel the timer first, but >> then we'd have to call TimerSet to re-arm ourselves, and GetTimeInMillis >> is potentially expensive. >> >> Reported-by: >> https://devtalk.nvidia.com/default/topic/1028172/linux/titan-v-ubuntu-16-04lts-and-387-34-driver-crashes-badly/post/5230967/#5230967 >> Signed-off-by: Adam Jackson <[email protected]> >> --- >> render/animcur.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/render/animcur.c b/render/animcur.c >> index 058bc1b323..797029443c 100644 >> --- a/render/animcur.c >> +++ b/render/animcur.c >> @@ -151,11 +151,20 @@ AnimCurTimerNotify(OsTimerPtr timer, CARD32 now, void >> *arg) >> return ac->elts[elt].delay; >> } >> >> +static void >> +AnimCurCancelTimer(DeviceIntPtr pDev) >> +{ >> + CursorPtr cur = pDev->spriteInfo->anim.pCursor; >> + >> + if (IsAnimCur(cur)) >> + TimerCancel(GetAnimCur(cur)->timer); >> +} >> + >> static Bool >> AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr >> pCursor) >> { >> AnimCurScreenPtr as = GetAnimCurScreen(pScreen); >> - Bool ret; >> + Bool ret = TRUE; >> >> if (IsFloating(pDev)) >> return FALSE; >> @@ -165,8 +174,10 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr >> pScreen, CursorPtr pCursor) >> if (pCursor != pDev->spriteInfo->anim.pCursor) { >> AnimCurPtr ac = GetAnimCur(pCursor); >> >> - ret = (*pScreen->DisplayCursor) >> - (pDev, pScreen, ac->elts[0].pCursor); >> + AnimCurCancelTimer(pDev); >> + ret = (*pScreen->DisplayCursor) (pDev, pScreen, >> + ac->elts[0].pCursor); >> + > > This is a slight change in behavior if DisplayCursor fails since it will > cancel the previous timer whereas before it wouldn't. Are there any > weird cases where failing to change the cursor is expected and canceling > animation of the previous cursor would be a problem? > > Assuming not, both changes > Reviewed-by: Aaron Plattner <[email protected]> > > I'll try to put together a build to test these today.
I re-verified the crash and verified that this series fixes it, so you can add Tested-by: Aaron Plattner <[email protected]> >> if (ret) { >> pDev->spriteInfo->anim.elt = 0; >> pDev->spriteInfo->anim.pCursor = pCursor; >> @@ -176,15 +187,9 @@ AnimCurDisplayCursor(DeviceIntPtr pDev, ScreenPtr >> pScreen, CursorPtr pCursor) >> AnimCurTimerNotify, pDev); >> } >> } >> - else >> - ret = TRUE; >> } >> else { >> - CursorPtr old = pDev->spriteInfo->anim.pCursor; >> - >> - if (old && IsAnimCur(old)) >> - TimerCancel(GetAnimCur(old)->timer); >> - >> + AnimCurCancelTimer(pDev); >> pDev->spriteInfo->anim.pCursor = 0; >> pDev->spriteInfo->anim.pScreen = 0; >> ret = (*pScreen->DisplayCursor) (pDev, pScreen, pCursor); >> > _______________________________________________ [email protected]: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
