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

[Daniel Vetter <daniel@ffwll.ch>]

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

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.

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_crtc.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 24c5434..93f80a5 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2874,7 +2874,8 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  
>  static int drm_mode_cursor_common(struct drm_device *dev,
>  				  struct drm_mode_cursor2 *req,
> -				  struct drm_file *file_priv)
> +				  struct drm_file *file_priv,
> +				  bool from_2)
>  {
>  	struct drm_crtc *crtc;
>  	int ret = 0;
> @@ -2907,7 +2908,8 @@ static int drm_mode_cursor_common(struct drm_device *dev,
>  			goto out;
>  		}
>  		/* Turns off the cursor if handle is 0 */
> -		if (crtc->funcs->cursor_set2)
> +		if (crtc->funcs->cursor_set2 &&
> +		    (from_2 || !crtc->funcs->cursor_set))
>  			ret = crtc->funcs->cursor_set2(crtc, file_priv, req->handle,
>  						      req->width, req->height, req->hot_x, req->hot_y);
>  		else
> @@ -2953,7 +2955,7 @@ int drm_mode_cursor_ioctl(struct drm_device *dev,
>  	memcpy(&new_req, req, sizeof(struct drm_mode_cursor));
>  	new_req.hot_x = new_req.hot_y = 0;
>  
> -	return drm_mode_cursor_common(dev, &new_req, file_priv);
> +	return drm_mode_cursor_common(dev, &new_req, file_priv, false);
>  }
>  
>  /**
> @@ -2976,7 +2978,7 @@ int drm_mode_cursor2_ioctl(struct drm_device *dev,
>  {
>  	struct drm_mode_cursor2 *req = data;
>  
> -	return drm_mode_cursor_common(dev, req, file_priv);
> +	return drm_mode_cursor_common(dev, req, file_priv, true);
>  }
>  
>  /**
> -- 
> 2.4.3
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel