[PATCH v6 xserver 7/7] xf86Cursor: Add hw cursor support for prime
Alex Goins
agoins at nvidia.com
Wed Sep 7 23:32:37 UTC 2016
Hi Hans,
Thanks for this, it will definitely be a big improvement.
I've been testing these patches against the NVIDIA driver. Some concerns -
What would be the best way to query from the master if the slave is using a HW
cursor? We typically composite the cursor onto the shared pixmap, and with this
it results in a double-cursor, so we need a way to tell if we shouldn't be doing
that.
I'm encountering some very strange issues when running these with PRIME
Synchronization enabled, even after fixing our internal cursor handling. When
moving the cursor around without damaging anything, it's smooth. When having an
application causing damage, and thus flipping, without the cursor moving, it's
smooth. When moving the cursor AND causing damage, both the cursor and the
flipping seem to be going at roughly 30 Hz, half speed.
Skipping the call to either drmModeMoveCursor() in drmmode_set_cursor_position()
or the call to drmModePageFlip() in drmmode_SharedPixmapFlip() alleviates the
issue for flipping or cursor moving, respectively, so I'm inclined to say that
drmModeMoveCursor() or drmModePageFlip() are interfering with each other. Could
that be?
Thanks,
Alex
On Wed, 7 Sep 2016, Aaron Plattner wrote:
> On 09/07/2016 08:51 AM, Hans de Goede wrote:
> > Hi,
> >
> > On 07-09-16 17:38, Aaron Plattner wrote:
> >> On 09/07/2016 08:24 AM, Hans de Goede wrote:
> >>> Hi,
> >>>
> >>> On 07-09-16 17:02, Aaron Plattner wrote:
> >>>> 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?
> >>>
> >>> I tried to stay as close as possible to the original check for
> >>> slave outputs being active in xf86CursorSetCursor(), which checked:
> >>> xorg_list_is_empty(&pScreen->pixmap_dirty_list)
> >>>
> >>> using xf86CollectEnabledOutputs will not work, it starts with:
> >>>
> >>> if (scrn->is_gpu)
> >>> return FALSE;
> >>
> >> What I meant is that xf86CollectEnabledOutputs will cause
> >> xf86InitialConfiguration to not enable any outputs on GPU screens, so
> >> any outputs that are enabled later must necessarily be PRIME display
> >> sinks and should have the cursor displayed.
> >
> > Ah, I see, yes that sounds reasonable, but as said for the
> > initial patch I want to stay as close as possible to the original
> > check for slave outputs being active in xf86CursorSetCursor(), which
> > checked: xorg_list_is_empty(&pScreen->pixmap_dirty_list)
> >
> > But we can certainly switch to another check in later patches.
>
> That's fair.
>
> The change looks okay to me, but I'd like to get Alex's opinion too
> since he worked on prime the most recently.
>
> -- Aaron
>
> > Regards,
> >
> > Hans
> >
> >
> >
> >
> >
> >>
> >> It used to be the case that GPU screens would allocate a framebuffer
> >> and xf86InitialConfiguration would display it, but that check that was
> >> added to xf86CollectEnabledOutputs fixed that problem.
> >>
> >>>>> + 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.
> >>>
> >>> Right, if any GPUs fail to hide the cursor we get 2 cursors,
> >>> that is no different from what we've today, today the master
> >>> will use a hw cursor until a slave-output becomes active, if
> >>> the master then fails to hide the cursor we end up with 2
> >>> cursors. If hiding a cursor fails, therse is nothing we can do
> >>> really.
> >>>
> >>>> In addition, if some dumb slave screen fails
> >>>> xf86ScreenSetCursor(pSlave, NullCursor, x, y) consistently, this code
> >>>> will overrun the stack.
> >>>
> >>> No I already checked that we cannot get unlimited recursion (one
> >>> should always check that when recursing) xf86ScreenSetCursor contains:
> >>>
> >>> if (pCurs == NullCursor) {
> >>> (*infoPtr->HideCursor) (infoPtr->pScrn);
> >>> return TRUE;
> >>> }
> >>>
> >>> Right at the top so xf86SetCursor will always succeed when we call
> >>> it with the NullCursor.
> >>
> >> Ah, okay. Thanks. I missed that part.
> >>
> >>>>
> >>>> We can assert that setting a null cursor should always succeed, but
> >>>> it's a complicated enough code path that that's not obvious.
> >>>
> >>> See above, we do not actually set a NullCursor instead we call
> >>> infoPtr->HideCursor and always return TRUE.
> >>>
> >>>>
> >>>>> + }
> >>>>> + }
> >>>>> + 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 =
> >>>>>
>
>
More information about the xorg-devel
mailing list