* [PATCH 0/2] drm: Support input-boosted panel self-refresh exit @ 2021-11-03 23:40 Brian Norris 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris 2021-11-03 23:40 ` [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events Brian Norris 0 siblings, 2 replies; 18+ messages in thread From: Brian Norris @ 2021-11-03 23:40 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark, Daniel Vetter, 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 [PATCH 0/2] drm: Support input-boosted panel self-refresh exit Brian Norris @ 2021-11-03 23:40 ` Brian Norris 2021-11-04 10:47 ` kernel test robot ` (4 more replies) 2021-11-03 23:40 ` [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events Brian Norris 1 sibling, 5 replies; 18+ messages in thread From: Brian Norris @ 2021-11-03 23:40 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark, Daniel Vetter, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris @ 2021-11-04 10:47 ` kernel test robot 2021-11-04 21:45 ` Brian Norris 2021-11-04 13:20 ` kernel test robot ` (3 subsequent siblings) 4 siblings, 1 reply; 18+ messages in thread From: kernel test robot @ 2021-11-04 10:47 UTC (permalink / raw) To: Brian Norris, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: kbuild-all, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-04 10:47 ` kernel test robot @ 2021-11-04 21:45 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2021-11-04 21:45 UTC (permalink / raw) To: kernel test robot Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, kbuild-all, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris 2021-11-04 10:47 ` kernel test robot @ 2021-11-04 13:20 ` kernel test robot 2021-11-04 13:46 ` kernel test robot ` (2 subsequent siblings) 4 siblings, 0 replies; 18+ messages in thread From: kernel test robot @ 2021-11-04 13:20 UTC (permalink / raw) To: Brian Norris, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: llvm, kbuild-all, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris 2021-11-04 10:47 ` kernel test robot 2021-11-04 13:20 ` kernel test robot @ 2021-11-04 13:46 ` kernel test robot 2021-11-05 23:11 ` Dmitry Torokhov 2021-11-13 0:52 ` Doug Anderson 4 siblings, 0 replies; 18+ messages in thread From: kernel test robot @ 2021-11-04 13:46 UTC (permalink / raw) To: Brian Norris, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: kbuild-all, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen [-- 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 --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris ` (2 preceding siblings ...) 2021-11-04 13:46 ` kernel test robot @ 2021-11-05 23:11 ` Dmitry Torokhov 2021-11-13 0:52 ` Doug Anderson 4 siblings, 0 replies; 18+ messages in thread From: Dmitry Torokhov @ 2021-11-05 23:11 UTC (permalink / raw) To: Brian Norris Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris ` (3 preceding siblings ...) 2021-11-05 23:11 ` Dmitry Torokhov @ 2021-11-13 0:52 ` Doug Anderson 2021-11-17 18:38 ` Brian Norris 2021-11-17 22:27 ` Rob Clark 4 siblings, 2 replies; 18+ messages in thread From: Doug Anderson @ 2021-11-13 0:52 UTC (permalink / raw) To: Brian Norris Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-13 0:52 ` Doug Anderson @ 2021-11-17 18:38 ` Brian Norris 2021-11-17 20:47 ` Dmitry Torokhov 2021-11-17 22:27 ` Rob Clark 1 sibling, 1 reply; 18+ messages in thread From: Brian Norris @ 2021-11-17 18:38 UTC (permalink / raw) To: Doug Anderson Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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/ ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-17 18:38 ` Brian Norris @ 2021-11-17 20:47 ` Dmitry Torokhov 2021-11-17 20:56 ` Doug Anderson 0 siblings, 1 reply; 18+ messages in thread From: Dmitry Torokhov @ 2021-11-17 20:47 UTC (permalink / raw) To: Brian Norris Cc: Doug Anderson, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-17 20:47 ` Dmitry Torokhov @ 2021-11-17 20:56 ` Doug Anderson 0 siblings, 0 replies; 18+ messages in thread From: Doug Anderson @ 2021-11-17 20:56 UTC (permalink / raw) To: Dmitry Torokhov Cc: Brian Norris, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper 2021-11-13 0:52 ` Doug Anderson 2021-11-17 18:38 ` Brian Norris @ 2021-11-17 22:27 ` Rob Clark 1 sibling, 0 replies; 18+ messages in thread From: Rob Clark @ 2021-11-17 22:27 UTC (permalink / raw) To: Doug Anderson Cc: Brian Norris, Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, Linux Kernel Mailing List, linux-input, David Airlie, open list:ARM/Rockchip SoC..., Kristian H . Kristensen, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-03 23:40 [PATCH 0/2] drm: Support input-boosted panel self-refresh exit Brian Norris 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris @ 2021-11-03 23:40 ` Brian Norris 2021-11-13 0:52 ` Doug Anderson 2021-11-17 19:12 ` Daniel Vetter 1 sibling, 2 replies; 18+ messages in thread From: Brian Norris @ 2021-11-03 23:40 UTC (permalink / raw) To: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann Cc: Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark, Daniel Vetter, 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-03 23:40 ` [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events Brian Norris @ 2021-11-13 0:52 ` Doug Anderson 2021-11-17 18:24 ` Brian Norris 2021-11-17 19:12 ` Daniel Vetter 1 sibling, 1 reply; 18+ messages in thread From: Doug Anderson @ 2021-11-13 0:52 UTC (permalink / raw) To: Brian Norris Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-13 0:52 ` Doug Anderson @ 2021-11-17 18:24 ` Brian Norris 0 siblings, 0 replies; 18+ messages in thread From: Brian Norris @ 2021-11-17 18:24 UTC (permalink / raw) To: Doug Anderson Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Rob Clark, Rob Clark, Daniel Vetter 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-03 23:40 ` [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events Brian Norris 2021-11-13 0:52 ` Doug Anderson @ 2021-11-17 19:12 ` Daniel Vetter 2021-11-17 19:36 ` Brian Norris 1 sibling, 1 reply; 18+ messages in thread From: Daniel Vetter @ 2021-11-17 19:12 UTC (permalink / raw) To: Brian Norris Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-17 19:12 ` Daniel Vetter @ 2021-11-17 19:36 ` Brian Norris 2021-11-18 6:39 ` Daniel Vetter 0 siblings, 1 reply; 18+ messages in thread From: Brian Norris @ 2021-11-17 19:36 UTC (permalink / raw) To: Daniel Vetter Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events 2021-11-17 19:36 ` Brian Norris @ 2021-11-18 6:39 ` Daniel Vetter 0 siblings, 0 replies; 18+ messages in thread From: Daniel Vetter @ 2021-11-18 6:39 UTC (permalink / raw) To: Brian Norris Cc: Maarten Lankhorst, Maxime Ripard, Thomas Zimmermann, Andrzej Hajda, Dmitry Torokhov, linux-kernel, linux-input, David Airlie, linux-rockchip, Kristian H . Kristensen, Doug Anderson, Rob Clark, Rob Clark 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-11-18 6:39 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-11-03 23:40 [PATCH 0/2] drm: Support input-boosted panel self-refresh exit Brian Norris 2021-11-03 23:40 ` [PATCH 1/2] drm/input_helper: Add new input-handling helper Brian Norris 2021-11-04 10:47 ` kernel test robot 2021-11-04 21:45 ` Brian Norris 2021-11-04 13:20 ` kernel test robot 2021-11-04 13:46 ` kernel test robot 2021-11-05 23:11 ` Dmitry Torokhov 2021-11-13 0:52 ` Doug Anderson 2021-11-17 18:38 ` Brian Norris 2021-11-17 20:47 ` Dmitry Torokhov 2021-11-17 20:56 ` Doug Anderson 2021-11-17 22:27 ` Rob Clark 2021-11-03 23:40 ` [PATCH 2/2] drm/self_refresh: Disable self-refresh on input events Brian Norris 2021-11-13 0:52 ` Doug Anderson 2021-11-17 18:24 ` Brian Norris 2021-11-17 19:12 ` Daniel Vetter 2021-11-17 19:36 ` Brian Norris 2021-11-18 6:39 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).