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

[Rob Clark]

On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote:
>
> On Wed, 17 Nov 2021 14:48:40 -0800
> Brian Norris <briannorris@xxxxxxxxxxxx> wrote:
>
> > A variety of applications have found it useful to listen to
> > user-initiated input events to make decisions within a DRM driver, given
> > that input events are often the first sign that we're going to start
> > doing latency-sensitive activities:
> >
> >  * Panel self-refresh: software-directed self-refresh (e.g., with
> >    Rockchip eDP) is especially latency sensitive. In some cases, it can
> >    take 10s of milliseconds for a panel to exit self-refresh, which can
> >    be noticeable. Rockchip RK3399 Chrome OS systems have always shipped
> >    with an input_handler boost, that preemptively exits self-refresh
> >    whenever there is input activity.
> >
> >  * GPU drivers: on GPU-accelerated desktop systems, we may need to
> >    render new frames immediately after user activity. Powering up the
> >    GPU can take enough time that it is worthwhile to start this process
> >    as soon as there is input activity. Many Chrome OS systems also ship
> >    with an input_handler boost that powers up the GPU.
> >
> > This patch provides a small helper library that abstracts some of the
> > input-subsystem details around picking which devices to listen to, and
> > some other boilerplate. This will be used in the next patch to implement
> > the first bullet: preemptive exit for panel self-refresh.
> >
> > Bits of this are adapted from code the Android and/or Chrome OS kernels
> > have been carrying for a while.
> >
> > Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> > ---
>
> 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__ */
>