[RESEND,v4] xfree86: Immediately handle failure to set HW cursor
Hans de Goede
hdegoede at redhat.com
Thu Sep 1 14:24:20 UTC 2016
Hi,
On 22-06-16 10:48, 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>
> Reviewed-by: Keith Packard <keithp at keithp.com>
> Cc: Michael Thayer <michael.thayer at oracle.com>
> Cc: Michel Dänzer <michel at daenzer.net>
Is this one still needed; or is this fully covered by the switch to
load_cursor_argb_check() ? :
https://cgit.freedesktop.org/xorg/xserver/commit/?id=14c21ea1c9496638b1feb8e6145c440fb4f1d14b
?
Note because of the above commit this patch no longer applies, I've
a rebased version in my personal tree and I plan to send out a pull-req
with assorted fixes soonish, so if this is still needed I'll include it,
no need to resend.
Regards,
Hans
> ---
> hw/xfree86/drivers/modesetting/drmmode_display.c | 15 ++++++----
> hw/xfree86/modes/xf86Crtc.h | 4 +++
> hw/xfree86/modes/xf86Cursors.c | 35 ++++++++++++++++++------
> hw/xfree86/ramdac/xf86Cursor.h | 1 +
> hw/xfree86/ramdac/xf86HWCurs.c | 18 +++++++++---
> 5 files changed, 54 insertions(+), 19 deletions(-)
>
> diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c b/hw/xfree86/drivers/modesetting/drmmode_display.c
> index 7e5901a71bcc..15285cb9f8f5 100644
> --- a/hw/xfree86/drivers/modesetting/drmmode_display.c
> +++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
> @@ -543,7 +543,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;
> @@ -561,7 +561,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;
> }
> @@ -576,7 +576,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
> @@ -609,12 +612,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
> @@ -861,7 +864,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 fb1dd4f31890..7a2a47360cd3 100644
> --- a/hw/xfree86/modes/xf86Crtc.h
> +++ b/hw/xfree86/modes/xf86Crtc.h
> @@ -195,6 +195,8 @@ typedef struct _xf86CrtcFuncs {
> */
> void
> (*show_cursor) (xf86CrtcPtr crtc);
> + Bool
> + (*show_cursor_check) (xf86CrtcPtr crtc);
>
> /**
> * Hide cursor
> @@ -991,6 +993,8 @@ static _X_INLINE _X_DEPRECATED void xf86_reload_cursors(ScreenPtr screen) {}
> */
> 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 d1e3302b0ef9..1a8e9d5d8708 100644
> --- a/hw/xfree86/modes/xf86Cursors.c
> +++ b/hw/xfree86/modes/xf86Cursors.c
> @@ -333,17 +333,26 @@ 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->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;
> }
>
> -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 +361,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);
> }
>
> static void
> @@ -624,7 +641,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 320ec0ce621e..c69d9022184d 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
>
More information about the xorg-devel
mailing list