I got questions on this changeset:
changeset: 354:c3ff0b26f664
date: Mon Dec 10 13:52:47 2007 +0000
Decode mouse event packet dz value and passes it as a wheel event into
the input stream.
Signed-off-by: Pat Campbell <plc@novell.com>
diff -r 7232a025140f -r c3ff0b26f664 drivers/xen/fbfront/xenkbd.c
--- a/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:51:57 2007 +0000
+++ b/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:52:47 2007 +0000
@@ -64,8 +64,13 @@ static irqreturn_t input_handler(int rq,
dev = info->ptr;
switch (event->type) {
case XENKBD_TYPE_MOTION:
- input_report_rel(dev, REL_X, event->motion.rel_x);
- input_report_rel(dev, REL_Y, event->motion.rel_y);
+ if ( event->motion.rel_z == 1 || event->motion.rel_z == -1 ) {
+ input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z);
+ }
+ else {
+ input_report_rel(dev, REL_X, event->motion.rel_x);
+ input_report_rel(dev, REL_Y, event->motion.rel_y);
+ }
I don''t understand the conditional. Why is rel_z to be used *only*
when it''s 1 or -1, and why are rel_x and rel_y to be used *only* when
rel_z isn''t? That sure is a weird protocol, and it isn''t
documented
anywhere...
break;
case XENKBD_TYPE_KEY:
dev = NULL;
@@ -81,8 +86,13 @@ static irqreturn_t input_handler(int rq,
event->key.keycode);
break;
case XENKBD_TYPE_POS:
- input_report_abs(dev, ABS_X, event->pos.abs_x);
- input_report_abs(dev, ABS_Y, event->pos.abs_y);
+ if ( event->pos.abs_z == 1 || event->pos.abs_z == -1 ) {
+ input_report_rel(dev, REL_WHEEL, 0 - event->pos.abs_z);
+ }
+ else {
+ input_report_abs(dev, ABS_X, event->pos.abs_x);
+ input_report_abs(dev, ABS_Y, event->pos.abs_y);
+ }
And why isn''t this using REL_ABS?
break;
}
if (dev)
@@ -152,7 +162,7 @@ int __devinit xenkbd_probe(struct xenbus
ptr->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS);
for (i = BTN_LEFT; i <= BTN_TASK; i++)
set_bit(i, ptr->keybit);
- ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y);
+ 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);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Markus Armbruster wrote:> I got questions on this changeset: > > changeset: 354:c3ff0b26f664 > date: Mon Dec 10 13:52:47 2007 +0000 > > Decode mouse event packet dz value and passes it as a wheel event into > the input stream. > > Signed-off-by: Pat Campbell <plc@novell.com> > > diff -r 7232a025140f -r c3ff0b26f664 drivers/xen/fbfront/xenkbd.c > --- a/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:51:57 2007 +0000 > +++ b/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:52:47 2007 +0000 > @@ -64,8 +64,13 @@ static irqreturn_t input_handler(int rq, > dev = info->ptr; > switch (event->type) { > case XENKBD_TYPE_MOTION: > - input_report_rel(dev, REL_X, event->motion.rel_x); > - input_report_rel(dev, REL_Y, event->motion.rel_y); > + if ( event->motion.rel_z == 1 || event->motion.rel_z == -1 ) { > + input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); > + } > + else { > + input_report_rel(dev, REL_X, event->motion.rel_x); > + input_report_rel(dev, REL_Y, event->motion.rel_y); > + } > > I don''t understand the conditional. Why is rel_z to be used *only* > when it''s 1 or -1, and why are rel_x and rel_y to be used *only* when > rel_z isn''t? That sure is a weird protocol, and it isn''t documented > anywhere... >In my testing I never saw a case where the rel_x and rel_y were not zero or the abs_x and abs_y changed when a z event came thru. A small attempt to not flood the input stream with redundant data. Probably for clarity should have been: (same for the abs case) if (event->motion.rel_z != 0) input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); input_report_rel(dev, REL_X, event->motion.rel_x); input_report_rel(dev, REL_Y, event->motion.rel_y);> break; > case XENKBD_TYPE_KEY: > dev = NULL; > @@ -81,8 +86,13 @@ static irqreturn_t input_handler(int rq, > event->key.keycode); > break; > case XENKBD_TYPE_POS: > - input_report_abs(dev, ABS_X, event->pos.abs_x); > - input_report_abs(dev, ABS_Y, event->pos.abs_y); > + if ( event->pos.abs_z == 1 || event->pos.abs_z == -1 ) { > + input_report_rel(dev, REL_WHEEL, 0 - event->pos.abs_z); > + } > + else { > + input_report_abs(dev, ABS_X, event->pos.abs_x); > + input_report_abs(dev, ABS_Y, event->pos.abs_y); > + } > > And why isn''t this using REL_ABS? >I assume you meant to ask why not ABS_WHEEL. Xorg 6.9 evdev driver does not decode ABS_WHEEL. Xorg 7.3 decodes both REL and ABS WHEEL but ABS_WHEEL requires extra xorg.conf input options. We get greater coverage by using REL_WHEEL and reduce the need to edit xorg.conf.> break; > } > if (dev) > @@ -152,7 +162,7 @@ int __devinit xenkbd_probe(struct xenbus > ptr->evbit[0] = BIT(EV_KEY) | BIT(EV_REL) | BIT(EV_ABS); > for (i = BTN_LEFT; i <= BTN_TASK; i++) > set_bit(i, ptr->keybit); > - ptr->relbit[0] = BIT(REL_X) | BIT(REL_Y); > + 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); > >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Pat Campbell <plc@novell.com> writes:> Markus Armbruster wrote: >> I got questions on this changeset: >> >> changeset: 354:c3ff0b26f664 >> date: Mon Dec 10 13:52:47 2007 +0000 >> >> Decode mouse event packet dz value and passes it as a wheel event into >> the input stream. >> >> Signed-off-by: Pat Campbell <plc@novell.com> >> >> diff -r 7232a025140f -r c3ff0b26f664 drivers/xen/fbfront/xenkbd.c >> --- a/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:51:57 2007 +0000 >> +++ b/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:52:47 2007 +0000 >> @@ -64,8 +64,13 @@ static irqreturn_t input_handler(int rq, >> dev = info->ptr; >> switch (event->type) { >> case XENKBD_TYPE_MOTION: >> - input_report_rel(dev, REL_X, event->motion.rel_x); >> - input_report_rel(dev, REL_Y, event->motion.rel_y); >> + if ( event->motion.rel_z == 1 || event->motion.rel_z == -1 ) { >> + input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); >> + } >> + else { >> + input_report_rel(dev, REL_X, event->motion.rel_x); >> + input_report_rel(dev, REL_Y, event->motion.rel_y); >> + } >> >> I don''t understand the conditional. Why is rel_z to be used *only* >> when it''s 1 or -1, and why are rel_x and rel_y to be used *only* when >> rel_z isn''t? That sure is a weird protocol, and it isn''t documented >> anywhere... >> > In my testing I never saw a case where the rel_x and rel_y were not zero > or the abs_x and abs_y changed when a z event came thru. A small > attempt to not flood the input stream with redundant data.Assuming conditions you observed in your tests will hold elsehwere in space or time is calling for trouble :)> Probably for clarity should have been: (same for the abs case) > if (event->motion.rel_z != 0) > input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); > input_report_rel(dev, REL_X, event->motion.rel_x); > input_report_rel(dev, REL_Y, event->motion.rel_y);Why use REL_WHEEL and not REL_Z? Why suppress zero Z-axis motion, but not X/Y-axis?>> break; >> case XENKBD_TYPE_KEY: >> dev = NULL; >> @@ -81,8 +86,13 @@ static irqreturn_t input_handler(int rq, >> event->key.keycode); >> break; >> case XENKBD_TYPE_POS: >> - input_report_abs(dev, ABS_X, event->pos.abs_x); >> - input_report_abs(dev, ABS_Y, event->pos.abs_y); >> + if ( event->pos.abs_z == 1 || event->pos.abs_z == -1 ) { >> + input_report_rel(dev, REL_WHEEL, 0 - event->pos.abs_z); >> + } >> + else { >> + input_report_abs(dev, ABS_X, event->pos.abs_x); >> + input_report_abs(dev, ABS_Y, event->pos.abs_y); >> + } >> >> And why isn''t this using REL_ABS? >> > I assume you meant to ask why not ABS_WHEEL. Xorg 6.9 evdev driver doesYes.> not decode ABS_WHEEL. Xorg 7.3 decodes both REL and ABS WHEEL but > ABS_WHEEL requires extra xorg.conf input options. We get greater > coverage by using REL_WHEEL and reduce the need to edit xorg.conf.Okay, that''s fair. [...] _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster wrote:> Pat Campbell <plc@novell.com> writes: > > >> Markus Armbruster wrote: >> >>> I got questions on this changeset: >>> >>> changeset: 354:c3ff0b26f664 >>> date: Mon Dec 10 13:52:47 2007 +0000 >>> >>> Decode mouse event packet dz value and passes it as a wheel event into >>> the input stream. >>> >>> Signed-off-by: Pat Campbell <plc@novell.com> >>> >>> diff -r 7232a025140f -r c3ff0b26f664 drivers/xen/fbfront/xenkbd.c >>> --- a/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:51:57 2007 +0000 >>> +++ b/drivers/xen/fbfront/xenkbd.c Mon Dec 10 13:52:47 2007 +0000 >>> @@ -64,8 +64,13 @@ static irqreturn_t input_handler(int rq, >>> dev = info->ptr; >>> switch (event->type) { >>> case XENKBD_TYPE_MOTION: >>> - input_report_rel(dev, REL_X, event->motion.rel_x); >>> - input_report_rel(dev, REL_Y, event->motion.rel_y); >>> + if ( event->motion.rel_z == 1 || event->motion.rel_z == -1 ) { >>> + input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); >>> + } >>> + else { >>> + input_report_rel(dev, REL_X, event->motion.rel_x); >>> + input_report_rel(dev, REL_Y, event->motion.rel_y); >>> + } >>> >>> I don''t understand the conditional. Why is rel_z to be used *only* >>> when it''s 1 or -1, and why are rel_x and rel_y to be used *only* when >>> rel_z isn''t? That sure is a weird protocol, and it isn''t documented >>> anywhere... >>> >>> >> In my testing I never saw a case where the rel_x and rel_y were not zero >> or the abs_x and abs_y changed when a z event came thru. A small >> attempt to not flood the input stream with redundant data. >> > > Assuming conditions you observed in your tests will hold elsehwere in > space or time is calling for trouble :) > > >> Probably for clarity should have been: (same for the abs case) >> if (event->motion.rel_z != 0) >> input_report_rel(dev, REL_WHEEL, 0 - event->motion.rel_z); >> input_report_rel(dev, REL_X, event->motion.rel_x); >> input_report_rel(dev, REL_Y, event->motion.rel_y); >> > > Why use REL_WHEEL and not REL_Z? >Same answer as below, Xorg 6.9 does not decode REL_Z. Xorg.7.3 handles REL_Z and REL_WHEEL as the same.> Why suppress zero Z-axis motion, but not X/Y-axis? > >Hmm. Delving back into X Xorg 6.9 case REL_WHEEL: if (value > 0) PostButtonClicks(pInfo, wheel_up_button, value); if (value < 0) PostButtonClicks(pInfo, wheel_down_button, -value); break; Xorg 7.3 Just sends the value up, gets ignored later I guess I saw that sending a ''0'' for REL_WHEEL was a waste of time and decided to filtered it out. Also not really necessary if it makes the code easier to understand.>>> break; >>> case XENKBD_TYPE_KEY: >>> dev = NULL; >>> @@ -81,8 +86,13 @@ static irqreturn_t input_handler(int rq, >>> event->key.keycode); >>> break; >>> case XENKBD_TYPE_POS: >>> - input_report_abs(dev, ABS_X, event->pos.abs_x); >>> - input_report_abs(dev, ABS_Y, event->pos.abs_y); >>> + if ( event->pos.abs_z == 1 || event->pos.abs_z == -1 ) { >>> + input_report_rel(dev, REL_WHEEL, 0 - event->pos.abs_z); >>> + } >>> + else { >>> + input_report_abs(dev, ABS_X, event->pos.abs_x); >>> + input_report_abs(dev, ABS_Y, event->pos.abs_y); >>> + } >>> >>> And why isn''t this using REL_ABS? >>> >>> >> I assume you meant to ask why not ABS_WHEEL. Xorg 6.9 evdev driver does >> > > Yes. > > >> not decode ABS_WHEEL. Xorg 7.3 decodes both REL and ABS WHEEL but >> ABS_WHEEL requires extra xorg.conf input options. We get greater >> coverage by using REL_WHEEL and reduce the need to edit xorg.conf. >> > > Okay, that''s fair. > > [...] > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xensource.com > http://lists.xensource.com/xen-devel >_______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
All right, I think I understand now how this works. 1. X reports wheel motion as button 4 & 5. 2. VNC viewer duly transmits that into QEMU. 3. QEMU converts it to -1/+1 on z-axis (pointer_event() in vnc.c). 4. Your patch transmits that to the vkbd frontend. Bug: struct xenkbd_position claims abs_z is absolute, which is not true. Question: is that the protocol we want? More below. 5. The vkbd frontend stuffs the z-axis motion into the input layer as REL_WHEEL, with the sign reversed. Bug: it ignores movement other than -1/+1. A case can be made for ignoring 0. Bug: when it acts on z-axis movement, it ignores x/y movement / position. 6. X converts the wheel movement back to button 4 & 5. Weird, isn''t it? I''m not sure we want to encode wheel events as z-axis motion in the vkbd frontend/backend protocol. Wouldn''t it make more sense to encode it as buttons? _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Markus Armbruster <armbru@redhat.com> writes:> All right, I think I understand now how this works. > > 1. X reports wheel motion as button 4 & 5. > > 2. VNC viewer duly transmits that into QEMU. > > 3. QEMU converts it to -1/+1 on z-axis (pointer_event() in vnc.c). > > 4. Your patch transmits that to the vkbd frontend. > > Bug: struct xenkbd_position claims abs_z is absolute, which is not > true.Patch appended.> Question: is that the protocol we want? More below. > > 5. The vkbd frontend stuffs the z-axis motion into the input layer as > REL_WHEEL, with the sign reversed. > > Bug: it ignores movement other than -1/+1. A case can be made for > ignoring 0. > > Bug: when it acts on z-axis movement, it ignores x/y movement / > position.Fixed in changeset 439:1edfea26a2a9> 6. X converts the wheel movement back to button 4 & 5.[...] diff -r 43de9d7c3c63 drivers/xen/fbfront/xenkbd.c --- a/drivers/xen/fbfront/xenkbd.c Tue Feb 26 17:59:18 2008 +0000 +++ b/drivers/xen/fbfront/xenkbd.c Thu Feb 28 09:44:59 2008 +0100 @@ -66,7 +66,7 @@ static irqreturn_t input_handler(int rq, case XENKBD_TYPE_MOTION: if (event->motion.rel_z) input_report_rel(dev, REL_WHEEL, - 0 - event->motion.rel_z); + -event->motion.rel_z); input_report_rel(dev, REL_X, event->motion.rel_x); input_report_rel(dev, REL_Y, event->motion.rel_y); break; @@ -84,9 +84,9 @@ static irqreturn_t input_handler(int rq, event->key.keycode); break; case XENKBD_TYPE_POS: - if (event->pos.abs_z) + if (event->pos.rel_z) input_report_rel(dev, REL_WHEEL, - 0 - event->pos.abs_z); + -event->pos.rel_z); input_report_abs(dev, ABS_X, event->pos.abs_x); input_report_abs(dev, ABS_Y, event->pos.abs_y); break; diff -r 43de9d7c3c63 include/xen/interface/io/kbdif.h --- a/include/xen/interface/io/kbdif.h Tue Feb 26 17:59:18 2008 +0000 +++ b/include/xen/interface/io/kbdif.h Thu Feb 28 09:44:59 2008 +0100 @@ -65,7 +65,7 @@ struct xenkbd_position uint8_t type; /* XENKBD_TYPE_POS */ int32_t abs_x; /* absolute X position (in FB pixels) */ int32_t abs_y; /* absolute Y position (in FB pixels) */ - int32_t abs_z; /* absolute Z position (wheel) */ + int32_t rel_z; /* relative Z motion (wheel) */ }; #define XENKBD_IN_EVENT_SIZE 40 _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
On 28/2/08 09:02, "Markus Armbruster" <armbru@redhat.com> wrote:>> 4. Your patch transmits that to the vkbd frontend. >> >> Bug: struct xenkbd_position claims abs_z is absolute, which is not >> true. > > Patch appended.Also needs changes to xen/include/public/io/kbdif.h and to tools/ioemu/hw/xenfb.c. I won''t check in the Linux side without having a patch for the other half too. -- Keir _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel
Buddy patch for ioemu:
Rename struct xenkbd_position member abs_z to rel_z
Z-axis motion is relative, not absolute.
diff -r 2a8eaba24bf0 tools/ioemu/hw/xenfb.c
--- a/tools/ioemu/hw/xenfb.c Tue Feb 26 15:11:51 2008 +0000
+++ b/tools/ioemu/hw/xenfb.c Thu Feb 28 13:57:13 2008 +0100
@@ -592,7 +592,8 @@ static int xenfb_send_key(struct xenfb *
}
/* Send a relative mouse movement event */
-static int xenfb_send_motion(struct xenfb *xenfb, int rel_x, int rel_y, int
rel_z)
+static int xenfb_send_motion(struct xenfb *xenfb,
+ int rel_x, int rel_y, int rel_z)
{
union xenkbd_in_event event;
@@ -606,7 +607,8 @@ static int xenfb_send_motion(struct xenf
}
/* Send an absolute mouse movement event */
-static int xenfb_send_position(struct xenfb *xenfb, int abs_x, int abs_y, int
abs_z)
+static int xenfb_send_position(struct xenfb *xenfb,
+ int abs_x, int abs_y, int rel_z)
{
union xenkbd_in_event event;
@@ -614,7 +616,7 @@ static int xenfb_send_position(struct xe
event.type = XENKBD_TYPE_POS;
event.pos.abs_x = abs_x;
event.pos.abs_y = abs_y;
- event.pos.abs_z = abs_z;
+ event.pos.rel_z = rel_z;
return xenfb_kbd_event(xenfb, &event);
}
diff -r 2a8eaba24bf0 xen/include/public/io/kbdif.h
--- a/xen/include/public/io/kbdif.h Tue Feb 26 15:11:51 2008 +0000
+++ b/xen/include/public/io/kbdif.h Thu Feb 28 13:57:13 2008 +0100
@@ -65,7 +65,7 @@ struct xenkbd_position
uint8_t type; /* XENKBD_TYPE_POS */
int32_t abs_x; /* absolute X position (in FB pixels) */
int32_t abs_y; /* absolute Y position (in FB pixels) */
- int32_t abs_z; /* absolute Z position (wheel) */
+ int32_t rel_z; /* relative Z motion (wheel) */
};
#define XENKBD_IN_EVENT_SIZE 40
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
Samuel Thibault
2008-Feb-28 13:59 UTC
[Xen-devel] [PATCH] stubdom [Re: PVFB wheel events (z-axis)]
stubdom: Fix compilation after abs_z to rel_z change
diff -r 5f7bfdeb8748 extras/mini-os/kernel.c
--- a/extras/mini-os/kernel.c Thu Feb 28 13:44:28 2008 +0000
+++ b/extras/mini-os/kernel.c Thu Feb 28 13:55:57 2008 +0000
@@ -360,13 +360,13 @@ static void kbdfront_thread(void *p)
refresh_cursor(x, y);
break;
case XENKBD_TYPE_POS:
- printk("pos x:%d y:%d z:%d\n",
+ printk("pos x:%d y:%d dz:%d\n",
event.pos.abs_x,
event.pos.abs_y,
- event.pos.abs_z);
+ event.pos.rel_z);
x = event.pos.abs_x;
y = event.pos.abs_y;
- z = event.pos.abs_z;
+ z = event.pos.rel_z;
clip_cursor(&x, &y);
refresh_cursor(x, y);
break;
diff -r 5f7bfdeb8748 tools/ioemu/hw/xenfb.c
--- a/tools/ioemu/hw/xenfb.c Thu Feb 28 13:44:28 2008 +0000
+++ b/tools/ioemu/hw/xenfb.c Thu Feb 28 13:55:57 2008 +0000
@@ -1230,7 +1230,7 @@ static void xenfb_kbd_handler(void *opaq
int n, i;
DisplayState *s = opaque;
static int buttons;
- static int x, y, z;
+ static int x, y;
n = kbdfront_receive(kbd_dev, buf, KBD_NUM_BATCH);
for (i = 0; i < n; i++) {
@@ -1244,7 +1244,6 @@ static void xenfb_kbd_handler(void *opaq
{
int new_x = buf[i].pos.abs_x;
int new_y = buf[i].pos.abs_y;
- int new_z = buf[i].pos.abs_z;
if (new_x >= s->width)
new_x = s->width - 1;
if (new_y >= s->height)
@@ -1253,18 +1252,17 @@ static void xenfb_kbd_handler(void *opaq
kbd_mouse_event(
new_x * 0x7FFF / (s->width - 1),
new_y * 0x7FFF / (s->height - 1),
- new_z,
+ buf[i].pos.rel_z,
buttons);
} else {
kbd_mouse_event(
new_x - x,
new_y - y,
- new_z - z,
+ buf[i].pos.rel_z,
buttons);
}
x = new_x;
y = new_y;
- z = new_z;
break;
}
@@ -1289,7 +1287,7 @@ static void xenfb_kbd_handler(void *opaq
kbd_mouse_event(
x * 0x7FFF / s->width,
y * 0x7FFF / s->height,
- z,
+ 0,
buttons);
else
kbd_mouse_event(0, 0, 0, buttons);
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel