| Message ID | 20211117144807.v2.1.I09b516eff75ead160a6582dd557e7e7e900c9e8e@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: Support input-boosted panel self-refresh exit | expand |
On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > A variety of applications have found it useful to listen to > user-initiated input events to make decisions within a DRM driver, given > that input events are often the first sign that we're going to start > doing latency-sensitive activities: > > * Panel self-refresh: software-directed self-refresh (e.g., with > Rockchip eDP) is especially latency sensitive. In some cases, it can > take 10s of milliseconds for a panel to exit self-refresh, which can > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > with an input_handler boost, that preemptively exits self-refresh > whenever there is input activity. > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > render new frames immediately after user activity. Powering up the > GPU can take enough time that it is worthwhile to start this process > as soon as there is input activity. Many Chrome OS systems also ship > with an input_handler boost that powers up the GPU. > > This patch provides a small helper library that abstracts some of the > input-subsystem details around picking which devices to listen to, and > some other boilerplate. This will be used in the next patch to implement > the first bullet: preemptive exit for panel self-refresh. > > Bits of this are adapted from code the Android and/or Chrome OS kernels > have been carrying for a while. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- > > Changes in v2: > - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER > - Remove void*; users should use container_of() > - Document the callback context > > drivers/gpu/drm/Kconfig | 6 ++ > drivers/gpu/drm/Makefile | 2 + > drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ > include/drm/drm_input_helper.h | 41 +++++++++ Please add documentation for this and include it under Documentation/gpu/drm-kms-helpers.rst in a suitable place. Standards for core code should be overview DOC: with references to key functions/structs, and all driver visible structs, functions (static inline in header or exported) fully documented. > 4 files changed, 192 insertions(+) > create mode 100644 drivers/gpu/drm/drm_input_helper.c > create mode 100644 include/drm/drm_input_helper.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index fb144617055b..381476b10a9d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > If in doubt, say "N". > > +config DRM_INPUT_HELPER > + def_bool y > + depends on DRM_KMS_HELPER > + depends on INPUT Uh please no configs for each thing, it just makes everything more complex. Do we _really_ need this? > + > config DRM_KMS_HELPER > tristate > depends on DRM > + select DRM_INPUT_HELPER if INPUT > help > CRTC helpers for KMS drivers. > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 1c41156deb5f..9a6494aa45e6 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > drm_atomic_state_helper.o drm_damage_helper.o \ > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > + > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > new file mode 100644 > index 000000000000..470f90865c7c > --- /dev/null > +++ b/drivers/gpu/drm/drm_input_helper.c > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Google, Inc. > + */ > +#include <linux/input.h> > +#include <linux/slab.h> > + > +#include <drm/drm_device.h> > +#include <drm/drm_input_helper.h> > + > +/** > + * DOC: overview > + * > + * This helper library provides a thin wrapper around input handles, so that > + * DRM drivers can easily perform domain-specific actions in response to user > + * activity. e.g., if someone is moving a mouse, we're likely to want to > + * display something soon, and we should exit panel self-refresh. > + */ > + > +static void drm_input_event(struct input_handle *handle, unsigned int type, > + unsigned int code, int value) > +{ > + struct drm_input_handler *handler = handle->handler->private; > + > + handler->callback(handler); > +} > + > +static int drm_input_connect(struct input_handler *handler, > + struct input_dev *dev, > + const struct input_device_id *id) > +{ > + struct input_handle *handle; > + int error; > + > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > + if (!handle) > + return -ENOMEM; > + > + handle->dev = dev; > + handle->handler = handler; > + handle->name = "drm-input-helper"; > + > + error = input_register_handle(handle); > + if (error) > + goto err2; > + > + error = input_open_device(handle); > + if (error) > + goto err1; > + > + return 0; > + > +err1: > + input_unregister_handle(handle); > +err2: > + kfree(handle); > + return error; > +} > + > +static void drm_input_disconnect(struct input_handle *handle) > +{ > + input_close_device(handle); > + input_unregister_handle(handle); > + kfree(handle); > +} > + > +static const struct input_device_id drm_input_ids[] = { > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_ABSBIT, > + .evbit = { BIT_MASK(EV_ABS) }, > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > + BIT_MASK(ABS_MT_POSITION_X) | > + BIT_MASK(ABS_MT_POSITION_Y) }, > + }, /* multi-touch touchscreen */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_ABS) }, > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > + > + }, /* stylus or joystick device */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > + }, /* pointer (e.g. trackpad, mouse) */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > + }, /* keyboard */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_KEYBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > + }, /* joysticks not caught by ABS_X above */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_KEYBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > + }, /* gamepad */ > + { }, > +}; > + > +int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler) > +{ > + int ret; > + > + if (!handler->callback) > + return -EINVAL; > + > + handler->handler.event = drm_input_event; > + handler->handler.connect = drm_input_connect; > + handler->handler.disconnect = drm_input_disconnect; > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > + dev_name(dev->dev)); > + if (!handler->handler.name) > + return -ENOMEM; > + > + handler->handler.id_table = drm_input_ids; > + handler->handler.private = handler; > + > + ret = input_register_handler(&handler->handler); > + if (ret) > + goto err; > + > + return 0; > + > +err: > + kfree(handler->handler.name); > + return ret; > +} > +EXPORT_SYMBOL(drm_input_handle_register); > + > +void drm_input_handle_unregister(struct drm_input_handler *handler) > +{ > + input_unregister_handler(&handler->handler); > + kfree(handler->handler.name); > +} > +EXPORT_SYMBOL(drm_input_handle_unregister); > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h > new file mode 100644 > index 000000000000..7904f397b934 > --- /dev/null > +++ b/include/drm/drm_input_helper.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2021 Google, Inc. > + */ > +#ifndef __DRM_INPUT_HELPER_H__ > +#define __DRM_INPUT_HELPER_H__ > + > +#include <linux/input.h> > + > +struct drm_device; > + > +struct drm_input_handler { > + /* > + * Callback to call for input activity. Will be called in an atomic > + * context. How atomic? Like hardirq, and nasty spinlocks held? > + */ > + void (*callback)(struct drm_input_handler *handler); > + > + struct input_handler handler; > +}; > + > +#if defined(CONFIG_DRM_INPUT_HELPER) > + > +int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler); > +void drm_input_handle_unregister(struct drm_input_handler *handler); > + > +#else /* !CONFIG_DRM_INPUT_HELPER */ > + > +static inline int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler) > +{ > + return 0; > +} I guess the reason behind the helper is that you also want to use this in drivers or maybe drm/sched? Anyway I think it looks all reasonable. Definitely need an ack from input people that the event list you have is a good choice, I have no idea what that all does. Maybe also document that part a bit more. -Daniel > + > +static inline void > +drm_input_handle_unregister(struct drm_input_handler *handler) {} > + > +#endif /* CONFIG_DRM_INPUT_HELPER */ > + > +#endif /* __DRM_INPUT_HELPER_H__ */ > -- > 2.34.0.rc1.387.gb447b232ab-goog >
On Wed, 17 Nov 2021 14:48:40 -0800 Brian Norris <briannorris@chromium.org> wrote: > A variety of applications have found it useful to listen to > user-initiated input events to make decisions within a DRM driver, given > that input events are often the first sign that we're going to start > doing latency-sensitive activities: > > * Panel self-refresh: software-directed self-refresh (e.g., with > Rockchip eDP) is especially latency sensitive. In some cases, it can > take 10s of milliseconds for a panel to exit self-refresh, which can > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > with an input_handler boost, that preemptively exits self-refresh > whenever there is input activity. > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > render new frames immediately after user activity. Powering up the > GPU can take enough time that it is worthwhile to start this process > as soon as there is input activity. Many Chrome OS systems also ship > with an input_handler boost that powers up the GPU. > > This patch provides a small helper library that abstracts some of the > input-subsystem details around picking which devices to listen to, and > some other boilerplate. This will be used in the next patch to implement > the first bullet: preemptive exit for panel self-refresh. > > Bits of this are adapted from code the Android and/or Chrome OS kernels > have been carrying for a while. > > Signed-off-by: Brian Norris <briannorris@chromium.org> > --- Thanks Simon for the CC. Hi Brian, while this feature in general makes sense and sounds good, to start warming up display hardware early when something might start to happen, this particular proposal has many problems from UAPI perspective (as it has none). Comments below. Btw. if PSR is that slow to wake up from, how much do you actually gain from this input event watching? I would imagine the improvement to not be noticeable. I think some numbers about how much this feature helps would be really good, even if they are quite specific use cases. You also need to identify the userspace components, because I think different display servers are very different in their reaction speed. If KMS gets a pageflip or modeset in no time after an input event, then what's the gain. OTOH, if the display server is locking on to vblank, there might be a delay worth avoiding. But then, is it worth short-circuiting the wake-up in kernel vs. adding a new ioctl that userspace could hit to start the warming up process? > > Changes in v2: > - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER > - Remove void*; users should use container_of() > - Document the callback context > > drivers/gpu/drm/Kconfig | 6 ++ > drivers/gpu/drm/Makefile | 2 + > drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ > include/drm/drm_input_helper.h | 41 +++++++++ > 4 files changed, 192 insertions(+) > create mode 100644 drivers/gpu/drm/drm_input_helper.c > create mode 100644 include/drm/drm_input_helper.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index fb144617055b..381476b10a9d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > If in doubt, say "N". > > +config DRM_INPUT_HELPER > + def_bool y > + depends on DRM_KMS_HELPER > + depends on INPUT > + > config DRM_KMS_HELPER > tristate > depends on DRM > + select DRM_INPUT_HELPER if INPUT > help > CRTC helpers for KMS drivers. > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 1c41156deb5f..9a6494aa45e6 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > drm_atomic_state_helper.o drm_damage_helper.o \ > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > + > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > new file mode 100644 > index 000000000000..470f90865c7c > --- /dev/null > +++ b/drivers/gpu/drm/drm_input_helper.c > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Google, Inc. > + */ > +#include <linux/input.h> > +#include <linux/slab.h> > + > +#include <drm/drm_device.h> > +#include <drm/drm_input_helper.h> > + > +/** > + * DOC: overview > + * > + * This helper library provides a thin wrapper around input handles, so that > + * DRM drivers can easily perform domain-specific actions in response to user > + * activity. e.g., if someone is moving a mouse, we're likely to want to > + * display something soon, and we should exit panel self-refresh. > + */ > + > +static void drm_input_event(struct input_handle *handle, unsigned int type, > + unsigned int code, int value) > +{ > + struct drm_input_handler *handler = handle->handler->private; > + > + handler->callback(handler); > +} > + > +static int drm_input_connect(struct input_handler *handler, > + struct input_dev *dev, > + const struct input_device_id *id) > +{ > + struct input_handle *handle; > + int error; > + > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > + if (!handle) > + return -ENOMEM; > + > + handle->dev = dev; > + handle->handler = handler; > + handle->name = "drm-input-helper"; > + > + error = input_register_handle(handle); > + if (error) > + goto err2; > + > + error = input_open_device(handle); Does this literally open the input device, just like when userspace opens the input device? How do you know userspace is using this input device at all? If userspace is not using the input device, then DRM should not be opening it either, as it must have no effect on anything. If you open an input device that userspace does not use, you also cause a power consumption regression, because now the input device itself is active and possibly flooding the kernel with events (e.g. an accelerometer). > + if (error) > + goto err1; > + > + return 0; > + > +err1: > + input_unregister_handle(handle); > +err2: > + kfree(handle); > + return error; > +} > + > +static void drm_input_disconnect(struct input_handle *handle) > +{ > + input_close_device(handle); > + input_unregister_handle(handle); > + kfree(handle); > +} > + > +static const struct input_device_id drm_input_ids[] = { > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_ABSBIT, > + .evbit = { BIT_MASK(EV_ABS) }, > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > + BIT_MASK(ABS_MT_POSITION_X) | > + BIT_MASK(ABS_MT_POSITION_Y) }, > + }, /* multi-touch touchscreen */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_ABS) }, > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > + > + }, /* stylus or joystick device */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > + }, /* pointer (e.g. trackpad, mouse) */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > + }, /* keyboard */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_KEYBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > + }, /* joysticks not caught by ABS_X above */ > + { > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > + INPUT_DEVICE_ID_MATCH_KEYBIT, > + .evbit = { BIT_MASK(EV_KEY) }, > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > + }, /* gamepad */ I don't think this hardcoded policy belongs in the kernel, nor even works. I believe classifying input devices is not that simple. Spearheading that is libinput which relies on udev tagging the devices with their types, and that is done based on a hwdb maintained by I think the systemd project. Or maybe libinput has its own db nowadays as well, I'm not sure. Also, joysticks and gamepads are something display servers generally do not open. An application might open some while it's running, but not all the time. Joysticks could be very chatty while opened, game controllers might have accelerometers, etc. > + { }, > +}; > + > +int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler) > +{ > + int ret; > + > + if (!handler->callback) > + return -EINVAL; > + > + handler->handler.event = drm_input_event; > + handler->handler.connect = drm_input_connect; > + handler->handler.disconnect = drm_input_disconnect; > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > + dev_name(dev->dev)); > + if (!handler->handler.name) > + return -ENOMEM; > + > + handler->handler.id_table = drm_input_ids; > + handler->handler.private = handler; > + > + ret = input_register_handler(&handler->handler); Yet another problem here is that this completely ignores the concept of physical seats. Of course it does so, because seats are a pure userspace concept. The kernel VT console already has problems because the kernel has no concept of seats, which means that if there is a second seat defined and a desktop running on it, while the first seat is in the normal VT text mode, then everything typed in the desktop will be delivered to the VT shell as well! (This has a possible workaround in userspace [1], by opening the evdev input devices in some kind of exclusive mode - which is not common practise AFAIK.) Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler stop getting events? So, separate physical seats are a thing in userspace. A seat has at least one DRM device for output, and any number of input devices. A user session runs on a seat, and can access the devices on that seat only. This means you can run multiple independent physical seats on the same machine, provided each one has its own gfx card. The seats are configured with udev rules adding ID_SEAT property to the devices. How will you keep those seats independent, so that activity on one seat does not cause all the other seats to ramp up their gfx cards? Thanks, pq [1] https://gitlab.freedesktop.org/wayland/weston/-/issues/434 > + if (ret) > + goto err; > + > + return 0; > + > +err: > + kfree(handler->handler.name); > + return ret; > +} > +EXPORT_SYMBOL(drm_input_handle_register); > + > +void drm_input_handle_unregister(struct drm_input_handler *handler) > +{ > + input_unregister_handler(&handler->handler); > + kfree(handler->handler.name); > +} > +EXPORT_SYMBOL(drm_input_handle_unregister); > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h > new file mode 100644 > index 000000000000..7904f397b934 > --- /dev/null > +++ b/include/drm/drm_input_helper.h > @@ -0,0 +1,41 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2021 Google, Inc. > + */ > +#ifndef __DRM_INPUT_HELPER_H__ > +#define __DRM_INPUT_HELPER_H__ > + > +#include <linux/input.h> > + > +struct drm_device; > + > +struct drm_input_handler { > + /* > + * Callback to call for input activity. Will be called in an atomic > + * context. > + */ > + void (*callback)(struct drm_input_handler *handler); > + > + struct input_handler handler; > +}; > + > +#if defined(CONFIG_DRM_INPUT_HELPER) > + > +int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler); > +void drm_input_handle_unregister(struct drm_input_handler *handler); > + > +#else /* !CONFIG_DRM_INPUT_HELPER */ > + > +static inline int drm_input_handle_register(struct drm_device *dev, > + struct drm_input_handler *handler) > +{ > + return 0; > +} > + > +static inline void > +drm_input_handle_unregister(struct drm_input_handler *handler) {} > + > +#endif /* CONFIG_DRM_INPUT_HELPER */ > + > +#endif /* __DRM_INPUT_HELPER_H__ */
Hi Daniel, Thanks for the review. Lots to address elsewhere, but I can respond here first: On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > If in doubt, say "N". > > > > +config DRM_INPUT_HELPER > > + def_bool y > > + depends on DRM_KMS_HELPER > > + depends on INPUT > > Uh please no configs for each thing, it just makes everything more > complex. Do we _really_ need this? First, it's not a configurable option (a user will never see this nor have to answer Y/N to it); it only serves as an intermediary to express the CONFIG_INPUT dependency (which is necessary) without making DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run display stacks without the input subsystem.) The closest alternative I can think of with fewer Kconfig symbols is to just use CONFIG_INPUT directly in the code, to decide whether to provide the helpers or else just stub them out. But that has a problem of not properly expressing the =m vs. =y necessity: if, for example, CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker issues. In short, yes, I think we really need this. But I'm not a Kbuild expert. > > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h > > new file mode 100644 > > index 000000000000..7904f397b934 > > --- /dev/null > > +++ b/include/drm/drm_input_helper.h > > @@ -0,0 +1,41 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2021 Google, Inc. > > + */ > > +#ifndef __DRM_INPUT_HELPER_H__ > > +#define __DRM_INPUT_HELPER_H__ > > + > > +#include <linux/input.h> > > + > > +struct drm_device; > > + > > +struct drm_input_handler { > > + /* > > + * Callback to call for input activity. Will be called in an atomic > > + * context. > > How atomic? Like hardirq, and nasty spinlocks held? Maybe I should have just cribbed off the <linux/input.h> doc: * @event: event handler. This method is being called by input core with * interrupts disabled and dev->event_lock spinlock held and so * it may not sleep I probably don't want to propagate the subsystem details about which locks, but I guess I can be specific about "interrupts disabled" and "don't sleep". > > + */ > > + void (*callback)(struct drm_input_handler *handler); > > + > > + struct input_handler handler; > > +}; > > + > > +#if defined(CONFIG_DRM_INPUT_HELPER) > > + > > +int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler); > > +void drm_input_handle_unregister(struct drm_input_handler *handler); > > + > > +#else /* !CONFIG_DRM_INPUT_HELPER */ > > + > > +static inline int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler) > > +{ > > + return 0; > > +} > > I guess the reason behind the helper is that you also want to use this in > drivers or maybe drm/sched? I think my reasoning is heavily described in both the cover letter and the commit message. If that's not clear, can you point out which part? I'd gladly improve it :) But specifically, see the 2nd bullet from the commit message, which I've re-quoted down here: > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > render new frames immediately after user activity. Powering up the > > GPU can take enough time that it is worthwhile to start this process > > as soon as there is input activity. Many Chrome OS systems also ship > > with an input_handler boost that powers up the GPU. Rob Clark has patches to drm/msm to boost GPU power-up via a similar helper. > Anyway I think it looks all reasonable. Definitely need an ack from input > people I realized I failed to carry Dmitry's Ack from version 1 [1]. If this has a v3 in similar form, I'll carry it there. > that the event list you have is a good choice, I have no idea what > that all does. Maybe also document that part a bit more. I'm admittedly not an expert there, and this is actually one reason why we hoped to make this a library (that nobody wants to keep figuring out whether all those flags, etc., are really doing the right thing), but there are comments about what each entry is _trying_ to do. Are you suggesting more, as in, why "BTN_LEFT + EV_KEY" means "pointer"? Or why we match certain devices (because they represent likely user activity that will affect the display pipeline)? Or both? Anyway, I'll give it a shot, if we keep this. Brian [1] https://lore.kernel.org/all/YYW6FwSeNMK25ENm@google.com/
On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Wed, 17 Nov 2021 14:48:40 -0800 > Brian Norris <briannorris@chromium.org> wrote: > > > A variety of applications have found it useful to listen to > > user-initiated input events to make decisions within a DRM driver, given > > that input events are often the first sign that we're going to start > > doing latency-sensitive activities: > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > take 10s of milliseconds for a panel to exit self-refresh, which can > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > with an input_handler boost, that preemptively exits self-refresh > > whenever there is input activity. > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > render new frames immediately after user activity. Powering up the > > GPU can take enough time that it is worthwhile to start this process > > as soon as there is input activity. Many Chrome OS systems also ship > > with an input_handler boost that powers up the GPU. > > > > This patch provides a small helper library that abstracts some of the > > input-subsystem details around picking which devices to listen to, and > > some other boilerplate. This will be used in the next patch to implement > > the first bullet: preemptive exit for panel self-refresh. > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > have been carrying for a while. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > Thanks Simon for the CC. > > Hi Brian, > > while this feature in general makes sense and sounds good, to start > warming up display hardware early when something might start to happen, > this particular proposal has many problems from UAPI perspective (as it > has none). Comments below. > > Btw. if PSR is that slow to wake up from, how much do you actually gain > from this input event watching? I would imagine the improvement to not > be noticeable. > > I think some numbers about how much this feature helps would be really > good, even if they are quite specific use cases. You also need to > identify the userspace components, because I think different display > servers are very different in their reaction speed. > > If KMS gets a pageflip or modeset in no time after an input event, then > what's the gain. OTOH, if the display server is locking on to vblank, > there might be a delay worth avoiding. But then, is it worth > short-circuiting the wake-up in kernel vs. adding a new ioctl that > userspace could hit to start the warming up process? In my measurements, it takes userspace a frame or two to respond and get to the point of starting to build cmdstream (before eventually doing atomic/pageflip ioctl).. possibly longer if you don't also have a similar boost mechanism to spool up cpufreq But the important thing, IMO, is that atomic/pageflip ioctl is the cumulation of a long sequence of events.. input-boost is letting whatever it may be (PSR exit, GPU resume, etc) happen in parallel with that long sequence. BR, -R > > > > > Changes in v2: > > - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER > > - Remove void*; users should use container_of() > > - Document the callback context > > > > drivers/gpu/drm/Kconfig | 6 ++ > > drivers/gpu/drm/Makefile | 2 + > > drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ > > include/drm/drm_input_helper.h | 41 +++++++++ > > 4 files changed, 192 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_input_helper.c > > create mode 100644 include/drm/drm_input_helper.h > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index fb144617055b..381476b10a9d 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > If in doubt, say "N". > > > > +config DRM_INPUT_HELPER > > + def_bool y > > + depends on DRM_KMS_HELPER > > + depends on INPUT > > + > > config DRM_KMS_HELPER > > tristate > > depends on DRM > > + select DRM_INPUT_HELPER if INPUT > > help > > CRTC helpers for KMS drivers. > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 1c41156deb5f..9a6494aa45e6 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > > drm_atomic_state_helper.o drm_damage_helper.o \ > > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > > + > > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > > new file mode 100644 > > index 000000000000..470f90865c7c > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_input_helper.c > > @@ -0,0 +1,143 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Copyright (C) 2021 Google, Inc. > > + */ > > +#include <linux/input.h> > > +#include <linux/slab.h> > > + > > +#include <drm/drm_device.h> > > +#include <drm/drm_input_helper.h> > > + > > +/** > > + * DOC: overview > > + * > > + * This helper library provides a thin wrapper around input handles, so that > > + * DRM drivers can easily perform domain-specific actions in response to user > > + * activity. e.g., if someone is moving a mouse, we're likely to want to > > + * display something soon, and we should exit panel self-refresh. > > + */ > > + > > +static void drm_input_event(struct input_handle *handle, unsigned int type, > > + unsigned int code, int value) > > +{ > > + struct drm_input_handler *handler = handle->handler->private; > > + > > + handler->callback(handler); > > +} > > + > > +static int drm_input_connect(struct input_handler *handler, > > + struct input_dev *dev, > > + const struct input_device_id *id) > > +{ > > + struct input_handle *handle; > > + int error; > > + > > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > > + if (!handle) > > + return -ENOMEM; > > + > > + handle->dev = dev; > > + handle->handler = handler; > > + handle->name = "drm-input-helper"; > > + > > + error = input_register_handle(handle); > > + if (error) > > + goto err2; > > + > > + error = input_open_device(handle); > > Does this literally open the input device, just like when userspace > opens the input device? > > How do you know userspace is using this input device at all? If > userspace is not using the input device, then DRM should not be opening > it either, as it must have no effect on anything. > > If you open an input device that userspace does not use, you also cause > a power consumption regression, because now the input device itself is > active and possibly flooding the kernel with events (e.g. an > accelerometer). > > > + if (error) > > + goto err1; > > + > > + return 0; > > + > > +err1: > > + input_unregister_handle(handle); > > +err2: > > + kfree(handle); > > + return error; > > +} > > + > > +static void drm_input_disconnect(struct input_handle *handle) > > +{ > > + input_close_device(handle); > > + input_unregister_handle(handle); > > + kfree(handle); > > +} > > + > > +static const struct input_device_id drm_input_ids[] = { > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_ABSBIT, > > + .evbit = { BIT_MASK(EV_ABS) }, > > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > > + BIT_MASK(ABS_MT_POSITION_X) | > > + BIT_MASK(ABS_MT_POSITION_Y) }, > > + }, /* multi-touch touchscreen */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_ABS) }, > > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > > + > > + }, /* stylus or joystick device */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > > + }, /* pointer (e.g. trackpad, mouse) */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > > + }, /* keyboard */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > > + }, /* joysticks not caught by ABS_X above */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > > + }, /* gamepad */ > > I don't think this hardcoded policy belongs in the kernel, nor even > works. > > I believe classifying input devices is not that simple. Spearheading > that is libinput which relies on udev tagging the devices with their > types, and that is done based on a hwdb maintained by I think the > systemd project. Or maybe libinput has its own db nowadays as well, I'm > not sure. > > Also, joysticks and gamepads are something display servers generally do > not open. An application might open some while it's running, but not > all the time. Joysticks could be very chatty while opened, game > controllers might have accelerometers, etc. > > > + { }, > > +}; > > + > > +int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler) > > +{ > > + int ret; > > + > > + if (!handler->callback) > > + return -EINVAL; > > + > > + handler->handler.event = drm_input_event; > > + handler->handler.connect = drm_input_connect; > > + handler->handler.disconnect = drm_input_disconnect; > > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > > + dev_name(dev->dev)); > > + if (!handler->handler.name) > > + return -ENOMEM; > > + > > + handler->handler.id_table = drm_input_ids; > > + handler->handler.private = handler; > > + > > + ret = input_register_handler(&handler->handler); > > Yet another problem here is that this completely ignores the concept of > physical seats. Of course it does so, because seats are a pure > userspace concept. > > The kernel VT console already has problems because the kernel has no > concept of seats, which means that if there is a second seat defined and > a desktop running on it, while the first seat is in the normal VT text > mode, then everything typed in the desktop will be delivered to the VT > shell as well! (This has a possible workaround in userspace [1], by opening > the evdev input devices in some kind of exclusive mode - which is not > common practise AFAIK.) > > Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler > stop getting events? > > So, separate physical seats are a thing in userspace. A seat has at > least one DRM device for output, and any number of input devices. A > user session runs on a seat, and can access the devices on that seat > only. This means you can run multiple independent physical seats on the > same machine, provided each one has its own gfx card. The seats are > configured with udev rules adding ID_SEAT property to the devices. > > How will you keep those seats independent, so that activity on one seat > does not cause all the other seats to ramp up their gfx cards? > > > Thanks, > pq > > > [1] https://gitlab.freedesktop.org/wayland/weston/-/issues/434 > > > + if (ret) > > + goto err; > > + > > + return 0; > > + > > +err: > > + kfree(handler->handler.name); > > + return ret; > > +} > > +EXPORT_SYMBOL(drm_input_handle_register); > > + > > +void drm_input_handle_unregister(struct drm_input_handler *handler) > > +{ > > + input_unregister_handler(&handler->handler); > > + kfree(handler->handler.name); > > +} > > +EXPORT_SYMBOL(drm_input_handle_unregister); > > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h > > new file mode 100644 > > index 000000000000..7904f397b934 > > --- /dev/null > > +++ b/include/drm/drm_input_helper.h > > @@ -0,0 +1,41 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Copyright (C) 2021 Google, Inc. > > + */ > > +#ifndef __DRM_INPUT_HELPER_H__ > > +#define __DRM_INPUT_HELPER_H__ > > + > > +#include <linux/input.h> > > + > > +struct drm_device; > > + > > +struct drm_input_handler { > > + /* > > + * Callback to call for input activity. Will be called in an atomic > > + * context. > > + */ > > + void (*callback)(struct drm_input_handler *handler); > > + > > + struct input_handler handler; > > +}; > > + > > +#if defined(CONFIG_DRM_INPUT_HELPER) > > + > > +int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler); > > +void drm_input_handle_unregister(struct drm_input_handler *handler); > > + > > +#else /* !CONFIG_DRM_INPUT_HELPER */ > > + > > +static inline int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler) > > +{ > > + return 0; > > +} > > + > > +static inline void > > +drm_input_handle_unregister(struct drm_input_handler *handler) {} > > + > > +#endif /* CONFIG_DRM_INPUT_HELPER */ > > + > > +#endif /* __DRM_INPUT_HELPER_H__ */ >
Hi Pekka, Thanks for the thoughts and review. I've tried to respond below: On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > On Wed, 17 Nov 2021 14:48:40 -0800 > Brian Norris <briannorris@chromium.org> wrote: > > > A variety of applications have found it useful to listen to > > user-initiated input events to make decisions within a DRM driver, given > > that input events are often the first sign that we're going to start > > doing latency-sensitive activities: > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > take 10s of milliseconds for a panel to exit self-refresh, which can > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > with an input_handler boost, that preemptively exits self-refresh > > whenever there is input activity. > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > render new frames immediately after user activity. Powering up the > > GPU can take enough time that it is worthwhile to start this process > > as soon as there is input activity. Many Chrome OS systems also ship > > with an input_handler boost that powers up the GPU. > > > > This patch provides a small helper library that abstracts some of the > > input-subsystem details around picking which devices to listen to, and > > some other boilerplate. This will be used in the next patch to implement > > the first bullet: preemptive exit for panel self-refresh. > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > have been carrying for a while. > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > --- > > Thanks Simon for the CC. > > Hi Brian, > > while this feature in general makes sense and sounds good, to start > warming up display hardware early when something might start to happen, > this particular proposal has many problems from UAPI perspective (as it > has none). Comments below. > > Btw. if PSR is that slow to wake up from, how much do you actually gain > from this input event watching? I would imagine the improvement to not > be noticeable. Patch 2 has details. It's not really about precisely how slow PSR is, but how much foresight we can gain: in patch 2, I note that with my particular user space and system, I can start PSR-exit 50ms earlier than I would otherweise. (FWIW, this measurement is exactly the same it was with the original version written 4 years ago.) For how long PSR-exit takes: the measurements I'm able to do (via ftrace) show that drm_self_refresh_transition() takes between 35 and 55 ms. That's noticeable at 60 fps. And quite conveniently, the input-boost manages to hide nearly 100% of that latency. Typical use cases where one notices PSR latency (and where this 35-55ms matters) involve simply moving a cursor; it's very noticeable when you have more than a few frames of latency to "get started". > I think some numbers about how much this feature helps would be really > good, even if they are quite specific use cases. You also need to > identify the userspace components, because I think different display > servers are very different in their reaction speed. If my email address isn't obvious, I'm testing Chrome OS. I'm frankly not that familiar with the user space display stack, but for what I know, it's rather custom, developed within the Chromium project. Others on CC here could probably give you more detail, if you want specific answers, besides docs like this: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > If KMS gets a pageflip or modeset in no time after an input event, then > what's the gain. OTOH, if the display server is locking on to vblank, > there might be a delay worth avoiding. But then, is it worth > short-circuiting the wake-up in kernel vs. adding a new ioctl that > userspace could hit to start the warming up process? Rob responded to the first part to some extent (there is definitely gain to be had). To the last part: I wrote a simple debugfs hook to allow user space to force a PSR exit, and then a simple user space program to read input events and smash that debugfs file whenever it sees one. Testing in the same scenarios, this appears to lose less than 100 microseconds versus the in-kernel approach, which is negligible for this use case. (I'm not sure about the other use cases.) So, this is technically doable in user space. I can't speak to the ease of _actually_ integrating this into even our own Chrome display manager, but I highly doubt it will get integrated into others. I'd posit this should weigh into the relative worth, but otherwise can't really give you an answer there. I'd also note, software-directed PSR is so far designed to be completely opaque to user space. There's no way to disable it; no way to know it's active; and no way to know anything about the parameters it's computing (like average entry/exit delay). Would you suggest a whole set of new IOCTLs for this? > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 1c41156deb5f..9a6494aa45e6 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > > drm_atomic_state_helper.o drm_damage_helper.o \ > > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > > + > > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > > new file mode 100644 > > index 000000000000..470f90865c7c > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_input_helper.c > > +static int drm_input_connect(struct input_handler *handler, > > + struct input_dev *dev, > > + const struct input_device_id *id) > > +{ > > + struct input_handle *handle; > > + int error; > > + > > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > > + if (!handle) > > + return -ENOMEM; > > + > > + handle->dev = dev; > > + handle->handler = handler; > > + handle->name = "drm-input-helper"; > > + > > + error = input_register_handle(handle); > > + if (error) > > + goto err2; > > + > > + error = input_open_device(handle); > > Does this literally open the input device, just like when userspace > opens the input device? I believe so. Dmitry mentioned something to this effect on earlier versions, but I found that the input_handler does not operate at all if this specific handle wasn't opened. (All handles are independent, and each over their own |open| count.) This part is unfortunate, I agree. If we really want this in-kernel, perhaps I could find a way to tweak the input_handler API. > How do you know userspace is using this input device at all? If > userspace is not using the input device, then DRM should not be opening > it either, as it must have no effect on anything. > > If you open an input device that userspace does not use, you also cause > a power consumption regression, because now the input device itself is > active and possibly flooding the kernel with events (e.g. an > accelerometer). Well, I don't think accelerometers show up as input devices, but I suppose your point could apply to actual input devices. > > + if (error) > > + goto err1; > > + > > + return 0; > > + > > +err1: > > + input_unregister_handle(handle); > > +err2: > > + kfree(handle); > > + return error; > > +} > > + > > +static void drm_input_disconnect(struct input_handle *handle) > > +{ > > + input_close_device(handle); > > + input_unregister_handle(handle); > > + kfree(handle); > > +} > > + > > +static const struct input_device_id drm_input_ids[] = { > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_ABSBIT, > > + .evbit = { BIT_MASK(EV_ABS) }, > > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > > + BIT_MASK(ABS_MT_POSITION_X) | > > + BIT_MASK(ABS_MT_POSITION_Y) }, > > + }, /* multi-touch touchscreen */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_ABS) }, > > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > > + > > + }, /* stylus or joystick device */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > > + }, /* pointer (e.g. trackpad, mouse) */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > > + }, /* keyboard */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > > + }, /* joysticks not caught by ABS_X above */ > > + { > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > + .evbit = { BIT_MASK(EV_KEY) }, > > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > > + }, /* gamepad */ > > I don't think this hardcoded policy belongs in the kernel, nor even > works. Define "works"? It's shipping in various forms on a variety of Android and Chrome OS systems, where it has a noticeable performance benefit, and isn't known to have significant power-consumption issues. > I believe classifying input devices is not that simple. Spearheading > that is libinput which relies on udev tagging the devices with their > types, and that is done based on a hwdb maintained by I think the > systemd project. Or maybe libinput has its own db nowadays as well, I'm > not sure. I'm not that familiar with libinput, etc., but I expect most of what it needs to do is irrelevant to these kinds of use cases. We don't care at all about what character sets or even what type of device is in use, in most cases. As long as it could reasonably be called user input, it's good enough. Also, for most use cases here, the penalty for small inaccuracies is small. Especially for something like panel self-refresh, we'd rather not have it enabled at all, than have it performing poorly. > Also, joysticks and gamepads are something display servers generally do > not open. An application might open some while it's running, but not > all the time. Joysticks could be very chatty while opened, game > controllers might have accelerometers, etc. > > > + { }, > > +}; > > + > > +int drm_input_handle_register(struct drm_device *dev, > > + struct drm_input_handler *handler) > > +{ > > + int ret; > > + > > + if (!handler->callback) > > + return -EINVAL; > > + > > + handler->handler.event = drm_input_event; > > + handler->handler.connect = drm_input_connect; > > + handler->handler.disconnect = drm_input_disconnect; > > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > > + dev_name(dev->dev)); > > + if (!handler->handler.name) > > + return -ENOMEM; > > + > > + handler->handler.id_table = drm_input_ids; > > + handler->handler.private = handler; > > + > > + ret = input_register_handler(&handler->handler); > > Yet another problem here is that this completely ignores the concept of > physical seats. Of course it does so, because seats are a pure > userspace concept. > > The kernel VT console already has problems because the kernel has no > concept of seats, which means that if there is a second seat defined and > a desktop running on it, while the first seat is in the normal VT text > mode, then everything typed in the desktop will be delivered to the VT > shell as well! (This has a possible workaround in userspace [1], by opening > the evdev input devices in some kind of exclusive mode - which is not > common practise AFAIK.) Sure. I'd bet the intersection of systems that use SW-directed PSR and "multi-seat" is negligibly close to zero, but I can't guarantee that. Chalk one up for a user space policy. > Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler > stop getting events? I believe so. [snip] Brian
On Thu, 18 Nov 2021 15:30:38 -0800 Rob Clark <robdclark@gmail.com> wrote: > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris <briannorris@chromium.org> wrote: > > > > > A variety of applications have found it useful to listen to > > > user-initiated input events to make decisions within a DRM driver, given > > > that input events are often the first sign that we're going to start > > > doing latency-sensitive activities: > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > with an input_handler boost, that preemptively exits self-refresh > > > whenever there is input activity. > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > render new frames immediately after user activity. Powering up the > > > GPU can take enough time that it is worthwhile to start this process > > > as soon as there is input activity. Many Chrome OS systems also ship > > > with an input_handler boost that powers up the GPU. > > > > > > This patch provides a small helper library that abstracts some of the > > > input-subsystem details around picking which devices to listen to, and > > > some other boilerplate. This will be used in the next patch to implement > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > have been carrying for a while. > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > In my measurements, it takes userspace a frame or two to respond and > get to the point of starting to build cmdstream (before eventually > doing atomic/pageflip ioctl).. possibly longer if you don't also have > a similar boost mechanism to spool up cpufreq > > But the important thing, IMO, is that atomic/pageflip ioctl is the > cumulation of a long sequence of events.. input-boost is letting > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > that long sequence. Right, exactly. That is why I was musing about a *new* ioctl that userspace could hit as soon as any input device fd (or network fd!) shows signs of life. Would that be enough, avoiding all the annoying questions about which input and DRM devices should participate here (and what about non-input devices that still want to trigger the warm-up, like network traffic, e.g. remote control?), or does it really need to be kernel internal to be fast enough? As Brian wrote about his quick hack to test that via debugfs, sounds like the userspace solution would be totally sufficient. Thanks, pq
On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > Hi Daniel, > > Thanks for the review. Lots to address elsewhere, but I can respond > here first: > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > --- a/drivers/gpu/drm/Kconfig > > > +++ b/drivers/gpu/drm/Kconfig > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > If in doubt, say "N". > > > > > > +config DRM_INPUT_HELPER > > > + def_bool y > > > + depends on DRM_KMS_HELPER > > > + depends on INPUT > > > > Uh please no configs for each thing, it just makes everything more > > complex. Do we _really_ need this? > > First, it's not a configurable option (a user will never see this nor > have to answer Y/N to it); it only serves as an intermediary to express > the CONFIG_INPUT dependency (which is necessary) without making > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > display stacks without the input subsystem.) I'm not so much worried about the user cost, but the maintenance cost. Kbuild config complexity is ridiculous, anything that adds even a bit is really silly. > The closest alternative I can think of with fewer Kconfig symbols is to > just use CONFIG_INPUT directly in the code, to decide whether to provide > the helpers or else just stub them out. But that has a problem of not > properly expressing the =m vs. =y necessity: if, for example, > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > issues. Usually this is done by providing static inline dummy implementations in the headers. That avoids having to sprinkle new Kconfig symbols all over. > In short, yes, I think we really need this. But I'm not a Kbuild expert. > > > > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h > > > new file mode 100644 > > > index 000000000000..7904f397b934 > > > --- /dev/null > > > +++ b/include/drm/drm_input_helper.h > > > @@ -0,0 +1,41 @@ > > > +/* SPDX-License-Identifier: GPL-2.0 */ > > > +/* > > > + * Copyright (C) 2021 Google, Inc. > > > + */ > > > +#ifndef __DRM_INPUT_HELPER_H__ > > > +#define __DRM_INPUT_HELPER_H__ > > > + > > > +#include <linux/input.h> > > > + > > > +struct drm_device; > > > + > > > +struct drm_input_handler { > > > + /* > > > + * Callback to call for input activity. Will be called in an atomic > > > + * context. > > > > How atomic? Like hardirq, and nasty spinlocks held? > > Maybe I should have just cribbed off the <linux/input.h> doc: > > * @event: event handler. This method is being called by input core with > * interrupts disabled and dev->event_lock spinlock held and so > * it may not sleep > > I probably don't want to propagate the subsystem details about which > locks, but I guess I can be specific about "interrupts disabled" and > "don't sleep". You can also do hyperlinks in the generated htmldocs and just reference that: https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#highlights-and-cross-references > > > > + */ > > > + void (*callback)(struct drm_input_handler *handler); > > > + > > > + struct input_handler handler; > > > +}; > > > + > > > +#if defined(CONFIG_DRM_INPUT_HELPER) > > > + > > > +int drm_input_handle_register(struct drm_device *dev, > > > + struct drm_input_handler *handler); > > > +void drm_input_handle_unregister(struct drm_input_handler *handler); > > > + > > > +#else /* !CONFIG_DRM_INPUT_HELPER */ > > > + > > > +static inline int drm_input_handle_register(struct drm_device *dev, > > > + struct drm_input_handler *handler) > > > +{ > > > + return 0; > > > +} > > > > I guess the reason behind the helper is that you also want to use this in > > drivers or maybe drm/sched? > > I think my reasoning is heavily described in both the cover letter and > the commit message. If that's not clear, can you point out which part? > I'd gladly improve it :) > > But specifically, see the 2nd bullet from the commit message, which I've > re-quoted down here: > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > render new frames immediately after user activity. Powering up the > > > GPU can take enough time that it is worthwhile to start this process > > > as soon as there is input activity. Many Chrome OS systems also ship > > > with an input_handler boost that powers up the GPU. > > Rob Clark has patches to drm/msm to boost GPU power-up via a similar > helper. Yeah this question was just for confirmation, might be good to include that other patch set too for the full picture. > > Anyway I think it looks all reasonable. Definitely need an ack from input > > people > > I realized I failed to carry Dmitry's Ack from version 1 [1]. If this > has a v3 in similar form, I'll carry it there. > > > that the event list you have is a good choice, I have no idea what > > that all does. Maybe also document that part a bit more. > > I'm admittedly not an expert there, and this is actually one reason why > we hoped to make this a library (that nobody wants to keep figuring out > whether all those flags, etc., are really doing the right thing), but > there are comments about what each entry is _trying_ to do. Are you > suggesting more, as in, why "BTN_LEFT + EV_KEY" means "pointer"? Or why > we match certain devices (because they represent likely user activity > that will affect the display pipeline)? Or both? Anyway, I'll give it a > shot, if we keep this. So maybe this is all very obvious for input folks, and comments about what each does is overkill. But I think in the kerneldoc for gfx folks it would be good to explain what kind of events this listens for, like iirc you listen to key-up not key-down, since often the boost has expired by the time the key is actually lifted? Stuff like that I think would be good to explain the why behind the choice of entries in the list. Or that we try to listen to some pointer/mouse events (all of them? only "important" ones?) -Daniel > > Brian > > [1] https://lore.kernel.org/all/YYW6FwSeNMK25ENm@google.com/
On Thu, 18 Nov 2021 17:46:10 -0800 Brian Norris <briannorris@chromium.org> wrote: > Hi Pekka, > > Thanks for the thoughts and review. I've tried to respond below: > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris <briannorris@chromium.org> wrote: > > > > > A variety of applications have found it useful to listen to > > > user-initiated input events to make decisions within a DRM driver, given > > > that input events are often the first sign that we're going to start > > > doing latency-sensitive activities: > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > with an input_handler boost, that preemptively exits self-refresh > > > whenever there is input activity. > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > render new frames immediately after user activity. Powering up the > > > GPU can take enough time that it is worthwhile to start this process > > > as soon as there is input activity. Many Chrome OS systems also ship > > > with an input_handler boost that powers up the GPU. > > > > > > This patch provides a small helper library that abstracts some of the > > > input-subsystem details around picking which devices to listen to, and > > > some other boilerplate. This will be used in the next patch to implement > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > have been carrying for a while. > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > Patch 2 has details. It's not really about precisely how slow PSR is, > but how much foresight we can gain: in patch 2, I note that with my > particular user space and system, I can start PSR-exit 50ms earlier than > I would otherweise. (FWIW, this measurement is exactly the same it was > with the original version written 4 years ago.) > > For how long PSR-exit takes: the measurements I'm able to do (via > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > manages to hide nearly 100% of that latency. > > Typical use cases where one notices PSR latency (and where this 35-55ms > matters) involve simply moving a cursor; it's very noticeable when you > have more than a few frames of latency to "get started". Hi Brian, that is very interesting, thanks. I would never have expected to have userspace take *that* long to react. But, that sounds like it could be just your userspace software stack. > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > not that familiar with the user space display stack, but for what I > know, it's rather custom, developed within the Chromium project. Others > on CC here could probably give you more detail, if you want specific > answers, besides docs like this: > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > Rob responded to the first part to some extent (there is definitely gain > to be had). > > To the last part: I wrote a simple debugfs hook to allow user space to > force a PSR exit, and then a simple user space program to read input > events and smash that debugfs file whenever it sees one. Testing in the > same scenarios, this appears to lose less than 100 microseconds versus > the in-kernel approach, which is negligible for this use case. (I'm not > sure about the other use cases.) > > So, this is technically doable in user space. This is crucial information I would like you to include in some commit message. I think it is very interesting for the reviewers. Maybe also copy that in the cover letter. In my opinion there is a clear and obvious decision due that measurement: Add the new ioctl for userspace to hit, do not try to hardcode or upload the wake-up policy into the kernel. > I can't speak to the ease of _actually_ integrating this into even our > own Chrome display manager, but I highly doubt it will get integrated > into others. I'd posit this should weigh into the relative worth, but > otherwise can't really give you an answer there. I think such a thing would be very simple to add to any display server. They already have hooks for things like resetting idle timeout timers on any relevant input event. > I'd also note, software-directed PSR is so far designed to be completely > opaque to user space. There's no way to disable it; no way to know it's > active; and no way to know anything about the parameters it's computing > (like average entry/exit delay). Would you suggest a whole set of new > IOCTLs for this? Just one ioctl on the DRM device: "Hey, wake up!". Because that's what your patch does in-kernel, right? If there are use case specific parameters, then how did you intend to allow adjusting those in your proposal? > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > > index 1c41156deb5f..9a6494aa45e6 100644 > > > --- a/drivers/gpu/drm/Makefile > > > +++ b/drivers/gpu/drm/Makefile > > > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > > > drm_atomic_state_helper.o drm_damage_helper.o \ > > > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > > > > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > > > + > > > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > > > new file mode 100644 > > > index 000000000000..470f90865c7c > > > --- /dev/null > > > +++ b/drivers/gpu/drm/drm_input_helper.c > > > > +static int drm_input_connect(struct input_handler *handler, > > > + struct input_dev *dev, > > > + const struct input_device_id *id) > > > +{ > > > + struct input_handle *handle; > > > + int error; > > > + > > > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > > > + if (!handle) > > > + return -ENOMEM; > > > + > > > + handle->dev = dev; > > > + handle->handler = handler; > > > + handle->name = "drm-input-helper"; > > > + > > > + error = input_register_handle(handle); > > > + if (error) > > > + goto err2; > > > + > > > + error = input_open_device(handle); > > > > Does this literally open the input device, just like when userspace > > opens the input device? > > I believe so. Dmitry mentioned something to this effect on earlier > versions, but I found that the input_handler does not operate at all if > this specific handle wasn't opened. (All handles are independent, and > each over their own |open| count.) > > This part is unfortunate, I agree. If we really want this in-kernel, > perhaps I could find a way to tweak the input_handler API. Ok. Sounds like this can have a clear technical solution, and then this issue is solved for good. This might also remove the device type filtering problem. > > How do you know userspace is using this input device at all? If > > userspace is not using the input device, then DRM should not be opening > > it either, as it must have no effect on anything. > > > > If you open an input device that userspace does not use, you also cause > > a power consumption regression, because now the input device itself is > > active and possibly flooding the kernel with events (e.g. an > > accelerometer). > > Well, I don't think accelerometers show up as input devices, but I > suppose your point could apply to actual input devices. My understanding is that accelerometers are evdev (input) devices, especially when used as input e.g. for controlling games. I'm not aware of any other interface for it. Even audio sockets are input devices for detecting whether a plug has been plugged in, but those probably wouldn't flood anything. > > > + if (error) > > > + goto err1; > > > + > > > + return 0; > > > + > > > +err1: > > > + input_unregister_handle(handle); > > > +err2: > > > + kfree(handle); > > > + return error; > > > +} > > > + > > > +static void drm_input_disconnect(struct input_handle *handle) > > > +{ > > > + input_close_device(handle); > > > + input_unregister_handle(handle); > > > + kfree(handle); > > > +} > > > + > > > +static const struct input_device_id drm_input_ids[] = { > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > + INPUT_DEVICE_ID_MATCH_ABSBIT, > > > + .evbit = { BIT_MASK(EV_ABS) }, > > > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > > > + BIT_MASK(ABS_MT_POSITION_X) | > > > + BIT_MASK(ABS_MT_POSITION_Y) }, > > > + }, /* multi-touch touchscreen */ > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > + .evbit = { BIT_MASK(EV_ABS) }, > > > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > > > + > > > + }, /* stylus or joystick device */ > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > > > + }, /* pointer (e.g. trackpad, mouse) */ > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > > > + }, /* keyboard */ > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > > > + }, /* joysticks not caught by ABS_X above */ > > > + { > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > > > + }, /* gamepad */ > > > > I don't think this hardcoded policy belongs in the kernel, nor even > > works. > > Define "works"? Makes the right decision in at least all those cases where current desktop userspace (udev + hwdb + libinput) already makes the correct decision. From what I've seen, it looks like end users come with bug reports every now and then when some hardware manufacturer was lazy or inventive with their HID descriptors. > It's shipping in various forms on a variety of Android > and Chrome OS systems, where it has a noticeable performance benefit, > and isn't known to have significant power-consumption issues. Peter Hutterer could probably say more, I confess I am quite pessimistic. > > I believe classifying input devices is not that simple. Spearheading > > that is libinput which relies on udev tagging the devices with their > > types, and that is done based on a hwdb maintained by I think the > > systemd project. Or maybe libinput has its own db nowadays as well, I'm > > not sure. > > I'm not that familiar with libinput, etc., but I expect most of what it > needs to do is irrelevant to these kinds of use cases. We don't care at > all about what character sets or even what type of device is in use, in > most cases. As long as it could reasonably be called user input, it's > good enough. > > Also, for most use cases here, the penalty for small inaccuracies is > small. Especially for something like panel self-refresh, we'd rather not > have it enabled at all, than have it performing poorly. This problem will diminish once your patches stop literally opening the input devices and listens only on input devices that are actually opened by userspace. When that happens, I'm not sure you even need this device type filtering at all. The remaining problem is the seat designation. > > Also, joysticks and gamepads are something display servers generally do > > not open. An application might open some while it's running, but not > > all the time. Joysticks could be very chatty while opened, game > > controllers might have accelerometers, etc. > > > > > + { }, > > > +}; > > > + > > > +int drm_input_handle_register(struct drm_device *dev, > > > + struct drm_input_handler *handler) > > > +{ > > > + int ret; > > > + > > > + if (!handler->callback) > > > + return -EINVAL; > > > + > > > + handler->handler.event = drm_input_event; > > > + handler->handler.connect = drm_input_connect; > > > + handler->handler.disconnect = drm_input_disconnect; > > > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > > > + dev_name(dev->dev)); > > > + if (!handler->handler.name) > > > + return -ENOMEM; > > > + > > > + handler->handler.id_table = drm_input_ids; > > > + handler->handler.private = handler; > > > + > > > + ret = input_register_handler(&handler->handler); > > > > Yet another problem here is that this completely ignores the concept of > > physical seats. Of course it does so, because seats are a pure > > userspace concept. > > > > The kernel VT console already has problems because the kernel has no > > concept of seats, which means that if there is a second seat defined and > > a desktop running on it, while the first seat is in the normal VT text > > mode, then everything typed in the desktop will be delivered to the VT > > shell as well! (This has a possible workaround in userspace [1], by opening > > the evdev input devices in some kind of exclusive mode - which is not > > common practise AFAIK.) > > Sure. > > I'd bet the intersection of systems that use SW-directed PSR and > "multi-seat" is negligibly close to zero, but I can't guarantee that. > Chalk one up for a user space policy. Your cover letter has also the other bullet point: ramping up GPUs. That applies to a lot more systems than PSR, right? Maybe that is an acceptable trade-off: be 100 µs faster (your measurement) by ramping up all GPUs in a system instead of only the relevant ones? Or maybe that will hurt normal gaming computers by ramping up the iGPU when the OS and game only uses the dGPU, which makes iGPU eat away the CPU power budget, causing the CPU to slow down? I suppose that would be handled by ramping up only GPUs that userspace has opened. > > Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler > > stop getting events? > > I believe so. I suppose you would not want that? The solution to the VT console problem is for userspace to start using EVIOCGRAB and that could regress the warm-up machinery. In summary, there are all these open questions and annoying little problems, and none of these issues would exist if userspace would drive the warm-up explicitly. Thanks, pq
On Fri, Nov 19, 2021 at 11:54:19AM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark <robdclark@gmail.com> wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris <briannorris@chromium.org> wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > > with an input_handler boost, that preemptively exits self-refresh > > > > whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > render new frames immediately after user activity. Powering up the > > > > GPU can take enough time that it is worthwhile to start this process > > > > as soon as there is input activity. Many Chrome OS systems also ship > > > > with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. Random idea ... should we perhaps let userspace connect the boosting? I.e. we do a bunch of standardized boost targets (render clocks, display sr exit), and userspace can then connect it to whichever input device it wants to? That also avoids the multi-user lol of us boosting the wrong seat, we could do a drm ioctl where you pass it an eventfd and essentially say "listen to this mkay?" That way the boosting would also neatly get passed along with compositors as we vt switch them, in case you have one that's all tablet, and another one (console emulation) that's kbd only. Also this avoids the latency problem perhaps of a compositor which just dumbly paints every frame because it's VR or something like that, so never any sr exit possible. Just an idea, compositor people pls shred it :-) -Daniel
On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 17:46:10 -0800 > Brian Norris <briannorris@chromium.org> wrote: > > > Hi Pekka, > > > > Thanks for the thoughts and review. I've tried to respond below: > > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris <briannorris@chromium.org> wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > > with an input_handler boost, that preemptively exits self-refresh > > > > whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > render new frames immediately after user activity. Powering up the > > > > GPU can take enough time that it is worthwhile to start this process > > > > as soon as there is input activity. Many Chrome OS systems also ship > > > > with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > Patch 2 has details. It's not really about precisely how slow PSR is, > > but how much foresight we can gain: in patch 2, I note that with my > > particular user space and system, I can start PSR-exit 50ms earlier than > > I would otherweise. (FWIW, this measurement is exactly the same it was > > with the original version written 4 years ago.) > > > > For how long PSR-exit takes: the measurements I'm able to do (via > > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > > manages to hide nearly 100% of that latency. > > > > Typical use cases where one notices PSR latency (and where this 35-55ms > > matters) involve simply moving a cursor; it's very noticeable when you > > have more than a few frames of latency to "get started". > > Hi Brian, > > that is very interesting, thanks. > > I would never have expected to have userspace take *that* long to > react. But, that sounds like it could be just your userspace software > stack. In the other subthread we're talking about making this more explicit. Maybe we need to combine this with a "I expect to take this many milliseconds to get the first frame out" value. That way compositors which take 50ms (which frankly is shocking slow) can set that, and kms can enable sr exit (since sr exit will actually help here). But other compositors which expect to get the first frame out in maybe 20 can spec that, and then the driver will not sr exit (because too high chances we'll just make shit slower), and instead will only boost render clocks. Thoughts? -Daniel > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > > not that familiar with the user space display stack, but for what I > > know, it's rather custom, developed within the Chromium project. Others > > on CC here could probably give you more detail, if you want specific > > answers, besides docs like this: > > > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > Rob responded to the first part to some extent (there is definitely gain > > to be had). > > > > To the last part: I wrote a simple debugfs hook to allow user space to > > force a PSR exit, and then a simple user space program to read input > > events and smash that debugfs file whenever it sees one. Testing in the > > same scenarios, this appears to lose less than 100 microseconds versus > > the in-kernel approach, which is negligible for this use case. (I'm not > > sure about the other use cases.) > > > > So, this is technically doable in user space. > > This is crucial information I would like you to include in some commit > message. I think it is very interesting for the reviewers. Maybe also > copy that in the cover letter. > > In my opinion there is a clear and obvious decision due that > measurement: Add the new ioctl for userspace to hit, do not try to > hardcode or upload the wake-up policy into the kernel. > > > I can't speak to the ease of _actually_ integrating this into even our > > own Chrome display manager, but I highly doubt it will get integrated > > into others. I'd posit this should weigh into the relative worth, but > > otherwise can't really give you an answer there. > > I think such a thing would be very simple to add to any display server. > They already have hooks for things like resetting idle timeout timers on > any relevant input event. > > > I'd also note, software-directed PSR is so far designed to be completely > > opaque to user space. There's no way to disable it; no way to know it's > > active; and no way to know anything about the parameters it's computing > > (like average entry/exit delay). Would you suggest a whole set of new > > IOCTLs for this? > > Just one ioctl on the DRM device: "Hey, wake up!". Because that's what > your patch does in-kernel, right? > > If there are use case specific parameters, then how did you intend to > allow adjusting those in your proposal? > > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > > > index 1c41156deb5f..9a6494aa45e6 100644 > > > > --- a/drivers/gpu/drm/Makefile > > > > +++ b/drivers/gpu/drm/Makefile > > > > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ > > > > drm_atomic_state_helper.o drm_damage_helper.o \ > > > > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > > > > > > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > > > > + > > > > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > > > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > > > diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c > > > > new file mode 100644 > > > > index 000000000000..470f90865c7c > > > > --- /dev/null > > > > +++ b/drivers/gpu/drm/drm_input_helper.c > > > > > > +static int drm_input_connect(struct input_handler *handler, > > > > + struct input_dev *dev, > > > > + const struct input_device_id *id) > > > > +{ > > > > + struct input_handle *handle; > > > > + int error; > > > > + > > > > + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); > > > > + if (!handle) > > > > + return -ENOMEM; > > > > + > > > > + handle->dev = dev; > > > > + handle->handler = handler; > > > > + handle->name = "drm-input-helper"; > > > > + > > > > + error = input_register_handle(handle); > > > > + if (error) > > > > + goto err2; > > > > + > > > > + error = input_open_device(handle); > > > > > > Does this literally open the input device, just like when userspace > > > opens the input device? > > > > I believe so. Dmitry mentioned something to this effect on earlier > > versions, but I found that the input_handler does not operate at all if > > this specific handle wasn't opened. (All handles are independent, and > > each over their own |open| count.) > > > > This part is unfortunate, I agree. If we really want this in-kernel, > > perhaps I could find a way to tweak the input_handler API. > > Ok. Sounds like this can have a clear technical solution, and then this > issue is solved for good. This might also remove the device type > filtering problem. > > > > How do you know userspace is using this input device at all? If > > > userspace is not using the input device, then DRM should not be opening > > > it either, as it must have no effect on anything. > > > > > > If you open an input device that userspace does not use, you also cause > > > a power consumption regression, because now the input device itself is > > > active and possibly flooding the kernel with events (e.g. an > > > accelerometer). > > > > Well, I don't think accelerometers show up as input devices, but I > > suppose your point could apply to actual input devices. > > My understanding is that accelerometers are evdev (input) devices, > especially when used as input e.g. for controlling games. I'm not aware > of any other interface for it. > > Even audio sockets are input devices for detecting whether a plug has > been plugged in, but those probably wouldn't flood anything. > > > > > > + if (error) > > > > + goto err1; > > > > + > > > > + return 0; > > > > + > > > > +err1: > > > > + input_unregister_handle(handle); > > > > +err2: > > > > + kfree(handle); > > > > + return error; > > > > +} > > > > + > > > > +static void drm_input_disconnect(struct input_handle *handle) > > > > +{ > > > > + input_close_device(handle); > > > > + input_unregister_handle(handle); > > > > + kfree(handle); > > > > +} > > > > + > > > > +static const struct input_device_id drm_input_ids[] = { > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > > + INPUT_DEVICE_ID_MATCH_ABSBIT, > > > > + .evbit = { BIT_MASK(EV_ABS) }, > > > > + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = > > > > + BIT_MASK(ABS_MT_POSITION_X) | > > > > + BIT_MASK(ABS_MT_POSITION_Y) }, > > > > + }, /* multi-touch touchscreen */ > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > > + .evbit = { BIT_MASK(EV_ABS) }, > > > > + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } > > > > + > > > > + }, /* stylus or joystick device */ > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > > + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, > > > > + }, /* pointer (e.g. trackpad, mouse) */ > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, > > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > > + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, > > > > + }, /* keyboard */ > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > > + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, > > > > + }, /* joysticks not caught by ABS_X above */ > > > > + { > > > > + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | > > > > + INPUT_DEVICE_ID_MATCH_KEYBIT, > > > > + .evbit = { BIT_MASK(EV_KEY) }, > > > > + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, > > > > + }, /* gamepad */ > > > > > > I don't think this hardcoded policy belongs in the kernel, nor even > > > works. > > > > Define "works"? > > Makes the right decision in at least all those cases where current > desktop userspace (udev + hwdb + libinput) already makes the correct > decision. From what I've seen, it looks like end users come with bug > reports every now and then when some hardware manufacturer was lazy or > inventive with their HID descriptors. > > > It's shipping in various forms on a variety of Android > > and Chrome OS systems, where it has a noticeable performance benefit, > > and isn't known to have significant power-consumption issues. > > Peter Hutterer could probably say more, I confess I am quite > pessimistic. > > > > I believe classifying input devices is not that simple. Spearheading > > > that is libinput which relies on udev tagging the devices with their > > > types, and that is done based on a hwdb maintained by I think the > > > systemd project. Or maybe libinput has its own db nowadays as well, I'm > > > not sure. > > > > I'm not that familiar with libinput, etc., but I expect most of what it > > needs to do is irrelevant to these kinds of use cases. We don't care at > > all about what character sets or even what type of device is in use, in > > most cases. As long as it could reasonably be called user input, it's > > good enough. > > > > Also, for most use cases here, the penalty for small inaccuracies is > > small. Especially for something like panel self-refresh, we'd rather not > > have it enabled at all, than have it performing poorly. > > This problem will diminish once your patches stop literally opening the > input devices and listens only on input devices that are actually > opened by userspace. When that happens, I'm not sure you even need this > device type filtering at all. The remaining problem is the seat > designation. > > > > Also, joysticks and gamepads are something display servers generally do > > > not open. An application might open some while it's running, but not > > > all the time. Joysticks could be very chatty while opened, game > > > controllers might have accelerometers, etc. > > > > > > > + { }, > > > > +}; > > > > + > > > > +int drm_input_handle_register(struct drm_device *dev, > > > > + struct drm_input_handler *handler) > > > > +{ > > > > + int ret; > > > > + > > > > + if (!handler->callback) > > > > + return -EINVAL; > > > > + > > > > + handler->handler.event = drm_input_event; > > > > + handler->handler.connect = drm_input_connect; > > > > + handler->handler.disconnect = drm_input_disconnect; > > > > + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", > > > > + dev_name(dev->dev)); > > > > + if (!handler->handler.name) > > > > + return -ENOMEM; > > > > + > > > > + handler->handler.id_table = drm_input_ids; > > > > + handler->handler.private = handler; > > > > + > > > > + ret = input_register_handler(&handler->handler); > > > > > > Yet another problem here is that this completely ignores the concept of > > > physical seats. Of course it does so, because seats are a pure > > > userspace concept. > > > > > > The kernel VT console already has problems because the kernel has no > > > concept of seats, which means that if there is a second seat defined and > > > a desktop running on it, while the first seat is in the normal VT text > > > mode, then everything typed in the desktop will be delivered to the VT > > > shell as well! (This has a possible workaround in userspace [1], by opening > > > the evdev input devices in some kind of exclusive mode - which is not > > > common practise AFAIK.) > > > > Sure. > > > > I'd bet the intersection of systems that use SW-directed PSR and > > "multi-seat" is negligibly close to zero, but I can't guarantee that. > > Chalk one up for a user space policy. > > Your cover letter has also the other bullet point: ramping up GPUs. > That applies to a lot more systems than PSR, right? > > Maybe that is an acceptable trade-off: be 100 µs faster (your > measurement) by ramping up all GPUs in a system instead of only the > relevant ones? > > Or maybe that will hurt normal gaming computers by ramping up the iGPU > when the OS and game only uses the dGPU, which makes iGPU eat away the > CPU power budget, causing the CPU to slow down? I suppose that would be > handled by ramping up only GPUs that userspace has opened. > > > > Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler > > > stop getting events? > > > > I believe so. > > I suppose you would not want that? > > The solution to the VT console problem is for userspace to start using > EVIOCGRAB and that could regress the warm-up machinery. > > In summary, there are all these open questions and annoying little > problems, and none of these issues would exist if userspace would drive > the warm-up explicitly. > > > Thanks, > pq
On Friday, November 19th, 2021 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote: > Random idea ... should we perhaps let userspace connect the boosting? I.e. > we do a bunch of standardized boost targets (render clocks, display sr > exit), and userspace can then connect it to whichever input device it > wants to? On IRC we discussed having user-space hand over a FD to the kernel. When the FD becomes readable, the kernel triggers the boost. This would let user-space use e.g. an input device, an eventfd, or an epoll FD with any combination of these as the boost signal.
On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote: > On Friday, November 19th, 2021 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote: > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > we do a bunch of standardized boost targets (render clocks, display sr > > exit), and userspace can then connect it to whichever input device it > > wants to? > > On IRC we discussed having user-space hand over a FD to the kernel. When the FD > becomes readable, the kernel triggers the boost. > > This would let user-space use e.g. an input device, an eventfd, or an epoll FD > with any combination of these as the boost signal. Can userspace filter eventfd appropriately like we do here? And can they get at that maybe 2nd eventfd from logind or whatever there is on distros where /dev access is locked down for compositors/users. I do agree that if we can do this generically maybe we should, but also the use-case for input boosting is pretty well defined. I think it's just about making sure that compositors is in control, and that we don't make it worse (e.g. with the sr exit adding latency when the compositor can redraw quickly enough). -Daniel
On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark <robdclark@gmail.com> wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris <briannorris@chromium.org> wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > > with an input_handler boost, that preemptively exits self-refresh > > > > whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > render new frames immediately after user activity. Powering up the > > > > GPU can take enough time that it is worthwhile to start this process > > > > as soon as there is input activity. Many Chrome OS systems also ship > > > > with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. > The question is, I think we want to use this for at least a couple different things.. PSR exit, and/or early GPU wakeup/boost. So I'm not quite sure where/what this ioctl should be. Different drivers may have different uses. Also, because of the CrOS userspace sandbox architecture, the place that could do a kms ioctl isn't really the place that would know when to call the ioctl. But if we want to move the policy out of the kernel, one approach would just be to have some sysfs where userspace could configure the input_device_id[] filter.. BR, -R
Hi, On Fri, Nov 19, 2021 at 8:39 AM Rob Clark <robdclark@gmail.com> wrote: > > On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > On Thu, 18 Nov 2021 15:30:38 -0800 > > Rob Clark <robdclark@gmail.com> wrote: > > > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@gmail.com> wrote: > > > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris <briannorris@chromium.org> wrote: > > > > > > > > > A variety of applications have found it useful to listen to > > > > > user-initiated input events to make decisions within a DRM driver, given > > > > > that input events are often the first sign that we're going to start > > > > > doing latency-sensitive activities: > > > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > > > with an input_handler boost, that preemptively exits self-refresh > > > > > whenever there is input activity. > > > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > > render new frames immediately after user activity. Powering up the > > > > > GPU can take enough time that it is worthwhile to start this process > > > > > as soon as there is input activity. Many Chrome OS systems also ship > > > > > with an input_handler boost that powers up the GPU. > > > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > > input-subsystem details around picking which devices to listen to, and > > > > > some other boilerplate. This will be used in the next patch to implement > > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > > have been carrying for a while. > > > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > > --- > > > > > > > > Thanks Simon for the CC. > > > > > > > > Hi Brian, > > > > > > > > while this feature in general makes sense and sounds good, to start > > > > warming up display hardware early when something might start to happen, > > > > this particular proposal has many problems from UAPI perspective (as it > > > > has none). Comments below. > > > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > > from this input event watching? I would imagine the improvement to not > > > > be noticeable. > > > > > > > > I think some numbers about how much this feature helps would be really > > > > good, even if they are quite specific use cases. You also need to > > > > identify the userspace components, because I think different display > > > > servers are very different in their reaction speed. > > > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > > there might be a delay worth avoiding. But then, is it worth > > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > > userspace could hit to start the warming up process? > > > > > > In my measurements, it takes userspace a frame or two to respond and > > > get to the point of starting to build cmdstream (before eventually > > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > > a similar boost mechanism to spool up cpufreq > > > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > > cumulation of a long sequence of events.. input-boost is letting > > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > > that long sequence. > > > > Right, exactly. That is why I was musing about a *new* ioctl that > > userspace could hit as soon as any input device fd (or network fd!) > > shows signs of life. Would that be enough, avoiding all the annoying > > questions about which input and DRM devices should participate here > > (and what about non-input devices that still want to trigger the > > warm-up, like network traffic, e.g. remote control?), or does it really > > need to be kernel internal to be fast enough? > > > > As Brian wrote about his quick hack to test that via debugfs, sounds > > like the userspace solution would be totally sufficient. > > > > The question is, I think we want to use this for at least a couple > different things.. PSR exit, and/or early GPU wakeup/boost. So I'm > not quite sure where/what this ioctl should be. Different drivers may > have different uses. Also, because of the CrOS userspace sandbox > architecture, the place that could do a kms ioctl isn't really the > place that would know when to call the ioctl. > > But if we want to move the policy out of the kernel, one approach > would just be to have some sysfs where userspace could configure the > input_device_id[] filter.. While I'm fine with being overridden, in my mind: 1. Having a simple set of rules that works for "simple" devices that's built-in to the kernel seems like a nice thing to have. Something like what Brian has is pretty simple and works for a pretty wide variety of devices. It's also not very much code nor all that complex. 2. IMO the answer for some of the more complex use cases is to allow them to disable the simple in-kernel solution (we can debate about Kconfig, sysfs, or ioctl) and control this with whatever complicated logic they want to implement in userspace. If it were up to me, I would make this as raw as possible. I wouldn't add optimizations to make it easy for userspace to specify which input devices or anything, I'd just let userspace figure out how to pay attention to the events it cares about, make decisions, and then tell the kernel about it. -Doug
Hi Daniel, On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote: > On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > > --- a/drivers/gpu/drm/Kconfig > > > > +++ b/drivers/gpu/drm/Kconfig > > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > > > If in doubt, say "N". > > > > > > > > +config DRM_INPUT_HELPER > > > > + def_bool y > > > > + depends on DRM_KMS_HELPER > > > > + depends on INPUT > > > > > > Uh please no configs for each thing, it just makes everything more > > > complex. Do we _really_ need this? > > > > First, it's not a configurable option (a user will never see this nor > > have to answer Y/N to it); it only serves as an intermediary to express > > the CONFIG_INPUT dependency (which is necessary) without making > > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > > display stacks without the input subsystem.) > > I'm not so much worried about the user cost, but the maintenance cost. > Kbuild config complexity is ridiculous, anything that adds even a bit is > really silly. > > > The closest alternative I can think of with fewer Kconfig symbols is to > > just use CONFIG_INPUT directly in the code, to decide whether to provide > > the helpers or else just stub them out. But that has a problem of not > > properly expressing the =m vs. =y necessity: if, for example, > > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > > issues. > > Usually this is done by providing static inline dummy implementations in > the headers. That avoids having to sprinkle new Kconfig symbols all over. Right, I already did that, and I'm not sprinkling CONFIG_DRM_INPUT_HELPER much. (I do include one around the module parameter, because it doesn't make much sense to have the module parameter even exist, if the underlying feature is stubbed out.) But that doesn't solve the problem in my last sentence, involving tristates. The "stub inline" approach only works well for boolean features -- either built-in, or disabled. Once your feature is in a module, you need to ensure that no built-in code depends on it. Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If so, I can just add a 'select' or 'depend' and drop this intermediate symbol. If not, then what do you expect to happen with DRM_KMS_HELPER=y and CONFIG_INPUT=m? Brian
On Fri, 19 Nov 2021 16:56:01 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 17:46:10 -0800 > > Brian Norris <briannorris@chromium.org> wrote: > > > > > Hi Pekka, > > > > > > Thanks for the thoughts and review. I've tried to respond below: > > > > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris <briannorris@chromium.org> wrote: > > > > > > > > > A variety of applications have found it useful to listen to > > > > > user-initiated input events to make decisions within a DRM driver, given > > > > > that input events are often the first sign that we're going to start > > > > > doing latency-sensitive activities: > > > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > > Rockchip eDP) is especially latency sensitive. In some cases, it can > > > > > take 10s of milliseconds for a panel to exit self-refresh, which can > > > > > be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > > > with an input_handler boost, that preemptively exits self-refresh > > > > > whenever there is input activity. > > > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > > render new frames immediately after user activity. Powering up the > > > > > GPU can take enough time that it is worthwhile to start this process > > > > > as soon as there is input activity. Many Chrome OS systems also ship > > > > > with an input_handler boost that powers up the GPU. > > > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > > input-subsystem details around picking which devices to listen to, and > > > > > some other boilerplate. This will be used in the next patch to implement > > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > > have been carrying for a while. > > > > > > > > > > Signed-off-by: Brian Norris <briannorris@chromium.org> > > > > > --- > > > > > > > > Thanks Simon for the CC. > > > > > > > > Hi Brian, > > > > > > > > while this feature in general makes sense and sounds good, to start > > > > warming up display hardware early when something might start to happen, > > > > this particular proposal has many problems from UAPI perspective (as it > > > > has none). Comments below. > > > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > > from this input event watching? I would imagine the improvement to not > > > > be noticeable. > > > > > > Patch 2 has details. It's not really about precisely how slow PSR is, > > > but how much foresight we can gain: in patch 2, I note that with my > > > particular user space and system, I can start PSR-exit 50ms earlier than > > > I would otherweise. (FWIW, this measurement is exactly the same it was > > > with the original version written 4 years ago.) > > > > > > For how long PSR-exit takes: the measurements I'm able to do (via > > > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > > > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > > > manages to hide nearly 100% of that latency. > > > > > > Typical use cases where one notices PSR latency (and where this 35-55ms > > > matters) involve simply moving a cursor; it's very noticeable when you > > > have more than a few frames of latency to "get started". > > > > Hi Brian, > > > > that is very interesting, thanks. > > > > I would never have expected to have userspace take *that* long to > > react. But, that sounds like it could be just your userspace software > > stack. > > In the other subthread we're talking about making this more explicit. > Maybe we need to combine this with a "I expect to take this many > milliseconds to get the first frame out" value. > > That way compositors which take 50ms (which frankly is shocking slow) can > set that, and kms can enable sr exit (since sr exit will actually help > here). But other compositors which expect to get the first frame out in > maybe 20 can spec that, and then the driver will not sr exit (because too > high chances we'll just make shit slower), and instead will only boost > render clocks. > > Thoughts? I wonder if the compositor or the userspace stack can know how long it usually takes to prepare the first KMS submission after a pause. I guess it would need to measure that at runtime. Hmm, doable I guess, sure. Input to output latency in general is interesting. However, that sounds like a pretty vague API with the delay value. I think it has a high risk of regressing into a boolean toggle by userspace choosing an arbitrary number and then assuming the threshold in the driver is always the same. Thanks, pq
On Fri, 19 Nov 2021 17:11:07 +0100 Daniel Vetter <daniel@ffwll.ch> wrote: > On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote: > > On Friday, November 19th, 2021 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > > we do a bunch of standardized boost targets (render clocks, display sr > > > exit), and userspace can then connect it to whichever input device it > > > wants to? > > > > On IRC we discussed having user-space hand over a FD to the kernel. When the FD > > becomes readable, the kernel triggers the boost. > > > > This would let user-space use e.g. an input device, an eventfd, or an epoll FD > > with any combination of these as the boost signal. > > Can userspace filter eventfd appropriately like we do here? And can they > get at that maybe 2nd eventfd from logind or whatever there is on distros > where /dev access is locked down for compositors/users. (Mind, eventfd is a specific thing, see 'man eventfd', and evdev/input device fd is different.) I don't think any of that is any problem when userspace prepares an epoll fd to be given to the boosting machinery. The boosting machinery could have several different targets as well, PSR vs. GPU clocks and whatnot. I envision a compositor to maintain an epoll fd for boosting by adding/removing the same device fds to it that it already uses in its operations. I don't see any need to open new device fds just for boosting. It's only the epoll fd given to the kernel and after that the epoll set can still be changed, right? The boosting machinery would never actually read or write the registered fd(s), so it would not interfere with the normal operations. But it also means the fd will remain readable until userspace services it. Userspace may need to set up that epoll set very carefully to have it work right (e.g. edge-triggered?). If your input handling is in a different process than the DRM poking for some reason, the epoll fd should still work if: - it is possible to use SCM_RIGHTS to pass the epollfd from the input process to the DRM process, and - you cannot extract the watched fds from an epoll fd. Do we have those assumptions today? Then the attack surface in the DRM process is limited to changing the epoll set of which fds can trigger boosting, but the DRM process can do that anyway. I also presume the input process can still add and remove fds from the epoll set even afterwards. > I do agree that if we can do this generically maybe we should, but also > the use-case for input boosting is pretty well defined. I think it's just > about making sure that compositors is in control, and that we don't make > it worse (e.g. with the sr exit adding latency when the compositor can > redraw quickly enough). The epollfd design sounds very good to me. One can register an arbitrary set of fds with it, and use even eventfds in the set to have purely software triggers. Thanks, pq
On Fri, Nov 19, 2021 at 11:07:16AM -0800, Brian Norris wrote: > Hi Daniel, > > On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote: > > On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > > > --- a/drivers/gpu/drm/Kconfig > > > > > +++ b/drivers/gpu/drm/Kconfig > > > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > > > > > If in doubt, say "N". > > > > > > > > > > +config DRM_INPUT_HELPER > > > > > + def_bool y > > > > > + depends on DRM_KMS_HELPER > > > > > + depends on INPUT > > > > > > > > Uh please no configs for each thing, it just makes everything more > > > > complex. Do we _really_ need this? > > > > > > First, it's not a configurable option (a user will never see this nor > > > have to answer Y/N to it); it only serves as an intermediary to express > > > the CONFIG_INPUT dependency (which is necessary) without making > > > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > > > display stacks without the input subsystem.) > > > > I'm not so much worried about the user cost, but the maintenance cost. > > Kbuild config complexity is ridiculous, anything that adds even a bit is > > really silly. > > > > > The closest alternative I can think of with fewer Kconfig symbols is to > > > just use CONFIG_INPUT directly in the code, to decide whether to provide > > > the helpers or else just stub them out. But that has a problem of not > > > properly expressing the =m vs. =y necessity: if, for example, > > > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > > > issues. > > > > Usually this is done by providing static inline dummy implementations in > > the headers. That avoids having to sprinkle new Kconfig symbols all over. > > Right, I already did that, and I'm not sprinkling > CONFIG_DRM_INPUT_HELPER much. (I do include one around the module > parameter, because it doesn't make much sense to have the module > parameter even exist, if the underlying feature is stubbed out.) > > But that doesn't solve the problem in my last sentence, involving > tristates. The "stub inline" approach only works well for boolean > features -- either built-in, or disabled. Once your feature is in a > module, you need to ensure that no built-in code depends on it. > > Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If > so, I can just add a 'select' or 'depend' and drop this intermediate > symbol. > If not, then what do you expect to happen with DRM_KMS_HELPER=y and > CONFIG_INPUT=m? Yeah just add the dependency. If you still want to keep it optional the way to do it is to add depends on FOO || FOO=n And then just have #if IS_ENABLED(FOO) around your inline wrappers. -Daniel
On Mon, Nov 22, 2021 at 11:43:42AM +0200, Pekka Paalanen wrote: > On Fri, 19 Nov 2021 17:11:07 +0100 > Daniel Vetter <daniel@ffwll.ch> wrote: > > > On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote: > > > On Friday, November 19th, 2021 at 16:53, Daniel Vetter <daniel@ffwll.ch> wrote: > > > > > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > > > we do a bunch of standardized boost targets (render clocks, display sr > > > > exit), and userspace can then connect it to whichever input device it > > > > wants to? > > > > > > On IRC we discussed having user-space hand over a FD to the kernel. When the FD > > > becomes readable, the kernel triggers the boost. > > > > > > This would let user-space use e.g. an input device, an eventfd, or an epoll FD > > > with any combination of these as the boost signal. > > > > Can userspace filter eventfd appropriately like we do here? And can they > > get at that maybe 2nd eventfd from logind or whatever there is on distros > > where /dev access is locked down for compositors/users. > > (Mind, eventfd is a specific thing, see 'man eventfd', and evdev/input > device fd is different.) Yeah I was a bit sloppy, but I knew. > I don't think any of that is any problem when userspace prepares an > epoll fd to be given to the boosting machinery. The boosting machinery > could have several different targets as well, PSR vs. GPU clocks and > whatnot. > > I envision a compositor to maintain an epoll fd for boosting by > adding/removing the same device fds to it that it already uses in its > operations. I don't see any need to open new device fds just for > boosting. It's only the epoll fd given to the kernel and after that the > epoll set can still be changed, right? > > The boosting machinery would never actually read or write the > registered fd(s), so it would not interfere with the normal operations. > But it also means the fd will remain readable until userspace services > it. Userspace may need to set up that epoll set very carefully to have > it work right (e.g. edge-triggered?). > > If your input handling is in a different process than the DRM poking > for some reason, the epoll fd should still work if: > - it is possible to use SCM_RIGHTS to pass the epollfd from the > input process to the DRM process, and > - you cannot extract the watched fds from an epoll fd. > > Do we have those assumptions today? > > Then the attack surface in the DRM process is limited to changing the > epoll set of which fds can trigger boosting, but the DRM process can do > that anyway. I also presume the input process can still add and remove > fds from the epoll set even afterwards. > > > I do agree that if we can do this generically maybe we should, but also > > the use-case for input boosting is pretty well defined. I think it's just > > about making sure that compositors is in control, and that we don't make > > it worse (e.g. with the sr exit adding latency when the compositor can > > redraw quickly enough). > > The epollfd design sounds very good to me. One can register an > arbitrary set of fds with it, and use even eventfds in the set to have > purely software triggers. Yeah I think just allowing to internall poll on any arbitrary fd sounds like a neat interface. Userspace should then be able to do whatever it wants to. -Daniel
Hi Pekka, On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 17:46:10 -0800 > Brian Norris <briannorris@chromium.org> wrote: > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris <briannorris@chromium.org> wrote: > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > Rob responded to the first part to some extent (there is definitely gain > > to be had). > > > > To the last part: I wrote a simple debugfs hook to allow user space to > > force a PSR exit, and then a simple user space program to read input > > events and smash that debugfs file whenever it sees one. Testing in the > > same scenarios, this appears to lose less than 100 microseconds versus > > the in-kernel approach, which is negligible for this use case. (I'm not > > sure about the other use cases.) > > > > So, this is technically doable in user space. > > This is crucial information I would like you to include in some commit > message. I think it is very interesting for the reviewers. Maybe also > copy that in the cover letter. > > In my opinion there is a clear and obvious decision due that > measurement: Add the new ioctl for userspace to hit, do not try to > hardcode or upload the wake-up policy into the kernel. Perhaps. I'll admit, I'm not eager to go write the fd-passing solutions that others are designing on the fly. I'm currently torn on whether I'll just live with this current patch set out-of-tree (or, y'all can decide that a simple, 99% working solution is better than no solution), because it's simple; or possibly figuring out how to utilize such an ioctl cleanly within our display manager. I'm not super hopeful on the latter. IOW, I'm approximately in line with Doug's thoughts: https://lore.kernel.org/all/CAD=FV=XARhZoj+0p-doxcbC=4K+NuMc=uR6wqG6kWk-MkPkNdQ@mail.gmail.com/ But then, we're obviously biased. > > I can't speak to the ease of _actually_ integrating this into even our > > own Chrome display manager, but I highly doubt it will get integrated > > into others. I'd posit this should weigh into the relative worth, but > > otherwise can't really give you an answer there. > > I think such a thing would be very simple to add to any display server. > They already have hooks for things like resetting idle timeout timers on > any relevant input event. > > > I'd also note, software-directed PSR is so far designed to be completely > > opaque to user space. There's no way to disable it; no way to know it's > > active; and no way to know anything about the parameters it's computing > > (like average entry/exit delay). Would you suggest a whole set of new > > IOCTLs for this? > > Just one ioctl on the DRM device: "Hey, wake up!". Because that's what > your patch does in-kernel, right? Well, we'd at least want something to advertise that the feature does something ("is supported") I think, otherwise we're just asking user space to do useless work. > If there are use case specific parameters, then how did you intend to > allow adjusting those in your proposal? Another commenter mentioned the latency tradeoff -- it's possible that there are panels/eDP-links that resume fast enough that one doesn't care to use this ioctl. For an in-kernel solution, one has all the data available and could use hardware information to make decisions, if needed. For a user space solution, we won't have any of that, and we'd have to work to expose that information. I suppose we could ignore that problem and only expose a minimal UAPI until we need something more, but it feels like exposing a UAPI for something is a critical point where one should make sure it's reasonably descriptive and useful. > > > How do you know userspace is using this input device at all? If > > > userspace is not using the input device, then DRM should not be opening > > > it either, as it must have no effect on anything. > > > > > > If you open an input device that userspace does not use, you also cause > > > a power consumption regression, because now the input device itself is > > > active and possibly flooding the kernel with events (e.g. an > > > accelerometer). > > > > Well, I don't think accelerometers show up as input devices, but I > > suppose your point could apply to actual input devices. > > My understanding is that accelerometers are evdev (input) devices, > especially when used as input e.g. for controlling games. I'm not aware > of any other interface for it. I'm not familiar with game-controlling accelerometers, but many types of accelerometers are serviced by drivers/iio/. And even if they register as input devices, do they match the ID list in this patch? > Even audio sockets are input devices for detecting whether a plug has > been plugged in, but those probably wouldn't flood anything. They also won't match the input_handler ID list, because they won't support the key or position combinations in the heuristic. > > > Yet another problem here is that this completely ignores the concept of > > > physical seats. Of course it does so, because seats are a pure > > > userspace concept. > > > > > > The kernel VT console already has problems because the kernel has no > > > concept of seats, which means that if there is a second seat defined and > > > a desktop running on it, while the first seat is in the normal VT text > > > mode, then everything typed in the desktop will be delivered to the VT > > > shell as well! (This has a possible workaround in userspace [1], by opening > > > the evdev input devices in some kind of exclusive mode - which is not > > > common practise AFAIK.) > > > > Sure. > > > > I'd bet the intersection of systems that use SW-directed PSR and > > "multi-seat" is negligibly close to zero, but I can't guarantee that. > > Chalk one up for a user space policy. > > Your cover letter has also the other bullet point: ramping up GPUs. > That applies to a lot more systems than PSR, right? > > Maybe that is an acceptable trade-off: be 100 µs faster (your > measurement) by ramping up all GPUs in a system instead of only the > relevant ones? > > Or maybe that will hurt normal gaming computers by ramping up the iGPU > when the OS and game only uses the dGPU, which makes iGPU eat away the > CPU power budget, causing the CPU to slow down? I suppose that would be > handled by ramping up only GPUs that userspace has opened. FWIW, the current work we have out-of-tree involves only select GPU drivers that know they are slow to ramp up. If this were generalized, then yes, it could potentially have undesireable side effects. I'm certainly not an expert on Rob's work though, so I can't speak to this very much, but I imagine we could resolve the {d,i}GPU problem easily. Brian
On Tue, Nov 30, 2021 at 12:35:45PM -0800, Brian Norris wrote: > Hi Pekka, > > On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 17:46:10 -0800 > > Brian Norris <briannorris@chromium.org> wrote: > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris <briannorris@chromium.org> wrote: > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > > there might be a delay worth avoiding. But then, is it worth > > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > > userspace could hit to start the warming up process? > > > > > > Rob responded to the first part to some extent (there is definitely gain > > > to be had). > > > > > > To the last part: I wrote a simple debugfs hook to allow user space to > > > force a PSR exit, and then a simple user space program to read input > > > events and smash that debugfs file whenever it sees one. Testing in the > > > same scenarios, this appears to lose less than 100 microseconds versus > > > the in-kernel approach, which is negligible for this use case. (I'm not > > > sure about the other use cases.) > > > > > > So, this is technically doable in user space. > > > > This is crucial information I would like you to include in some commit > > message. I think it is very interesting for the reviewers. Maybe also > > copy that in the cover letter. > > > > In my opinion there is a clear and obvious decision due that > > measurement: Add the new ioctl for userspace to hit, do not try to > > hardcode or upload the wake-up policy into the kernel. > > Perhaps. > > I'll admit, I'm not eager to go write the fd-passing solutions that > others are designing on the fly. I'm currently torn on whether I'll just > live with this current patch set out-of-tree (or, y'all can decide that > a simple, 99% working solution is better than no solution), because it's > simple; or possibly figuring out how to utilize such an ioctl cleanly > within our display manager. I'm not super hopeful on the latter. > > IOW, I'm approximately in line with Doug's thoughts: > https://lore.kernel.org/all/CAD=FV=XARhZoj+0p-doxcbC=4K+NuMc=uR6wqG6kWk-MkPkNdQ@mail.gmail.com/ > But then, we're obviously biased. > > > > I can't speak to the ease of _actually_ integrating this into even our > > > own Chrome display manager, but I highly doubt it will get integrated > > > into others. I'd posit this should weigh into the relative worth, but > > > otherwise can't really give you an answer there. > > > > I think such a thing would be very simple to add to any display server. > > They already have hooks for things like resetting idle timeout timers on > > any relevant input event. > > > > > I'd also note, software-directed PSR is so far designed to be completely > > > opaque to user space. There's no way to disable it; no way to know it's > > > active; and no way to know anything about the parameters it's computing > > > (like average entry/exit delay). Would you suggest a whole set of new > > > IOCTLs for this? > > > > Just one ioctl on the DRM device: "Hey, wake up!". Because that's what > > your patch does in-kernel, right? > > Well, we'd at least want something to advertise that the feature does > something ("is supported") I think, otherwise we're just asking user > space to do useless work. > > > If there are use case specific parameters, then how did you intend to > > allow adjusting those in your proposal? > > Another commenter mentioned the latency tradeoff -- it's possible that > there are panels/eDP-links that resume fast enough that one doesn't care > to use this ioctl. For an in-kernel solution, one has all the data > available and could use hardware information to make decisions, if > needed. For a user space solution, we won't have any of that, and we'd > have to work to expose that information. > > I suppose we could ignore that problem and only expose a minimal UAPI > until we need something more, but it feels like exposing a UAPI for > something is a critical point where one should make sure it's reasonably > descriptive and useful. > > > > > How do you know userspace is using this input device at all? If > > > > userspace is not using the input device, then DRM should not be opening > > > > it either, as it must have no effect on anything. > > > > > > > > If you open an input device that userspace does not use, you also cause > > > > a power consumption regression, because now the input device itself is > > > > active and possibly flooding the kernel with events (e.g. an > > > > accelerometer). > > > > > > Well, I don't think accelerometers show up as input devices, but I > > > suppose your point could apply to actual input devices. fwiw, filtering INPUT_PROP_ACCELEROMETER would go a long way towards ignoring accelerometers. > > My understanding is that accelerometers are evdev (input) devices, > > especially when used as input e.g. for controlling games. I'm not aware > > of any other interface for it. > I'm not familiar with game-controlling accelerometers, but many types of > accelerometers are serviced by drivers/iio/. you can also unbind devices and use hidraw directly. > And even if they register as input devices, do they match the ID list in > this patch? device type assignment is problematic, but i think in this case it doesn't matter if the associations are a bit rough. You don't care about the type of device, you merely care about "is this likely to be used". And I think for that the list is good enough. though tbh, having this policy in userspace would still be better IMO. Cheers, Peter > > Even audio sockets are input devices for detecting whether a plug has > > been plugged in, but those probably wouldn't flood anything. > > They also won't match the input_handler ID list, because they won't > support the key or position combinations in the heuristic. > > > > > Yet another problem here is that this completely ignores the concept of > > > > physical seats. Of course it does so, because seats are a pure > > > > userspace concept. > > > > > > > > The kernel VT console already has problems because the kernel has no > > > > concept of seats, which means that if there is a second seat defined and > > > > a desktop running on it, while the first seat is in the normal VT text > > > > mode, then everything typed in the desktop will be delivered to the VT > > > > shell as well! (This has a possible workaround in userspace [1], by opening > > > > the evdev input devices in some kind of exclusive mode - which is not > > > > common practise AFAIK.) > > > > > > Sure. > > > > > > I'd bet the intersection of systems that use SW-directed PSR and > > > "multi-seat" is negligibly close to zero, but I can't guarantee that. > > > Chalk one up for a user space policy. > > > > Your cover letter has also the other bullet point: ramping up GPUs. > > That applies to a lot more systems than PSR, right? > > > > Maybe that is an acceptable trade-off: be 100 µs faster (your > > measurement) by ramping up all GPUs in a system instead of only the > > relevant ones? > > > > Or maybe that will hurt normal gaming computers by ramping up the iGPU > > when the OS and game only uses the dGPU, which makes iGPU eat away the > > CPU power budget, causing the CPU to slow down? I suppose that would be > > handled by ramping up only GPUs that userspace has opened. > > FWIW, the current work we have out-of-tree involves only select GPU > drivers that know they are slow to ramp up. If this were generalized, > then yes, it could potentially have undesireable side effects. I'm > certainly not an expert on Rob's work though, so I can't speak to this > very much, but I imagine we could resolve the {d,i}GPU problem easily. > > Brian
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index fb144617055b..381476b10a9d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST If in doubt, say "N". +config DRM_INPUT_HELPER + def_bool y + depends on DRM_KMS_HELPER + depends on INPUT + config DRM_KMS_HELPER tristate depends on DRM + select DRM_INPUT_HELPER if INPUT help CRTC helpers for KMS drivers. diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1c41156deb5f..9a6494aa45e6 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ drm_atomic_state_helper.o drm_damage_helper.o \ drm_format_helper.o drm_self_refresh_helper.o drm_rect.o +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o + drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c new file mode 100644 index 000000000000..470f90865c7c --- /dev/null +++ b/drivers/gpu/drm/drm_input_helper.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2021 Google, Inc. + */ +#include <linux/input.h> +#include <linux/slab.h> + +#include <drm/drm_device.h> +#include <drm/drm_input_helper.h> + +/** + * DOC: overview + * + * This helper library provides a thin wrapper around input handles, so that + * DRM drivers can easily perform domain-specific actions in response to user + * activity. e.g., if someone is moving a mouse, we're likely to want to + * display something soon, and we should exit panel self-refresh. + */ + +static void drm_input_event(struct input_handle *handle, unsigned int type, + unsigned int code, int value) +{ + struct drm_input_handler *handler = handle->handler->private; + + handler->callback(handler); +} + +static int drm_input_connect(struct input_handler *handler, + struct input_dev *dev, + const struct input_device_id *id) +{ + struct input_handle *handle; + int error; + + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL); + if (!handle) + return -ENOMEM; + + handle->dev = dev; + handle->handler = handler; + handle->name = "drm-input-helper"; + + error = input_register_handle(handle); + if (error) + goto err2; + + error = input_open_device(handle); + if (error) + goto err1; + + return 0; + +err1: + input_unregister_handle(handle); +err2: + kfree(handle); + return error; +} + +static void drm_input_disconnect(struct input_handle *handle) +{ + input_close_device(handle); + input_unregister_handle(handle); + kfree(handle); +} + +static const struct input_device_id drm_input_ids[] = { + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | + INPUT_DEVICE_ID_MATCH_ABSBIT, + .evbit = { BIT_MASK(EV_ABS) }, + .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] = + BIT_MASK(ABS_MT_POSITION_X) | + BIT_MASK(ABS_MT_POSITION_Y) }, + }, /* multi-touch touchscreen */ + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, + .evbit = { BIT_MASK(EV_ABS) }, + .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) } + + }, /* stylus or joystick device */ + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, + .evbit = { BIT_MASK(EV_KEY) }, + .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) }, + }, /* pointer (e.g. trackpad, mouse) */ + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT, + .evbit = { BIT_MASK(EV_KEY) }, + .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) }, + }, /* keyboard */ + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | + INPUT_DEVICE_ID_MATCH_KEYBIT, + .evbit = { BIT_MASK(EV_KEY) }, + .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) }, + }, /* joysticks not caught by ABS_X above */ + { + .flags = INPUT_DEVICE_ID_MATCH_EVBIT | + INPUT_DEVICE_ID_MATCH_KEYBIT, + .evbit = { BIT_MASK(EV_KEY) }, + .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) }, + }, /* gamepad */ + { }, +}; + +int drm_input_handle_register(struct drm_device *dev, + struct drm_input_handler *handler) +{ + int ret; + + if (!handler->callback) + return -EINVAL; + + handler->handler.event = drm_input_event; + handler->handler.connect = drm_input_connect; + handler->handler.disconnect = drm_input_disconnect; + handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s", + dev_name(dev->dev)); + if (!handler->handler.name) + return -ENOMEM; + + handler->handler.id_table = drm_input_ids; + handler->handler.private = handler; + + ret = input_register_handler(&handler->handler); + if (ret) + goto err; + + return 0; + +err: + kfree(handler->handler.name); + return ret; +} +EXPORT_SYMBOL(drm_input_handle_register); + +void drm_input_handle_unregister(struct drm_input_handler *handler) +{ + input_unregister_handler(&handler->handler); + kfree(handler->handler.name); +} +EXPORT_SYMBOL(drm_input_handle_unregister); diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h new file mode 100644 index 000000000000..7904f397b934 --- /dev/null +++ b/include/drm/drm_input_helper.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2021 Google, Inc. + */ +#ifndef __DRM_INPUT_HELPER_H__ +#define __DRM_INPUT_HELPER_H__ + +#include <linux/input.h> + +struct drm_device; + +struct drm_input_handler { + /* + * Callback to call for input activity. Will be called in an atomic + * context. + */ + void (*callback)(struct drm_input_handler *handler); + + struct input_handler handler; +}; + +#if defined(CONFIG_DRM_INPUT_HELPER) + +int drm_input_handle_register(struct drm_device *dev, + struct drm_input_handler *handler); +void drm_input_handle_unregister(struct drm_input_handler *handler); + +#else /* !CONFIG_DRM_INPUT_HELPER */ + +static inline int drm_input_handle_register(struct drm_device *dev, + struct drm_input_handler *handler) +{ + return 0; +} + +static inline void +drm_input_handle_unregister(struct drm_input_handler *handler) {} + +#endif /* CONFIG_DRM_INPUT_HELPER */ + +#endif /* __DRM_INPUT_HELPER_H__ */
A variety of applications have found it useful to listen to user-initiated input events to make decisions within a DRM driver, given that input events are often the first sign that we're going to start doing latency-sensitive activities: * Panel self-refresh: software-directed self-refresh (e.g., with Rockchip eDP) is especially latency sensitive. In some cases, it can take 10s of milliseconds for a panel to exit self-refresh, which can be noticeable. Rockchip RK3399 Chrome OS systems have always shipped with an input_handler boost, that preemptively exits self-refresh whenever there is input activity. * GPU drivers: on GPU-accelerated desktop systems, we may need to render new frames immediately after user activity. Powering up the GPU can take enough time that it is worthwhile to start this process as soon as there is input activity. Many Chrome OS systems also ship with an input_handler boost that powers up the GPU. This patch provides a small helper library that abstracts some of the input-subsystem details around picking which devices to listen to, and some other boilerplate. This will be used in the next patch to implement the first bullet: preemptive exit for panel self-refresh. Bits of this are adapted from code the Android and/or Chrome OS kernels have been carrying for a while. Signed-off-by: Brian Norris <briannorris@chromium.org> --- Changes in v2: - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER - Remove void*; users should use container_of() - Document the callback context drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ include/drm/drm_input_helper.h | 41 +++++++++ 4 files changed, 192 insertions(+) create mode 100644 drivers/gpu/drm/drm_input_helper.c create mode 100644 include/drm/drm_input_helper.h