[PATCH v6 xserver 7/7] xf86Cursor: Add hw cursor support for prime

[Aaron Plattner]

Adding Alex.

On 09/07/2016 05:26 AM, Hans de Goede wrote:
> From: Dave Airlie <airlied at redhat.com>
>
> Currently with PRIME if we detect a secondary GPU,
> we switch to using SW cursors, this isn't optimal,
> esp for the intel/nvidia combinations, we have
> no choice for the USB offload devices.
>
> This patch checks on each slave screen if hw
> cursors are enabled, and also calls set cursor
> and move cursor on all screens.
>
> Cc: Aaron Plattner <aplattner at nvidia.com>
> Signed-off-by: Dave Airlie <airlied at redhat.com>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> Reviewed-by: Michel Dänzer <michel.daenzer at amd.com>
> ---
> Changes in v6 (Hans de Goede):
> -Move removal of xorg_list_is_empty(&pScreen->pixmap_dirty_list)
>  check from xf86CursorSetCursor() back to this patch (accidentally moved to
>  "xf86Cursor: Add xf86CheckHWCursor() helper function" in v5)
>
> Changes in v5 (Hans de Goede):
> -Split out various helper functions into separate patches
> -Fix cursor showing in the wrong spot on slave-outputs when rotation is used
>
> Changes in v4 (Hans de Goede):
> -Fix crash when xf86ScreenSetCursor() gets called on a hot-plugged GPU
>
> Changes in v3 (Hans de Goede):
> -Re-indent xf86CursorScreenCheckHW
> -Loop over slave-outputs instead of over pixmap_dirty_list, Aaron Plattner has
>  indicated that the nvidia driver does not use pixmap_dirty_list and with
>  the recent "randr/xf86: Add PRIME Synchronization / Double Buffer" changes
>  there may be 2 pixmaps pointing to the same GPU screen on the pixmap_dirty_list twice
> -Make xf86CurrentCursor work when called on GPU screen pointers
>  (fixes the modesetting driver as outputslave crashing)
> -Fix both hw and sw cursor showing at the same time when an usb slave-gpu is
>  present at cold plug time
>
> Changes in v2:
> -hotplugging causes the driver to try and register
>  the cursor private, however we can't register
>  these at runtime yet, we need to add support for
>  reallocation of cursor screen privates. However
>  all hotplugged devices currently don't support
>  HW cursors, so punt for now. This means GPUs
>  already in the system will get hw cursors
>  and USB ones won't which is all we care about
>  presently.
> -drop list empty checks (Eric)
> -fixup comments (Michel)
> ---
>  dix/dispatch.c                 |  4 +++
>  hw/xfree86/ramdac/xf86Cursor.c |  2 +-
>  hw/xfree86/ramdac/xf86HWCurs.c | 78 +++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 78 insertions(+), 6 deletions(-)
>
> diff --git a/dix/dispatch.c b/dix/dispatch.c
> index a3c2fbb..21c387e 100644
> --- a/dix/dispatch.c
> +++ b/dix/dispatch.c
> @@ -3965,6 +3965,10 @@ AddGPUScreen(Bool (*pfnInit) (ScreenPtr /*pScreen */ ,
>
>      update_desktop_dimensions();
>
> +    if (!dixPrivatesCreated(PRIVATE_CURSOR))
> +        dixRegisterScreenPrivateKey(&cursorScreenDevPriv, pScreen,
> +                                    PRIVATE_CURSOR, 0);
> +
>      return i;
>  }
>
> diff --git a/hw/xfree86/ramdac/xf86Cursor.c b/hw/xfree86/ramdac/xf86Cursor.c
> index 623a65b..afcce53 100644
> --- a/hw/xfree86/ramdac/xf86Cursor.c
> +++ b/hw/xfree86/ramdac/xf86Cursor.c
> @@ -337,7 +337,7 @@ xf86CursorSetCursor(DeviceIntPtr pDev, ScreenPtr pScreen, CursorPtr pCurs,
>              return;
>          }
>
> -        if (infoPtr->pScrn->vtSema && xorg_list_is_empty(&pScreen->pixmap_dirty_list) &&
> +        if (infoPtr->pScrn->vtSema &&
>              (ScreenPriv->ForceHWCursorCount ||
>               xf86CheckHWCursor(pScreen, cursor, infoPtr))) {
>
> diff --git a/hw/xfree86/ramdac/xf86HWCurs.c b/hw/xfree86/ramdac/xf86HWCurs.c
> index 0f6990a..25d9f5d 100644
> --- a/hw/xfree86/ramdac/xf86HWCurs.c
> +++ b/hw/xfree86/ramdac/xf86HWCurs.c
> @@ -17,6 +17,7 @@
>  #include "cursorstr.h"
>  #include "mi.h"
>  #include "mipointer.h"
> +#include "randrstr.h"
>  #include "xf86CursorPriv.h"
>
>  #include "servermd.h"
> @@ -129,8 +130,8 @@ xf86ShowCursor(xf86CursorInfoPtr infoPtr)
>      return TRUE;
>  }
>
> -Bool
> -xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr)
> +static Bool
> +xf86ScreenCheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr)
>  {
>      return
>          (cursor->bits->argb && infoPtr->UseHWCursorARGB &&
> @@ -142,7 +143,29 @@ xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr
>  }
>
>  Bool
> -xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> +xf86CheckHWCursor(ScreenPtr pScreen, CursorPtr cursor, xf86CursorInfoPtr infoPtr)
> +{
> +    ScreenPtr pSlave;
> +
> +    if (!xf86ScreenCheckHWCursor(pScreen, cursor, infoPtr))
> +        return FALSE;
> +
> +    /* ask each driver consuming a pixmap if it can support HW cursor */
> +    xorg_list_for_each_entry(pSlave, &pScreen->slave_list, slave_head) {
> +        xf86CursorScreenPtr sPriv;
> +
> +        if (!RRHasScanoutPixmap(pSlave))
> +            continue;

A hypothetical future version of the nvidia driver that supports being 
loaded as a GPU screen probably wouldn't use the RR scanout pixmap 
stuff, but I suppose we can deal with that later if it turns out to be a 
problem.

Is this just to avoid showing the cursor on a GPU screen that is 
displaying the dummy framebuffer that it allocated at ScreenInit? Can we 
just rely on xf86CollectEnabledOutputs returning FALSE so we can assert 
that enabled outputs on a GPU screen are always displaying the master 
screen?

> +        sPriv = dixLookupPrivate(&pSlave->devPrivates, xf86CursorScreenKey);
> +        if (!xf86ScreenCheckHWCursor(pSlave, cursor, sPriv->CursorInfoPtr))
> +            return FALSE;
> +    }
> +    return TRUE;
> +}
> +
> +static Bool
> +xf86ScreenSetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>  {
>      xf86CursorScreenPtr ScreenPriv =
>          (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
> @@ -155,6 +178,10 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>          return TRUE;
>      }
>
> +    /* Hot plugged GPU's do not have a CursorScreenKey, force sw cursor */
> +    if (!_dixGetScreenPrivateKey(CursorScreenKey, pScreen))
> +        return FALSE;
> +
>      bits =
>          dixLookupScreenPrivate(&pCurs->devPrivates, CursorScreenKey, pScreen);
>
> @@ -187,6 +214,31 @@ xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
>
>  }
>
> +Bool
> +xf86SetCursor(ScreenPtr pScreen, CursorPtr pCurs, int x, int y)
> +{
> +    ScreenPtr pSlave;
> +
> +    if (!xf86ScreenSetCursor(pScreen, pCurs, x, y))
> +        return FALSE;
> +
> +    /* 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 master (and successfully set slave) cursors,
> +             * otherwise both the hw and sw cursor will show.
> +             */
> +            xf86SetCursor(pScreen, NullCursor, x, y);
> +            return FALSE;

I don't know why it would happen, but in theory setting a HW cursor 
could fail on slave N+1 and then clearing it could fail on slave N, 
leaving N+1 displaying a HW cursor. In addition, if some dumb slave 
screen fails xf86ScreenSetCursor(pSlave, NullCursor, x, y) consistently, 
this code will overrun the stack.

We can assert that setting a null cursor should always succeed, but it's 
a complicated enough code path that that's not obvious.

> +        }
> +    }
> +    return TRUE;
> +}
> +
>  void
>  xf86SetTransparentCursor(ScreenPtr pScreen)
>  {
> @@ -209,8 +261,8 @@ xf86SetTransparentCursor(ScreenPtr pScreen)
>      xf86ShowCursor(infoPtr);
>  }
>
> -void
> -xf86MoveCursor(ScreenPtr pScreen, int x, int y)
> +static void
> +xf86ScreenMoveCursor(ScreenPtr pScreen, int x, int y)
>  {
>      xf86CursorScreenPtr ScreenPriv =
>          (xf86CursorScreenPtr) dixLookupPrivate(&pScreen->devPrivates,
> @@ -224,6 +276,22 @@ xf86MoveCursor(ScreenPtr pScreen, int x, int y)
>  }
>
>  void
> +xf86MoveCursor(ScreenPtr pScreen, int x, int y)
> +{
> +    ScreenPtr pSlave;
> +
> +    xf86ScreenMoveCursor(pScreen, x, y);
> +
> +    /* 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);
> +    }
> +}
> +
> +void
>  xf86RecolorCursor(ScreenPtr pScreen, CursorPtr pCurs, Bool displayed)
>  {
>      xf86CursorScreenPtr ScreenPriv =
>

-- 
Aaron

nvpublic