[PATCH v6 xserver 7/7] xf86Cursor: Add hw cursor support for prime
Hans de Goede
hdegoede at redhat.com
Wed Sep 7 15:24:53 UTC 2016
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;
>> + 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.
>
> 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 =
>>
>
Regards,
Hans
More information about the xorg-devel
mailing list