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

[Daniel Vetter]

On Mon, Nov 30, 2015 at 04:52:22PM +0100, Thomas Hellstrom wrote:
> On 11/30/2015 03:55 PM, Daniel Vetter wrote:
> > 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.
> 
> You're probably right, but in this particular case a master using the
> legacy vmware hotspot would be severely confused by another master using
> the drm core hotspot, and vice versa even if both thought they did
> everything right and reset "their" hotspot at master_set().
> 
> Another option would of course be to use some heuristic to try to
> determine what hotspot is the "correct" one to use, instead of adding them.

Yeah, a one-off for vmwgfx shouldn't cause problems, since we already have
the master hooks for vmwgfx anyway. Just wanted to add that doing this in
generic code is imo fraught with peril and long-term probably not
something where we want to go to. I think it's perfectly fine if you add a
bit of code to vmwgfx to reset things and make the interplay with
vmw-legacy and drm hotspot work.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch