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

[Daniel Vetter]

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?

> >>> The other problem seems to be that X is racy with updating cursors (since
> >>> it does a setPos and setCursor separately, despite that this ioctl can do
> >>> it in one go) and so if you move the hotspot you get a jumpy cursor.
> >>> Radeon tried to paper over that but fundamentally you need to fix X and
> >>> use atomic (or at least universal plane cursor support) to fix this.
> >>>
> >>> Given all that I'm leaning towards "the future is atomic", let's please
> >>> not add hacks to legacy code. Aside: Atomic doesn't have the hotspot-x/y
> >>> properties yet, but really the only reason was that we don't yet have an
> >>> atomic driver needing them. Adding those props will be a tiny patch of a
> >>> few lines.
> >> Sinclair has started working on atomic support for vmwgfx, but we really
> >> need to fix this also in
> >> the legacy code, ugly or not.
> > I was thinking maybe it would be cleaner to not reset the hotspot to
> > 0,0 in drm_mode_cursor_ioctl() and instead keep what was previously
> > configured, and only do the reset to 0,0 in lastclose (or some other
> > suitable place)? But I suppose that could break something else that
> > assume that drm_mode_cursor_ioctl() will do the reset.
> 
> If something assumes that drm_mode_cursor_ioctl() will reset the
> hotspot, then we're already in trouble because then we have different
> semantics of the same ioctl assumed by user-space.
> 
> However, from what I can tell, getting the cursor hotspot position from
> within core DRM would require new lock protected hotspot coordinate
> members in the crtc structure...

Yeah, and for atomic we definitely want those in drm_crtc_state, but for
legacy we'd need it in drm_crtc. I'd prefer if we don't need to add it to
both places ;-)

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