[PATCH xserver v2] xfree86: Immediately handle failure to set HW cursor
Michael Thayer
michael.thayer at oracle.com
Mon Apr 4 20:09:24 UTC 2016
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.
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