[PATCH 0/2] drm: Support input-boosted panel self-refresh exit

[PATCH 0/2] drm: Support input-boosted panel self-refresh exit

[Brian Norris]

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.

I implement the first bullet in this series, and I also compared with
some out-of-tree patches for the second, to ensure this could be useful
there too.

Past work on upstreaming a variety of Chromebook display patches got
held up for this particular feature, as there was some desire to make it
a bit more generic, for one. See the latest here:

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

I significantly rewrote this to adapt it to the new common
drm_self_refresh_helpers and to add a new drm_input_helper thin library,
so I only carry my own authorship on this series.

Admittedly, this "drm_input_helper" library is barely DRM-specific at
all, except that all display- and GPU-related input-watchers are likely
to want to watch similar device behavior (unlike, say, rfkill or led
input_handler code). If we find other applications need the same
behavior outside of drivers/gpu/drm/, maybe this could find a home
elsewhere.


Brian Norris (2):
  drm/input_helper: Add new input-handling helper
  drm/self_refresh: Disable self-refresh on input events

 drivers/gpu/drm/Makefile                  |   3 +-
 drivers/gpu/drm/drm_input_helper.c        | 143 ++++++++++++++++++++++
 drivers/gpu/drm/drm_self_refresh_helper.c |  54 +++++++-
 include/drm/drm_input_helper.h            |  22 ++++
 4 files changed, 215 insertions(+), 7 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_input_helper.c
 create mode 100644 include/drm/drm_input_helper.h

-- 
2.34.0.rc0.344.g81b53c2807-goog

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

[Brian Norris]

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>
---

 drivers/gpu/drm/Makefile           |   3 +-
 drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++
 include/drm/drm_input_helper.h     |  22 +++++
 3 files changed, 167 insertions(+), 1 deletion(-)
 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/Makefile b/drivers/gpu/drm/Makefile
index 0dff40bb863c..378761685b47 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -49,7 +49,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
 		drm_scdc_helper.o drm_gem_atomic_helper.o \
 		drm_gem_framebuffer_helper.o \
 		drm_atomic_state_helper.o drm_damage_helper.o \
-		drm_format_helper.o drm_self_refresh_helper.o
+		drm_format_helper.o drm_self_refresh_helper.o \
+		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
diff --git a/drivers/gpu/drm/drm_input_helper.c b/drivers/gpu/drm/drm_input_helper.c
new file mode 100644
index 000000000000..a31efc0d3030
--- /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->priv);
+}
+
+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..b4d68896830c
--- /dev/null
+++ b/include/drm/drm_input_helper.h
@@ -0,0 +1,22 @@
+/* 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 {
+	struct input_handler handler;
+	void *priv;
+	void (*callback)(void *priv);
+};
+
+int drm_input_handle_register(struct drm_device *dev,
+			      struct drm_input_handler *handler);
+void drm_input_handle_unregister(struct drm_input_handler *handler);
+
+#endif /* __DRM_INPUT_HELPER_H__ */
-- 
2.34.0.rc0.344.g81b53c2807-goog

[PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Brian Norris]

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.com/
  [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().

 drivers/gpu/drm/drm_self_refresh_helper.c | 54 ++++++++++++++++++++---
 1 file changed, 48 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c
index dd33fec5aabd..dcab061cc90a 100644
--- a/drivers/gpu/drm/drm_self_refresh_helper.c
+++ b/drivers/gpu/drm/drm_self_refresh_helper.c
@@ -15,6 +15,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 +59,17 @@ 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;
 
 	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 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 +96,9 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work)
 		goto out;
 	}
 
+	if (crtc->state->self_refresh_active == enable)
+		goto out;
+
 	if (!crtc_state->enable)
 		goto out;
 
@@ -107,8 +111,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 +133,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
@@ -223,6 +236,20 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)
 }
 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(void *data)
