[PATCH xserver v2] xfree86: Immediately handle failure to set HW cursor

[Alexandre Courbot]

Hello Michael,

On 04/06/2016 02:40 AM, Michael Thayer wrote:
> On 04.04.2016 22:09, Michael Thayer wrote:
>> Hello,
>>
>> Please excuse the top posting.  Tested this in a dual-head non-mirrored
>> virtual machine and hit the following problem: xf86CursorSetCursor() in
>> hw/xfree86/ramdac/xf86Cursor.c calls xf86SetCursor() which fails because
>> the cursor is not visible on one of the two screens, and falls back to
>> software cursor rendering as a result.  Perhaps xf86_crtc_show_cursor()
>> should return TRUE if crtc->cursor_shown is FALSE?
>>
>> I'm afraid I will be away from the computer again for a few weeks, so it
>> will be a while before I can test again.
>
> I would suggest changing this function as follows:
>
> static Bool
> xf86_crtc_show_cursor(xf86CrtcPtr crtc)
> {
>      if (!crtc->cursor_shown && crtc->cursor_in_range) {
>          crtc->cursor_shown = TRUE;
>          if (crtc->funcs->show_cursor_check) {
>              return crtc->funcs->show_cursor_check(crtc);
>          } else {
>              crtc->funcs->show_cursor(crtc);
>          }
>      }
>      return TRUE;
> }
>
> As mentioned previously, crtc->cursor_shown will be FALSE if the cursor
> is hidden on a particular crtc, and that is not an error. Also,
> xf86_crtc_show_cursor() is called from xf86_crtc_set_cursor_position()
> without checking the return value, sometimes with cursor_shown FALSE
> when the cursor moves from one screen to another, and if we keep
> cursor_shown FALSE we will get a call to the kernel driver every time
> the position changes.

Thanks for catching that! Indeed I clearly overlooked the multi-head case.

Your proposed function looks fine, but maybe cursor_shown should not be 
set to TRUE if show_cursor_check fails? How about the following:

static Bool
xf86_crtc_show_cursor(xf86CrtcPtr crtc)
{
     if (!crtc->cursor_in_range)
         return TRUE;

     if (!crtc->cursor_shown) {
         if (crtc->funcs->show_cursor_check) {
             crtc->cursor_shown = crtc->funcs->show_cursor_check(crtc);
         } else {
             crtc->funcs->show_cursor(crtc);
             crtc->cursor_shown = TRUE;
         }
     }

     return crtc->cursor_shown;
}

Basically we want to return TRUE if the cursor is not supposed to be 
visible on this CRTC, which is what the first two lines do.

> However, I also just realised that modesetting still does not implement
> load_cursor_argb_check() - I really don't know how I forgot that.
> Perhaps just implementing that would solve your problem?

Ah, we discussed this in a previous email actually (see 
https://patchwork.freedesktop.org/patch/77422/ ). IIRC the problem with 
relying on load_cursor_argb_check() is that drmmode_show_cursor()'s 
errors are still not caught, which is ultimately what we need to do to 
fix this issue.

I will send a v3 with the fixed xf86_crtc_show_cursor() function - 
thanks again!

Alex.