[PATCH xserver v2] xfree86: Immediately handle failure to set HW cursor
Michael Thayer
michael.thayer at oracle.com
Tue Apr 5 17:40:15 UTC 2016
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.
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?
Regards,
Michael
>
> On 30.03.2016 08:41, Alexandre Courbot wrote:
>> There is currently no reliable way to report failure to set a HW
>> cursor. Still such failures can happen if e.g. the MODE_CURSOR DRM
>> ioctl fails (which currently happens at least with modesetting on Tegra
>> for format incompatibility reasons).
>>
>> As failures are currently handled by setting the HW cursor size to
>> (0,0), the fallback to SW cursor will not happen until the next time the
>> cursor changes and xf86CursorSetCursor() is called again. In the
>> meantime, the cursor will be invisible to the user.
>>
>> This patch addresses that by adding _xf86CrtcFuncs::set_cursor_check and
>> _xf86CursorInfoRec::ShowCursorCheck hook variants that return booleans.
>> This allows to propagate errors up to xf86CursorSetCursor(), which can
>> then fall back to using the SW cursor immediately.
>>
>> Signed-off-by: Alexandre Courbot <acourbot at nvidia.com>
>> ---
>> Changes since v1:
>> - Keep the old hooks unchanged to preserve compatibility
>>
>> hw/xfree86/drivers/modesetting/drmmode_display.c | 15 +++++++-----
>> hw/xfree86/modes/xf86Crtc.h | 4 ++++
>> hw/xfree86/modes/xf86Cursors.c | 30
>> +++++++++++++++++-------
>> hw/xfree86/ramdac/xf86Cursor.h | 1 +
>> hw/xfree86/ramdac/xf86HWCurs.c | 18 ++++++++++----
>> 5 files changed, 50 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> index f573a27f4985..28d663c3d22e 100644
>> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
>> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
>> @@ -532,7 +532,7 @@ drmmode_set_cursor_position(xf86CrtcPtr crtc, int
>> x, int y)
>> drmModeMoveCursor(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
>> x, y);
>> }
>>
>> -static void
>> +static Bool
>> drmmode_set_cursor(xf86CrtcPtr crtc)
>> {
>> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> @@ -551,7 +551,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>> handle, ms->cursor_width,
>> ms->cursor_height,
>> cursor->bits->xhot, cursor->bits->yhot);
>> if (!ret)
>> - return;
>> + return TRUE;
>> if (ret == -EINVAL)
>> use_set_cursor2 = FALSE;
>> }
>> @@ -566,7 +566,10 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
>> cursor_info->MaxWidth = cursor_info->MaxHeight = 0;
>> drmmode_crtc->drmmode->sw_cursor = TRUE;
>> /* fallback to swcursor */
>> + return FALSE;
>> }
>> +
>> + return TRUE;
>> }
>>
>> static void
>> @@ -599,12 +602,12 @@ drmmode_hide_cursor(xf86CrtcPtr crtc)
>> ms->cursor_width, ms->cursor_height);
>> }
>>
>> -static void
>> -drmmode_show_cursor(xf86CrtcPtr crtc)
>> +static Bool
>> +drmmode_show_cursor_check(xf86CrtcPtr crtc)
>> {
>> drmmode_crtc_private_ptr drmmode_crtc = crtc->driver_private;
>> drmmode_crtc->cursor_up = TRUE;
>> - drmmode_set_cursor(crtc);
>> + return drmmode_set_cursor(crtc);
>> }
>>
>> static void
>> @@ -844,7 +847,7 @@ static const xf86CrtcFuncsRec drmmode_crtc_funcs = {
>> .set_mode_major = drmmode_set_mode_major,
>> .set_cursor_colors = drmmode_set_cursor_colors,
>> .set_cursor_position = drmmode_set_cursor_position,
>> - .show_cursor = drmmode_show_cursor,
>> + .show_cursor_check = drmmode_show_cursor_check,
>> .hide_cursor = drmmode_hide_cursor,
>> .load_cursor_argb = drmmode_load_cursor_argb,
>>
>> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
>> index 8b0160845a02..4bc4d6fc78fd 100644
>> --- a/hw/xfree86/modes/xf86Crtc.h
>> +++ b/hw/xfree86/modes/xf86Crtc.h
>> @@ -187,6 +187,8 @@ typedef struct _xf86CrtcFuncs {
>> */
>> void
>> (*show_cursor) (xf86CrtcPtr crtc);
>> + Bool
>> + (*show_cursor_check) (xf86CrtcPtr crtc);
>>
>> /**
>> * Hide cursor
>> @@ -982,6 +984,8 @@ extern _X_EXPORT void
>> */
>> extern _X_EXPORT void
>> xf86_show_cursors(ScrnInfoPtr scrn);
>> +extern _X_EXPORT Bool
>> + xf86_show_cursors_check(ScrnInfoPtr scrn);
>>
>> /**
>> * Called by the driver to turn cursors off
>> diff --git a/hw/xfree86/modes/xf86Cursors.c
>> b/hw/xfree86/modes/xf86Cursors.c
>> index 5df1ab73a37e..ae581001c222 100644
>> --- a/hw/xfree86/modes/xf86Cursors.c
>> +++ b/hw/xfree86/modes/xf86Cursors.c
>> @@ -333,17 +333,23 @@ xf86_hide_cursors(ScrnInfoPtr scrn)
>> }
>> }
>>
>> -static void
>> +static Bool
>> xf86_crtc_show_cursor(xf86CrtcPtr crtc)
>> {
>> if (!crtc->cursor_shown && crtc->cursor_in_range) {
>> - crtc->funcs->show_cursor(crtc);
>> - crtc->cursor_shown = TRUE;
>> + 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;
>> }
>>
>> -void
>> -xf86_show_cursors(ScrnInfoPtr scrn)
>> +Bool
>> +xf86_show_cursors_check(ScrnInfoPtr scrn)
>> {
>> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>> int c;
>> @@ -352,9 +358,17 @@ xf86_show_cursors(ScrnInfoPtr scrn)
>> for (c = 0; c < xf86_config->num_crtc; c++) {
>> xf86CrtcPtr crtc = xf86_config->crtc[c];
>>
>> - if (crtc->enabled)
>> - xf86_crtc_show_cursor(crtc);
>> + if (crtc->enabled && !xf86_crtc_show_cursor(crtc))
>> + return FALSE;
>> }
>> +
>> + return TRUE;
>> +}
>> +
>> +void
>> +xf86_show_cursors(ScrnInfoPtr scrn)
>> +{
>> + xf86_show_cursors_check(scrn);
>> }
>>
>> void
>> @@ -631,7 +645,7 @@ xf86_cursors_init(ScreenPtr screen, int max_width,
>> int max_height, int flags)
>> cursor_info->SetCursorPosition = xf86_set_cursor_position;
>> cursor_info->LoadCursorImageCheck = xf86_load_cursor_image;
>> cursor_info->HideCursor = xf86_hide_cursors;
>> - cursor_info->ShowCursor = xf86_show_cursors;
>> + cursor_info->ShowCursorCheck = xf86_show_cursors_check;
>> cursor_info->UseHWCursor = xf86_use_hw_cursor;
>> if (flags & HARDWARE_CURSOR_ARGB) {
>> cursor_info->UseHWCursorARGB = xf86_use_hw_cursor_argb;
>> diff --git a/hw/xfree86/ramdac/xf86Cursor.h
>> b/hw/xfree86/ramdac/xf86Cursor.h
>> index 6e88240d9ab7..a1afb1c86ed9 100644
>> --- a/hw/xfree86/ramdac/xf86Cursor.h
>> +++ b/hw/xfree86/ramdac/xf86Cursor.h
>> @@ -16,6 +16,7 @@ typedef struct _xf86CursorInfoRec {
>> Bool (*LoadCursorImageCheck) (ScrnInfoPtr pScrn, unsigned char
>> *bits);
>> void (*HideCursor) (ScrnInfoPtr pScrn);
>> void (*ShowCursor) (ScrnInfoPtr pScrn);
>> + Bool (*ShowCursorCheck) (ScrnInfoPtr pScrn);
>> unsigned char *(*RealizeCursor) (struct _xf86CursorInfoRec *,
>> CursorPtr);
>> Bool (*UseHWCursor) (ScreenPtr, CursorPtr);
>>
>> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c
>> b/hw/xfree86/ramdac/xf86HWCurs.c
>> index 84febe0df5cd..458781cae7e9 100644
>> --- a/hw/xfree86/ramdac/xf86HWCurs.c
>> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
>> @@ -89,7 +89,8 @@ xf86InitHardwareCursor(ScreenPtr pScreen,
>> xf86CursorInfoPtr infoPtr)
>> if (!infoPtr->SetCursorPosition ||
>> !xf86DriverHasLoadCursorImage(infoPtr) ||
>> !infoPtr->HideCursor ||
>> - !infoPtr->ShowCursor || !infoPtr->SetCursorColors)
>> + (!infoPtr->ShowCursor && !infoPtr->ShowCursorCheck) ||
>> + !infoPtr->SetCursorColors)
>> return FALSE;
>>
>> if (infoPtr->RealizeCursor) {
>> @@ -119,6 +120,15 @@ xf86InitHardwareCursor(ScreenPtr pScreen,
>> xf86CursorInfoPtr infoPtr)
>> return TRUE;
>> }
>>
>> +static Bool
>> +xf86ShowCursor(xf86CursorInfoPtr infoPtr)
>> +{
>> + if (*infoPtr->ShowCursorCheck)
>> + return (*infoPtr->ShowCursorCheck) (infoPtr->pScrn);
>> + (*infoPtr->ShowCursor) (infoPtr->pScrn);
>> + return TRUE;
>> +}
>> +
>> Bool
>> xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>> {
>> @@ -161,8 +171,8 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs,
>> int x, int y)
>>
>> (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y);
>>
>> - (*infoPtr->ShowCursor) (infoPtr->pScrn);
>> - return TRUE;
>> + return xf86ShowCursor(infoPtr);
>> +
>> }
>>
>> void
>> @@ -184,7 +194,7 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>> xf86DriverLoadCursorImage (infoPtr,
>> ScreenPriv->transparentData);
>>
>> - (*infoPtr->ShowCursor) (infoPtr->pScrn);
>> + xf86ShowCursor(infoPtr);
>> }
>>
>> void
>>
>
--
Michael Thayer | VirtualBox engineer
ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt
ORACLE Deutschland B.V. & Co. KG
Hauptverwaltung: Riesstraße 25, D-80992 München
Registergericht: Amtsgericht München, HRA 95603
Komplementärin: ORACLE Deutschland Verwaltung B.V.
Hertogswetering 163/167, 3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
More information about the xorg-devel
mailing list