+{
+	struct drm_self_refresh_data *sr_data = data;
+
+	schedule_work(&sr_data->exit_work);
+}
 /**
  * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
  * @crtc: the crtc which supports self refresh supported displays
@@ -232,6 +259,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 +271,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);
 	sr_data->crtc = crtc;
 	mutex_init(&sr_data->avg_mutex);
 	ewma_psr_time_init(&sr_data->entry_avg_ms);
@@ -256,8 +285,19 @@ 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);
 
+	sr_data->input_handler.callback = drm_self_refresh_input_event;
+	sr_data->input_handler.priv = sr_data;
+	ret = drm_input_handle_register(crtc->dev, &sr_data->input_handler);
+	if (ret)
+		goto err;
+
 	crtc->self_refresh_data = sr_data;
+
 	return 0;
+
+err:
+	kfree(sr_data);
+	return ret;
 }
 EXPORT_SYMBOL(drm_self_refresh_helper_init);
 
@@ -275,7 +315,9 @@ void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc)
 
 	crtc->self_refresh_data = NULL;
 
+	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.rc0.344.g81b53c2807-goog

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3521 bytes --]

Hi Brian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.15 next-20211104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: openrisc-randconfig-c004-20211104 (attached as .config)
compiler: or1k-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
        git checkout 57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=openrisc SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   or1k-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_disconnect':
>> drm_input_helper.c:(.text+0x48): undefined reference to `input_close_device'
   drm_input_helper.c:(.text+0x48): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_close_device'
>> or1k-linux-ld: drm_input_helper.c:(.text+0x50): undefined reference to `input_unregister_handle'
   drm_input_helper.c:(.text+0x50): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_unregister_handle'
   or1k-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_connect':
>> drm_input_helper.c:(.text+0xd0): undefined reference to `input_register_handle'
   drm_input_helper.c:(.text+0xd0): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_register_handle'
>> or1k-linux-ld: drm_input_helper.c:(.text+0xe4): undefined reference to `input_open_device'
   drm_input_helper.c:(.text+0xe4): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_open_device'
   or1k-linux-ld: drm_input_helper.c:(.text+0xf8): undefined reference to `input_unregister_handle'
   drm_input_helper.c:(.text+0xf8): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_unregister_handle'
   or1k-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_handle_register':
>> drm_input_helper.c:(.text+0x1d4): undefined reference to `input_register_handler'
   drm_input_helper.c:(.text+0x1d4): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_register_handler'
   or1k-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_handle_unregister':
>> drm_input_helper.c:(.text+0x234): undefined reference to `input_unregister_handler'
   drm_input_helper.c:(.text+0x234): relocation truncated to fit: R_OR1K_INSN_REL_26 against undefined symbol `input_unregister_handler'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 23714 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 2276 bytes --]

Hi Brian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.15 next-20211104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: arm-randconfig-r001-20211104 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 847a6807332b13f43704327c2d30103ec0347c77)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm cross compiling tool for clang build
        # apt-get install binutils-arm-linux-gnueabi
        # https://github.com/0day-ci/linux/commit/57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
        git checkout 57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=arm 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>, old ones prefixed by <<):

>> ERROR: modpost: "input_unregister_handler" [drivers/gpu/drm/drm_kms_helper.ko] undefined!
>> ERROR: modpost: "input_close_device" [drivers/gpu/drm/drm_kms_helper.ko] undefined!
>> ERROR: modpost: "input_unregister_handle" [drivers/gpu/drm/drm_kms_helper.ko] undefined!
>> ERROR: modpost: "input_open_device" [drivers/gpu/drm/drm_kms_helper.ko] undefined!
>> ERROR: modpost: "input_register_handle" [drivers/gpu/drm/drm_kms_helper.ko] undefined!
>> ERROR: modpost: "input_register_handler" [drivers/gpu/drm/drm_kms_helper.ko] undefined!

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32458 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3380 bytes --]

Hi Brian,

I love your patch! Yet something to improve:

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v5.15 next-20211104]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
base:   git://anongit.freedesktop.org/drm/drm drm-next
config: nios2-randconfig-r012-20211104 (attached as .config)
compiler: nios2-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Brian-Norris/drm-Support-input-boosted-panel-self-refresh-exit/20211104-074200
        git checkout 57dffacf2caa97c6d4c205ff6d43ffaa200ab54a
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=nios2 SHELL=/bin/bash

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   nios2-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_handle_register':
   drm_input_helper.c:(.text+0xa4): undefined reference to `input_register_handler'
   drm_input_helper.c:(.text+0xa4): relocation truncated to fit: R_NIOS2_CALL26 against `input_register_handler'
   nios2-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_disconnect':
   drm_input_helper.c:(.text+0xf0): undefined reference to `input_close_device'
   drm_input_helper.c:(.text+0xf0): relocation truncated to fit: R_NIOS2_CALL26 against `input_close_device'
>> nios2-linux-ld: drm_input_helper.c:(.text+0xf8): undefined reference to `input_unregister_handle'
   drm_input_helper.c:(.text+0xf8): relocation truncated to fit: R_NIOS2_CALL26 against `input_unregister_handle'
   nios2-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_handle_unregister':
   drm_input_helper.c:(.text+0x124): undefined reference to `input_unregister_handler'
   drm_input_helper.c:(.text+0x124): relocation truncated to fit: R_NIOS2_CALL26 against `input_unregister_handler'
   nios2-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_connect':
   drm_input_helper.c:(.text+0x18c): undefined reference to `input_register_handle'
   drm_input_helper.c:(.text+0x18c): relocation truncated to fit: R_NIOS2_CALL26 against `input_register_handle'
>> nios2-linux-ld: drm_input_helper.c:(.text+0x19c): undefined reference to `input_open_device'
   drm_input_helper.c:(.text+0x19c): relocation truncated to fit: R_NIOS2_CALL26 against `input_open_device'
   nios2-linux-ld: drm_input_helper.c:(.text+0x1ac): undefined reference to `input_unregister_handle'
   drm_input_helper.c:(.text+0x1ac): relocation truncated to fit: R_NIOS2_CALL26 against `input_unregister_handle'

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35866 bytes --]

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

[Brian Norris]

On Thu, Nov 04, 2021 at 06:47:34PM +0800, kernel test robot wrote:
> Hi Brian,

Hi robot!

> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>

I'm not sure how to do that in a v2 patch -- one doesn't normally credit
"Reported-by" for every testing/review improvement on an in-development
patch series.

> All errors (new ones prefixed by >>):
> 
>    or1k-linux-ld: drivers/gpu/drm/drm_input_helper.o: in function `drm_input_disconnect':
> >> drm_input_helper.c:(.text+0x48): undefined reference to `input_close_device'

I've cooked a local change to separate the CONFIG_INPUT dependencies
under another Kconfig symbol which optionally builds this stuff into the
drm helper module, only when CONFIG_INPUT is present. That should fix
the build issues while still leaving DRM_KMS_HELPER workable with or
without CONFIG_INPUT.

I'll wait to see if there's any other feedback before spinning though.

Brian

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

[Dmitry Torokhov]

On Wed, Nov 03, 2021 at 04:40:17PM -0700, 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.

If this is acceptable to DRM folks I am OK with this from input POV. The
only think I would call out that the callback is executed in atomic
context with interrupts disabled.

Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Doug Anderson]

Hi,

On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris@chromium.org> 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.

So IMO this is a really useful thing and I'm in support of it landing.
It's not much code and it clearly gives a big benefit. However, I
would request a CONFIG option to control this so that if someone
really finds some use case where it isn't needed or if they find a
good way to do this in userspace without latency problems then they
can turn it off. Does that sound reasonable?


> 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.com/
>   [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().

Yeah, I'm no expert here either. I gave a review a shot anyway since
it's been all quiet, but adult supervision is probably required...

I can believe that you are safe from corrupting things, but I think
you still have locking problems, don't you? What about this:

1. PSR is _not_ active but we're 1 microsecond away from entering PSR

2. Input event comes through.

3. Start executing drm_self_refresh_transition(false).

4. PSR timer expires and starts executing drm_self_refresh_transition(true).

5. Input event "wins the race" but sees that PSR is already disabled => noop

6. PSR timer gets the lock now. Starts PSR transition.

Wouldn't it be better to cancel / reschedule any PSR entry as soon as
you see the input event?


-Doug

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

[Doug Anderson]

Hi,

On Wed, Nov 3, 2021 at 4:40 PM 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>
> ---
>
>  drivers/gpu/drm/Makefile           |   3 +-
>  drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++
>  include/drm/drm_input_helper.h     |  22 +++++
>  3 files changed, 167 insertions(+), 1 deletion(-)
>  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/Makefile b/drivers/gpu/drm/Makefile
> index 0dff40bb863c..378761685b47 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -49,7 +49,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
>                 drm_scdc_helper.o drm_gem_atomic_helper.o \
>                 drm_gem_framebuffer_helper.o \
>                 drm_atomic_state_helper.o drm_damage_helper.o \
> -               drm_format_helper.o drm_self_refresh_helper.o
> +               drm_format_helper.o drm_self_refresh_helper.o \
> +               drm_input_helper.o

Note that the Makefile part conflicts with drm-misc-next right now.
It's not very hard to resolve, but since this would likely land there
maybe it'd be nice to resolve?

Other than that, this seems sane to me and much better than copying
this between places, so assuming that the build problems get resolved
then I'm happy.

I guess one random thought I had is whether there would be an
appropriate place to put this that _wasn't_ in DRM. I still wonder
whether we'll ever try to upstream something like the cpufreq boost
driver that we're carrying around and using in Chrome OS. If so, it
would want to use these same helpers and it'd be pretty awkward for it
to have to reach into DRM. ...any chance we could just land these
helpers somewhere more generic?


> +struct drm_input_handler {
> +       struct input_handler handler;
> +       void *priv;
> +       void (*callback)(void *priv);
> +};

Super bike-sheddy comment is that "void *" data arguments are not
super common in the kernel. I would have expected the callback
function to just be passed a pointer to the "struct drm_input_handler"
and it could find any other data it needed via "container_of". I won't
insist though. ;-)


-Doug

Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Brian Norris]

Hi Doug,

On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
> On Wed, Nov 3, 2021 at 4:40 PM Brian Norris <briannorris@chromium.org> wrote:
...
> > Leverage a new drm_input_helper library to get easy access to
> > likely-relevant input event callbacks.
>
> So IMO this is a really useful thing and I'm in support of it landing.
> It's not much code and it clearly gives a big benefit. However, I
> would request a CONFIG option to control this so that if someone
> really finds some use case where it isn't needed or if they find a
> good way to do this in userspace without latency problems then they
> can turn it off. Does that sound reasonable?

Sure, I think so. This feature is unfortunately on the borderline of
"policy" (which we normally avoid baking into the kernel), so having
some control over it is probably a good idea -- e.g., module
parameter, CONFIG_*, or both.

I suppose that would make sense to be a "self_refresh"-level control,
and not a "drm_input_helper"-level control? Because different
applications (PSR, GPU boost, etc.) may have different characteristics
and reasons for leveraging this or not.

> > 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.com/
> >   [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().
>
> Yeah, I'm no expert here either. I gave a review a shot anyway since
> it's been all quiet, but adult supervision is probably required...

Thanks ;)

> I can believe that you are safe from corrupting things, but I think
> you still have locking problems, don't you? What about this:
>
> 1. PSR is _not_ active but we're 1 microsecond away from entering PSR
>
> 2. Input event comes through.
>
> 3. Start executing drm_self_refresh_transition(false).
>
> 4. PSR timer expires and starts executing drm_self_refresh_transition(true).
>
> 5. Input event "wins the race" but sees that PSR is already disabled => noop
>
> 6. PSR timer gets the lock now. Starts PSR transition.
>
> Wouldn't it be better to cancel / reschedule any PSR entry as soon as
> you see the input event?

I did think about that option (calling mod_timer to delay the next PSR
entry), but thought it was a bit excessive, at least in terms of
calling it a "race" -- the race between steps #5 and #6 are
essentially equivalent to the natural (unsolvable) race between #1 and
#2 (we can't really read the future about input events).

But rereading your explanation and thinking again, I see that you're
pointing out less of a "race" in the traditional sense, and more of a
missing part of this feature: I think what you're really saying is
that input events should not only exit PSR, but they should delay PSR
(re)entry for some time. With my current patch, input events only
enforce any delay time window if we were already in PSR.

I'll try to factor that into the next version. Thanks!

Brian

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

[Brian Norris]

On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:

(Snip other comments; they seem reasonable, and I'll factor them into
the next version)

> I guess one random thought I had is whether there would be an
> appropriate place to put this that _wasn't_ in DRM. I still wonder
> whether we'll ever try to upstream something like the cpufreq boost
> driver that we're carrying around and using in Chrome OS. If so, it
> would want to use these same helpers and it'd be pretty awkward for it
> to have to reach into DRM. ...any chance we could just land these
> helpers somewhere more generic?

Yeah, I was torn on what to do here as well. I'd rather land something
than nothing, and when reading past conversations, it sounded like
Dmitry didn't want this kind of thing in drivers/input/ [1]. I'd love
to be wrong here though.

I'm not sure where else this would belong though -- either in the
producing subsystem (input) or the consuming one(s) (drm, cpufreq). We
could make up some odd middle ground I suppose (lib/?), but that seems
pretty artificial.

I guess one question is, what is this abstracting, and is that
abstraction actually a shared need for multiple subsystems? I think
the abstraction is, "impending user activity; <component X> should
prepare itself". That general need is exactly the same for the cases
I'm aware of. And if there is any tuning needed (e.g., ignore input
device Y; or turn the whole thing off, because we're ignoring input
for now), that would also seem to be a shared need.

Anyway, back to my first paragraph: I'll plan on keeping this as-is
(as a DRM helper) unless I hear otherwise from input folks.

Brian

[1] https://lore.kernel.org/all/20180416174117.GA77055@dtor-ws/

Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Daniel Vetter]

On Thu, Nov 4, 2021 at 12:40 AM Brian Norris <briannorris@chromium.org> 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>

Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl
should pick this up, you have all the maintainers but not the list.
-Daniel

> ---
> This was in part picked up from:
>
>   https://lore.kernel.org/all/20180405095000.9756-25-enric.balletbo@collabora.com/
>   [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().
>
>  drivers/gpu/drm/drm_self_refresh_helper.c | 54 ++++++++++++++++++++---
>  1 file changed, 48 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_self_refresh_helper.c b/drivers/gpu/drm/drm_self_refresh_helper.c
> index dd33fec5aabd..dcab061cc90a 100644
> --- a/drivers/gpu/drm/drm_self_refresh_helper.c
> +++ b/drivers/gpu/drm/drm_self_refresh_helper.c
> @@ -15,6 +15,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 +59,17 @@ 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;
>
>         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 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 +96,9 @@ static void drm_self_refresh_helper_entry_work(struct work_struct *work)
>                 goto out;
>         }
>
> +       if (crtc->state->self_refresh_active == enable)
> +               goto out;
> +
>         if (!crtc_state->enable)
>                 goto out;
>
> @@ -107,8 +111,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 +133,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
> @@ -223,6 +236,20 @@ void drm_self_refresh_helper_alter_state(struct drm_atomic_state *state)
>  }
>  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(void *data)
> +{
> +       struct drm_self_refresh_data *sr_data = data;
> +
> +       schedule_work(&sr_data->exit_work);
> +}
>  /**
>   * drm_self_refresh_helper_init - Initializes self refresh helpers for a crtc
>   * @crtc: the crtc which supports self refresh supported displays
> @@ -232,6 +259,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 +271,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);
>         sr_data->crtc = crtc;
>         mutex_init(&sr_data->avg_mutex);
>         ewma_psr_time_init(&sr_data->entry_avg_ms);
> @@ -256,8 +285,19 @@ 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);
>
> +       sr_data->input_handler.callback = drm_self_refresh_input_event;
> +       sr_data->input_handler.priv = sr_data;
> +       ret = drm_input_handle_register(crtc->dev, &sr_data->input_handler);
> +       if (ret)
> +               goto err;
> +
>         crtc->self_refresh_data = sr_data;
> +
>         return 0;
> +
> +err:
> +       kfree(sr_data);
> +       return ret;
>  }
>  EXPORT_SYMBOL(drm_self_refresh_helper_init);
>
> @@ -275,7 +315,9 @@ void drm_self_refresh_helper_cleanup(struct drm_crtc *crtc)
>
>         crtc->self_refresh_data = NULL;
>
> +       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.rc0.344.g81b53c2807-goog
>


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

Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Brian Norris]

On Wed, Nov 17, 2021 at 11:12 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl
> should pick this up, you have all the maintainers but not the list.

Oops, I don't know how that happened. I guess I sometimes have to trim
get_maintainer output, since it likes to hoover up a bunch of
barely-relevant previous committers. I must have been too aggressive.

I'll plan on sending v2 to dri-devel, but let me know (privately if
you'd like) if you'd prefer a pure RESEND of v1.

Brian

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

[Dmitry Torokhov]

On Wed, Nov 17, 2021 at 10:38:58AM -0800, Brian Norris wrote:
> On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
> 
> (Snip other comments; they seem reasonable, and I'll factor them into
> the next version)
> 
> > I guess one random thought I had is whether there would be an
> > appropriate place to put this that _wasn't_ in DRM. I still wonder
> > whether we'll ever try to upstream something like the cpufreq boost
> > driver that we're carrying around and using in Chrome OS. If so, it
> > would want to use these same helpers and it'd be pretty awkward for it
> > to have to reach into DRM. ...any chance we could just land these
> > helpers somewhere more generic?
> 
> Yeah, I was torn on what to do here as well. I'd rather land something
> than nothing, and when reading past conversations, it sounded like
> Dmitry didn't want this kind of thing in drivers/input/ [1]. I'd love
> to be wrong here though.

I simply feel that input_handler is already a very simple abstraction
and trying to specialize it to simplify users further is not productive.

> 
> I'm not sure where else this would belong though -- either in the
> producing subsystem (input) or the consuming one(s) (drm, cpufreq). We
> could make up some odd middle ground I suppose (lib/?), but that seems
> pretty artificial.
> 
> I guess one question is, what is this abstracting, and is that
> abstraction actually a shared need for multiple subsystems? I think
> the abstraction is, "impending user activity; <component X> should
> prepare itself". That general need is exactly the same for the cases
> I'm aware of. And if there is any tuning needed (e.g., ignore input
> device Y; or turn the whole thing off, because we're ignoring input
> for now), that would also seem to be a shared need.
> 
> Anyway, back to my first paragraph: I'll plan on keeping this as-is
> (as a DRM helper) unless I hear otherwise from input folks.
> 
> Brian
> 
> [1] https://lore.kernel.org/all/20180416174117.GA77055@dtor-ws/

Thanks.

-- 
Dmitry

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

[Doug Anderson]

Hi,

On Wed, Nov 17, 2021 at 12:47 PM Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
>
> On Wed, Nov 17, 2021 at 10:38:58AM -0800, Brian Norris wrote:
> > On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > (Snip other comments; they seem reasonable, and I'll factor them into
> > the next version)
> >
> > > I guess one random thought I had is whether there would be an
> > > appropriate place to put this that _wasn't_ in DRM. I still wonder
> > > whether we'll ever try to upstream something like the cpufreq boost
> > > driver that we're carrying around and using in Chrome OS. If so, it
> > > would want to use these same helpers and it'd be pretty awkward for it
> > > to have to reach into DRM. ...any chance we could just land these
> > > helpers somewhere more generic?
> >
> > Yeah, I was torn on what to do here as well. I'd rather land something
> > than nothing, and when reading past conversations, it sounded like
> > Dmitry didn't want this kind of thing in drivers/input/ [1]. I'd love
> > to be wrong here though.
>
> I simply feel that input_handler is already a very simple abstraction
> and trying to specialize it to simplify users further is not productive.

I guess, if nothing else, it would be nice to avoid the tables that
we'd have to copy between DRM and cpufreq: the set of input devices
that are likely a sign that the user is interacting with the device.
It always seemed weird to copy that from place to place and if there's
ever a new input device to add it would be annoying to have to update
it everywhere.

It would be nice to avoid some of the other boilerplate code here
connecting things together when all we need is a callback, but I agree
that if those were copied it wouldn't be the end of the world.

-Doug

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

[Rob Clark]

On Fri, Nov 12, 2021 at 4:52 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Nov 3, 2021 at 4:40 PM 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>
> > ---
> >
> >  drivers/gpu/drm/Makefile           |   3 +-
> >  drivers/gpu/drm/drm_input_helper.c | 143 +++++++++++++++++++++++++++++
> >  include/drm/drm_input_helper.h     |  22 +++++
> >  3 files changed, 167 insertions(+), 1 deletion(-)
> >  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/Makefile b/drivers/gpu/drm/Makefile
> > index 0dff40bb863c..378761685b47 100644
> > --- a/drivers/gpu/drm/Makefile
> > +++ b/drivers/gpu/drm/Makefile
> > @@ -49,7 +49,8 @@ drm_kms_helper-y := drm_bridge_connector.o drm_crtc_helper.o drm_dp_helper.o \
> >                 drm_scdc_helper.o drm_gem_atomic_helper.o \
> >                 drm_gem_framebuffer_helper.o \
> >                 drm_atomic_state_helper.o drm_damage_helper.o \
> > -               drm_format_helper.o drm_self_refresh_helper.o
> > +               drm_format_helper.o drm_self_refresh_helper.o \
> > +               drm_input_helper.o
>
> Note that the Makefile part conflicts with drm-misc-next right now.
> It's not very hard to resolve, but since this would likely land there
> maybe it'd be nice to resolve?
>
> Other than that, this seems sane to me and much better than copying
> this between places, so assuming that the build problems get resolved
> then I'm happy.
>
> I guess one random thought I had is whether there would be an
> appropriate place to put this that _wasn't_ in DRM. I still wonder
> whether we'll ever try to upstream something like the cpufreq boost
> driver that we're carrying around and using in Chrome OS. If so, it
> would want to use these same helpers and it'd be pretty awkward for it
> to have to reach into DRM. ...any chance we could just land these
> helpers somewhere more generic?
>

One of the problems with cpu input boost is the cooldown period
(anti-spacebar-heater) aspect vs relatively short boost period.. in
that it is short enough that you need to consider that certain keys
(ie. modifier keys) should boost on key-release instead of key-press.

That isn't really an issue for triggers to exit PSR, or what we use
input-boost for downstream in drm/msm (where it isn't really boost,
just an early trigger to start powering up the GPU if it is suspended)

BR,
-R

Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events

[Daniel Vetter]

On Wed, Nov 17, 2021 at 8:37 PM Brian Norris <briannorris@chromium.org> wrote:
>
> On Wed, Nov 17, 2021 at 11:12 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > Can you pls resend with dri-devel on cc? scripts/get_maintainers.pl
> > should pick this up, you have all the maintainers but not the list.
>
> Oops, I don't know how that happened. I guess I sometimes have to trim
> get_maintainer output, since it likes to hoover up a bunch of
> barely-relevant previous committers. I must have been too aggressive.
>
> I'll plan on sending v2 to dri-devel, but let me know (privately if
> you'd like) if you'd prefer a pure RESEND of v1.

Nah just for next version is fine, assuming you include all the
context in in-patch changelog and all that so new readers can catch
up.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch