[PATCH xserver 1/2] animcur: Fix transitions between animated cursors

[Aaron Plattner]

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 <ajax at redhat.com>
> ---
>  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 <aplattner at nvidia.com>

I'll try to put together a build to test these today.

>              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);
>