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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=XOda7vwUPY+WpGLzMwwrbrhQ9RqBQw4LhPwD6Sqhf7vw@mail.gmail.com>
Date:   Fri, 12 Nov 2021 16:52:15 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        linux-kernel@...r.kernel.org, linux-input@...r.kernel.org,
        David Airlie <airlied@...ux.ie>,
        linux-rockchip@...ts.infradead.org,
        "Kristian H . Kristensen" <hoegsberg@...gle.com>,
        Rob Clark <robdclark@...omium.org>,
        Rob Clark <robdclark@...il.com>,
        Daniel Vetter <daniel@...ll.ch>
Subject: Re: [PATCH 1/2] drm/input_helper: Add new input-handling helper

Hi,

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