Re: [PATCH xserver 2/5] xfree86/modes: Check for CRTC transforms in xf86_use_hw_cursor(_argb)

[Michel Dänzer]

On 24.12.2015 16:57, Michel Dänzer wrote:
> From: Michel Dänzer 
> 
> We currently don't handle transforms for the HW cursor image, so return
> FALSE to signal a software cursor must be used if a transform is in use
> on any CRTC.
> 
> Signed-off-by: Michel Dänzer 
> ---
>  hw/xfree86/modes/xf86Cursors.c | 11 +++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/hw/xfree86/modes/xf86Cursors.c b/hw/xfree86/modes/xf86Cursors.c
> index f487890..14bd5f6 100644
> --- a/hw/xfree86/modes/xf86Cursors.c
> +++ b/hw/xfree86/modes/xf86Cursors.c
> @@ -515,11 +515,22 @@ xf86_use_hw_cursor(ScreenPtr screen, CursorPtr cursor)
>  ScrnInfoPtr scrn = xf86ScreenToScrn(screen);
>  xf86CrtcConfigPtr xf86_config = XF86_CRTC_CONFIG_PTR(scrn);
>  xf86CursorInfoPtr cursor_info = xf86_config->cursor_info;
> +int c;
>  
>  if (cursor->bits->width > cursor_info->MaxWidth ||
>  cursor->bits->height > cursor_info->MaxHeight)
>  return FALSE;
>  
> +for (c = 0; c < xf86_config->num_crtc; c++) {
> +xf86CrtcPtr crtc = xf86_config->crtc[c];
> +
> +if (!crtc->enabled)
> +continue;
> +
> +if (crtc->transform_in_use)
> +return FALSE;
> +}

Turns out crtc->transform_in_use is also TRUE for rotation, which works
correctly with a HW cursor. crtc->transformPresent needs to be checked
again instead.


-- 
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: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver 3/5] xfree86: Re-set current cursor after RandR 1.2 CRTC configuration change

[Keith Packard]

Michel Dänzer  writes:

> From: Michel Dänzer 
>
> Add xf86CursorResetCursor, which allows switching between HW and SW
> cursor depending on the current state.
>
> Call it from xf86DisableUnusedFunctions, which is called after any CRTC
> configuration change such as setting a mode or disabling a CRTC. This
> makes sure that SW cursor is used e.g. while a transform is in use on
> any CRTC or while there are active PRIME output slaves, and enables HW
> cursor again once none of those conditions are true anymore.
>
> Signed-off-by: Michel Dänzer 

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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

Re: [PATCH xserver 4/5] modesetting: Remove xf86_reload_cursors call

[Keith Packard]

Michel Dänzer  writes:

> From: Michel Dänzer 
>
> It was already disabled, but it's definitely no longer needed now that
> xf86CursorResetCursor is getting called for each CRTC configuration
> change.

I think we can remove xf86_reload_cursors from the server now. It is not
called anywhere else.

-- 
-keith


signature.asc
Description: PGP signature
___
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

Re: [PATCH xserver 5/5] modesetting: Allow CRTC transforms to actually take effect

[Keith Packard]

Michel Dänzer  writes:

> From: Michel Dänzer 
>
> Setting crtc->transformPresent to FALSE was preventing the transform
> from actually taking effect and putting RandR into a confused state.
>
> Now that the RandR 1.2 cursor code handles transforms correctly, we can
> allow them to properly take effect.
>
> Signed-off-by: Michel Dänzer 

I've already marked this as:

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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

Re: [PATCH xserver 2/5] xfree86/modes: Check for CRTC transforms in xf86_use_hw_cursor(_argb)

[Keith Packard]

Michel Dänzer  writes:

> From: Michel Dänzer 
>
> We currently don't handle transforms for the HW cursor image, so return
> FALSE to signal a software cursor must be used if a transform is in use
> on any CRTC.

Reviewed-by: Keith Packard 

-- 
-keith


signature.asc
Description: PGP signature
___
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

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

[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;