Skip to contentSkip to conversation

fbfront: advertise either absolute or relative coordinates

PATCH In review Olaf Hering 4 participants 9 messages Mar 10, 2011 – Mar 11, 2011

Subsystems (2)
Input drivers Xen hypervisor interface
lore
Share & tools
lore

Olaf Hering

author
#0 lore raw

PATCH 1/1 fbfront: advertise either absolute or relative coordinates

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 <[email protected]>
---
 drivers/xen/fbfront/xenkbd.c |   28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)
drivers/xen/fbfront/xenkbd.c +14 -14
Index: 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;
#1 lore raw
Quoted 55 lines · 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 <[email protected]>
---
 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
===================================================================

drivers/xen/fbfront/xenkbd.c +9 -2
@@ -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);

drivers/xen/fbfront/xenkbd.c int __devinit xenkbd_probe(struct xenbus · line 174

Quoted 10 lines
drivers/xen/fbfront/xenkbd.c +4 -1
+	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?

author
#2 lore raw
Quoted 2 lines · Stefano Stabellini

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

#3 lore raw
Quoted 6 lines · Olaf Hering
Quoted 2 lines · Stefano Stabellini

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 <[email protected]>

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 <[email protected]>
Signed-off-by Stefano Stabellini <[email protected]>
drivers/input/xen-kbdfront.c +15 -14
index 7f85a86..24fc38b 100644
@@ -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;
#4 lore raw

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

src/xenfb.c +12 -4
index a822057..5aade4c 100644
@@ -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 = {
#5 lore raw
Quoted 19 lines · 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

Quoted 5 lines

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?

#6 lore raw
Quoted 4 lines · Stefano Stabellini

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

#7 lore raw
Quoted 5 lines · John Haxby
Quoted 3 lines · Stefano Stabellini

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.

#8 lore raw
In reply to Eamon Walsh
Quoted 1 line · 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.

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