On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:

A variety of applications have found it useful to listen to user-initiated input events to make decisions within a DRM driver, given that input events are often the first sign that we're going to start doing latency-sensitive activities:

• Panel self-refresh: software-directed self-refresh (e.g., with Rockchip eDP) is especially latency sensitive. In some cases, it can take 10s of milliseconds for a panel to exit self-refresh, which can be noticeable. Rockchip RK3399 Chrome OS systems have always shipped with an input_handler boost, that preemptively exits self-refresh whenever there is input activity.

• GPU drivers: on GPU-accelerated desktop systems, we may need to render new frames immediately after user activity. Powering up the GPU can take enough time that it is worthwhile to start this process as soon as there is input activity. Many Chrome OS systems also ship with an input_handler boost that powers up the GPU.

This patch provides a small helper library that abstracts some of the input-subsystem details around picking which devices to listen to, and some other boilerplate. This will be used in the next patch to implement the first bullet: preemptive exit for panel self-refresh.

Bits of this are adapted from code the Android and/or Chrome OS kernels have been carrying for a while.

Signed-off-by: Brian Norris briannorris@chromium.org

Changes in v2:

• Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER
• Remove void*; users should use container_of()
• Document the callback context

drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ include/drm/drm_input_helper.h | 41 +++++++++

Please add documentation for this and include it under Documentation/gpu/drm-kms-helpers.rst in a suitable place.

Standards for core code should be overview DOC: with references to key functions/structs, and all driver visible structs, functions (static inline in header or exported) fully documented.

4 files changed, 192 insertions(+) create mode 100644 drivers/gpu/drm/drm_input_helper.c create mode 100644 include/drm/drm_input_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index fb144617055b..381476b10a9d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST

 If in doubt, say "N".

+config DRM_INPUT_HELPER

• def_bool y
• depends on DRM_KMS_HELPER
• depends on INPUT

Uh please no configs for each thing, it just makes everything more complex. Do we _really_ need this?

config DRM_KMS_HELPER tristate depends on DRM

• select DRM_INPUT_HELPER if INPUT help CRTC helpers for KMS drivers.

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1c41156deb5f..9a6494aa45e6 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ drm_atomic_state_helper.o drm_damage_helper.o \ drm_format_helper.o drm_self_refresh_helper.o drm_rect.o

+drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o

drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c new file mode 100644 index 000000000000..470f90865c7c --- /dev/null +++ b/drivers/gpu/drm/drm_input_helper.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2021 Google, Inc.
• */

+#include <linux/input.h> +#include <linux/slab.h>

+#include <drm/drm_device.h> +#include <drm/drm_input_helper.h>

+/**

• • DOC: overview
• • This helper library provides a thin wrapper around input handles, so that
• • DRM drivers can easily perform domain-specific actions in response to user
• • activity. e.g., if someone is moving a mouse, we're likely to want to
• • display something soon, and we should exit panel self-refresh.
• */

+static void drm_input_event(struct input_handle *handle, unsigned int type,

• 	    unsigned int code, int value)
  

+{

• struct drm_input_handler *handler = handle->handler->private;
• handler->callback(handler);

+}

+static int drm_input_connect(struct input_handler *handler,

• 	     struct input_dev *dev,
  

• 	     const struct input_device_id *id)
  

+{

• struct input_handle *handle;
• int error;
• handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
• if (!handle)

• return -ENOMEM;
  

• handle->dev = dev;
• handle->handler = handler;
• handle->name = "drm-input-helper";
• error = input_register_handle(handle);
• if (error)

• goto err2;
  

• error = input_open_device(handle);
• if (error)

• goto err1;
  

• return 0;

+err1:

• input_unregister_handle(handle);

+err2:

• kfree(handle);
• return error;

+}

+static void drm_input_disconnect(struct input_handle *handle) +{

• input_close_device(handle);
• input_unregister_handle(handle);
• kfree(handle);

+}

+static const struct input_device_id drm_input_ids[] = {

• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_ABSBIT,
  

• .evbit = { BIT_MASK(EV_ABS) },
  

• .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] =
  

• 	    BIT_MASK(ABS_MT_POSITION_X) |
  

• 	    BIT_MASK(ABS_MT_POSITION_Y) },
  

• }, /* multi-touch touchscreen */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_ABS) },
  

• .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) }
  

• }, /* stylus or joystick device */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) },
  

• }, /* pointer (e.g. trackpad, mouse) */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) },
  

• }, /* keyboard */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_KEYBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) },
  

• }, /* joysticks not caught by ABS_X above */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_KEYBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) },
  

• }, /* gamepad */
• { },

+};

+int drm_input_handle_register(struct drm_device *dev,

• 	      struct drm_input_handler *handler)
  

+{

• int ret;
• if (!handler->callback)

• return -EINVAL;
  

• handler->handler.event = drm_input_event;
• handler->handler.connect = drm_input_connect;
• handler->handler.disconnect = drm_input_disconnect;
• handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s",

• 			  dev_name(dev->dev));
  

• if (!handler->handler.name)

• return -ENOMEM;
  

• handler->handler.id_table = drm_input_ids;
• handler->handler.private = handler;
• ret = input_register_handler(&handler->handler);
• if (ret)

• goto err;
  

• return 0;

+err:

• kfree(handler->handler.name);
• return ret;

+} +EXPORT_SYMBOL(drm_input_handle_register);

+void drm_input_handle_unregister(struct drm_input_handler *handler) +{

• input_unregister_handler(&handler->handler);
• kfree(handler->handler.name);

+} +EXPORT_SYMBOL(drm_input_handle_unregister); diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h new file mode 100644 index 000000000000..7904f397b934 --- /dev/null +++ b/include/drm/drm_input_helper.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2021 Google, Inc.
• */

