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

[Hans de Goede]

Hi,

On 09-11-17 02:34, Alex Goins wrote:
> 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.

I understand, but I now realize that was partially bad advice, IMHO
we really don't want to grow the ABI, if at all possible we want to
shrink it. In my mind exporting a function is less likely to lead to
future ABI breaks then exporting the contents of a struct, hence
my preference for the helper function.

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

Ok.


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

I'll leave further discussion of this between you and Michel Däzner. FWIW
I do believe that Michel's suggestion to implement the master-driven cursor
overlay in some generic place so it works with all the drivers is a better
solution then allowing drivers to opt-out.

Regards,

Hans