Xen devel - Mar 2011 - [PATCH] input/xen-fbfront: 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>

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;
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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;
 

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

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?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel