| Message ID | 1362752571-2056-3-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gerd Hoffmann <kraxel@redhat.com> writes: > Add a linked list of keyboard handlers. Added handlers will go > to the head of the list. Removed handlers will be zapped from > the list. The head of the list will be used for events. > > This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events > will be re-routed to the ps/2 kbd instead of being discarded. > > [ v2: fix cut+paste bug found my Markus ] In the future, please put patch history information below the --- line. > Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> > --- Put it here. Thanks! > hw/hid.c | 4 ++-- > hw/hid.h | 1 + > include/ui/console.h | 6 ++++-- > ui/input.c | 37 +++++++++++++++++++++++++------------ > 4 files changed, 32 insertions(+), 16 deletions(-) [...]
On 03/11/13 10:25, Markus Armbruster wrote: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> Add a linked list of keyboard handlers. Added handlers will go >> to the head of the list. Removed handlers will be zapped from >> the list. The head of the list will be used for events. >> >> This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events >> will be re-routed to the ps/2 kbd instead of being discarded. >> >> [ v2: fix cut+paste bug found my Markus ] > > In the future, please put patch history information below the --- line. > >> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >> --- > > Put it here. Thanks! How do I get it there? thanks, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > On 03/11/13 10:25, Markus Armbruster wrote: >> Gerd Hoffmann <kraxel@redhat.com> writes: >> >>> Add a linked list of keyboard handlers. Added handlers will go >>> to the head of the list. Removed handlers will be zapped from >>> the list. The head of the list will be used for events. >>> >>> This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events >>> will be re-routed to the ps/2 kbd instead of being discarded. >>> >>> [ v2: fix cut+paste bug found my Markus ] >> >> In the future, please put patch history information below the --- line. >> >>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >>> --- >> >> Put it here. Thanks! > > How do I get it there? You can just stick a '---' prior to the history in the git commit. git-format-patch and git-am will then do the right thing. Just make sure you don't push those commits. Process them via git-am like you would a patch from someone else. Regards, Anthony Liguori > > thanks, > Gerd
On 8 March 2013 14:22, Gerd Hoffmann <kraxel@redhat.com> wrote: > Add a linked list of keyboard handlers. Added handlers will go > to the head of the list. Removed handlers will be zapped from > the list. The head of the list will be used for events. > > This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events > will be re-routed to the ps/2 kbd instead of being discarded. Ah, multiple keyboard handlers. I've actually been carrying something similar in the qemu-linaro tree for ages, inherited from Nokia for n900 support: http://git.linaro.org/gitweb?p=qemu/qemu-linaro.git;a=commitdiff;h=6be1ca3d2b79118fce871aa8454336502b3a5ca4 The major difference is that where this code sends key events to the first handler only, the Nokia patch sends events to all keys, because the use case it's trying to deal with is "some keys should go to one device and some to another" (for example, most keys go to a usb keyboard but F-keys are treated as being like some of the hardware buttons on the phone, and they go to a different device in the model). Maybe we could deal with this by having kbd_put_keycode() send the event to every register keyboard until one of them says it has handled it [ie have the put_kbd callback return a handled/not-handled return]. That would still leave the issue that you need to make sure that keyboard handlers get called in the right order (perhaps a priority thing on registering?) Anyway, no objection to this patch (and I'm certainly not trying to say you should fix my problems in this patch), I just thought I'd mention this other usecase for multiple keyboards while it was in my head. thanks -- PMM
Anthony Liguori <aliguori@us.ibm.com> writes: > Gerd Hoffmann <kraxel@redhat.com> writes: > >> On 03/11/13 10:25, Markus Armbruster wrote: >>> Gerd Hoffmann <kraxel@redhat.com> writes: >>> >>>> Add a linked list of keyboard handlers. Added handlers will go >>>> to the head of the list. Removed handlers will be zapped from >>>> the list. The head of the list will be used for events. >>>> >>>> This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events >>>> will be re-routed to the ps/2 kbd instead of being discarded. >>>> >>>> [ v2: fix cut+paste bug found my Markus ] >>> >>> In the future, please put patch history information below the --- line. >>> >>>> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> >>>> --- >>> >>> Put it here. Thanks! >> >> How do I get it there? > > You can just stick a '---' prior to the history in the git commit. > git-format-patch and git-am will then do the right thing. > > Just make sure you don't push those commits. Process them via git-am > like you would a patch from someone else. Another convenient place is the cover letter.
Hi, > The major difference is that where this code sends key events > to the first handler only, the Nokia patch sends events to > all keys, because the use case it's trying to deal with is > "some keys should go to one device and some to another" > (for example, most keys go to a usb keyboard but F-keys > are treated as being like some of the hardware buttons on > the phone, and they go to a different device in the model). > Maybe we could deal with this by having kbd_put_keycode() > send the event to every register keyboard until one of > them says it has handled it [ie have the put_kbd callback > return a handled/not-handled return]. Yea, sending to all doesn't fly in general as guests with ps/2 + usb kbd would see all keys twice then ... Having a "handled" return code makes sense to me. IIRC glib has something simliar (return true == handled, return false == call next handler please). > That would still > leave the issue that you need to make sure that keyboard > handlers get called in the right order (perhaps a priority > thing on registering?) A priority field is more robust I think. > Anyway, no objection to this patch (and I'm certainly not > trying to say you should fix my problems in this patch), > I just thought I'd mention this other usecase for multiple > keyboards while it was in my head. Should be easy to add support for this on top of this series. cheers, Gerd
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> The major difference is that where this code sends key events >> to the first handler only, the Nokia patch sends events to >> all keys, because the use case it's trying to deal with is >> "some keys should go to one device and some to another" >> (for example, most keys go to a usb keyboard but F-keys >> are treated as being like some of the hardware buttons on >> the phone, and they go to a different device in the model). > >> Maybe we could deal with this by having kbd_put_keycode() >> send the event to every register keyboard until one of >> them says it has handled it [ie have the put_kbd callback >> return a handled/not-handled return]. > > Yea, sending to all doesn't fly in general as guests > with ps/2 + usb kbd would see all keys twice then ... > > Having a "handled" return code makes sense to me. IIRC glib has > something simliar (return true == handled, return false == call next > handler please). > >> That would still >> leave the issue that you need to make sure that keyboard >> handlers get called in the right order (perhaps a priority >> thing on registering?) > > A priority field is more robust I think. > >> Anyway, no objection to this patch (and I'm certainly not >> trying to say you should fix my problems in this patch), >> I just thought I'd mention this other usecase for multiple >> keyboards while it was in my head. > > Should be easy to add support for this on top of this series. I don't really have an issue with this series. It's a reasonable incremental step. But I wrote an ps2 qtest recently and had some thoughts about how we handle input and how we could do it in a better fashion. We don't really have the notion of an input device backend/frontend. There's no great way for a user to see what input devices there are or participate in which ones should have input. QOM can fill this void. We could make a QOM interface, InputInterface, loosely modelled after the Linux input layer. The interface would have methods like: struct InputInterface { uint32_t (*get_capabilities)(InputInterface *iface); const char *(get_description)(InputInterface *iface); void (*begin_batch)(InputInterface *iface); void (*event)(InputInterface *iface, uint16_t type, uint16_t code, int32_t value); void (*end_batch)(InputInterface *iface); }; Then any device capable of acting as an input device would implement this interface. qom-list can then be used to enumerate any Input devices. The VNC server (or GTK UI) could also be modelled as an object and you could select which input device received keyboard input by hooking up the properties. It also makes the devices easier to unit test because you can call the interface methods directly. An interface like this would also work for richer input mechanisms like touchscreens. Regards, Anthony Liguori > > cheers, > Gerd
diff --git a/hw/hid.c b/hw/hid.c index 89b5415..6be00ec 100644 --- a/hw/hid.c +++ b/hw/hid.c @@ -415,7 +415,7 @@ void hid_free(HIDState *hs) { switch (hs->kind) { case HID_KEYBOARD: - qemu_remove_kbd_event_handler(); + qemu_remove_kbd_event_handler(hs->kbd.eh_entry); break; case HID_MOUSE: case HID_TABLET: @@ -431,7 +431,7 @@ void hid_init(HIDState *hs, int kind, HIDEventFunc event) hs->event = event; if (hs->kind == HID_KEYBOARD) { - qemu_add_kbd_event_handler(hid_keyboard_event, hs); + hs->kbd.eh_entry = qemu_add_kbd_event_handler(hid_keyboard_event, hs); } else if (hs->kind == HID_MOUSE) { hs->ptr.eh_entry = qemu_add_mouse_event_handler(hid_pointer_event, hs, 0, "QEMU HID Mouse"); diff --git a/hw/hid.h b/hw/hid.h index 56c71ed..2567879 100644 --- a/hw/hid.h +++ b/hw/hid.h @@ -31,6 +31,7 @@ typedef struct HIDKeyboardState { uint8_t leds; uint8_t key[16]; int32_t keys; + QEMUPutKbdEntry *eh_entry; } HIDKeyboardState; struct HIDState { diff --git a/include/ui/console.h b/include/ui/console.h index ce5a550..fd39d94 100644 --- a/include/ui/console.h +++ b/include/ui/console.h @@ -28,10 +28,12 @@ typedef void QEMUPutLEDEvent(void *opaque, int ledstate); typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state); typedef struct QEMUPutMouseEntry QEMUPutMouseEntry; +typedef struct QEMUPutKbdEntry QEMUPutKbdEntry; typedef struct QEMUPutLEDEntry QEMUPutLEDEntry; -void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque); -void qemu_remove_kbd_event_handler(void); +QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, + void *opaque); +void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry); QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func, void *opaque, int absolute, const char *name); diff --git a/ui/input.c b/ui/input.c index 87a23df..57e963b 100644 --- a/ui/input.c +++ b/ui/input.c @@ -41,18 +41,25 @@ struct QEMUPutMouseEntry { QTAILQ_ENTRY(QEMUPutMouseEntry) node; }; +struct QEMUPutKbdEntry { + QEMUPutKBDEvent *put_kbd; + void *opaque; + QTAILQ_ENTRY(QEMUPutKbdEntry) next; +}; + struct QEMUPutLEDEntry { QEMUPutLEDEvent *put_led; void *opaque; QTAILQ_ENTRY(QEMUPutLEDEntry) next; }; -static QEMUPutKBDEvent *qemu_put_kbd_event; -static void *qemu_put_kbd_event_opaque; -static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers); +static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = + QTAILQ_HEAD_INITIALIZER(led_handlers); +static QTAILQ_HEAD(, QEMUPutKbdEntry) kbd_handlers = + QTAILQ_HEAD_INITIALIZER(kbd_handlers); static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers = QTAILQ_HEAD_INITIALIZER(mouse_handlers); -static NotifierList mouse_mode_notifiers = +static NotifierList mouse_mode_notifiers = NOTIFIER_LIST_INITIALIZER(mouse_mode_notifiers); static const int key_defs[] = { @@ -306,16 +313,20 @@ void qmp_send_key(KeyValueList *keys, bool has_hold_time, int64_t hold_time, muldiv64(get_ticks_per_sec(), hold_time, 1000)); } -void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque) +QEMUPutKbdEntry *qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque) { - qemu_put_kbd_event_opaque = opaque; - qemu_put_kbd_event = func; + QEMUPutKbdEntry *entry; + + entry = g_malloc0(sizeof(QEMUPutKbdEntry)); + entry->put_kbd = func; + entry->opaque = opaque; + QTAILQ_INSERT_HEAD(&kbd_handlers, entry, next); + return entry; } -void qemu_remove_kbd_event_handler(void) +void qemu_remove_kbd_event_handler(QEMUPutKbdEntry *entry) { - qemu_put_kbd_event_opaque = NULL; - qemu_put_kbd_event = NULL; + QTAILQ_REMOVE(&kbd_handlers, entry, next); } static void check_mode_change(void) @@ -399,11 +410,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) void kbd_put_keycode(int keycode) { + QEMUPutKbdEntry *entry = QTAILQ_FIRST(&kbd_handlers); + if (!runstate_is_running() && !runstate_check(RUN_STATE_SUSPENDED)) { return; } - if (qemu_put_kbd_event) { - qemu_put_kbd_event(qemu_put_kbd_event_opaque, keycode); + if (entry) { + entry->put_kbd(entry->opaque, keycode); } }
Add a linked list of keyboard handlers. Added handlers will go to the head of the list. Removed handlers will be zapped from the list. The head of the list will be used for events. This fixes the keyboard-dead-after-usb-kbd-unplug issue, key events will be re-routed to the ps/2 kbd instead of being discarded. [ v2: fix cut+paste bug found my Markus ] Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- hw/hid.c | 4 ++-- hw/hid.h | 1 + include/ui/console.h | 6 ++++-- ui/input.c | 37 +++++++++++++++++++++++++------------ 4 files changed, 32 insertions(+), 16 deletions(-)