[prev in list] [next in list] [prev in thread] [next in thread]
List: freedesktop-xorg-devel
Subject: Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor
From: Keith Packard <keithp () keithp ! com>
Date: 2015-12-24 18:06:04
Message-ID: 8660znsovn.fsf () hiro ! keithp ! com
[Download RAW message or body]
[Attachment #2 (multipart/signed)]
[Attachment #4 (multipart/mixed)]
Michel D=C3=A4nzer <michel@daenzer.net> writes:
> From: Michel D=C3=A4nzer <michel.daenzer@amd.com>
>
> This reduces code duplication.
>
> One side effect of this change is that xf86_config->cursor will no longer
> be updated for cursors which cannot use the HW cursor.
That means we'll be holding on to the last HW cursor in use on the
screen 'forever'; not a big deal, but doesn't seem great.
> Signed-off-by: Michel D=C3=A4nzer <michel.daenzer@amd.com>
> ---
>
> The side effect might actually be a bugfix? Haven't noticed any actual
> problems because of this though.
The referencing counting was added to xf86Cursors.c (by me) back in 2007
to avoid having the cursor get freed at the wrong time.
xf86_config->cursor is read in only two places: xf86_reload_cursors and
xf86_set_cursor_colors. xf86_set_cursor_colors is only called from the
ramdac cursor code, and is never called when a SW cursor is
displayed. However, xf86_reload_cursors is currently called from drivers
during modesetting, and so may well be called when a SW cursor is
displayed.
Reading the code, I can't see any reason we can't just use
ScreenPriv->cursor instead of saving another reference in this code; any
time we're using the HW cursor, that will be correct, and anytime we're
not using the HW cursor, we won't be doing anything with it. This will
let unused cursors get freed sooner, and eliminate this twisty bit of
extra code here.
This is untested, but 'should' work.
["0001-xfree86-modes-Remove-extra-reference-to-current-curs.patch" (text/x-diff)]
From da2e5c4f6e08b765a6a3e7337c0da8dd60b531e0 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Thu, 24 Dec 2015 09:51:49 -0800
Subject: [PATCH xserver] xfree86/modes: Remove extra reference to current
cursor
Just use the cursor reference in the xf86CursorScreenRec that is
managed in xf86Cursor.c
Signed-off-by: Keith Packard <keithp@keithp.com>
---
hw/xfree86/drivers/modesetting/drmmode_display.c | 3 +--
hw/xfree86/modes/xf86Crtc.h | 1 -
hw/xfree86/modes/xf86Cursors.c | 20 +++-----------------
hw/xfree86/ramdac/xf86Cursor.c | 9 +++++++++
hw/xfree86/ramdac/xf86Cursor.h | 1 +
5 files changed, 14 insertions(+), 20 deletions(-)
diff --git a/hw/xfree86/drivers/modesetting/drmmode_display.c \
b/hw/xfree86/drivers/modesetting/drmmode_display.c index 0d34ca1..17bfa4f 100644
--- a/hw/xfree86/drivers/modesetting/drmmode_display.c
+++ b/hw/xfree86/drivers/modesetting/drmmode_display.c
@@ -496,8 +496,7 @@ drmmode_set_cursor(xf86CrtcPtr crtc)
int ret;
if (use_set_cursor2) {
- xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(crtc->scrn);
- CursorPtr cursor = xf86_config->cursor;
+ CursorPtr cursor = xf86CurrentCursor(crtc->scrn->pScreen);
ret =
drmModeSetCursor2(drmmode->fd, drmmode_crtc->mode_crtc->crtc_id,
diff --git a/hw/xfree86/modes/xf86Crtc.h b/hw/xfree86/modes/xf86Crtc.h
index 8b01608..69abc52 100644
--- a/hw/xfree86/modes/xf86Crtc.h
+++ b/hw/xfree86/modes/xf86Crtc.h
@@ -709,7 +709,6 @@ typedef struct _xf86CrtcConfig {
/* Cursor information */
xf86CursorInfoPtr cursor_info;
- CursorPtr cursor;
CARD8 *cursor_image;
Bool cursor_on;
CARD32 cursor_fg, cursor_bg;
diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
index 321cde7..09dbecb 100644
--- a/hw/xfree86/modes/xf86Cursors.c
+++ b/hw/xfree86/modes/xf86Cursors.c
@@ -287,7 +287,7 @@ xf86_set_cursor_colors(ScrnInfoPtr scrn, int bg, int fg)
{
ScreenPtr screen = scrn->pScreen;
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
- CursorPtr cursor = xf86_config->cursor;
+ CursorPtr cursor = xf86CurrentCursor(screen);
int c;
CARD8 *bits = cursor ?
dixLookupScreenPrivate(&cursor->devPrivates, CursorScreenKey, screen)
@@ -516,11 +516,6 @@ xf86_use_hw_cursor(ScreenPtr screen, CursorPtr cursor)
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
- cursor = RefCursor(cursor);
- if (xf86_config->cursor)
- FreeCursor(xf86_config->cursor, None);
- xf86_config->cursor = cursor;
-
if (cursor->bits->width > cursor_info->MaxWidth ||
cursor->bits->height > cursor_info->MaxHeight)
return FALSE;
@@ -535,11 +530,6 @@ xf86_use_hw_cursor_argb(ScreenPtr screen, CursorPtr cursor)
xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
- cursor = RefCursor(cursor);
- if (xf86_config->cursor)
- FreeCursor(xf86_config->cursor, None);
- xf86_config->cursor = cursor;
-
/* Make sure ARGB support is available */
if ((cursor_info->Flags & HARDWARE_CURSOR_ARGB) == 0)
return FALSE;
@@ -633,7 +623,6 @@ xf86_cursors_init(ScreenPtr screen, int max_width, int \
max_height, int flags) cursor_info->LoadCursorARGBCheck = xf86_load_cursor_argb;
}
- xf86_config->cursor = NULL;
xf86_hide_cursors(scrn);
return xf86InitCursor(screen, cursor_info);
@@ -676,7 +665,8 @@ xf86_reload_cursors(ScreenPtr screen)
if (!cursor_info)
return;
- cursor = xf86_config->cursor;
+ cursor = xf86CurrentCursor(screen);
+
GetSpritePosition(inputInfo.pointer, &x, &y);
if (!(cursor_info->Flags & HARDWARE_CURSOR_UPDATE_UNHIDDEN))
(*cursor_info->HideCursor) (scrn);
@@ -711,8 +701,4 @@ xf86_cursors_fini(ScreenPtr screen)
}
free(xf86_config->cursor_image);
xf86_config->cursor_image = NULL;
- if (xf86_config->cursor) {
- FreeCursor(xf86_config->cursor, None);
- xf86_config->cursor = NULL;
- }
}
diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
index 2a54571..a354295 100644
--- a/hw/xfree86/ramdac/xf86Cursor.c
+++ b/hw/xfree86/ramdac/xf86Cursor.c
@@ -438,6 +438,15 @@ xf86ForceHWCursor(ScreenPtr pScreen, Bool on)
}
}
+CursorPtr
+xf86CurrentCursor(ScreenPtr pScreen)
+{
+ xf86CursorScreenPtr ScreenPriv =
+ (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
+ xf86CursorScreenKey);
+ return ScreenPriv->CurrentCursor;
+}
+
xf86CursorInfoPtr
xf86CreateCursorInfoRec(void)
{
diff --git a/hw/xfree86/ramdac/xf86Cursor.h b/hw/xfree86/ramdac/xf86Cursor.h
index 8c98bb1..dd10c48 100644
--- a/hw/xfree86/ramdac/xf86Cursor.h
+++ b/hw/xfree86/ramdac/xf86Cursor.h
@@ -60,6 +60,7 @@ extern _X_EXPORT Bool xf86InitCursor(ScreenPtr pScreen,
extern _X_EXPORT xf86CursorInfoPtr xf86CreateCursorInfoRec(void);
extern _X_EXPORT void xf86DestroyCursorInfoRec(xf86CursorInfoPtr);
extern _X_EXPORT void xf86ForceHWCursor(ScreenPtr pScreen, Bool on);
+extern _X_EXPORT CursorPtr xf86CurrentCursor(ScreenPtr pScreen);
#define HARDWARE_CURSOR_INVERT_MASK 0x00000001
#define HARDWARE_CURSOR_AND_SOURCE_WITH_MASK 0x00000002
--
2.6.4
--
-keith
["signature.asc" (application/pgp-signature)]
[Attachment #10 (text/plain)]
_______________________________________________
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic