Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-09 Thread Keith Packard 
Michel Dänzer  writes:

> Sounds good to me.
>
> One thing that might be nice to do as part of your patch would be moving
> the xf86_config->cursor = NULL assignment from xf86_cursors_init to
> xf86_hide_cursors. That way xf86_config->cursor would always be NULL
> while the cursor layer isn't using the hardware cursor.

HideCursor gets called when setting the cursor image if
HARDWARE_CURSOR_UPDATE_UNHIDDEN isn't set, which means we'd have to save
the pointer in the LoadCursorImageCheck and LoadCursorARGBCheck
functions.

The LoadCursorImageCheck function doesn't get the CursorPtr.  So, we
need the xf86CurrentCursor function that I had written before.

From c133e4d9fac39862d7922f59080cc7ce40d1108b Mon Sep 17 00:00:00 2001
From: Keith Packard 
Date: Wed, 9 Mar 2016 11:13:14 -0800
Subject: [PATCH xserver] xfree86: Set xf86CrtcConfigRec cursor pointer to NULL
 in HideCursor

This makes the cursor pointer held by xf86Cursors.c get reset to NULL
whenever the cursor isn't displayed, and means that the reference
count held in xf86Cursor.c is sufficient to cover the reference in
xf86Cursors.c.

As HideCursor may be called in the cursor loading path after
UseHWCursor or UseHWCursorARGB when HARDWARE_CURSOR_UPDATE_UNHIDDEN
isn't set in the Flags field, the setting of the cursor pointer had to
be moved to the LoadCursor paths.

LoadCursorARGBCheck gets the cursor pointer, but LoadCursorImageCheck
does not. For LoadCursorImageCheck, I added a new function,
xf86CurrentCursor, which returns the current cursor. With this new
function, we can eliminate the cursor pointer from the
xf86CrtcConfigRec, once drivers are converted over to use it.

Signed-off-by: Keith Packard 
---
 hw/xfree86/modes/xf86Cursors.c | 18 ++
 hw/xfree86/ramdac/xf86Cursor.c |  9 +
 hw/xfree86/ramdac/xf86Cursor.h |  1 +
 3 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
index 5df1ab7..8f9d589 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(>devPrivates, CursorScreenKey, screen)
@@ -325,6 +325,7 @@ xf86_hide_cursors(ScrnInfoPtr scrn)
 int c;
 
 xf86_config->cursor_on = FALSE;
+xf86_config->cursor = NULL;
 for (c = 0; c < xf86_config->num_crtc; c++) {
 xf86CrtcPtr crtc = xf86_config->crtc[c];
 
@@ -456,6 +457,7 @@ xf86_crtc_load_cursor_image(xf86CrtcPtr crtc, CARD8 *src)
 CARD8 *cursor_image;
 const Rotation rotation = xf86_crtc_cursor_rotation(crtc);
 
+xf86_config->cursor = xf86CurrentCursor(xf86ScrnToScreen(scrn));
 crtc->cursor_argb = FALSE;
 
 if (rotation == RR_Rotate_0)
@@ -517,11 +519,6 @@ xf86_use_hw_cursor(ScreenPtr screen, CursorPtr cursor)
 xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
 int c;
 
-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;
@@ -593,6 +590,7 @@ xf86_load_cursor_argb(ScrnInfoPtr scrn, CursorPtr cursor)
 xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
 int c;
 
+xf86_config->cursor = cursor;
 for (c = 0; c < xf86_config->num_crtc; c++) {
 xf86CrtcPtr crtc = xf86_config->crtc[c];
 
@@ -638,7 +636,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);
@@ -681,7 +678,7 @@ xf86_reload_cursors(ScreenPtr screen)
 if (!cursor_info)
 return;
 
-cursor = xf86_config->cursor;
+cursor = xf86CurrentCursor(screen);
 GetSpritePosition(inputInfo.pointer, , );
 if (!(cursor_info->Flags & HARDWARE_CURSOR_UPDATE_UNHIDDEN))
 (*cursor_info->HideCursor) (scrn);
@@ -716,8 +713,5 @@ 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;
-}
+xf86_config->cursor = NULL;
 }
diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
index c061b80..dda4349 100644
--- a/hw/xfree86/ramdac/xf86Cursor.c
+++ b/hw/xfree86/ramdac/xf86Cursor.c
@@ -462,6 +462,15 @@ xf86ForceHWCursor(ScreenPtr pScreen, Bool on)
 }
 }
 
+CursorPtr

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-08 Thread Michel Dänzer 
On 09.03.2016 12:46, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> Unfortunately, the reality is that at least the radeon and amdgpu
>> drivers do use it.
> 
> Even modesetting uses it; there isn't any other public interface to get
> the current cursor.
> 
> What do you think about eliminating the RefCursor/FreeCursor bits and
> just letting the upper layer hold the reference?

Sounds good to me.

One thing that might be nice to do as part of your patch would be moving
the xf86_config->cursor = NULL assignment from xf86_cursors_init to
xf86_hide_cursors. That way xf86_config->cursor would always be NULL
while the cursor layer isn't using the hardware cursor.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-08 Thread Keith Packard 
Michel Dänzer  writes:

> Unfortunately, the reality is that at least the radeon and amdgpu
> drivers do use it.

Even modesetting uses it; there isn't any other public interface to get
the current cursor.

What do you think about eliminating the RefCursor/FreeCursor bits and
just letting the upper layer hold the reference?

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-08 Thread Michel Dänzer 
On 09.03.2016 05:57, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> It seems to work fine in my quick testing. However, I'm not sure it's
>> worth it, given that v2 of patch 1 fixes holding on to the last HW
>> cursor indefinitely, and given the churn it would cause for external
>> drivers.
> 
> I guess I don't see how this would have any effect on external drivers;
> they shouldn't be looking inside the xf86CrtcConfig structure for the
> cursor in any case.

Unfortunately, the reality is that at least the radeon and amdgpu
drivers do use it.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-08 Thread Keith Packard 
Michel Dänzer  writes:

> It seems to work fine in my quick testing. However, I'm not sure it's
> worth it, given that v2 of patch 1 fixes holding on to the last HW
> cursor indefinitely, and given the churn it would cause for external
> drivers.

I guess I don't see how this would have any effect on external drivers;
they shouldn't be looking inside the xf86CrtcConfig structure for the
cursor in any case.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2016-03-08 Thread Michel Dänzer 
On 25.12.2015 03:06, Keith Packard wrote:
> Michel Dänzer  writes:
> 
>> From: Michel Dänzer 
>>
>> 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.

Fixed in v2 of patch 1 of this series. If you or anyone could review
that (and possibly v2 of patch 2, though that's mostly identical with
v1, which you reviewed), I'll send a pull request for patches 1-3 & 5.


> 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.

It seems to work fine in my quick testing. However, I'm not sure it's
worth it, given that v2 of patch 1 fixes holding on to the last HW
cursor indefinitely, and given the churn it would cause for external
drivers.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 1/5] xfree86/modes: Refactor xf86_use_hw_cursor_argb to use xf86_use_hw_cursor

2015-12-24 Thread Keith Packard 
Michel Dänzer  writes:

> From: Michel Dänzer 
>
> 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änzer 
> ---
>
> 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.

From da2e5c4f6e08b765a6a3e7337c0da8dd60b531e0 Mon Sep 17 00:00:00 2001
From: Keith Packard 
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 
---
 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(>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;