[PATCH 1/2] drm: Avoid calling the cursor_set2 callback from the drm_mode_cursor ioctl

[Daniel Vetter]

On Mon, Nov 30, 2015 at 02:35:53PM +0100, Thomas Hellstrom wrote:
> On 11/29/2015 10:18 AM, Daniel Vetter wrote:
> > On Fri, Nov 27, 2015 at 01:24:11PM +0100, Thomas Hellstrom wrote:
> >> On 11/27/2015 01:02 PM, Ville Syrjälä wrote:
> >>> On Fri, Nov 27, 2015 at 12:42:15PM +0100, Thomas Hellstrom wrote:
> >>>> On 11/27/2015 11:11 AM, Daniel Vetter wrote:
> >>>>> On Thu, Nov 26, 2015 at 10:52:14AM -0800, Thomas Hellstrom wrote:
> >>>>>> If the drm_mode_cursor_ioctl is called and the cursor_set2 callback is
> >>>>>> implemented, the cursor hotspot is set to (0,0) which is incompatible
> >>>>>> with vmwgfx where the hotspot should instead remain unchanged.
> >>>>>>
> >>>>>> So if the driver implements both cursor_set2 and cursor_set, prefer calling
> >>>>>> the latter from the drm_mode_cursor ioctl.
> >>>>>>
> >>>>>> Signed-off-by: Thomas Hellstrom <thellstrom at vmware.com>
> >>>>> That looks like papering over a bug in the client - it simply shouldn't
> >>>>> mix these two if it expects the hotspot to stick around. There was also
> >>>>> recently a big discussion about hotspot behaviour, and the conclusion was
> >>>>> that qxl got it all wrong. Since that was specifically added for qxl I'm
> >>>>> not sure how well this was ever tested ...
> >>>> No, this is not the case, This is for old Xorg userspace that first sets
> >>>> the hotspot using our ancient
> >>>> driver-private ioctl and then calls drm_mode_cursor() to update the cursor.
> >>>>
> >>>> Now if we were to implement cursor_set2, which is apparently needed to
> >>>> get gnome-shell/Wayland cursors in the right place, without this fix, it
> >>>> would break old Xorg, so we don't have much choice in this case from
> >>>> what I can tell.
> >>>>
> >>>> The root problem here is that the drm_mode_cursor() behaviour in the
> >>>> presence of cursor_set2 didn't take the existing vmware hotspot
> >>>> semantics into account.
> > Ugh. I think the simplest solution is to not mix up the two hotspots, i.e.
> > separately keep track of both the legacy vmwgfx hotspot and the drm core
> > hotspot. Then in the actual cursor set code add them. A bit of ugly in the
> > vmwgfx code (but not much), instead of leaking that driver private legacy
> > ioctl semantics into drm core. Would that work?
> >
> >
> Hmm.
> 
> Yes it would probably work. Good idea.
> I guess we just need to make sure that both hotspots are reset to (0,0)
> at master drop.

Resetting kms state is an entire different can of worms topic. On one hand
we don't want it, since if you apply changes from the default (at early
boot or maybe with kernel cmdline options) we want userspace to take over
all those settings. Otoh if your compositor dies and leaves a mess behind
we should reset to /something/, but due to the above it's undefined what
the reset values should be. At least in general. The other problem is
trying to figure out when exactly you should restore - we already have
plenty fun trying to decide when to restore fbcon, with a pile of hacks.

With atomic at least userspace can take a full snapshot of all kms state,
even if it doesn't understand all the properties. Only requirement is that
all tunables are exported as properties. Then it can always restore that
snapshot. Given that I'm leaning towars "this should be solved in
userspace by some priveldged daemon like logind". Means old userspace is
out of luck if X dies (at least for all the properties that don't get
naturally overwritten), but that's been the case since forever.

Trying to do that in the kernel is imo something that's not really
possible - the kernel knows too little about overall system state and
configuration.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch