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

[Thomas Hellstrom <thellstrom@vmware.com>]

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

/Thomas







>
>> Thanks,
>> Thomas
>>
>>
>>> 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
>>>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIBAg&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=LvaoWKneZBMqoqvonmNbtrQ9sY1dL5unCG_D2_Xb--Y&s=IpenYkOeOFSMkCBvMp1_TEam7-KdFX4zbfCC943GC3M&e= 
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_dri-2Ddevel&d=BQIDAw&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=vpukPkBtpoNQp2IUKuFviOmPNYWVKmen3Jeeu55zmEA&m=lwkgECy6-cIOL48-pV4s5urKAJj4oXa5XJTz2d7w2lU&s=3KpOcVke0GPiMcmHBTMvxHe0SpUBMxF_D3A76OhRXtE&e= 


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel