[PATCH] fbfront: advertise either absolute or relative coordinates

[PATCH] fbfront: advertise either absolute or relative coordinates

[Olaf Hering]

A virtualized display device is usually viewed with the vncviewer
application, either by 'xm vnc domU' or with vncviewer localhost:port.
vncviewer and the RFB protocol provides absolute coordinates to the
virtual display. These coordinates are either passed through to a PV
guest or converted to relative coordinates for a HVM guest.

A PV guest receives these coordinates and passes them to the kernels
evdev driver. There it can be picked up by applications such as the
xorg-input drivers. Using absolute coordinates avoids issues such as
guest mouse pointer not tracking host mouse pointer due to wrong mouse
acceleration settings in the guests X display.

Advertise either absolute or relative coordinates to the input system
and the evdev driver, depending on what dom0 provides. The xorg-input
driver prefers relative coordinates even if a devices provides both.

Signed-off-by: Olaf Hering <olaf@aepfle.de>

---
 drivers/xen/fbfront/xenkbd.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

Index: linux-2.6.18-xen.hg/drivers/xen/fbfront/xenkbd.c
===================================================================
--- linux-2.6.18-xen.hg.orig/drivers/xen/fbfront/xenkbd.c
+++ linux-2.6.18-xen.hg/drivers/xen/fbfront/xenkbd.c
@@ -104,7 +104,7 @@ static irqreturn_t input_handler(int rq,
 int __devinit xenkbd_probe(struct xenbus_device *dev,
 			   const struct xenbus_device_id *id)
 {
-	int ret, i;
+	int ret, i, abs;
 	struct xenkbd_info *info;
 	struct input_dev *kbd, *ptr;
 
@@ -123,6 +123,11 @@ int __devinit xenkbd_probe(struct xenbus
 	info->page->in_cons = info->page->in_prod = 0;
 	info->page->out_cons = info->page->out_prod = 0;
 
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-abs-pointer", "%d", &abs) < 0)
+		abs = 0;
+	if (abs)
+		xenbus_printf(XBT_NIL, dev->nodename, "request-abs-pointer", "1");
+
 	/* keyboard */
 	kbd = input_allocate_device();
 	if (!kbd)
@@ -155,10 +160,15 @@ int __devinit xenkbd_probe(struct xenbus
 	ptr->id.bustype = BUS_PCI;
 	ptr->id.vendor = 0x5853;
 	ptr->id.product = 0xfffe;
-	ptr->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
+	ptr->evbit[0] = BIT(EV_KEY);
+	if (abs)
+		ptr->evbit[0] |= BIT(EV_ABS);
+	else {
+		ptr->evbit[0] |= BIT(EV_REL);
+		ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
+	}
 	for (i = BTN_LEFT; i <= BTN_TASK; i++)
 		set_bit(i, ptr->keybit);
-	ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
 	input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
 	input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
 
@@ -263,7 +273,7 @@ static void xenkbd_backend_changed(struc
 				   enum xenbus_state backend_state)
 {
 	struct xenkbd_info *info = dev->dev.driver_data;
-	int ret, val;
+	int val;
 
 	switch (backend_state) {
 	case XenbusStateInitialising:
@@ -276,16 +286,6 @@ static void xenkbd_backend_changed(struc
 
 	case XenbusStateInitWait:
 	InitWait:
-		ret = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-				   "feature-abs-pointer", "%d", &val);
-		if (ret < 0)
-			val = 0;
-		if (val) {
-			ret = xenbus_printf(XBT_NIL, info->xbdev->nodename,
-					    "request-abs-pointer", "1");
-			if (ret)
-				; /* FIXME */
-		}
 		xenbus_switch_state(dev, XenbusStateConnected);
 		break;

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Thu, 10 Mar 2011, Olaf Hering wrote:
> A virtualized display device is usually viewed with the vncviewer
> application, either by 'xm vnc domU' or with vncviewer localhost:port.
> vncviewer and the RFB protocol provides absolute coordinates to the
> virtual display. These coordinates are either passed through to a PV
> guest or converted to relative coordinates for a HVM guest.
> 
> A PV guest receives these coordinates and passes them to the kernels
> evdev driver. There it can be picked up by applications such as the
> xorg-input drivers. Using absolute coordinates avoids issues such as
> guest mouse pointer not tracking host mouse pointer due to wrong mouse
> acceleration settings in the guests X display.
> 
> Advertise either absolute or relative coordinates to the input system
> and the evdev driver, depending on what dom0 provides. The xorg-input
> driver prefers relative coordinates even if a devices provides both.
> 
> Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> ---
>  drivers/xen/fbfront/xenkbd.c |   28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> Index: linux-2.6.18-xen.hg/drivers/xen/fbfront/xenkbd.c
> ===================================================================
> --- linux-2.6.18-xen.hg.orig/drivers/xen/fbfront/xenkbd.c
> +++ linux-2.6.18-xen.hg/drivers/xen/fbfront/xenkbd.c
> @@ -104,7 +104,7 @@ static irqreturn_t input_handler(int rq,
>  int __devinit xenkbd_probe(struct xenbus_device *dev,
>  			   const struct xenbus_device_id *id)
>  {
> -	int ret, i;
> +	int ret, i, abs;
>  	struct xenkbd_info *info;
>  	struct input_dev *kbd, *ptr;
>  
> @@ -123,6 +123,11 @@ int __devinit xenkbd_probe(struct xenbus
>  	info->page->in_cons = info->page->in_prod = 0;
>  	info->page->out_cons = info->page->out_prod = 0;
>  
> +	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-abs-pointer", "%d", &abs) < 0)
> +		abs = 0;
> +	if (abs)
> +		xenbus_printf(XBT_NIL, dev->nodename, "request-abs-pointer", "1");
> +
>  	/* keyboard */
>  	kbd = input_allocate_device();
>  	if (!kbd)
> @@ -155,10 +160,15 @@ int __devinit xenkbd_probe(struct xenbus
>  	ptr->id.bustype = BUS_PCI;
>  	ptr->id.vendor = 0x5853;
>  	ptr->id.product = 0xfffe;
> -	ptr->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
> +	ptr->evbit[0] = BIT(EV_KEY);
> +	if (abs)
> +		ptr->evbit[0] |= BIT(EV_ABS);
> +	else {
> +		ptr->evbit[0] |= BIT(EV_REL);
> +		ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
> +	}
>  	for (i = BTN_LEFT; i <= BTN_TASK; i++)
>  		set_bit(i, ptr->keybit);
> -	ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
>  	input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
>  	input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
>  

does this mean that we are not getting any wheel events when we are
using absolute coordinates?

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Olaf Hering]

On Thu, Mar 10, Stefano Stabellini wrote:

> does this mean that we are not getting any wheel events when we are
> using absolute coordinates?

Do you get them now?
In my testing vncviewer does not pass them and qemu-dm does not receive
them. Or whatever gets send to qemu-dm gets converted to abs_x+abs_y, if
I read the code correctly.

Olaf

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Thu, 10 Mar 2011, Olaf Hering wrote:
> On Thu, Mar 10, Stefano Stabellini wrote:
> 
> > does this mean that we are not getting any wheel events when we are
> > using absolute coordinates?
> 
> Do you get them now?

Yes, if I disable absolute coordinates.
In order to fix the wheel problem it is sufficient to add:

+	input_set_capability(ptr, EV_REL, REL_WHEEL);

The following is a rebased version of your patch plus the small
correction on the latest 2.6.38 RC, I am going to send it upstream if
you don't mind.

---

From: Olaf Hering <olaf@aepfle.de>

A virtualized display device is usually viewed with the vncviewer
application, either by 'xm vnc domU' or with vncviewer localhost:port.
vncviewer and the RFB protocol provides absolute coordinates to the
virtual display. These coordinates are either passed through to a PV
guest or converted to relative coordinates for a HVM guest.

A PV guest receives these coordinates and passes them to the kernels
evdev driver. There it can be picked up by applications such as the
xorg-input drivers. Using absolute coordinates avoids issues such as
guest mouse pointer not tracking host mouse pointer due to wrong mouse
acceleration settings in the guests X display.

Advertise either absolute or relative coordinates to the input system
and the evdev driver, depending on what dom0 provides. The xorg-input
driver prefers relative coordinates even if a devices provides both.

Signed-off-by: Olaf Hering <olaf@aepfle.de>
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
index 7f85a86..24fc38b 100644
--- a/drivers/input/xen-kbdfront.c
+++ b/drivers/input/xen-kbdfront.c
@@ -110,7 +110,7 @@ static irqreturn_t input_handler(int rq, void *dev_id)
 static int __devinit xenkbd_probe(struct xenbus_device *dev,
 				  const struct xenbus_device_id *id)
 {
-	int ret, i;
+	int ret, i, abs;
 	struct xenkbd_info *info;
 	struct input_dev *kbd, *ptr;
 
@@ -128,6 +128,11 @@ static int __devinit xenkbd_probe(struct xenbus_device *dev,
 	if (!info->page)
 		goto error_nomem;
 
+	if (xenbus_scanf(XBT_NIL, dev->otherend, "feature-abs-pointer", "%d", &abs) < 0)
+		abs = 0;
+	if (abs)
+		xenbus_printf(XBT_NIL, dev->nodename, "request-abs-pointer", "1");
+
 	/* keyboard */
 	kbd = input_allocate_device();
 	if (!kbd)
@@ -160,10 +165,16 @@ static int __devinit xenkbd_probe(struct xenbus_device *dev,
 	ptr->id.bustype = BUS_PCI;
 	ptr->id.vendor = 0x5853;
 	ptr->id.product = 0xfffe;
-	ptr->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
+	ptr->evbit[0] = BIT(EV_KEY);
+	if (abs)
+		ptr->evbit[0] |= BIT(EV_ABS);
+	else {
+		ptr->evbit[0] |= BIT(EV_REL);
+		ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y);
+	}
+	input_set_capability(ptr, EV_REL, REL_WHEEL);
 	for (i = BTN_LEFT; i <= BTN_TASK; i++)
 		set_bit(i, ptr->keybit);
-	ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y) | BIT(REL_WHEEL);
 	input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
 	input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
 
@@ -272,7 +283,7 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
 				   enum xenbus_state backend_state)
 {
 	struct xenkbd_info *info = dev_get_drvdata(&dev->dev);
-	int ret, val;
+	int val;
 
 	switch (backend_state) {
 	case XenbusStateInitialising:
@@ -285,16 +296,6 @@ static void xenkbd_backend_changed(struct xenbus_device *dev,
 
 	case XenbusStateInitWait:
 InitWait:
-		ret = xenbus_scanf(XBT_NIL, info->xbdev->otherend,
-				   "feature-abs-pointer", "%d", &val);
-		if (ret < 0)
-			val = 0;
-		if (val) {
-			ret = xenbus_printf(XBT_NIL, info->xbdev->nodename,
-					    "request-abs-pointer", "1");
-			if (ret)
-				pr_warning("can't request abs-pointer\n");
-		}
 		xenbus_switch_state(dev, XenbusStateConnected);
 		break;

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Eamon Walsh]

We have had an issue here where the "request-abs-pointer" key is appearing in XenStore _after_ the backend has gone to the Connected state.  Qemu xenfb.c checks for this key in the "connect()" callback, causing it to miss the key and default to relative coordinates.

xen be: vkbd-0: frontend update: page-ref
xen be: vkbd-0: frontend not ready (yet)
xen be: vkbd-0: frontend update: page-gref
xen be: vkbd-0: frontend not ready (yet)
xen be: vkbd-0: frontend update: event-channel
xen be: vkbd-0: frontend not ready (yet)
xen be: vkbd-0: frontend state: Initialising -> Initialised
xen be: vkbd-0: frontend update: state
xen be: vkbd-0: bind evtchn port 53
xen be: vkbd-0: ring ref 13, remote-port 11, local-port 53
xen be: vkbd-0: backend state: InitWait -> Connected               <----
xen be: vkbd-0: backend update: state
xen be: vkbd-0: frontend update: request-abs-pointer               <----
xen be: vkbd-0: frontend state: Initialised -> Connected
xen be: vkbd-0: frontend update: state


This could be a problem specific to our display server, and it only happens sometimes. But we have been carrying the patch below in our xenfb.c.  It makes sense to respond to a direct watch on the key itself instead of looking for it at a state change.  This may be suitable for upstream to Qemu.

I also note that the check in Qemu was previously made even before the connected state.  It was changed in July, see:
http://web.archiveorange.com/archive/v/1XS1v5z9uIQLGujGBDhT


diff --git a/src/xenfb.c b/src/xenfb.c
index a822057..5aade4c 100644
--- a/src/xenfb.c
+++ b/src/xenfb.c
@@ -181,10 +181,6 @@ static int input_connect(struct XenDevice *xendev)
     struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
     int rc;
 
-    if (xenstore_read_fe_int(xendev, "request-abs-pointer",
-                             &in->abs_pointer_wanted) == -1)
-	in->abs_pointer_wanted = 0;
-
     rc = common_bind(&in->c);
     if (rc != 0)
 	return rc;
@@ -199,6 +195,17 @@ static void input_disconnect(struct XenDevice *xendev)
     xen_linpicker_input_disconnect(in);
 }
 
+static void input_frontend_changed(struct XenDevice *xendev, const char *node)
+{
+    struct XenInput *in = container_of(xendev, struct XenInput, c.xendev);
+
+    if (strcmp(node, "request-abs-pointer") == 0) {
+	if (xenstore_read_fe_int(xendev, "request-abs-pointer",
+				 &in->abs_pointer_wanted) < 0)
+	    in->abs_pointer_wanted = 0;
+    }
+}
+
 static void input_event(struct XenDevice *xendev)
 {
     struct XenInput *xenfb = container_of(xendev, struct XenInput, c.xendev);
@@ -754,6 +761,7 @@ struct XenDevOps xen_kbdmouse_ops = {
     .connect    = input_connect,
     .disconnect = input_disconnect,
     .event      = input_event,
+    .frontend_changed = input_frontend_changed,
 };
 
 struct XenDevOps xen_framebuffer_ops = {

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Thu, 10 Mar 2011, Eamon Walsh wrote:
> We have had an issue here where the "request-abs-pointer" key is appearing in XenStore _after_ the backend has gone to the Connected state.  Qemu xenfb.c checks for this key in the "connect()" callback, causing it to miss the key and default to relative coordinates.
> 
> xen be: vkbd-0: frontend update: page-ref
> xen be: vkbd-0: frontend not ready (yet)
> xen be: vkbd-0: frontend update: page-gref
> xen be: vkbd-0: frontend not ready (yet)
> xen be: vkbd-0: frontend update: event-channel
> xen be: vkbd-0: frontend not ready (yet)
> xen be: vkbd-0: frontend state: Initialising -> Initialised
> xen be: vkbd-0: frontend update: state
> xen be: vkbd-0: bind evtchn port 53
> xen be: vkbd-0: ring ref 13, remote-port 11, local-port 53
> xen be: vkbd-0: backend state: InitWait -> Connected               <----
> xen be: vkbd-0: backend update: state
> xen be: vkbd-0: frontend update: request-abs-pointer               <----
> xen be: vkbd-0: frontend state: Initialised -> Connected
> xen be: vkbd-0: frontend update: state
> 
> 
> This could be a problem specific to our display server, and it only happens sometimes. But we have been carrying the patch below in our xenfb.c.  It makes sense to respond to a direct watch on the key itself instead of looking for it at a state change.  This may be suitable for upstream to Qemu.
> 
> I also note that the check in Qemu was previously made even before the connected state.  It was changed in July, see:
> http://web.archiveorange.com/archive/v/1XS1v5z9uIQLGujGBDhT
> 

The patch looks sane.
Could you please rebase it on upstream qemu and send it to qemu-devel?

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[John Haxby]

On 11/03/11 11:58, Stefano Stabellini wrote:
> The patch looks sane.
> Could you please rebase it on upstream qemu and send it to qemu-devel?
>
>

Please hold off until we find out what happened to the earlier patch.

jch

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Fri, 11 Mar 2011, John Haxby wrote:
> On 11/03/11 11:58, Stefano Stabellini wrote:
> > The patch looks sane.
> > Could you please rebase it on upstream qemu and send it to qemu-devel?
> >
> Please hold off until we find out what happened to the earlier patch.
 
You are right, sorry I forgot about those :P
I think your two patches were a good fix, also I like the distinction
between the connected and initialise state in xenstore.
However they were lost somewhere in qemu-devel and never applied.
I'll try to prod the qemu maintainers again.

Re: [PATCH] fbfront: advertise either absolute or relative coordinates

[John Haxby]

On 10/03/11 19:58, Eamon Walsh wrote:
> We have had an issue here where the "request-abs-pointer" key is appearing in XenStore _after_ the backend has gone to the Connected state.  Qemu xenfb.c checks for this key in the "connect()" callback, causing it to miss the key and default to relative coordinates.

This was submitted to qemu-devel ages ago -- 27-Aug-2010 according to my
archives.   That was in fact the second time it was submitted (by
Stefano) but it was never picked up despite a couple of reminders :-(

This is the original thread on xen-devel:
http://markmail.org/thread/fikfzaxgaj4ay6oe

And the resend-thread in qemu-devel:
http://lists.gnu.org/archive/html/qemu-devel/2010-08/msg01515.html

I think the patch you proposed, Eamon, is the same as the one that
didn't work properly -- it's certainly a lot simpler than the two-part
patch that I wrote (and that patch has been in production for OracleVM
for a while, it seems solid).

jch