[PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

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] input/xen-fbfront: advertise either absolute or relative coordinates

[Dmitry Torokhov]

Hi Stefano,

On Fri, Mar 11, 2011 at 11:30:10AM +0000, Stefano Stabellini wrote:
> 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.

So if I am reading this correctly the original version handled changes
in backend capabilities and could switch between delivering either
relative or absolute coordinates. The new version selects the
capabilities at boot time and sticks with them. Was it really the
intended behavior?

BTW, drivers/input is intended for core input and handlers stuff with
drivers suppsed to be in drivers/input/<type>. Would you mind if I moved
this driver to drivers/input/misc?

Thanks.

-- 
Dmitry

Re: [PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Olaf Hering]

On Sat, Mar 12, Dmitry Torokhov wrote:

> Hi Stefano,
> 
> On Fri, Mar 11, 2011 at 11:30:10AM +0000, Stefano Stabellini wrote:
> > 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.
> 
> So if I am reading this correctly the original version handled changes
> in backend capabilities and could switch between delivering either
> relative or absolute coordinates. The new version selects the
> capabilities at boot time and sticks with them. Was it really the
> intended behavior?

Yes, as mentioned in the description above, using absolute coordinates in
the guests X11 is prefered because it avoids that the guest mouse
pointer gets out of sync with the host/desktop mouse pointer.
Very old Xen versions did not send absolute coordinates, but recent
versions do.

Olaf

Re: [PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Sun, 13 Mar 2011, Dmitry Torokhov wrote:
> Hi Stefano,
> 
> On Fri, Mar 11, 2011 at 11:30:10AM +0000, Stefano Stabellini wrote:
> > 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.
> 
> So if I am reading this correctly the original version handled changes
> in backend capabilities and could switch between delivering either
> relative or absolute coordinates. The new version selects the
> capabilities at boot time and sticks with them. Was it really the
> intended behavior?
 
Yes, as Olaf said, we prefer absolute coordinates so we want to make
sure absolute coordinates are the ones that are used if both are
available.


> BTW, drivers/input is intended for core input and handlers stuff with
> drivers suppsed to be in drivers/input/<type>. Would you mind if I moved
> this driver to drivers/input/misc?

I am OK with it.
Konrad, any comments on this?

Re: [PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Konrad Rzeszutek Wilk]

> > BTW, drivers/input is intended for core input and handlers stuff with
> > drivers suppsed to be in drivers/input/<type>. Would you mind if I moved
> > this driver to drivers/input/misc?
> 
> I am OK with it.
> Konrad, any comments on this?

Go for it! Dmitry, were you thinking to do that for 2.6.39 or 2.6.40?

I've two more patches for xen-fbfront (and one for xen-kbdfront) that I was thinking
could go for 2.6.39:

http://lists.colo.xensource.com/archives/html/xen-devel/2011-03/msg00383.html

oh, and they seemed to have not been posted here. I stuck them 
in

git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git #devel/fb_kb_front_use_page_gref

and was testing them. Do you want me to repost them on LKML with you
on the CC?

Re: [PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Dmitry Torokhov]

On Mon, Mar 14, 2011 at 11:03:21AM -0400, Konrad Rzeszutek Wilk wrote:
> > > BTW, drivers/input is intended for core input and handlers stuff with
> > > drivers suppsed to be in drivers/input/<type>. Would you mind if I moved
> > > this driver to drivers/input/misc?
> > 
> > I am OK with it.
> > Konrad, any comments on this?
> 
> Go for it! Dmitry, were you thinking to do that for 2.6.39 or 2.6.40?
> 

.39 I guess.

> I've two more patches for xen-fbfront (and one for xen-kbdfront) that I was thinking
> could go for 2.6.39:
> 
> http://lists.colo.xensource.com/archives/html/xen-devel/2011-03/msg00383.html
> 
> oh, and they seemed to have not been posted here. I stuck them 
> in
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/konrad/xen.git #devel/fb_kb_front_use_page_gref
> 
> and was testing them. Do you want me to repost them on LKML with you
> on the CC?
> 

Yes, please.

-- 
Dmitry

Re: [PATCH] input/xen-fbfront: advertise either absolute or relative coordinates

[Dmitry Torokhov]

On Mon, Mar 14, 2011 at 11:01:44AM +0100, Olaf Hering wrote:
> On Sat, Mar 12, Dmitry Torokhov wrote:
> 
> > Hi Stefano,
> > 
> > On Fri, Mar 11, 2011 at 11:30:10AM +0000, Stefano Stabellini wrote:
> > > 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.
> > 
> > So if I am reading this correctly the original version handled changes
> > in backend capabilities and could switch between delivering either
> > relative or absolute coordinates. The new version selects the
> > capabilities at boot time and sticks with them. Was it really the
> > intended behavior?
> 
> Yes, as mentioned in the description above, using absolute coordinates in
> the guests X11 is prefered because it avoids that the guest mouse
> pointer gets out of sync with the host/desktop mouse pointer.
> Very old Xen versions did not send absolute coordinates, but recent
> versions do.
> 

OK, as long as we have understanding that there is no concern with
migrating to a version of hypervisor that does not support absolute
coordinates reporting.

BTW, I believe the code should be rearranged a bit, like in the patch
below (this way we do not set up abs params if device works in relative
mode).

Thanks.

-- 
Dmitry

Input: xen-kbdfront - advertise either absolute or relative coordinates

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>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/xen-kbdfront.c |   44 +++++++++++++++++++++++-------------------
 1 files changed, 24 insertions(+), 20 deletions(-)


diff --git a/drivers/input/xen-kbdfront.c b/drivers/input/xen-kbdfront.c
index 7f85a86..53e6273 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)
@@ -137,11 +142,12 @@ static int __devinit xenkbd_probe(struct xenbus_device *dev,
 	kbd->id.bustype = BUS_PCI;
 	kbd->id.vendor = 0x5853;
 	kbd->id.product = 0xffff;
-	kbd->evbit[0] = BIT(EV_KEY);
+
+	__set_bit(EV_KEY, kbd->evbit);
 	for (i = KEY_ESC; i < KEY_UNKNOWN; i++)
-		set_bit(i, kbd->keybit);
+		__set_bit(i, kbd->keybit);
 	for (i = KEY_OK; i < KEY_MAX; i++)
-		set_bit(i, kbd->keybit);
+		__set_bit(i, kbd->keybit);
 
 	ret = input_register_device(kbd);
 	if (ret) {
@@ -160,12 +166,20 @@ 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);
+
+	if (abs) {
+		__set_bit(EV_ABS, ptr->evbit);
+		input_set_abs_params(ptr, ABS_X, 0, XENFB_WIDTH, 0, 0);
+		input_set_abs_params(ptr, ABS_Y, 0, XENFB_HEIGHT, 0, 0);
+	} else {
+		input_set_capability(ptr, EV_REL, REL_X);
+		input_set_capability(ptr, EV_REL, REL_Y);
+	}
+	input_set_capability(ptr, EV_REL, REL_WHEEL);
+
+	__set_bit(EV_KEY, ptr->evbit);
 	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);
+		__set_bit(i, ptr->keybit);
 
 	ret = input_register_device(ptr);
 	if (ret) {
@@ -272,7 +286,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 +299,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] input/xen-fbfront: advertise either absolute or relative coordinates

[Stefano Stabellini]

On Tue, 15 Mar 2011, Dmitry Torokhov wrote:
> > Yes, as mentioned in the description above, using absolute coordinates in
> > the guests X11 is prefered because it avoids that the guest mouse
> > pointer gets out of sync with the host/desktop mouse pointer.
> > Very old Xen versions did not send absolute coordinates, but recent
> > versions do.
> > 
> 
> OK, as long as we have understanding that there is no concern with
> migrating to a version of hypervisor that does not support absolute
> coordinates reporting.
> 

Absolute coordinates have been present in xenfb since years now, so I
don't think is a concern anymore.


> BTW, I believe the code should be rearranged a bit, like in the patch
> below (this way we do not set up abs params if device works in relative
> mode).

Looks fine, thanks!
Would you be OK with carrying this patch in your tree or do you want me
to add it to mine?