+#ifndef __DRM_INPUT_HELPER_H__ +#define __DRM_INPUT_HELPER_H__

+#include <linux/input.h>

+struct drm_device;

+struct drm_input_handler {

• /*

• * Callback to call for input activity. Will be called in an atomic
  

• * context.
  

How atomic? Like hardirq, and nasty spinlocks held?

• */
  

• void (*callback)(struct drm_input_handler *handler);
• struct input_handler handler;

+};

+#if defined(CONFIG_DRM_INPUT_HELPER)

+int drm_input_handle_register(struct drm_device *dev,

• 	      struct drm_input_handler *handler);
  

+void drm_input_handle_unregister(struct drm_input_handler *handler);

+#else /* !CONFIG_DRM_INPUT_HELPER */

+static inline int drm_input_handle_register(struct drm_device *dev,

• 			    struct drm_input_handler *handler)
  

+{

• return 0;

+}

I guess the reason behind the helper is that you also want to use this in drivers or maybe drm/sched?

Anyway I think it looks all reasonable. Definitely need an ack from input people that the event list you have is a good choice, I have no idea what that all does. Maybe also document that part a bit more. -Daniel

+static inline void +drm_input_handle_unregister(struct drm_input_handler *handler) {}

+#endif /* CONFIG_DRM_INPUT_HELPER */

+#endif /* __DRM_INPUT_HELPER_H__ */

2.34.0.rc1.387.gb447b232ab-goog

On 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-an...

...

...

• */
  

• void (*callback)(struct drm_input_handler *handler);
• struct input_handler handler;

+};

+#if defined(CONFIG_DRM_INPUT_HELPER)

+int drm_input_handle_register(struct drm_device *dev,

• 	      struct drm_input_handler *handler);
  

+void drm_input_handle_unregister(struct drm_input_handler *handler);

+#else /* !CONFIG_DRM_INPUT_HELPER */

+static inline int drm_input_handle_register(struct drm_device *dev,

• 			    struct drm_input_handler *handler)
  

+{

• return 0;

+}

I guess the reason behind the helper is that you also want to use this in drivers or maybe drm/sched?

I think my reasoning is heavily described in both the cover letter and the commit message. If that's not clear, can you point out which part? I'd gladly improve it :)

But specifically, see the 2nd bullet from the commit message, which I've re-quoted down here:

...

...

• GPU drivers: on GPU-accelerated desktop systems, we may need to render new frames immediately after user activity. Powering up the GPU can take enough time that it is worthwhile to start this process as soon as there is input activity. Many Chrome OS systems also ship with an input_handler boost that powers up the GPU.

Rob Clark has patches to drm/msm to boost GPU power-up via a similar helper.

Yeah this question was just for confirmation, might be good to include that other patch set too for the full picture.

...

Anyway I think it looks all reasonable. Definitely need an ack from input people

I realized I failed to carry Dmitry's Ack from version 1 [1]. If this has a v3 in similar form, I'll carry it there.

...

that the event list you have is a good choice, I have no idea what that all does. Maybe also document that part a bit more.

I'm admittedly not an expert there, and this is actually one reason why we hoped to make this a library (that nobody wants to keep figuring out whether all those flags, etc., are really doing the right thing), but there are comments about what each entry is _trying_ to do. Are you suggesting more, as in, why "BTN_LEFT + EV_KEY" means "pointer"? Or why we match certain devices (because they represent likely user activity that will affect the display pipeline)? Or both? Anyway, I'll give it a shot, if we keep this.

So maybe this is all very obvious for input folks, and comments about what each does is overkill.

But I think in the kerneldoc for gfx folks it would be good to explain what kind of events this listens for, like iirc you listen to key-up not key-down, since often the boost has expired by the time the key is actually lifted? Stuff like that I think would be good to explain the why behind the choice of entries in the list. Or that we try to listen to some pointer/mouse events (all of them? only "important" ones?) -Daniel

Brian

[1] https://lore.kernel.org/all/YYW6FwSeNMK25ENm@google.com/

On Fri, Nov 19, 2021 at 11:07:16AM -0800, Brian Norris wrote:

Hi Daniel,

On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote:

...

On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote:

...

On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote:

...

On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:

...

--- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST

 If in doubt, say "N".

+config DRM_INPUT_HELPER

• def_bool y
• depends on DRM_KMS_HELPER
• depends on INPUT

Uh please no configs for each thing, it just makes everything more complex. Do we _really_ need this?

First, it's not a configurable option (a user will never see this nor have to answer Y/N to it); it only serves as an intermediary to express the CONFIG_INPUT dependency (which is necessary) without making DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run display stacks without the input subsystem.)

I'm not so much worried about the user cost, but the maintenance cost. Kbuild config complexity is ridiculous, anything that adds even a bit is really silly.

...

The closest alternative I can think of with fewer Kconfig symbols is to just use CONFIG_INPUT directly in the code, to decide whether to provide the helpers or else just stub them out. But that has a problem of not properly expressing the =m vs. =y necessity: if, for example, CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker issues.

Usually this is done by providing static inline dummy implementations in the headers. That avoids having to sprinkle new Kconfig symbols all over.

Right, I already did that, and I'm not sprinkling CONFIG_DRM_INPUT_HELPER much. (I do include one around the module parameter, because it doesn't make much sense to have the module parameter even exist, if the underlying feature is stubbed out.)

But that doesn't solve the problem in my last sentence, involving tristates. The "stub inline" approach only works well for boolean features -- either built-in, or disabled. Once your feature is in a module, you need to ensure that no built-in code depends on it.

Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If so, I can just add a 'select' or 'depend' and drop this intermediate symbol. If not, then what do you expect to happen with DRM_KMS_HELPER=y and CONFIG_INPUT=m?

Yeah just add the dependency. If you still want to keep it optional the way to do it is to add

depends on FOO || FOO=n

And then just have #if IS_ENABLED(FOO) around your inline wrappers. -Daniel

On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen ppaalanen@gmail.com wrote:

On Wed, 17 Nov 2021 14:48:40 -0800 Brian Norris briannorris@chromium.org wrote:

...

A variety of applications have found it useful to listen to user-initiated input events to make decisions within a DRM driver, given that input events are often the first sign that we're going to start doing latency-sensitive activities:

• Panel self-refresh: software-directed self-refresh (e.g., with Rockchip eDP) is especially latency sensitive. In some cases, it can take 10s of milliseconds for a panel to exit self-refresh, which can be noticeable. Rockchip RK3399 Chrome OS systems have always shipped with an input_handler boost, that preemptively exits self-refresh whenever there is input activity.

• GPU drivers: on GPU-accelerated desktop systems, we may need to render new frames immediately after user activity. Powering up the GPU can take enough time that it is worthwhile to start this process as soon as there is input activity. Many Chrome OS systems also ship with an input_handler boost that powers up the GPU.

This patch provides a small helper library that abstracts some of the input-subsystem details around picking which devices to listen to, and some other boilerplate. This will be used in the next patch to implement the first bullet: preemptive exit for panel self-refresh.

Bits of this are adapted from code the Android and/or Chrome OS kernels have been carrying for a while.

Signed-off-by: Brian Norris briannorris@chromium.org

Thanks Simon for the CC.

Hi Brian,

while this feature in general makes sense and sounds good, to start warming up display hardware early when something might start to happen, this particular proposal has many problems from UAPI perspective (as it has none). Comments below.

Btw. if PSR is that slow to wake up from, how much do you actually gain from this input event watching? I would imagine the improvement to not be noticeable.

I think some numbers about how much this feature helps would be really good, even if they are quite specific use cases. You also need to identify the userspace components, because I think different display servers are very different in their reaction speed.

If KMS gets a pageflip or modeset in no time after an input event, then what's the gain. OTOH, if the display server is locking on to vblank, there might be a delay worth avoiding. But then, is it worth short-circuiting the wake-up in kernel vs. adding a new ioctl that userspace could hit to start the warming up process?

In my measurements, it takes userspace a frame or two to respond and get to the point of starting to build cmdstream (before eventually doing atomic/pageflip ioctl).. possibly longer if you don't also have a similar boost mechanism to spool up cpufreq

But the important thing, IMO, is that atomic/pageflip ioctl is the cumulation of a long sequence of events.. input-boost is letting whatever it may be (PSR exit, GPU resume, etc) happen in parallel with that long sequence.

BR, -R

...

Changes in v2:

• Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER
• Remove void*; users should use container_of()
• Document the callback context

drivers/gpu/drm/Kconfig | 6 ++ drivers/gpu/drm/Makefile | 2 + drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++ include/drm/drm_input_helper.h | 41 +++++++++ 4 files changed, 192 insertions(+) create mode 100644 drivers/gpu/drm/drm_input_helper.c create mode 100644 include/drm/drm_input_helper.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index fb144617055b..381476b10a9d 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST

    If in doubt, say "N".

+config DRM_INPUT_HELPER

• def_bool y
  

• depends on DRM_KMS_HELPER
  

• depends on INPUT
  

config DRM_KMS_HELPER tristate depends on DRM

• select DRM_INPUT_HELPER if INPUT
  help
    CRTC helpers for KMS drivers.
  
  

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1c41156deb5f..9a6494aa45e6 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ drm_atomic_state_helper.o drm_damage_helper.o \ drm_format_helper.o drm_self_refresh_helper.o drm_rect.o

+drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o

drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c new file mode 100644 index 000000000000..470f90865c7c --- /dev/null +++ b/drivers/gpu/drm/drm_input_helper.c @@ -0,0 +1,143 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (C) 2021 Google, Inc.
• */

+#include <linux/input.h> +#include <linux/slab.h>

+#include <drm/drm_device.h> +#include <drm/drm_input_helper.h>

+/**

• • DOC: overview
• • This helper library provides a thin wrapper around input handles, so that
• • DRM drivers can easily perform domain-specific actions in response to user
• • activity. e.g., if someone is moving a mouse, we're likely to want to
• • display something soon, and we should exit panel self-refresh.
• */

+static void drm_input_event(struct input_handle *handle, unsigned int type,

•                     unsigned int code, int value)
  

+{

• struct drm_input_handler *handler = handle->handler->private;
  

• handler->callback(handler);
  

+}

+static int drm_input_connect(struct input_handler *handler,

•                      struct input_dev *dev,
  

•                      const struct input_device_id *id)
  

+{

• struct input_handle *handle;
  

• int error;
  

• handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
  

• if (!handle)
  

•         return -ENOMEM;
  

• handle->dev = dev;
  

• handle->handler = handler;
  

• handle->name = "drm-input-helper";
  

• error = input_register_handle(handle);
  

• if (error)
  

•         goto err2;
  

• error = input_open_device(handle);
  

Does this literally open the input device, just like when userspace opens the input device?

How do you know userspace is using this input device at all? If userspace is not using the input device, then DRM should not be opening it either, as it must have no effect on anything.

If you open an input device that userspace does not use, you also cause a power consumption regression, because now the input device itself is active and possibly flooding the kernel with events (e.g. an accelerometer).

...

• if (error)
  

•         goto err1;
  

• return 0;
  

+err1:

• input_unregister_handle(handle);
  

+err2:

• kfree(handle);
  

• return error;
  

+}

+static void drm_input_disconnect(struct input_handle *handle) +{

• input_close_device(handle);
  

• input_unregister_handle(handle);
  

• kfree(handle);
  

+}

+static const struct input_device_id drm_input_ids[] = {

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

•                  INPUT_DEVICE_ID_MATCH_ABSBIT,
  

•         .evbit = { BIT_MASK(EV_ABS) },
  

•         .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] =
  

•                     BIT_MASK(ABS_MT_POSITION_X) |
  

•                     BIT_MASK(ABS_MT_POSITION_Y) },
  

• }, /* multi-touch touchscreen */
  

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

•         .evbit = { BIT_MASK(EV_ABS) },
  

•         .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) }
  

• }, /* stylus or joystick device */
  

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

•         .evbit = { BIT_MASK(EV_KEY) },
  

•         .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) },
  

• }, /* pointer (e.g. trackpad, mouse) */
  

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

•         .evbit = { BIT_MASK(EV_KEY) },
  

•         .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) },
  

• }, /* keyboard */
  

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

•                  INPUT_DEVICE_ID_MATCH_KEYBIT,
  

•         .evbit = { BIT_MASK(EV_KEY) },
  

•         .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) },
  

• }, /* joysticks not caught by ABS_X above */
  

• {
  

•         .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

•                  INPUT_DEVICE_ID_MATCH_KEYBIT,
  

•         .evbit = { BIT_MASK(EV_KEY) },
  

•         .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) },
  

• }, /* gamepad */
  

I don't think this hardcoded policy belongs in the kernel, nor even works.

I believe classifying input devices is not that simple. Spearheading that is libinput which relies on udev tagging the devices with their types, and that is done based on a hwdb maintained by I think the systemd project. Or maybe libinput has its own db nowadays as well, I'm not sure.

Also, joysticks and gamepads are something display servers generally do not open. An application might open some while it's running, but not all the time. Joysticks could be very chatty while opened, game controllers might have accelerometers, etc.

...

• { },
  

+};

+int drm_input_handle_register(struct drm_device *dev,

•                       struct drm_input_handler *handler)
  

+{

• int ret;
  

• if (!handler->callback)
  

•         return -EINVAL;
  

• handler->handler.event = drm_input_event;
  

• handler->handler.connect = drm_input_connect;
  

• handler->handler.disconnect = drm_input_disconnect;
  

• handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s",
  

•                                   dev_name(dev->dev));
  

• if (!handler->handler.name)
  

•         return -ENOMEM;
  

• handler->handler.id_table = drm_input_ids;
  

• handler->handler.private = handler;
  

• ret = input_register_handler(&handler->handler);
  

Yet another problem here is that this completely ignores the concept of physical seats. Of course it does so, because seats are a pure userspace concept.

The kernel VT console already has problems because the kernel has no concept of seats, which means that if there is a second seat defined and a desktop running on it, while the first seat is in the normal VT text mode, then everything typed in the desktop will be delivered to the VT shell as well! (This has a possible workaround in userspace [1], by opening the evdev input devices in some kind of exclusive mode - which is not common practise AFAIK.)

Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler stop getting events?

So, separate physical seats are a thing in userspace. A seat has at least one DRM device for output, and any number of input devices. A user session runs on a seat, and can access the devices on that seat only. This means you can run multiple independent physical seats on the same machine, provided each one has its own gfx card. The seats are configured with udev rules adding ID_SEAT property to the devices.

How will you keep those seats independent, so that activity on one seat does not cause all the other seats to ramp up their gfx cards?

Thanks, pq

[1] https://gitlab.freedesktop.org/wayland/weston/-/issues/434

...

• if (ret)
  

•         goto err;
  

• return 0;
  

+err:

• kfree(handler->handler.name);
  

• return ret;
  

+} +EXPORT_SYMBOL(drm_input_handle_register);

+void drm_input_handle_unregister(struct drm_input_handler *handler) +{

• input_unregister_handler(&handler->handler);
  

• kfree(handler->handler.name);
  

+} +EXPORT_SYMBOL(drm_input_handle_unregister); diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h new file mode 100644 index 000000000000..7904f397b934 --- /dev/null +++ b/include/drm/drm_input_helper.h @@ -0,0 +1,41 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*

• • Copyright (C) 2021 Google, Inc.
• */

+#ifndef __DRM_INPUT_HELPER_H__ +#define __DRM_INPUT_HELPER_H__

+#include <linux/input.h>

+struct drm_device;

+struct drm_input_handler {

• /*
  

•  * Callback to call for input activity. Will be called in an atomic
  

•  * context.
  

•  */
  

• void (*callback)(struct drm_input_handler *handler);
  

• struct input_handler handler;
  

+};

+#if defined(CONFIG_DRM_INPUT_HELPER)

+int drm_input_handle_register(struct drm_device *dev,

•                       struct drm_input_handler *handler);
  

+void drm_input_handle_unregister(struct drm_input_handler *handler);

+#else /* !CONFIG_DRM_INPUT_HELPER */

+static inline int drm_input_handle_register(struct drm_device *dev,

•                                     struct drm_input_handler *handler)
  

+{

• return 0;
  

+}

+static inline void +drm_input_handle_unregister(struct drm_input_handler *handler) {}

+#endif /* CONFIG_DRM_INPUT_HELPER */

+#endif /* __DRM_INPUT_HELPER_H__ */

On Thu, 18 Nov 2021 17:46:10 -0800 Brian Norris briannorris@chromium.org wrote:

Hi Pekka,

Thanks for the thoughts and review. I've tried to respond below:

On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote:

...

On Wed, 17 Nov 2021 14:48:40 -0800 Brian Norris briannorris@chromium.org wrote:

...

A variety of applications have found it useful to listen to user-initiated input events to make decisions within a DRM driver, given that input events are often the first sign that we're going to start doing latency-sensitive activities:

• Panel self-refresh: software-directed self-refresh (e.g., with Rockchip eDP) is especially latency sensitive. In some cases, it can take 10s of milliseconds for a panel to exit self-refresh, which can be noticeable. Rockchip RK3399 Chrome OS systems have always shipped with an input_handler boost, that preemptively exits self-refresh whenever there is input activity.

• GPU drivers: on GPU-accelerated desktop systems, we may need to render new frames immediately after user activity. Powering up the GPU can take enough time that it is worthwhile to start this process as soon as there is input activity. Many Chrome OS systems also ship with an input_handler boost that powers up the GPU.

This patch provides a small helper library that abstracts some of the input-subsystem details around picking which devices to listen to, and some other boilerplate. This will be used in the next patch to implement the first bullet: preemptive exit for panel self-refresh.

Bits of this are adapted from code the Android and/or Chrome OS kernels have been carrying for a while.

Signed-off-by: Brian Norris briannorris@chromium.org

Thanks Simon for the CC.

Hi Brian,

while this feature in general makes sense and sounds good, to start warming up display hardware early when something might start to happen, this particular proposal has many problems from UAPI perspective (as it has none). Comments below.

Btw. if PSR is that slow to wake up from, how much do you actually gain from this input event watching? I would imagine the improvement to not be noticeable.

Patch 2 has details. It's not really about precisely how slow PSR is, but how much foresight we can gain: in patch 2, I note that with my particular user space and system, I can start PSR-exit 50ms earlier than I would otherweise. (FWIW, this measurement is exactly the same it was with the original version written 4 years ago.)

For how long PSR-exit takes: the measurements I'm able to do (via ftrace) show that drm_self_refresh_transition() takes between 35 and 55 ms. That's noticeable at 60 fps. And quite conveniently, the input-boost manages to hide nearly 100% of that latency.

Typical use cases where one notices PSR latency (and where this 35-55ms matters) involve simply moving a cursor; it's very noticeable when you have more than a few frames of latency to "get started".

Hi Brian,

that is very interesting, thanks.

I would never have expected to have userspace take *that* long to react. But, that sounds like it could be just your userspace software stack.

...

I think some numbers about how much this feature helps would be really good, even if they are quite specific use cases. You also need to identify the userspace components, because I think different display servers are very different in their reaction speed.

If my email address isn't obvious, I'm testing Chrome OS. I'm frankly not that familiar with the user space display stack, but for what I know, it's rather custom, developed within the Chromium project. Others on CC here could probably give you more detail, if you want specific answers, besides docs like this:

https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md

...

If KMS gets a pageflip or modeset in no time after an input event, then what's the gain. OTOH, if the display server is locking on to vblank, there might be a delay worth avoiding. But then, is it worth short-circuiting the wake-up in kernel vs. adding a new ioctl that userspace could hit to start the warming up process?

Rob responded to the first part to some extent (there is definitely gain to be had).

To the last part: I wrote a simple debugfs hook to allow user space to force a PSR exit, and then a simple user space program to read input events and smash that debugfs file whenever it sees one. Testing in the same scenarios, this appears to lose less than 100 microseconds versus the in-kernel approach, which is negligible for this use case. (I'm not sure about the other use cases.)

So, this is technically doable in user space.

This is crucial information I would like you to include in some commit message. I think it is very interesting for the reviewers. Maybe also copy that in the cover letter.

In my opinion there is a clear and obvious decision due that measurement: Add the new ioctl for userspace to hit, do not try to hardcode or upload the wake-up policy into the kernel.

I can't speak to the ease of _actually_ integrating this into even our own Chrome display manager, but I highly doubt it will get integrated into others. I'd posit this should weigh into the relative worth, but otherwise can't really give you an answer there.

I think such a thing would be very simple to add to any display server. They already have hooks for things like resetting idle timeout timers on any relevant input event.

I'd also note, software-directed PSR is so far designed to be completely opaque to user space. There's no way to disable it; no way to know it's active; and no way to know anything about the parameters it's computing (like average entry/exit delay). Would you suggest a whole set of new IOCTLs for this?

Just one ioctl on the DRM device: "Hey, wake up!". Because that's what your patch does in-kernel, right?

If there are use case specific parameters, then how did you intend to allow adjusting those in your proposal?

...

...

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile index 1c41156deb5f..9a6494aa45e6 100644 --- a/drivers/gpu/drm/Makefile +++ b/drivers/gpu/drm/Makefile @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \ drm_atomic_state_helper.o drm_damage_helper.o \ drm_format_helper.o drm_self_refresh_helper.o drm_rect.o

+drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o

drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c new file mode 100644 index 000000000000..470f90865c7c --- /dev/null +++ b/drivers/gpu/drm/drm_input_helper.c

...

...

+static int drm_input_connect(struct input_handler *handler,

• 	     struct input_dev *dev,
  

• 	     const struct input_device_id *id)
  

+{

• struct input_handle *handle;
• int error;
• handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
• if (!handle)

• return -ENOMEM;
  

• handle->dev = dev;
• handle->handler = handler;
• handle->name = "drm-input-helper";
• error = input_register_handle(handle);
• if (error)

• goto err2;
  

• error = input_open_device(handle);

Does this literally open the input device, just like when userspace opens the input device?

I believe so. Dmitry mentioned something to this effect on earlier versions, but I found that the input_handler does not operate at all if this specific handle wasn't opened. (All handles are independent, and each over their own |open| count.)

This part is unfortunate, I agree. If we really want this in-kernel, perhaps I could find a way to tweak the input_handler API.

Ok. Sounds like this can have a clear technical solution, and then this issue is solved for good. This might also remove the device type filtering problem.

...

How do you know userspace is using this input device at all? If userspace is not using the input device, then DRM should not be opening it either, as it must have no effect on anything.

If you open an input device that userspace does not use, you also cause a power consumption regression, because now the input device itself is active and possibly flooding the kernel with events (e.g. an accelerometer).

Well, I don't think accelerometers show up as input devices, but I suppose your point could apply to actual input devices.

My understanding is that accelerometers are evdev (input) devices, especially when used as input e.g. for controlling games. I'm not aware of any other interface for it.

Even audio sockets are input devices for detecting whether a plug has been plugged in, but those probably wouldn't flood anything.

...

...

• if (error)

• goto err1;
  

• return 0;

+err1:

• input_unregister_handle(handle);

+err2:

• kfree(handle);
• return error;

+}

+static void drm_input_disconnect(struct input_handle *handle) +{

• input_close_device(handle);
• input_unregister_handle(handle);
• kfree(handle);

+}

+static const struct input_device_id drm_input_ids[] = {

• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_ABSBIT,
  

• .evbit = { BIT_MASK(EV_ABS) },
  

• .absbit = { [BIT_WORD(ABS_MT_POSITION_X)] =
  

• 	    BIT_MASK(ABS_MT_POSITION_X) |
  

• 	    BIT_MASK(ABS_MT_POSITION_Y) },
  

• }, /* multi-touch touchscreen */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_ABS) },
  

• .absbit = { [BIT_WORD(ABS_X)] = BIT_MASK(ABS_X) }
  

• }, /* stylus or joystick device */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(BTN_LEFT)] = BIT_MASK(BTN_LEFT) },
  

• }, /* pointer (e.g. trackpad, mouse) */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(KEY_ESC)] = BIT_MASK(KEY_ESC) },
  

• }, /* keyboard */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_KEYBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = {[BIT_WORD(BTN_JOYSTICK)] = BIT_MASK(BTN_JOYSTICK) },
  

• }, /* joysticks not caught by ABS_X above */
• {

• .flags = INPUT_DEVICE_ID_MATCH_EVBIT |
  

• 	 INPUT_DEVICE_ID_MATCH_KEYBIT,
  

• .evbit = { BIT_MASK(EV_KEY) },
  

• .keybit = { [BIT_WORD(BTN_GAMEPAD)] = BIT_MASK(BTN_GAMEPAD) },
  

• }, /* gamepad */

I don't think this hardcoded policy belongs in the kernel, nor even works.

Define "works"?

Makes the right decision in at least all those cases where current desktop userspace (udev + hwdb + libinput) already makes the correct decision. From what I've seen, it looks like end users come with bug reports every now and then when some hardware manufacturer was lazy or inventive with their HID descriptors.

It's shipping in various forms on a variety of Android and Chrome OS systems, where it has a noticeable performance benefit, and isn't known to have significant power-consumption issues.

Peter Hutterer could probably say more, I confess I am quite pessimistic.

...

I believe classifying input devices is not that simple. Spearheading that is libinput which relies on udev tagging the devices with their types, and that is done based on a hwdb maintained by I think the systemd project. Or maybe libinput has its own db nowadays as well, I'm not sure.

I'm not that familiar with libinput, etc., but I expect most of what it needs to do is irrelevant to these kinds of use cases. We don't care at all about what character sets or even what type of device is in use, in most cases. As long as it could reasonably be called user input, it's good enough.

Also, for most use cases here, the penalty for small inaccuracies is small. Especially for something like panel self-refresh, we'd rather not have it enabled at all, than have it performing poorly.

This problem will diminish once your patches stop literally opening the input devices and listens only on input devices that are actually opened by userspace. When that happens, I'm not sure you even need this device type filtering at all. The remaining problem is the seat designation.

...

Also, joysticks and gamepads are something display servers generally do not open. An application might open some while it's running, but not all the time. Joysticks could be very chatty while opened, game controllers might have accelerometers, etc.

...

• { },

+};

+int drm_input_handle_register(struct drm_device *dev,

• 	      struct drm_input_handler *handler)
  

