[PATCH xserver 3/3] ramdac: Handle master and slave cursors independently

[Alex Goins]

Thanks both for the quick feedback. Replies inline.

On Wed, 8 Nov 2017, Hans de Goede wrote:

> > https://lists.x.org/archives/xorg-devel/2016-September/050973.html implies
> > that
> > xf86CursorScreenPtr is part of the ABI. Mark xf86CursorPriv.h as such.
> 
> I'm not sure if exporting this is a good idea. I assume you are just
> after xf86CursorScreenPtr->SWCursor ?  If so it is probably a better
> idea to add a helper function taking pScreen which gets that and add
> that function to the ABI.

That works for me, this is just based on the past advice for drivers to lookup
xf86CursorScreenPtr to tell if we are using a SW cursor, which sets a precedent
of it being considered ABI.

> > +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 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.

You're right, there is a possibility of combined native and PRIME outputs. In
fact, I typically test on a desktop system with iGPU Multi-Monitor, where this
possibility is especially clear.

I overlooked the case of native + PRIME where the PRIME slave supports HW
cursor. Native + PRIME with non-HW cursor slave works, but the former doesn't.

On Wed, 8 Nov 2017, Michel Dänzer 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,
> 
> How and where exactly would this "compositing the cursor into the shared
> pixmap" happen? Looks like this is just left for the master screen
> driver to handle? If so, there probably needs to be a mechanism for the
> master screen driver to opt into this.

Yes, it's just left for the master screen the handle as if it were a hardware
cursor.

Prior to change 7b634067 this was the existing behavior of the NVIDIA driver +
PRIME outputs, due to the fact that the check to exclude HW cursor support on
PRIME outputs relied on pScreen->pixmap_dirty_list not being empty. With NVIDIA
as the master, X would successfully set a cursor on the master despite PRIME
being in use, and the master would use that to composite its own cursor into the
shared pixmap.

Change 7b634067 had the side effect of preventing this configuration, and the
intent here is to re-enable it as an alternative fallback to the server's SW
cursor, while maintaining the possibility of slave-driven HW cursor in
configurations that support it.

> > 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).
> 
> Frankly, that sounds like an issue with your direct rendering
> infrastructure. We used to have the same issue with DRI1, but no more
> with DRI2/DRI3 (we still have an intermittent cursor flickering issue
> though, but not corruption).

Really? I observe the same issue using mesa + modesetting + glamor + i915 +
mutter. Dragging a window around with software cursor produces a square of
corruption around the cursor.

In any case, I would like some means to bring back the pre-7b634067
functionality. In situations such as using UDL as a slave, it appears as a
regression. An opt-in mechanism for drivers that support master-driven cursor is
acceptable, I think. I'll follow up with a new patch set implementing the
feedback so far if you agree.

Thanks,
Alex