| Message ID | 1410750811-11156-1-git-send-email-michaelwr@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
Hi On Mon, Sep 15, 2014 at 5:13 AM, Michael Wright <michaelwr@android.com> wrote: > Improve gamepad support by introducing new LED constants for player > LEDs and the mappings from their corresponding HID usages introduced > in HUTTR47: http://www.usb.org/developers/hidpage/HUTRR47.pdf Why not introduce all 8 player LEDs as described in the document? You should be safe increasing LED_MAX to 0x1f. Otherwise, looks good to me. But please don't forgot to put maintainers on CC. Thanks David > Change-Id: If25d8a8e2570dfab3a35f9be5b1d03ab662f6b1c > Signed-off-by: Michael Wright <michaelwr@google.com> > --- > drivers/hid/hid-input.c | 4 ++++ > include/uapi/linux/input.h | 4 ++++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c > index 2619f7f..a1cb3bf 100644 > --- a/drivers/hid/hid-input.c > +++ b/drivers/hid/hid-input.c > @@ -633,6 +633,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel > case 0x4b: map_led (LED_MISC); break; /* "Generic Indicator" */ > case 0x19: map_led (LED_MAIL); break; /* "Message Waiting" */ > case 0x4d: map_led (LED_CHARGING); break; /* "External Power Connected" */ > + case 0x4f: map_led (LED_PLAYER_1); break; /* "Player 1" */ > + case 0x50: map_led (LED_PLAYER_2); break; /* "Player 2" */ > + case 0x51: map_led (LED_PLAYER_3); break; /* "Player 3" */ > + case 0x52: map_led (LED_PLAYER_4); break; /* "Player 4" */ > > default: goto ignore; > } > diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h > index 1874ebe..bb43f20 100644 > --- a/include/uapi/linux/input.h > +++ b/include/uapi/linux/input.h > @@ -908,6 +908,10 @@ struct input_keymap_entry { > #define LED_MISC 0x08 > #define LED_MAIL 0x09 > #define LED_CHARGING 0x0a > +#define LED_PLAYER_1 0x0b > +#define LED_PLAYER_2 0x0c > +#define LED_PLAYER_3 0x0d > +#define LED_PLAYER_4 0x0e > #define LED_MAX 0x0f > #define LED_CNT (LED_MAX+1) > > -- > 2.1.0.rc2.206.gedb03e5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Why not introduce all 8 player LEDs as described in the document? You > should be safe increasing LED_MAX to 0x1f. Wasn't sure if it was safe to bump it since it could potentially break apps until they recompile against the new API. I'll send another patchset with all of them. On Sun, Sep 14, 2014 at 10:07 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi > > On Mon, Sep 15, 2014 at 5:13 AM, Michael Wright <michaelwr@android.com> wrote: >> Improve gamepad support by introducing new LED constants for player >> LEDs and the mappings from their corresponding HID usages introduced >> in HUTTR47: http://www.usb.org/developers/hidpage/HUTRR47.pdf > > Why not introduce all 8 player LEDs as described in the document? You > should be safe increasing LED_MAX to 0x1f. > > Otherwise, looks good to me. But please don't forgot to put maintainers on CC. > > Thanks > David > >> Change-Id: If25d8a8e2570dfab3a35f9be5b1d03ab662f6b1c >> Signed-off-by: Michael Wright <michaelwr@google.com> >> --- >> drivers/hid/hid-input.c | 4 ++++ >> include/uapi/linux/input.h | 4 ++++ >> 2 files changed, 8 insertions(+) >> >> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c >> index 2619f7f..a1cb3bf 100644 >> --- a/drivers/hid/hid-input.c >> +++ b/drivers/hid/hid-input.c >> @@ -633,6 +633,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel >> case 0x4b: map_led (LED_MISC); break; /* "Generic Indicator" */ >> case 0x19: map_led (LED_MAIL); break; /* "Message Waiting" */ >> case 0x4d: map_led (LED_CHARGING); break; /* "External Power Connected" */ >> + case 0x4f: map_led (LED_PLAYER_1); break; /* "Player 1" */ >> + case 0x50: map_led (LED_PLAYER_2); break; /* "Player 2" */ >> + case 0x51: map_led (LED_PLAYER_3); break; /* "Player 3" */ >> + case 0x52: map_led (LED_PLAYER_4); break; /* "Player 4" */ >> >> default: goto ignore; >> } >> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h >> index 1874ebe..bb43f20 100644 >> --- a/include/uapi/linux/input.h >> +++ b/include/uapi/linux/input.h >> @@ -908,6 +908,10 @@ struct input_keymap_entry { >> #define LED_MISC 0x08 >> #define LED_MAIL 0x09 >> #define LED_CHARGING 0x0a >> +#define LED_PLAYER_1 0x0b >> +#define LED_PLAYER_2 0x0c >> +#define LED_PLAYER_3 0x0d >> +#define LED_PLAYER_4 0x0e >> #define LED_MAX 0x0f >> #define LED_CNT (LED_MAX+1) >> >> -- >> 2.1.0.rc2.206.gedb03e5 >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-input" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 15, 2014 at 11:03:47AM -0700, Michael Wright wrote: > > Why not introduce all 8 player LEDs as described in the document? You > > should be safe increasing LED_MAX to 0x1f. > > Wasn't sure if it was safe to bump it since it could potentially break apps > until they recompile against the new API. I'd rather we did not add any new input LED definitions but rather looked into switching to LED subsystem, at least for new LEDs. Thanks.
Hi On Mon, Sep 15, 2014 at 11:57 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Sep 15, 2014 at 11:03:47AM -0700, Michael Wright wrote: >> > Why not introduce all 8 player LEDs as described in the document? You >> > should be safe increasing LED_MAX to 0x1f. >> >> Wasn't sure if it was safe to bump it since it could potentially break apps >> until they recompile against the new API. > > I'd rather we did not add any new input LED definitions but rather > looked into switching to LED subsystem, at least for new LEDs. Why? The LED subsystem lacks any unprivileged API to control LEDs. There's no cdev to control write-access to the LED API. /sys has never been intended as unprivileged API so I'd really prefer if we add proper input codes to control those. At least I cannot see the advantage of the led subsystem over input EV_LED codes. Care to elaborate? Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi David, On Tue, Sep 16, 2014 at 12:15:08AM +0200, David Herrmann wrote: > Hi > > On Mon, Sep 15, 2014 at 11:57 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Mon, Sep 15, 2014 at 11:03:47AM -0700, Michael Wright wrote: > >> > Why not introduce all 8 player LEDs as described in the document? You > >> > should be safe increasing LED_MAX to 0x1f. > >> > >> Wasn't sure if it was safe to bump it since it could potentially break apps > >> until they recompile against the new API. > > > > I'd rather we did not add any new input LED definitions but rather > > looked into switching to LED subsystem, at least for new LEDs. > > Why? Because we have a proper subsystem that represents LEDs. > The LED subsystem lacks any unprivileged API to control LEDs. As far as I can see it is the same as for input devices. You just need to make sure the device owner can access needed attributes, such as brightness. > There's no cdev to control write-access to the LED API. /sys has never > been intended as unprivileged API chmod/chown works just fine on sysfs attributes. > so I'd really prefer if we add > proper input codes to control those. At least I cannot see the > advantage of the led subsystem over input EV_LED codes. Care to > elaborate? > LEDs should be controlled via LED subsystem. The fact that they happen to be located on a given device does not mean they belong to input subsystem. The same as LEds on network card are not part of network stack, etc. Thanks.
> LEDs should be controlled via LED subsystem. The fact that they happen > to be located on a given device does not mean they belong to input > subsystem. The same as LEds on network card are not part of network stack, > etc. I think the fundamental difference for me is that the primary input standard does have a specification for how to interact with LEDs, and right now the LED subsystem has no knowledge of HID whatsoever. Even if I did add the plumbing for HID over into the LED subsystem, is there an easy way to correlate input devices and their LEDs from userspace? On Mon, Sep 15, 2014 at 3:58 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Hi David, > > On Tue, Sep 16, 2014 at 12:15:08AM +0200, David Herrmann wrote: >> Hi >> >> On Mon, Sep 15, 2014 at 11:57 PM, Dmitry Torokhov >> <dmitry.torokhov@gmail.com> wrote: >> > On Mon, Sep 15, 2014 at 11:03:47AM -0700, Michael Wright wrote: >> >> > Why not introduce all 8 player LEDs as described in the document? You >> >> > should be safe increasing LED_MAX to 0x1f. >> >> >> >> Wasn't sure if it was safe to bump it since it could potentially break apps >> >> until they recompile against the new API. >> > >> > I'd rather we did not add any new input LED definitions but rather >> > looked into switching to LED subsystem, at least for new LEDs. >> >> Why? > > Because we have a proper subsystem that represents LEDs. > >> The LED subsystem lacks any unprivileged API to control LEDs. > > As far as I can see it is the same as for input devices. You just need > to make sure the device owner can access needed attributes, such as > brightness. > >> There's no cdev to control write-access to the LED API. /sys has never >> been intended as unprivileged API > > chmod/chown works just fine on sysfs attributes. > >> so I'd really prefer if we add >> proper input codes to control those. At least I cannot see the >> advantage of the led subsystem over input EV_LED codes. Care to >> elaborate? >> > > LEDs should be controlled via LED subsystem. The fact that they happen > to be located on a given device does not mean they belong to input > subsystem. The same as LEds on network card are not part of network stack, > etc. > > Thanks. > > -- > Dmitry
On Mon, Sep 15, 2014 at 04:41:42PM -0700, Michael Wright wrote: > > LEDs should be controlled via LED subsystem. The fact that they happen > > to be located on a given device does not mean they belong to input > > subsystem. The same as LEds on network card are not part of network stack, > > etc. > > I think the fundamental difference for me is that the primary input > standard does > have a specification for how to interact with LEDs, ... That predates LED subsystem. When we saw that it is not flexible enough LED subsystem was created. Input, for better or worse, has "sound" support as well, but I do not think anyone would want to to enhance it to support 7.1 surround output ;) > and right now the LED > subsystem has no knowledge of HID whatsoever. Then that is something that needs to be changed. > > Even if I did add the plumbing for HID over into the LED subsystem, is there an > easy way to correlate input devices and their LEDs from userspace? What does it mean "their led"? The led that happens to share the same physical package? Thanks.
On Mon, Sep 15, 2014 at 5:54 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > ... That predates LED subsystem. When we saw that it is not flexible enough LED > subsystem was created. Input, for better or worse, has "sound" support as well, > but I do not think anyone would want to to enhance it to support 7.1 surround > output ;) When I said standard I meant HID, not the input subsystem. I totally understand wanting to create a better separation of responsibilities internally. > What does it mean "their led"? The led that happens to share the same physical > package? Sure, but in this case we really care that they're part of the same physical package. We're lighting up the LED for the first player's input device, so we *need* this ability to correlate. LEDs also don't appear to have any uapi headers. Is the sysfs entry considered stable API? Are we supposed to classify what they're for (e.g. PLAYER_1 vs. PLAYER_2) by their name?
> Even if I did add the plumbing for HID over into the LED subsystem, is > there an > easy way to correlate input devices and their LEDs from userspace? You can follow the symlinks through the '/sys' filesystem. For example the following all point to the same LED. -- simon@womble:~$ ls /sys/class/input/js0/device/device/leds/0003\:054C\:0268.0001\:\:sony1 brightness device max_brightness power subsystem trigger uevent simon@womble:~$ ls /sys/class/leds/0003\:054C\:0268.0001\:\:sony1/ brightness device max_brightness power subsystem trigger uevent simon@womble:~$ ls /sys/bus/hid/devices/0003\:054C\:0268.0001/leds/0003\:054C\:0268.0001\:\:sony1/ brightness device max_brightness power subsystem trigger uevent -- One problem that exists is that many HID devices don't have an (embedded) serial number, so identification can be confused if you have more that one identical device. Simon. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 15, 2014 at 7:00 PM, <simon@mungewell.org> wrote: > One problem that exists is that many HID devices don't have an (embedded) > serial number, so identification can be confused if you have more that one > identical device. That's a pretty serious problem since this is explicitly for multiple gamepads connected to the same device, which will most likely be all the same type.
Hi On Tue, Sep 16, 2014 at 12:58 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: >> The LED subsystem lacks any unprivileged API to control LEDs. > > As far as I can see it is the same as for input devices. You just need > to make sure the device owner can access needed attributes, such as > brightness. > >> There's no cdev to control write-access to the LED API. /sys has never >> been intended as unprivileged API > > chmod/chown works just fine on sysfs attributes. Sure it is. You can even use ACLs. But that doesn't mean it provides a proper API environment. Udev maintainers have always said "sysfs is only writable by root". Reasons mostly being the following: * sysfs exists only once. Unlike char-devs, you cannot create multiple independent entry-points to the device node. Instead, there's only a single entry point which has to be shared between all users (across sessions, across containers, across user-namespaces, across pid-namespaces). * sysfs does not provide per-file contexts. Unlike char-devs, sysfs never knows the context of the user that writes into an attribute. It cannot associate private data to the open file and it cannot track the time the user releases the device again. It cannot multiplex across multiple simultaneous users. * sysfs is not "lightweight". People always say they don't want full blown char-devs because sysfs is much lighter. Don't know where that came from.. but looking at the amount of inodes and files needed for sysfs APIs, it's definitely not faster nor smaller than char-devs. Just look at IIO to see what happens if you write evdev-like APIs based on sysfs attributes "because it's lightweight". It's a mess. Sure, one solution would be to provide char-devs for LEDs. But then one char-dev per LED sounds like overkill, so my proposal is to include it in the input API like we always did. You're free to disagree on that. I'm just saying that sysfs APIs to access LEDs on GamePads (which this patch is mostly targeted at, I guess) is making user-space life miserable. It's the same discussion we had with backlights for years and we're trying eagerly to merge them into the DRM char-devs. Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
My issue with the LED subsystem is that to my knowledge, the only thing that it currently exposes is an opaque brightness value that has plenty of different meanings depending on what is actually being exposed. It would need to be overhauled to support the usecase at hand, which would be a lot more work than just applying this patch, on top of all the other problems already mentioned with finding and using the sysfs LED interface. With this change all the gamepad drivers could easily plumb through their device-specific way of exposing player IDs, and a userspace component could easily ensure consistency between logical player IDs and their LEDs. If such a thing was attempted currently with gamepads that are wired up to LED classes, the userspace component would need to have special knowledge of each driver to be able to know which special brightness value to set to get player N to light up on the controller. This isn't solvable without significantly expanding the interface to the LED subsystem. Michael, I assume such a userspace component is what you're shooting for with this work? In SteamOS the individual gamepad drivers are modified to automatically apply player IDs from the joydev device IDs, which in turn are the logical game controller IDs that are exposed by libraries such as SDL to games and as such are reflected through their UI. This was deemed too ad-hoc to be upstreamable and your patch seems like the right first step towards a cleaner, albeit slightly more complex solution. Trying to leverage the LED system seems a lot more complex, without adding any value to this solution. Thanks, - Pierre-Loup On 09/15/2014 09:06 PM, Michael Wright wrote: > On Mon, Sep 15, 2014 at 7:00 PM, <simon@mungewell.org> wrote: >> One problem that exists is that many HID devices don't have an (embedded) >> serial number, so identification can be confused if you have more that one >> identical device. > > > That's a pretty serious problem since this is explicitly for multiple > gamepads connected to the same device, which will most likely be all > the same type. > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Yes, pretty much exactly that. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Just so I can figure out the next steps, are you still convinced that new LEDs should go in the led subsystem Dmitry? Thanks,
On Tue, Sep 16, 2014 at 12:00:26PM +0200, David Herrmann wrote: > Hi > > On Tue, Sep 16, 2014 at 12:58 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > >> The LED subsystem lacks any unprivileged API to control LEDs. > > > > As far as I can see it is the same as for input devices. You just need > > to make sure the device owner can access needed attributes, such as > > brightness. > > > >> There's no cdev to control write-access to the LED API. /sys has never > >> been intended as unprivileged API > > > > chmod/chown works just fine on sysfs attributes. > > Sure it is. You can even use ACLs. But that doesn't mean it provides a > proper API environment. Udev maintainers have always said "sysfs is > only writable by root". Reasons mostly being the following: > > * sysfs exists only once. Unlike char-devs, you cannot create multiple > independent entry-points to the device node. Instead, there's only a > single entry point which has to be shared between all users (across > sessions, across containers, across user-namespaces, across > pid-namespaces). > > * sysfs does not provide per-file contexts. Unlike char-devs, sysfs > never knows the context of the user that writes into an attribute. It > cannot associate private data to the open file and it cannot track the > time the user releases the device again. It cannot multiplex across > multiple simultaneous users. > > * sysfs is not "lightweight". People always say they don't want full > blown char-devs because sysfs is much lighter. Don't know where that > came from.. but looking at the amount of inodes and files needed for > sysfs APIs, it's definitely not faster nor smaller than char-devs. I do not recall anyone saying char dev is super heavy. I was only saying that stuffing everything in input device does not make much sense and that full-blown input device structure is quite heavy in itself, without even talking about all interface drivers that can attach to it. > > Just look at IIO to see what happens if you write evdev-like APIs > based on sysfs attributes "because it's lightweight". It's a mess. > > Sure, one solution would be to provide char-devs for LEDs. Exactly. If something like chardev is better API for LEDs then do it. LED subsystem allows us to create many different "triggers". > But then > one char-dev per LED sounds like overkill, so my proposal is to Not really. I think it would be fine. > > include it in the input API like we always did. You're free to > disagree on that. I'm just saying that sysfs APIs to access LEDs on > GamePads (which this patch is mostly targeted at, I guess) is making > user-space life miserable. How can we make life not miserable, but still not merge this into input? I see the draw of saying "this is only for gamepads", "this is only for keyboards", etc... But then I see generic-purpose LEDs getting way into (we already had LED_MAIL, etc creep in), we have LEDs on network cards (which are incidentally not part of network stack), istorage enclosures have their LEDs, and all other LEds everywhere. > It's the same discussion we had with > backlights for years and we're trying eagerly to merge them into the > DRM char-devs. > So keyboard backlight is going to be controlled by display chardev? Or we are fragmenting backlights into different classes? Thanks.
On Mon, Sep 22, 2014 at 02:36:57PM -0700, Michael Wright wrote: > Just so I can figure out the next steps, are you still convinced that > new LEDs should go in the led subsystem Dmitry? At this point I would really prefer them not be part of input_dev structure. Actually there are patches to convert input core to use generic LEDs for the standard keyboard leds, sadly there is an open question of how to handle access via legacy and the new API together so I was not able to merge it [yet]. Thanks.
Hi On Tue, Sep 23, 2014 at 6:37 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: >> It's the same discussion we had with >> backlights for years and we're trying eagerly to merge them into the >> DRM char-devs. >> > > So keyboard backlight is going to be controlled by display chardev? Or > we are fragmenting backlights into different classes? The idea is to make DRM property changes propagate into a linked backlight. Applied to input-LEDs, it would mean setting LED_XYZ via input would cause an kernel-internal call into the linked led_class set_brightness(). This allows us to register an led-class per LED as you request, but at the same time allow easy access via input devices where it makes sense. Thanks David -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index 2619f7f..a1cb3bf 100644 --- a/drivers/hid/hid-input.c +++ b/drivers/hid/hid-input.c @@ -633,6 +633,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel case 0x4b: map_led (LED_MISC); break; /* "Generic Indicator" */ case 0x19: map_led (LED_MAIL); break; /* "Message Waiting" */ case 0x4d: map_led (LED_CHARGING); break; /* "External Power Connected" */ + case 0x4f: map_led (LED_PLAYER_1); break; /* "Player 1" */ + case 0x50: map_led (LED_PLAYER_2); break; /* "Player 2" */ + case 0x51: map_led (LED_PLAYER_3); break; /* "Player 3" */ + case 0x52: map_led (LED_PLAYER_4); break; /* "Player 4" */ default: goto ignore; } diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h index 1874ebe..bb43f20 100644 --- a/include/uapi/linux/input.h +++ b/include/uapi/linux/input.h @@ -908,6 +908,10 @@ struct input_keymap_entry { #define LED_MISC 0x08 #define LED_MAIL 0x09 #define LED_CHARGING 0x0a +#define LED_PLAYER_1 0x0b +#define LED_PLAYER_2 0x0c +#define LED_PLAYER_3 0x0d +#define LED_PLAYER_4 0x0e #define LED_MAX 0x0f #define LED_CNT (LED_MAX+1)
Improve gamepad support by introducing new LED constants for player LEDs and the mappings from their corresponding HID usages introduced in HUTTR47: http://www.usb.org/developers/hidpage/HUTRR47.pdf Change-Id: If25d8a8e2570dfab3a35f9be5b1d03ab662f6b1c Signed-off-by: Michael Wright <michaelwr@google.com> --- drivers/hid/hid-input.c | 4 ++++ include/uapi/linux/input.h | 4 ++++ 2 files changed, 8 insertions(+)