+{

• int ret;
• if (!handler->callback)

• return -EINVAL;
  

• handler->handler.event = drm_input_event;
• handler->handler.connect = drm_input_connect;
• handler->handler.disconnect = drm_input_disconnect;
• handler->handler.name = kasprintf(GFP_KERNEL, "drm-input-helper-%s",

• 			  dev_name(dev->dev));
  

• if (!handler->handler.name)

• return -ENOMEM;
  

• handler->handler.id_table = drm_input_ids;
• handler->handler.private = handler;
• ret = input_register_handler(&handler->handler);

Yet another problem here is that this completely ignores the concept of physical seats. Of course it does so, because seats are a pure userspace concept.

The kernel VT console already has problems because the kernel has no concept of seats, which means that if there is a second seat defined and a desktop running on it, while the first seat is in the normal VT text mode, then everything typed in the desktop will be delivered to the VT shell as well! (This has a possible workaround in userspace [1], by opening the evdev input devices in some kind of exclusive mode - which is not common practise AFAIK.)

Sure.

I'd bet the intersection of systems that use SW-directed PSR and "multi-seat" is negligibly close to zero, but I can't guarantee that. Chalk one up for a user space policy.

Your cover letter has also the other bullet point: ramping up GPUs. That applies to a lot more systems than PSR, right?

Maybe that is an acceptable trade-off: be 100 µs faster (your measurement) by ramping up all GPUs in a system instead of only the relevant ones?

Or maybe that will hurt normal gaming computers by ramping up the iGPU when the OS and game only uses the dGPU, which makes iGPU eat away the CPU power budget, causing the CPU to slow down? I suppose that would be handled by ramping up only GPUs that userspace has opened.

...

Btw. if userspace does use EVIOCGRAB, then will your in-kernel handler stop getting events?

I believe so.

I suppose you would not want that?

The solution to the VT console problem is for userspace to start using EVIOCGRAB and that could regress the warm-up machinery.

In summary, there are all these open questions and annoying little problems, and none of these issues would exist if userspace would drive the warm-up explicitly.

Thanks, pq

On Wed, Nov 17, 2021 at 02:48:41PM -0800, Brian Norris wrote:

To improve panel self-refresh exit latency, we speculatively start exiting when we receive input events. Occasionally, this may lead to false positives, but most of the time we get a head start on coming out of PSR. Depending on how userspace takes to produce a new frame in response to the event, this can completely hide the exit latency.

In local tests on Chrome OS (Rockchip RK3399 eDP), we've found that the input notifier gives us about a 50ms head start over the fb-update-initiated exit.

Leverage a new drm_input_helper library to get easy access to likely-relevant input event callbacks.

Inspired-by: Kristian H. Kristensen hoegsberg@google.com Signed-off-by: Brian Norris briannorris@chromium.org

---

This was in part picked up from:

https://lore.kernel.org/all/20180405095000.9756-25-enric.balletbo@collabora.... [PATCH v6 24/30] drm/rockchip: Disable PSR on input events

with significant rewrites/reworks:

• moved to common drm_input_helper and drm_self_refresh_helper implementation
• track state only through crtc->state->self_refresh_active

Note that I'm relatively unfamiliar with DRM locking expectations, but I believe access to drm_crtc->state (which helps us track redundant transitions) is OK under the locking provided by drm_atomic_get_crtc_state().

Changes in v2:

• Delay PSR re-entry, when already disabled
• Allow default configuration via Kconfig and modparam
• Replace void* with container_of()

Please include this kind of context in the commit message so it's not lost. It all looks like relevant information (yes other subsystems insist the patch changelog is outside of the commit message, ime more is lost information than gained in clarity with that).

drivers/gpu/drm/Kconfig | 16 ++++ drivers/gpu/drm/drm_self_refresh_helper.c | 98 +++++++++++++++++++---- 2 files changed, 100 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig index 381476b10a9d..698924ed9b6b 100644 --- a/drivers/gpu/drm/Kconfig +++ b/drivers/gpu/drm/Kconfig @@ -84,6 +84,22 @@ config DRM_INPUT_HELPER depends on DRM_KMS_HELPER depends on INPUT

+config DRM_SELF_REFRESH_INPUT_BOOST_DEFAULT

• bool "Preemptively exit panel self-refresh on input device activity" if EXPERT
• default y
• depends on DRM_INPUT_HELPER

Yeah no that doesn't work. First I really don't like tunables, and second much less as Kconfig.

The driver should know this somehow and set it correctly. Maybe auto-derived from panel timings + knowledge of how long a sr exit takes?

• help

•  Allows the generic DRM panel self-refresh helpers to factor in user
  

•  input activity to preemptively exit panel self-refresh, in order to
  

•  reduce potentially-visible latency when displaying new display
  

•  content. This is an optimization which often will do the right thing,
  

•  but can be disabled for experimentation or similar.
  

•  Saying Y enables the feature by default; this can also be configured
  

•  by module parameter, drm_kms_helper.self_refresh_input_boost.
  

•  If in doubt, say "Y".
  

config DRM_KMS_HELPER tristate depends on DRM diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c index dd33fec5aabd..ba4881e683b7 100644 --- a/drivers/gpu/drm/drm_self_refresh_helper.c +++ b/drivers/gpu/drm/drm_self_refresh_helper.c @@ -8,6 +8,7 @@ #include <linux/average.h> #include <linux/bitops.h> #include <linux/slab.h> +#include <linux/stringify.h> #include <linux/workqueue.h>

#include <drm/drm_atomic.h> @@ -15,6 +16,7 @@ #include <drm/drm_connector.h> #include <drm/drm_crtc.h> #include <drm/drm_device.h> +#include <drm/drm_input_helper.h> #include <drm/drm_mode_config.h> #include <drm/drm_modeset_lock.h> #include <drm/drm_print.h> @@ -58,17 +60,41 @@ DECLARE_EWMA(psr_time, 4, 4) struct drm_self_refresh_data { struct drm_crtc *crtc; struct delayed_work entry_work;

• struct work_struct exit_work;

• struct drm_input_handler input_handler;

• bool input_handler_registered;

  struct mutex avg_mutex; struct ewma_psr_time entry_avg_ms; struct ewma_psr_time exit_avg_ms;

};

-static void drm_self_refresh_helper_entry_work(struct work_struct *work) +static bool self_refresh_input_boost =

• IS_ENABLED(CONFIG_DRM_SELF_REFRESH_INPUT_BOOST_DEFAULT);

+#if defined(CONFIG_DRM_INPUT_HELPER) +module_param(self_refresh_input_boost, bool, 0644); +MODULE_PARM_DESC(self_refresh_input_boost,

•  "Enable panel self-refresh input boost [default="
  

•  __stringify(CONFIG_DRM_SELF_REFRESH_INPUT_BOOST_DEFAULT) "]");
  

+#endif /* CONFIG_DRM_INPUT_HELPER */

+static void drm_self_refresh_reschedule(struct drm_self_refresh_data *sr_data) +{

• unsigned int delay;
• mutex_lock(&sr_data->avg_mutex);
• delay = (ewma_psr_time_read(&sr_data->entry_avg_ms) +

•  ewma_psr_time_read(&sr_data->exit_avg_ms)) * 2;
  

• mutex_unlock(&sr_data->avg_mutex);
• mod_delayed_work(system_wq, &sr_data->entry_work,

• 	 msecs_to_jiffies(delay));
  

+}

+static void drm_self_refresh_transition(struct drm_self_refresh_data *sr_data,

• 			bool enable)
  

