[PATCH xserver 1/3] xfree86: Immediately handle failure to set HW cursor, v5
Hans de Goede
hdegoede at redhat.com
Mon Dec 5 10:05:14 UTC 2016
Hi,
On 05-12-16 10:25, Michael Thayer wrote:
> Hello Hans,
>
> Polite ping on this one.
I've already given this series my:
Reviewed-by: Hans de Goede <hdegoede at redhat.com>
There is not much else I can do, from here on it
is up to the xserver maintainers to pick up the series.
Regards,
Hans
>
> Regards and thanks
> Michael
>
> 01.10.2016 12:01, Hans de Goede wrote:
>> Hi,
>>
>> On 30-09-16 17:55, Michael Thayer wrote:
>>> Based on v4 by Alexandre Courbot <acourbot at nvidia.com>
>>>
>>> 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.
>>>
>>> v5: Updated the patch to apply to current git HEAD, split up into two
>>> patches (server and modesetting driver) and adjusted the code slightly
>>> to match surrounding code. I also removed the new exported function
>>> ShowCursorCheck(), as instead just changing ShowCursor() to return Bool
>>> should not affect its current callers.
>>>
>>> Signed-off-by: Michael Thayer <michael.thayer at oracle.com>
>>
>> Series looks good to me and is:
>>
>> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>>
>> As discussed this will need to wait till we start the 1.20
>> cycle before it can be merged (it contains a video driver ABI change)
>>
>> Regards,
>>
>> Hans
>>
>>
>>
>>> ---
>>> hw/xfree86/modes/xf86Crtc.h | 4 +++-
>>> hw/xfree86/modes/xf86Cursors.c | 40
>>> ++++++++++++++++++++++++++++++----------
>>> hw/xfree86/ramdac/xf86Cursor.h | 16 ++++++++++++++++
>>> hw/xfree86/ramdac/xf86HWCurs.c | 8 ++++----
>>> 4 files changed, 53 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
>>> index 14ba9d7..215eb2a 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
>>> @@ -993,7 +995,7 @@ static _X_INLINE _X_DEPRECATED void
>>> xf86_reload_cursors(ScreenPtr screen) {}
>>> /**
>>> * Called from EnterVT to turn the cursors back on
>>> */
>>> -extern _X_EXPORT void
>>> +extern _X_EXPORT Bool
>>> xf86_show_cursors(ScrnInfoPtr scrn);
>>>
>>> /**
>>> diff --git a/hw/xfree86/modes/xf86Cursors.c
>>> b/hw/xfree86/modes/xf86Cursors.c
>>> index 1bc2b27..9543eed 100644
>>> --- a/hw/xfree86/modes/xf86Cursors.c
>>> +++ b/hw/xfree86/modes/xf86Cursors.c
>>> @@ -210,9 +210,15 @@ set_bit(CARD8 *image, xf86CursorInfoPtr
>>> cursor_info, int x, int y, Bool mask)
>>>
>>> /*
>>> * Wrappers to deal with API compatibility with drivers that don't
>>> expose
>>> - * load_cursor_*_check
>>> + * *_cursor_*_check
>>> */
>>> static inline Bool
>>> +xf86_driver_has_show_cursor(xf86CrtcPtr crtc)
>>> +{
>>> + return crtc->funcs->show_cursor_check || crtc->funcs->show_cursor;
>>> +}
>>> +
>>> +static inline Bool
>>> xf86_driver_has_load_cursor_image(xf86CrtcPtr crtc)
>>> {
>>> return crtc->funcs->load_cursor_image_check ||
>>> crtc->funcs->load_cursor_image;
>>> @@ -225,6 +231,15 @@ xf86_driver_has_load_cursor_argb(xf86CrtcPtr crtc)
>>> }
>>>
>>> static inline Bool
>>> +xf86_driver_show_cursor(xf86CrtcPtr crtc)
>>> +{
>>> + if (crtc->funcs->show_cursor_check)
>>> + return crtc->funcs->show_cursor_check(crtc);
>>> + crtc->funcs->show_cursor(crtc);
>>> + return TRUE;
>>> +}
>>> +
>>> +static inline Bool
>>> xf86_driver_load_cursor_image(xf86CrtcPtr crtc, CARD8 *cursor_image)
>>> {
>>> if (crtc->funcs->load_cursor_image_check)
>>> @@ -333,16 +348,19 @@ 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)
>>> + crtc->cursor_shown = xf86_driver_show_cursor(crtc);
>>> +
>>> + return crtc->cursor_shown;
>>> }
>>>
>>> -void
>>> +Bool
>>> xf86_show_cursors(ScrnInfoPtr scrn)
>>> {
>>> xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>>> @@ -352,9 +370,11 @@ 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;
>>> }
>>>
>>> static void
>>> @@ -653,7 +673,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;
>>> 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 320ec0c..11a03b6 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);
>>>
>>> @@ -41,6 +42,21 @@ xf86DriverLoadCursorImage(xf86CursorInfoPtr
>>> infoPtr, unsigned char *bits)
>>> }
>>>
>>> static inline Bool
>>> +xf86DriverHasShowCursor(xf86CursorInfoPtr infoPtr)
>>> +{
>>> + return infoPtr->ShowCursorCheck || infoPtr->ShowCursor;
>>> +}
>>> +
>>> +static inline Bool
>>> +xf86DriverShowCursor(xf86CursorInfoPtr infoPtr)
>>> +{
>>> + if(infoPtr->ShowCursorCheck)
>>> + return infoPtr->ShowCursorCheck(infoPtr->pScrn);
>>> + infoPtr->ShowCursor(infoPtr->pScrn);
>>> + return TRUE;
>>> +}
>>> +
>>> +static inline Bool
>>> xf86DriverHasLoadCursorARGB(xf86CursorInfoPtr infoPtr)
>>> {
>>> return infoPtr->LoadCursorARGBCheck || infoPtr->LoadCursorARGB;
>>> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c
>>> b/hw/xfree86/ramdac/xf86HWCurs.c
>>> index e8966ed..bd287fc 100644
>>> --- a/hw/xfree86/ramdac/xf86HWCurs.c
>>> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
>>> @@ -90,7 +90,8 @@ xf86InitHardwareCursor(ScreenPtr pScreen,
>>> xf86CursorInfoPtr infoPtr)
>>> if (!infoPtr->SetCursorPosition ||
>>> !xf86DriverHasLoadCursorImage(infoPtr) ||
>>> !infoPtr->HideCursor ||
>>> - !infoPtr->ShowCursor || !infoPtr->SetCursorColors)
>>> + !xf86DriverHasShowCursor(infoPtr) ||
>>> + !infoPtr->SetCursorColors)
>>> return FALSE;
>>>
>>> if (infoPtr->RealizeCursor) {
>>> @@ -204,8 +205,7 @@ xf86ScreenSetCursor(ScreenPtr pScreen, CursorPtr
>>> pCurs, int x, int y)
>>>
>>> (*infoPtr->SetCursorPosition) (infoPtr->pScrn, x, y);
>>>
>>> - (*infoPtr->ShowCursor) (infoPtr->pScrn);
>>> - return TRUE;
>>> + return xf86DriverShowCursor(infoPtr);
>>> }
>>>
>>> Bool
>>> @@ -252,7 +252,7 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>>> xf86DriverLoadCursorImage (infoPtr,
>>> ScreenPriv->transparentData);
>>>
>>> - (*infoPtr->ShowCursor) (infoPtr->pScrn);
>>> + xf86DriverShowCursor(infoPtr);
>>> }
>>>
>>> static void
>>>
>
More information about the xorg-devel
mailing list