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

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

[Thomas Hellstrom]

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

[PATCH 2/2] drm/vmwgfx: Implement the cursor_set2 callback

[Thomas Hellstrom]

Fixes native drm clients like Fedora 23 Wayland which now appears to
be able to use cursor hotspots without strange cursor offsets.
Also fixes a couple of ignored error paths.

Signed-off-by: Thomas Hellstrom <thellstrom@vmware.com>
Reviewed-by: Sinclair Yeh <syeh@vmware.com>
---
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 43 +++++++++++++++++++++++++++---------
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h  |  3 +++
 drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c  |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c |  1 +
 drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c |  1 +
 5 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
index 9fcd7f8..62aa098 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c
@@ -133,8 +133,13 @@ void vmw_cursor_update_position(struct vmw_private *dev_priv,
 	vmw_mmio_write(++count, fifo_mem + SVGA_FIFO_CURSOR_COUNT);
 }
 
-int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
-			   uint32_t handle, uint32_t width, uint32_t height)
+
+/*
+ * vmw_du_crtc_cursor_set2 - Driver cursor_set2 callback.
+ */
+int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv,
+			    uint32_t handle, uint32_t width, uint32_t height,
+			    int32_t hot_x, int32_t hot_y)
 {
 	struct vmw_private *dev_priv = vmw_priv(crtc->dev);
 	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
@@ -185,33 +190,36 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 	}
 	if (du->cursor_dmabuf)
 		vmw_dmabuf_unreference(&du->cursor_dmabuf);
-
+
 	/* setup new image */
+	ret = 0;
 	if (surface) {
 		/* vmw_user_surface_lookup takes one reference */
 		du->cursor_surface = surface;
 
 		du->cursor_surface->snooper.crtc = crtc;
 		du->cursor_age = du->cursor_surface->snooper.age;
-		vmw_cursor_update_image(dev_priv, surface->snooper.image,
-					64, 64, du->hotspot_x, du->hotspot_y);
+		ret = vmw_cursor_update_image(dev_priv, surface->snooper.image,
+					      64, 64, hot_x, hot_y);
 	} else if (dmabuf) {
 		/* vmw_user_surface_lookup takes one reference */
 		du->cursor_dmabuf = dmabuf;
 
 		ret = vmw_cursor_update_dmabuf(dev_priv, dmabuf, width, height,
-					       du->hotspot_x, du->hotspot_y);
+					       hot_x, hot_y);
 	} else {
 		vmw_cursor_update_position(dev_priv, false, 0, 0);
-		ret = 0;
 		goto out;
 	}
 
-	vmw_cursor_update_position(dev_priv, true,
-				   du->cursor_x + du->hotspot_x,
-				   du->cursor_y + du->hotspot_y);
+	if (!ret) {
+		vmw_cursor_update_position(dev_priv, true,
+					   du->cursor_x + hot_x,
+					   du->cursor_y + hot_y);
+		du->hotspot_x = hot_x;
+		du->hotspot_y = hot_y;
+	}
 
-	ret = 0;
 out:
 	drm_modeset_unlock_all(dev_priv->dev);
 	drm_modeset_lock_crtc(crtc, crtc->cursor);
@@ -219,6 +227,19 @@ out:
 	return ret;
 }
 
+/*
+ * vmw_du_crtc_cursor_set - Driver cursor_set callback using
+ * default values for the cursor hotspot.
+ */
+int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
+			   uint32_t handle, uint32_t width, uint32_t height)
+{
+	struct vmw_display_unit *du = vmw_crtc_to_du(crtc);
+
+	return vmw_du_crtc_cursor_set2(crtc, file_priv, handle, width, height,
+				       du->hotspot_x, du->hotspot_y);
+}
+
 int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y)
 {
 	struct vmw_private *dev_priv = vmw_priv(crtc->dev);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
index 782df7c..43771d3 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.h
@@ -195,6 +195,9 @@ void vmw_du_crtc_gamma_set(struct drm_crtc *crtc,
 			   uint32_t start, uint32_t size);
 int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv,
 			   uint32_t handle, uint32_t width, uint32_t height);
+int vmw_du_crtc_cursor_set2(struct drm_crtc *crtc, struct drm_file *file_priv,
+			    uint32_t handle, uint32_t width, uint32_t height,
+			    int32_t hot_x, int32_t hot_y);
 int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y);
 int vmw_du_connector_dpms(struct drm_connector *connector, int mode);
 void vmw_du_connector_save(struct drm_connector *connector);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
index bb63e4d..4d4e354 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_ldu.c
@@ -298,6 +298,7 @@ static struct drm_crtc_funcs vmw_legacy_crtc_funcs = {
 	.save = vmw_du_crtc_save,
 	.restore = vmw_du_crtc_restore,
 	.cursor_set = vmw_du_crtc_cursor_set,
+	.cursor_set2 = vmw_du_crtc_cursor_set2,
 	.cursor_move = vmw_du_crtc_cursor_move,
 	.gamma_set = vmw_du_crtc_gamma_set,
 	.destroy = vmw_ldu_crtc_destroy,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
index b96d1ab..91950d0 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_scrn.c
@@ -534,6 +534,7 @@ static struct drm_crtc_funcs vmw_screen_object_crtc_funcs = {
 	.save = vmw_du_crtc_save,
 	.restore = vmw_du_crtc_restore,
 	.cursor_set = vmw_du_crtc_cursor_set,
+	.cursor_set2 = vmw_du_crtc_cursor_set2,
 	.cursor_move = vmw_du_crtc_cursor_move,
 	.gamma_set = vmw_du_crtc_gamma_set,
 	.destroy = vmw_sou_crtc_destroy,
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
index b1fc1c0..e438cb1 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_stdu.c
@@ -1044,6 +1044,7 @@ static struct drm_crtc_funcs vmw_stdu_crtc_funcs = {
 	.save = vmw_du_crtc_save,
 	.restore = vmw_du_crtc_restore,
 	.cursor_set = vmw_du_crtc_cursor_set,
+	.cursor_set2 = vmw_du_crtc_cursor_set2,
 	.cursor_move = vmw_du_crtc_cursor_move,
 	.gamma_set = vmw_du_crtc_gamma_set,
 	.destroy = vmw_stdu_crtc_destroy,
-- 
2.4.3

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

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

[Daniel Vetter]

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

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

[Thomas Hellstrom]

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.

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
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Ville Syrjälä]

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.

> 
> 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
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Hellstrom]

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

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Hellstrom]

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

/Thomas

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

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thomas Hellstrom]

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

/Thomas





> -Daniel

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

Re: [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@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Sinclair Yeh]

Reviewed-by: Sinclair Yeh <syeh@vmware.com>

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