{

• struct drm_self_refresh_data *sr_data = container_of(

• 		to_delayed_work(work),
  

• 		struct drm_self_refresh_data, entry_work);
  

  struct drm_crtc *crtc = sr_data->crtc; struct drm_device *dev = crtc->dev; struct drm_modeset_acquire_ctx ctx;

@@ -95,6 +121,14 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) goto out; }

• if (crtc->state->self_refresh_active == enable) {

• /* Exiting SR; delay re-entry for at least one more cycle. */
  

• if (!enable)
  

• 	drm_self_refresh_reschedule(sr_data);
  

• goto out;
  

• }
• if (!crtc_state->enable) goto out;

@@ -107,8 +141,8 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) goto out; }

• crtc_state->active = false;
• crtc_state->self_refresh_active = true;

• crtc_state->active = !enable;

• crtc_state->self_refresh_active = enable;

  ret = drm_atomic_commit(state); if (ret)

@@ -129,6 +163,15 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work) drm_modeset_acquire_fini(&ctx); }

+static void drm_self_refresh_helper_entry_work(struct work_struct *work) +{

• struct drm_self_refresh_data *sr_data = container_of(

• 		to_delayed_work(work),
  

• 		struct drm_self_refresh_data, entry_work);
  

• drm_self_refresh_transition(sr_data, true);

+}

/**

• drm_self_refresh_helper_update_avg_times - Updates a crtc's SR time averages
• @state: the state which has just been applied to hardware

@@ -202,7 +245,6 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)

for_each_new_crtc_in_state(state, crtc, crtc_state, i) { struct drm_self_refresh_data *sr_data;

• unsigned int delay;
  
  

  /* Don't trigger the entry timer when we're already in SR */ if (crtc_state->self_refresh_active)

@@ -212,17 +254,26 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state) if (!sr_data) continue;

• mutex_lock(&sr_data->avg_mutex);
  

• delay = (ewma_psr_time_read(&sr_data->entry_avg_ms) +
  

• 	 ewma_psr_time_read(&sr_data->exit_avg_ms)) * 2;
  

• mutex_unlock(&sr_data->avg_mutex);
  

• mod_delayed_work(system_wq, &sr_data->entry_work,
  

• 		 msecs_to_jiffies(delay));
  

• drm_self_refresh_reschedule(sr_data);
  

  }

} EXPORT_SYMBOL(drm_self_refresh_helper_alter_state);

+static void drm_self_refresh_helper_exit_work(struct work_struct *work) +{

• struct drm_self_refresh_data *sr_data = container_of(

• 	work, struct drm_self_refresh_data, exit_work);
  

• drm_self_refresh_transition(sr_data, false);

+}

+static void drm_self_refresh_input_event(struct drm_input_handler *handler) +{

• struct drm_self_refresh_data *sr_data = container_of(

• 	handler, struct drm_self_refresh_data, input_handler);
  

• schedule_work(&sr_data->exit_work);

Single worker with single state would also make it clear that we need to cancel the sr enter work here (or delay it at least), otherwise this wont' do much good when you try to exit right before we try to enter sr.

Also, exiting sr means a full atomic flip, so full frame upload generally, and that can take its sweet time. Won't that increase input -> output latency because we're guaranteed to miss a frame (at least on some hardware). How does this work? This might also tie into the "how to correctly tune this" question.

+} /**

• drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
• @crtc: the crtc which supports self refresh supported displays

@@ -232,6 +283,7 @@ EXPORT_SYMBOL(drm_self_refresh_helper_alter_state); int drm_self_refresh_helper_init(struct drm_crtc *crtc) { struct drm_self_refresh_data *sr_data = crtc->self_refresh_data;

• int ret;

  /* Helper is already initialized */ if (WARN_ON(sr_data))

@@ -243,6 +295,7 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc)

INIT_DELAYED_WORK(&sr_data->entry_work, drm_self_refresh_helper_entry_work);

• INIT_WORK(&sr_data->exit_work, drm_self_refresh_helper_exit_work);

Won't this result in all kinds of awkward synchronization issues when the two workers run in parallel?

I was expecting one worker here, and maybe some irqsafe spinlock protect state to drive state changes.

sr_data->crtc = crtc; mutex_init(&sr_data->avg_mutex); ewma_psr_time_init(&sr_data->entry_avg_ms); @@ -256,8 +309,22 @@ int drm_self_refresh_helper_init(struct drm_crtc *crtc) ewma_psr_time_add(&sr_data->entry_avg_ms, SELF_REFRESH_AVG_SEED_MS); ewma_psr_time_add(&sr_data->exit_avg_ms, SELF_REFRESH_AVG_SEED_MS);

• if (self_refresh_input_boost) {

• sr_data->input_handler.callback = drm_self_refresh_input_event;
  

• ret = drm_input_handle_register(crtc->dev,
  

• 				&sr_data->input_handler);
  

• if (ret)
  

• 	goto err;
  

• sr_data->input_handler_registered = true;
  

• }
• crtc->self_refresh_data = sr_data;
• return 0;

+err:

• kfree(sr_data);
• return ret;

} EXPORT_SYMBOL(drm_self_refresh_helper_init);

@@ -275,7 +342,10 @@ void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc)

crtc->self_refresh_data = NULL;

• if (sr_data->input_handler_registered)

Could we not push this check into the helper?

• drm_input_handle_unregister(&sr_data->input_handler);
  

  cancel_delayed_work_sync(&sr_data->entry_work);
• cancel_work_sync(&sr_data->exit_work); kfree(sr_data);

} EXPORT_SYMBOL(drm_self_refresh_helper_cleanup); -- 2.34.0.rc1.387.gb447b232ab-goog

Anyway by&large definitely something we want to support in upstream and sr helpers. -Daniel