Patchwork [v2,2/2] input: introduce keyboard handler list

login
register
mail settings
Submitter Gerd Hoffmann
Date March 8, 2013, 2:22 p.m.
Message ID <1362752571-2056-3-git-send-email-kraxel@redhat.com>
Download mbox | patch
Permalink /patch/226132/
State New
Headers show

Comments

Gerd Hoffmann - March 8, 2013, 2:22 p.m.
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(-)
Markus Armbruster - March 11, 2013, 9:25 a.m.
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(-)
[...]
Gerd Hoffmann - March 11, 2013, 9:56 a.m.
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
Anthony Liguori - March 11, 2013, 12:49 p.m.
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
Peter Maydell - March 11, 2013, 1:46 p.m.
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
Markus Armbruster - March 11, 2013, 1:52 p.m.
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.
Gerd Hoffmann - March 11, 2013, 2:51 p.m.
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
Anthony Liguori - March 11, 2013, 3:28 p.m.
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

Patch

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);
     }
 }