LinuxLists.cc - [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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 <[email protected]>
> ---
>
> 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
>

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-18 10:40:07

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Wed, 17 Nov 2021 14:48:40 -0800
Brian Norris <[email protected]> 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 <[email protected]>
> ---

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__ */

Attachments:

2021-11-18 19:30:48

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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/[email protected]/

2021-11-18 23:26:23

by Rob Clark

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <[email protected]> wrote:
>
> On Wed, 17 Nov 2021 14:48:40 -0800
> Brian Norris <[email protected]> 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 <[email protected]>
> > ---
>
> 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__ */
>

2021-11-19 01:46:16

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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 <[email protected]> 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 <[email protected]>
> > ---
>
> 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

2021-11-19 09:54:29

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Thu, 18 Nov 2021 15:30:38 -0800
Rob Clark <[email protected]> wrote:

> On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <[email protected]> wrote:
> >
> > On Wed, 17 Nov 2021 14:48:40 -0800
> > Brian Norris <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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

Attachments:

2021-11-19 10:01:24

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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/[email protected]/

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-19 10:38:49

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Thu, 18 Nov 2021 17:46:10 -0800
Brian Norris <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > ---
> >
> > 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

Attachments:

2021-11-19 15:53:43

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, Nov 19, 2021 at 11:54:19AM +0200, Pekka Paalanen wrote:
> On Thu, 18 Nov 2021 15:30:38 -0800
> Rob Clark <[email protected]> wrote:
>
> > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <[email protected]> wrote:
> > >
> > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > Brian Norris <[email protected]> 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 <[email protected]>
> > > > ---
> > >
> > > 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-19 15:56:12

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote:
> On Thu, 18 Nov 2021 17:46:10 -0800
> Brian Norris <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > ---
> > >
> > > 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

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-19 16:04:35

by Simon Ser

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Friday, November 19th, 2021 at 16:53, Daniel Vetter <[email protected]> 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.

2021-11-19 16:11:14

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote:
> On Friday, November 19th, 2021 at 16:53, Daniel Vetter <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-19 16:39:44

by Rob Clark

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen <[email protected]> wrote:
>
> On Thu, 18 Nov 2021 15:30:38 -0800
> Rob Clark <[email protected]> wrote:
>
> > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <[email protected]> wrote:
> > >
> > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > Brian Norris <[email protected]> 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 <[email protected]>
> > > > ---
> > >
> > > 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

2021-11-19 16:50:28

by Doug Anderson

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

Hi,

On Fri, Nov 19, 2021 at 8:39 AM Rob Clark <[email protected]> wrote:
>
> On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen <[email protected]> wrote:
> >
> > On Thu, 18 Nov 2021 15:30:38 -0800
> > Rob Clark <[email protected]> wrote:
> >
> > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <[email protected]> wrote:
> > > >
> > > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > > Brian Norris <[email protected]> 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 <[email protected]>
> > > > > ---
> > > >
> > > > 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

2021-11-19 19:07:22

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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

2021-11-22 09:25:17

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, 19 Nov 2021 16:56:01 +0100
Daniel Vetter <[email protected]> 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > >
> > > > 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

Attachments:

2021-11-22 09:43:52

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Fri, 19 Nov 2021 17:11:07 +0100
Daniel Vetter <[email protected]> wrote:

> On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote:
> > On Friday, November 19th, 2021 at 16:53, Daniel Vetter <[email protected]> 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

Attachments:

2021-11-25 15:33:32

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-25 15:39:45

[permalink] [raw]

Subject: Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper

On Mon, Nov 22, 2021 at 11:43:42AM +0200, Pekka Paalanen wrote:
> On Fri, 19 Nov 2021 17:11:07 +0100
> Daniel Vetter <[email protected]> wrote:
>
> > On Fri, Nov 19, 2021 at 04:04:28PM +0000, Simon Ser wrote:
> > > On Friday, November 19th, 2021 at 16:53, Daniel Vetter <[email protected]> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

2021-11-30 20:35:51