[PATCH xserver 3/3] ramdac: Handle master and slave cursors independently
Hans de Goede
hdegoede at redhat.com
Wed Nov 8 10:39:03 UTC 2017
Hi,
On 08-11-17 05:15, Alex Goins wrote:
> Change 7b634067 added HW cursor support for PRIME by removing the
> pixmap_dirty_list check from xf86CursorSetCursor() and making the requisite
> cursor functions set/check the cursor both on master and slave.
>
> However, before this change, drivers that did not make use of pixmap_dirty_list
> to implement PRIME master were able to pass this check. That may have been a
> bug, but in effect it allowed hardware cursor to be enabled with PRIME, where
> the master composites the cursor into the shared pixmap.
>
> Naturally, the slave driving an actual hardware cursor is preferable to the
> master compositing a cursor into the shared pixmap, but there are certain
> situations where the slave cannot drive a hardware cursor (certain DRM drivers,
> or when used with a transform). In these cases the master may still be capable
> of compositing a cursor, and that would be preferable to using the server's
> software cursor (due to the fact that it's unsynchronzied by OpenGL rendering to
> the root window, it can cause corruption with certain compositors).
>
> The PRIME HW cursor change works by checking both master and slave HW cursor
> capabilities, and setting the cursor on both. For masters such as modesetting,
> the master cursor capabilities check will pass but the cursor set will be a
> NOOP, effectively driving a hardware cursor only on the slave while still
> passing the check. However, if two different drivers with different sets of
> capabilities are used, HW cursor ends up only being used if the intersection of
> capabilities are supported. For example, if the master can drive a cursor with a
> transform, and the slave can't, checking capabilities on both means we
> unnecessarily fall back to SW cursor.
>
> To alleviate this issue while still prioritizing HW cursor on the slave, this
> change restructures the HW cursor code such that the slave aspects of these
> functions are tried first, falling back to the master if they fail.
>
> SlaveHWCursor, a flag for querying if the hardware cursor is being driven by
> slaves, is added to xf86CursorScreenRec, resulting in an ABI break.
>
> Signed-off-by: Alex Goins <agoins at nvidia.com>
> ---
> hw/xfree86/ramdac/xf86CursorPriv.h | 3 +
> hw/xfree86/ramdac/xf86HWCurs.c | 114 +++++++++++++++++++++++++++----------
> 2 files changed, 88 insertions(+), 29 deletions(-)
>
> diff --git a/hw/xfree86/ramdac/xf86CursorPriv.h b/hw/xfree86/ramdac/xf86CursorPriv.h
> index 397d2a1..83ea379 100644
> --- a/hw/xfree86/ramdac/xf86CursorPriv.h
> +++ b/hw/xfree86/ramdac/xf86CursorPriv.h
> @@ -35,6 +35,9 @@ typedef struct {
> Bool HWCursorForced;
>
> void *transparentData;
> +
> + /* If hardware cursor is being driven by slaves */
> + Bool SlaveHWCursor;
> } xf86CursorScreenRec, *xf86CursorScreenPtr;
>
> Bool xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y);
> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
> index 15b1cd7..786ed54 100644
> --- a/hw/xfree86/ramdac/xf86HWCurs.c
> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
> @@ -136,18 +136,13 @@ xf86ScreenCheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr i
> (!infoPtr->UseHWCursor || infoPtr->UseHWCursor(pScreen, cursor)));
> }
>
> -Bool
> -xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr)
> +static Bool
> +xf86CheckSlavesHWCursor_locked(ScreenPtr pScreen, CursorPtr cursor)
> {
> ScreenPtr pSlave;
> - Bool use_hw_cursor = TRUE;
> -
> - input_lock();
>
> - if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr)) {
> - use_hw_cursor = FALSE;
> - goto unlock;
> - }
> + Bool use_hw_cursor = FALSE;
> + Bool slave_consuming_pixmap = FALSE;
>
> /* ask each driver consuming a pixmap if it can support HW cursor */
> xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> @@ -156,6 +151,11 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr
> if (!RRHasScanoutPixmap(pSlave))
> continue;
>
> + if (!slave_consuming_pixmap) {
> + slave_consuming_pixmap = TRUE;
> + use_hw_cursor = TRUE;
> + }
> +
> sPriv = dixLookupPrivate(&pSlave->devPrivates, xf86CursorScreenKey);
> if (!sPriv) { /* NULL if Option "SWCursor", possibly other conditions */
> use_hw_cursor = FALSE;
> @@ -169,6 +169,26 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr
> }
> }
>
> + /* there are active slaves and they all support hw cursor */
> + return (slave_consuming_pixmap && use_hw_cursor);
> +}
> +
> +Bool
> +xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr)
> +{
> + Bool use_hw_cursor = FALSE;
> +
> + input_lock();
> +
> + if (xf86CheckSlavesHWCursor_locked(pScreen, cursor)) {
> + use_hw_cursor = TRUE;
> + goto unlock;
This goto seems wrong, what if the secondary GPU has outputs which
can do a hw-cursor, but the primary GPU also has active video outputs
and the primary GPU does not support a hw-cursor ?
I think you always need to check the master supports a hw-cursor.
> + }
> +
> + /* if there are no active slaves or not all of them support hw cursor, use
> + * hw cursor on master */
> + use_hw_cursor = xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr);
> +
> unlock:
> input_unlock();
>
> @@ -235,13 +255,35 @@ xf86ScreenSetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> return xf86DriverShowCursor(infoPtr);
> }
>
> +static Bool
> +xf86SetSlavesCursor_locked(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> +{
> + ScreenPtr pSlave;
> +
> + /* ask each slave driver to set the cursor. */
> + xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> + if (!RRHasScanoutPixmap(pSlave))
> + continue;
> +
> + if (!xf86ScreenSetCursor(pSlave, pCurs, x, y)) {
> + /*
> + * hide the slave cursors,
> + * so that we can gracefully fall back to master
> + */
> + xf86SetSlavesCursor_locked(pScreen, NullCursor, x, y);
> + return FALSE;
> + }
> + }
> +
> + return TRUE;
> +}
> +
> Bool
> xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> {
> xf86CursorScreenPtr ScreenPriv =
> (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
> xf86CursorScreenKey);
> - ScreenPtr pSlave;
> Bool ret = FALSE;
>
> input_lock();
> @@ -249,24 +291,36 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> x -= ScreenPriv->HotX;
> y -= ScreenPriv->HotY;
>
> - if (!xf86ScreenSetCursor(pScreen, pCurs, x, y))
> + if (pCurs == NullCursor) {
> + xf86SetSlavesCursor_locked(pScreen, pCurs, x, y);
> + xf86ScreenSetCursor(pScreen, pCurs, x, y);
> +
> + ret = TRUE;
> goto out;
> + }
>
> - /* ask each slave driver to set the cursor. */
> - xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> - if (!RRHasScanoutPixmap(pSlave))
> - continue;
> + /* if there are active slaves that support HW cursor, use instead of the
> + * master */
> + if (xf86CheckSlavesHWCursor_locked(pScreen, pCurs)) {
> + if (xf86SetSlavesCursor_locked(pScreen, pCurs, x, y)) {
>
> - if (!xf86ScreenSetCursor(pSlave, pCurs, x, y)) {
> - /*
> - * hide the master (and successfully set slave) cursors,
> - * otherwise both the hw and sw cursor will show.
> - */
> - xf86SetCursor(pScreen, NullCursor, x, y);
> + if (!ScreenPriv->SlaveHWCursor) {
> + xf86ScreenSetCursor(pScreen, NullCursor, x, y);
> + ScreenPriv->SlaveHWCursor = TRUE;
> + }
> +
> + ret = TRUE;
> goto out;
> }
> }
> - ret = TRUE;
> +
> + /* if there are no active slaves that support HW cursor, or using the HW
> + * cursor on them failed, use the master */
> + if (ScreenPriv->SlaveHWCursor) {
> + xf86SetSlavesCursor_locked(pScreen, NullCursor, x, y);
> + ScreenPriv->SlaveHWCursor = FALSE;
> + }
> + ret = xf86ScreenSetCursor(pScreen, pCurs, x, y);
>
> out:
> input_unlock();
Again what if both the primary and secondary GPU have active video outputs,
then you need to update the cursor on both. You seem te be thinking about
something like the XPS15 here, used with the nvidia driver forcing the
nvidia GPU to become the primary GPU, so the primary will not have any
video-outputs. But when using for example a Latitude E6430 in optimus
mode with nouveau, the intel GPU will be the primary, driving the LCD
panel and the nvidia GPU will be the secondary driving the HDMI output,
so you need to set the cursor on both.
> @@ -326,14 +380,16 @@ xf86MoveCursor(ScreenPtr pScreen, int x, int y)
> x -= ScreenPriv->HotX;
> y -= ScreenPriv->HotY;
>
> - xf86ScreenMoveCursor(pScreen, x, y);
> + if (ScreenPriv->SlaveHWCursor) {
> + /* ask each slave driver to move the cursor */
> + xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> + if (!RRHasScanoutPixmap(pSlave))
> + continue;
>
> - /* ask each slave driver to move the cursor */
> - xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> - if (!RRHasScanoutPixmap(pSlave))
> - continue;
> -
> - xf86ScreenMoveCursor(pSlave, x, y);
> + xf86ScreenMoveCursor(pSlave, x, y);
> + }
> + } else {
> + xf86ScreenMoveCursor(pScreen, x, y);
> }
>
> input_unlock();
>
Regards,
Hans
More information about the xorg-devel
mailing list