Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Tue, Nov 30, 2021 at 12:35:45PM -0800, Brian Norris wrote:
> Hi Pekka,
>
> On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote:
> > On Thu, 18 Nov 2021 17:46:10 -0800
> > Brian Norris wrote:
> > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote:
> > > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > > Brian Norris wrote:
> > > > If KMS gets a pageflip or modeset in no time after an input event, then
> > > > what's the gain. OTOH, if the display server is locking on to vblank,
> > > > there might be a delay worth avoiding. But then, is it worth
> > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that
> > > > userspace could hit to start the warming up process?
> > >
> > > Rob responded to the first part to some extent (there is definitely gain
> > > to be had).
> > >
> > > To the last part: I wrote a simple debugfs hook to allow user space to
> > > force a PSR exit, and then a simple user space program to read input
> > > events and smash that debugfs file whenever it sees one. Testing in the
> > > same scenarios, this appears to lose less than 100 microseconds versus
> > > the in-kernel approach, which is negligible for this use case. (I'm not
> > > sure about the other use cases.)
> > >
> > > So, this is technically doable in user space.
> >
> > This is crucial information I would like you to include in some commit
> > message. I think it is very interesting for the reviewers. Maybe also
> > copy that in the cover letter.
> >
> > In my opinion there is a clear and obvious decision due that
> > measurement: Add the new ioctl for userspace to hit, do not try to
> > hardcode or upload the wake-up policy into the kernel.
>
> Perhaps.
>
> I'll admit, I'm not eager to go write the fd-passing solutions that
> others are designing on the fly. I'm currently torn on whether I'll just
> live with this current patch set out-of-tree (or, y'all can decide that
> a simple, 99% working solution is better than no solution), because it's
> simple; or possibly figuring out how to utilize such an ioctl cleanly
> within our display manager. I'm not super hopeful on the latter.
>
> IOW, I'm approximately in line with Doug's thoughts:
> https://lore.kernel.org/all/CAD=FV=XARhZoj+0p-doxcbC=4K+NuMc=ur6wqg6kwk-mkpk...@mail.gmail.com/
> But then, we're obviously biased.
>
> > > I can't speak to the ease of _actually_ integrating this into even our
> > > own Chrome display manager, but I highly doubt it will get integrated
> > > into others. I'd posit this should weigh into the relative worth, but
> > > otherwise can't really give you an answer there.
> >
> > I think such a thing would be very simple to add to any display server.
> > They already have hooks for things like resetting idle timeout timers on
> > any relevant input event.
> >
> > > I'd also note, software-directed PSR is so far designed to be completely
> > > opaque to user space. There's no way to disable it; no way to know it's
> > > active; and no way to know anything about the parameters it's computing
> > > (like average entry/exit delay). Would you suggest a whole set of new
> > > IOCTLs for this?
> >
> > Just one ioctl on the DRM device: "Hey, wake up!". Because that's what
> > your patch does in-kernel, right?
>
> Well, we'd at least want something to advertise that the feature does
> something ("is supported") I think, otherwise we're just asking user
> space to do useless work.
>
> > If there are use case specific parameters, then how did you intend to
> > allow adjusting those in your proposal?
>
> Another commenter mentioned the latency tradeoff -- it's possible that
> there are panels/eDP-links that resume fast enough that one doesn't care
> to use this ioctl. For an in-kernel solution, one has all the data
> available and could use hardware information to make decisions, if
> needed. For a user space solution, we won't have any of that, and we'd
> have to work to expose that information.
>
> I suppose we could ignore that problem and only expose a minimal UAPI
> until we need something more, but it feels like exposing a UAPI for
> something is a critical point where one should make sure it's reasonably
> descriptive and useful.
>
> > > > How do you know userspace is using this input device at all? If
> > > > userspace is not using the input device, then DRM should not be opening
> > > > it either, as it must have no effect on anything.
> > > >
> > > > If you open an input device that userspace does not use, you also cause
> > > > a power consumption regression, because now the input device itself is
> > > > active and possibly flooding the kernel with events (e.g. an
> > > > accelerometer).
> > >
> > > Well, I don't think accelerometers show up as input devices, but I
> > > suppose your point could apply to actual input devices.
fwiw, filtering INPUT_PROP_ACCELEROMETER would go a long way towards ignoring
accelerometers.
> > My understanding is that accelerometers are evdev (input)
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi Pekka,
On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote:
> On Thu, 18 Nov 2021 17:46:10 -0800
> Brian Norris wrote:
> > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote:
> > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > Brian Norris wrote:
> > > If KMS gets a pageflip or modeset in no time after an input event, then
> > > what's the gain. OTOH, if the display server is locking on to vblank,
> > > there might be a delay worth avoiding. But then, is it worth
> > > short-circuiting the wake-up in kernel vs. adding a new ioctl that
> > > userspace could hit to start the warming up process?
> >
> > Rob responded to the first part to some extent (there is definitely gain
> > to be had).
> >
> > To the last part: I wrote a simple debugfs hook to allow user space to
> > force a PSR exit, and then a simple user space program to read input
> > events and smash that debugfs file whenever it sees one. Testing in the
> > same scenarios, this appears to lose less than 100 microseconds versus
> > the in-kernel approach, which is negligible for this use case. (I'm not
> > sure about the other use cases.)
> >
> > So, this is technically doable in user space.
>
> This is crucial information I would like you to include in some commit
> message. I think it is very interesting for the reviewers. Maybe also
> copy that in the cover letter.
>
> In my opinion there is a clear and obvious decision due that
> measurement: Add the new ioctl for userspace to hit, do not try to
> hardcode or upload the wake-up policy into the kernel.
Perhaps.
I'll admit, I'm not eager to go write the fd-passing solutions that
others are designing on the fly. I'm currently torn on whether I'll just
live with this current patch set out-of-tree (or, y'all can decide that
a simple, 99% working solution is better than no solution), because it's
simple; or possibly figuring out how to utilize such an ioctl cleanly
within our display manager. I'm not super hopeful on the latter.
IOW, I'm approximately in line with Doug's thoughts:
https://lore.kernel.org/all/CAD=FV=XARhZoj+0p-doxcbC=4K+NuMc=ur6wqg6kwk-mkpk...@mail.gmail.com/
But then, we're obviously biased.
> > I can't speak to the ease of _actually_ integrating this into even our
> > own Chrome display manager, but I highly doubt it will get integrated
> > into others. I'd posit this should weigh into the relative worth, but
> > otherwise can't really give you an answer there.
>
> I think such a thing would be very simple to add to any display server.
> They already have hooks for things like resetting idle timeout timers on
> any relevant input event.
>
> > I'd also note, software-directed PSR is so far designed to be completely
> > opaque to user space. There's no way to disable it; no way to know it's
> > active; and no way to know anything about the parameters it's computing
> > (like average entry/exit delay). Would you suggest a whole set of new
> > IOCTLs for this?
>
> Just one ioctl on the DRM device: "Hey, wake up!". Because that's what
> your patch does in-kernel, right?
Well, we'd at least want something to advertise that the feature does
something ("is supported") I think, otherwise we're just asking user
space to do useless work.
> If there are use case specific parameters, then how did you intend to
> allow adjusting those in your proposal?
Another commenter mentioned the latency tradeoff -- it's possible that
there are panels/eDP-links that resume fast enough that one doesn't care
to use this ioctl. For an in-kernel solution, one has all the data
available and could use hardware information to make decisions, if
needed. For a user space solution, we won't have any of that, and we'd
have to work to expose that information.
I suppose we could ignore that problem and only expose a minimal UAPI
until we need something more, but it feels like exposing a UAPI for
something is a critical point where one should make sure it's reasonably
descriptive and useful.
> > > How do you know userspace is using this input device at all? If
> > > userspace is not using the input device, then DRM should not be opening
> > > it either, as it must have no effect on anything.
> > >
> > > If you open an input device that userspace does not use, you also cause
> > > a power consumption regression, because now the input device itself is
> > > active and possibly flooding the kernel with events (e.g. an
> > > accelerometer).
> >
> > Well, I don't think accelerometers show up as input devices, but I
> > suppose your point could apply to actual input devices.
>
> My understanding is that accelerometers are evdev (input) devices,
> especially when used as input e.g. for controlling games. I'm not aware
> of any other interface for it.
I'm not familiar with game-controlling accelerometers, but many types of
accelerometers are serviced by drivers/iio/.
And even if they register as input devices, do they match the ID list in
this patch?
> Even audio sockets are input
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Mon, Nov 22, 2021 at 11:43:42AM +0200, Pekka Paalanen wrote: > On Fri, 19 Nov 2021 17:11:07 +0100 > Daniel Vetter wrote: > > > On Fri, Nov 19, 2021 at 04:04:28PM +, Simon Ser wrote: > > > On Friday, November 19th, 2021 at 16:53, Daniel Vetter > > > wrote: > > > > > > > Random idea ... should we perhaps let userspace connect the boosting? > > > > I.e. > > > > we do a bunch of standardized boost targets (render clocks, display sr > > > > exit), and userspace can then connect it to whichever input device it > > > > wants to? > > > > > > On IRC we discussed having user-space hand over a FD to the kernel. When > > > the FD > > > becomes readable, the kernel triggers the boost. > > > > > > This would let user-space use e.g. an input device, an eventfd, or an > > > epoll FD > > > with any combination of these as the boost signal. > > > > Can userspace filter eventfd appropriately like we do here? And can they > > get at that maybe 2nd eventfd from logind or whatever there is on distros > > where /dev access is locked down for compositors/users. > > (Mind, eventfd is a specific thing, see 'man eventfd', and evdev/input > device fd is different.) Yeah I was a bit sloppy, but I knew. > I don't think any of that is any problem when userspace prepares an > epoll fd to be given to the boosting machinery. The boosting machinery > could have several different targets as well, PSR vs. GPU clocks and > whatnot. > > I envision a compositor to maintain an epoll fd for boosting by > adding/removing the same device fds to it that it already uses in its > operations. I don't see any need to open new device fds just for > boosting. It's only the epoll fd given to the kernel and after that the > epoll set can still be changed, right? > > The boosting machinery would never actually read or write the > registered fd(s), so it would not interfere with the normal operations. > But it also means the fd will remain readable until userspace services > it. Userspace may need to set up that epoll set very carefully to have > it work right (e.g. edge-triggered?). > > If your input handling is in a different process than the DRM poking > for some reason, the epoll fd should still work if: > - it is possible to use SCM_RIGHTS to pass the epollfd from the > input process to the DRM process, and > - you cannot extract the watched fds from an epoll fd. > > Do we have those assumptions today? > > Then the attack surface in the DRM process is limited to changing the > epoll set of which fds can trigger boosting, but the DRM process can do > that anyway. I also presume the input process can still add and remove > fds from the epoll set even afterwards. > > > I do agree that if we can do this generically maybe we should, but also > > the use-case for input boosting is pretty well defined. I think it's just > > about making sure that compositors is in control, and that we don't make > > it worse (e.g. with the sr exit adding latency when the compositor can > > redraw quickly enough). > > The epollfd design sounds very good to me. One can register an > arbitrary set of fds with it, and use even eventfds in the set to have > purely software triggers. Yeah I think just allowing to internall poll on any arbitrary fd sounds like a neat interface. Userspace should then be able to do whatever it wants to. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 11:07:16AM -0800, Brian Norris wrote: > Hi Daniel, > > On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote: > > On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > > > --- a/drivers/gpu/drm/Kconfig > > > > > +++ b/drivers/gpu/drm/Kconfig > > > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > > > > > If in doubt, say "N". > > > > > > > > > > +config DRM_INPUT_HELPER > > > > > + def_bool y > > > > > + depends on DRM_KMS_HELPER > > > > > + depends on INPUT > > > > > > > > Uh please no configs for each thing, it just makes everything more > > > > complex. Do we _really_ need this? > > > > > > First, it's not a configurable option (a user will never see this nor > > > have to answer Y/N to it); it only serves as an intermediary to express > > > the CONFIG_INPUT dependency (which is necessary) without making > > > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > > > display stacks without the input subsystem.) > > > > I'm not so much worried about the user cost, but the maintenance cost. > > Kbuild config complexity is ridiculous, anything that adds even a bit is > > really silly. > > > > > The closest alternative I can think of with fewer Kconfig symbols is to > > > just use CONFIG_INPUT directly in the code, to decide whether to provide > > > the helpers or else just stub them out. But that has a problem of not > > > properly expressing the =m vs. =y necessity: if, for example, > > > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > > > issues. > > > > Usually this is done by providing static inline dummy implementations in > > the headers. That avoids having to sprinkle new Kconfig symbols all over. > > Right, I already did that, and I'm not sprinkling > CONFIG_DRM_INPUT_HELPER much. (I do include one around the module > parameter, because it doesn't make much sense to have the module > parameter even exist, if the underlying feature is stubbed out.) > > But that doesn't solve the problem in my last sentence, involving > tristates. The "stub inline" approach only works well for boolean > features -- either built-in, or disabled. Once your feature is in a > module, you need to ensure that no built-in code depends on it. > > Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If > so, I can just add a 'select' or 'depend' and drop this intermediate > symbol. > If not, then what do you expect to happen with DRM_KMS_HELPER=y and > CONFIG_INPUT=m? Yeah just add the dependency. If you still want to keep it optional the way to do it is to add depends on FOO || FOO=n And then just have #if IS_ENABLED(FOO) around your inline wrappers. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, 19 Nov 2021 17:11:07 +0100 Daniel Vetter wrote: > On Fri, Nov 19, 2021 at 04:04:28PM +, Simon Ser wrote: > > On Friday, November 19th, 2021 at 16:53, Daniel Vetter > > wrote: > > > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > > we do a bunch of standardized boost targets (render clocks, display sr > > > exit), and userspace can then connect it to whichever input device it > > > wants to? > > > > On IRC we discussed having user-space hand over a FD to the kernel. When > > the FD > > becomes readable, the kernel triggers the boost. > > > > This would let user-space use e.g. an input device, an eventfd, or an epoll > > FD > > with any combination of these as the boost signal. > > Can userspace filter eventfd appropriately like we do here? And can they > get at that maybe 2nd eventfd from logind or whatever there is on distros > where /dev access is locked down for compositors/users. (Mind, eventfd is a specific thing, see 'man eventfd', and evdev/input device fd is different.) I don't think any of that is any problem when userspace prepares an epoll fd to be given to the boosting machinery. The boosting machinery could have several different targets as well, PSR vs. GPU clocks and whatnot. I envision a compositor to maintain an epoll fd for boosting by adding/removing the same device fds to it that it already uses in its operations. I don't see any need to open new device fds just for boosting. It's only the epoll fd given to the kernel and after that the epoll set can still be changed, right? The boosting machinery would never actually read or write the registered fd(s), so it would not interfere with the normal operations. But it also means the fd will remain readable until userspace services it. Userspace may need to set up that epoll set very carefully to have it work right (e.g. edge-triggered?). If your input handling is in a different process than the DRM poking for some reason, the epoll fd should still work if: - it is possible to use SCM_RIGHTS to pass the epollfd from the input process to the DRM process, and - you cannot extract the watched fds from an epoll fd. Do we have those assumptions today? Then the attack surface in the DRM process is limited to changing the epoll set of which fds can trigger boosting, but the DRM process can do that anyway. I also presume the input process can still add and remove fds from the epoll set even afterwards. > I do agree that if we can do this generically maybe we should, but also > the use-case for input boosting is pretty well defined. I think it's just > about making sure that compositors is in control, and that we don't make > it worse (e.g. with the sr exit adding latency when the compositor can > redraw quickly enough). The epollfd design sounds very good to me. One can register an arbitrary set of fds with it, and use even eventfds in the set to have purely software triggers. Thanks, pq pgpSIe2w2RHU0.pgp Description: OpenPGP digital signature
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, 19 Nov 2021 16:56:01 +0100 Daniel Vetter wrote: > On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 17:46:10 -0800 > > Brian Norris wrote: > > > > > Hi Pekka, > > > > > > Thanks for the thoughts and review. I've tried to respond below: > > > > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris 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 > > > > > --- > > > > > > > > Thanks Simon for the CC. > > > > > > > > Hi Brian, > > > > > > > > while this feature in general makes sense and sounds good, to start > > > > warming up display hardware early when something might start to happen, > > > > this particular proposal has many problems from UAPI perspective (as it > > > > has none). Comments below. > > > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > > from this input event watching? I would imagine the improvement to not > > > > be noticeable. > > > > > > Patch 2 has details. It's not really about precisely how slow PSR is, > > > but how much foresight we can gain: in patch 2, I note that with my > > > particular user space and system, I can start PSR-exit 50ms earlier than > > > I would otherweise. (FWIW, this measurement is exactly the same it was > > > with the original version written 4 years ago.) > > > > > > For how long PSR-exit takes: the measurements I'm able to do (via > > > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > > > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > > > manages to hide nearly 100% of that latency. > > > > > > Typical use cases where one notices PSR latency (and where this 35-55ms > > > matters) involve simply moving a cursor; it's very noticeable when you > > > have more than a few frames of latency to "get started". > > > > Hi Brian, > > > > that is very interesting, thanks. > > > > I would never have expected to have userspace take *that* long to > > react. But, that sounds like it could be just your userspace software > > stack. > > In the other subthread we're talking about making this more explicit. > Maybe we need to combine this with a "I expect to take this many > milliseconds to get the first frame out" value. > > That way compositors which take 50ms (which frankly is shocking slow) can > set that, and kms can enable sr exit (since sr exit will actually help > here). But other compositors which expect to get the first frame out in > maybe 20 can spec that, and then the driver will not sr exit (because too > high chances we'll just make shit slower), and instead will only boost > render clocks. > > Thoughts? I wonder if the compositor or the userspace stack can know how long it usually takes to prepare the first KMS submission after a pause. I guess it would need to measure that at runtime. Hmm, doable I guess, sure. Input to output latency in general is interesting. However, that sounds like a pretty vague API with the delay value. I think it has a high risk of regressing into a boolean toggle by userspace choosing an arbitrary number and then assuming the threshold in the driver is always the same. Thanks, pq pgpO_4Ma15NN4.pgp Description:
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi Daniel, On Fri, Nov 19, 2021 at 11:01:18AM +0100, Daniel Vetter wrote: > On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote: > > On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote: > > > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote: > > > > --- a/drivers/gpu/drm/Kconfig > > > > +++ b/drivers/gpu/drm/Kconfig > > > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > > > > > If in doubt, say "N". > > > > > > > > +config DRM_INPUT_HELPER > > > > + def_bool y > > > > + depends on DRM_KMS_HELPER > > > > + depends on INPUT > > > > > > Uh please no configs for each thing, it just makes everything more > > > complex. Do we _really_ need this? > > > > First, it's not a configurable option (a user will never see this nor > > have to answer Y/N to it); it only serves as an intermediary to express > > the CONFIG_INPUT dependency (which is necessary) without making > > DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run > > display stacks without the input subsystem.) > > I'm not so much worried about the user cost, but the maintenance cost. > Kbuild config complexity is ridiculous, anything that adds even a bit is > really silly. > > > The closest alternative I can think of with fewer Kconfig symbols is to > > just use CONFIG_INPUT directly in the code, to decide whether to provide > > the helpers or else just stub them out. But that has a problem of not > > properly expressing the =m vs. =y necessity: if, for example, > > CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker > > issues. > > Usually this is done by providing static inline dummy implementations in > the headers. That avoids having to sprinkle new Kconfig symbols all over. Right, I already did that, and I'm not sprinkling CONFIG_DRM_INPUT_HELPER much. (I do include one around the module parameter, because it doesn't make much sense to have the module parameter even exist, if the underlying feature is stubbed out.) But that doesn't solve the problem in my last sentence, involving tristates. The "stub inline" approach only works well for boolean features -- either built-in, or disabled. Once your feature is in a module, you need to ensure that no built-in code depends on it. Do you want DRM_KMS_HELPER to unconditionally depend on CONFIG_INPUT? If so, I can just add a 'select' or 'depend' and drop this intermediate symbol. If not, then what do you expect to happen with DRM_KMS_HELPER=y and CONFIG_INPUT=m? Brian
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi, On Fri, Nov 19, 2021 at 8:39 AM Rob Clark wrote: > > On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen wrote: > > > > On Thu, 18 Nov 2021 15:30:38 -0800 > > Rob Clark wrote: > > > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen > > > wrote: > > > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > > Brian Norris wrote: > > > > > > > > > A variety of applications have found it useful to listen to > > > > > user-initiated input events to make decisions within a DRM driver, > > > > > given > > > > > that input events are often the first sign that we're going to start > > > > > doing latency-sensitive activities: > > > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > > >Rockchip eDP) is especially latency sensitive. In some cases, it > > > > > can > > > > >take 10s of milliseconds for a panel to exit self-refresh, which > > > > > can > > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always > > > > > shipped > > > > >with an input_handler boost, that preemptively exits self-refresh > > > > >whenever there is input activity. > > > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > > >render new frames immediately after user activity. Powering up the > > > > >GPU can take enough time that it is worthwhile to start this > > > > > process > > > > >as soon as there is input activity. Many Chrome OS systems also > > > > > ship > > > > >with an input_handler boost that powers up the GPU. > > > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > > input-subsystem details around picking which devices to listen to, and > > > > > some other boilerplate. This will be used in the next patch to > > > > > implement > > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS > > > > > kernels > > > > > have been carrying for a while. > > > > > > > > > > Signed-off-by: Brian Norris > > > > > --- > > > > > > > > Thanks Simon for the CC. > > > > > > > > Hi Brian, > > > > > > > > while this feature in general makes sense and sounds good, to start > > > > warming up display hardware early when something might start to happen, > > > > this particular proposal has many problems from UAPI perspective (as it > > > > has none). Comments below. > > > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > > from this input event watching? I would imagine the improvement to not > > > > be noticeable. > > > > > > > > I think some numbers about how much this feature helps would be really > > > > good, even if they are quite specific use cases. You also need to > > > > identify the userspace components, because I think different display > > > > servers are very different in their reaction speed. > > > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > > there might be a delay worth avoiding. But then, is it worth > > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > > userspace could hit to start the warming up process? > > > > > > In my measurements, it takes userspace a frame or two to respond and > > > get to the point of starting to build cmdstream (before eventually > > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > > a similar boost mechanism to spool up cpufreq > > > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > > cumulation of a long sequence of events.. input-boost is letting > > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > > that long sequence. > > > > Right, exactly. That is why I was musing about a *new* ioctl that > > userspace could hit as soon as any input device fd (or network fd!) > > shows signs of life. Would that be enough, avoiding all the annoying > > questions about which input and DRM devices should participate here > > (and what about non-input devices that still want to trigger the > > warm-up, like network traffic, e.g. remote control?), or does it really > > need to be kernel internal to be fast enough? > > > > As Brian wrote about his quick hack to test that via debugfs, sounds > > like the userspace solution would be totally sufficient. > > > > The question is, I think we want to use this for at least a couple > different things.. PSR exit, and/or early GPU wakeup/boost. So I'm > not quite sure where/what this ioctl should be. Different drivers may > have different uses. Also, because of the CrOS userspace sandbox > architecture, the place that could do a kms ioctl isn't really the > place that would know when to call the ioctl. > > But if we want to move the policy out of the kernel, one approach > would just be to have some sysfs where userspace could configure the >
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 1:54 AM Pekka Paalanen wrote: > > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > >with an input_handler boost, that preemptively exits self-refresh > > > >whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > >render new frames immediately after user activity. Powering up the > > > >GPU can take enough time that it is worthwhile to start this process > > > >as soon as there is input activity. Many Chrome OS systems also ship > > > >with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. > The question is, I think we want to use this for at least a couple different things.. PSR exit, and/or early GPU wakeup/boost. So I'm not quite sure where/what this ioctl should be. Different drivers may have different uses. Also, because of the CrOS userspace sandbox architecture, the place that could do a kms ioctl isn't really the place that would know when to call the ioctl. But if we want to move the policy out of the kernel, one approach would just be to have some sysfs where userspace could configure the input_device_id[] filter.. BR, -R
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 04:04:28PM +, Simon Ser wrote: > On Friday, November 19th, 2021 at 16:53, Daniel Vetter > wrote: > > > Random idea ... should we perhaps let userspace connect the boosting? I.e. > > we do a bunch of standardized boost targets (render clocks, display sr > > exit), and userspace can then connect it to whichever input device it > > wants to? > > On IRC we discussed having user-space hand over a FD to the kernel. When the > FD > becomes readable, the kernel triggers the boost. > > This would let user-space use e.g. an input device, an eventfd, or an epoll FD > with any combination of these as the boost signal. Can userspace filter eventfd appropriately like we do here? And can they get at that maybe 2nd eventfd from logind or whatever there is on distros where /dev access is locked down for compositors/users. I do agree that if we can do this generically maybe we should, but also the use-case for input boosting is pretty well defined. I think it's just about making sure that compositors is in control, and that we don't make it worse (e.g. with the sr exit adding latency when the compositor can redraw quickly enough). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Friday, November 19th, 2021 at 16:53, Daniel Vetter wrote: > Random idea ... should we perhaps let userspace connect the boosting? I.e. > we do a bunch of standardized boost targets (render clocks, display sr > exit), and userspace can then connect it to whichever input device it > wants to? On IRC we discussed having user-space hand over a FD to the kernel. When the FD becomes readable, the kernel triggers the boost. This would let user-space use e.g. an input device, an eventfd, or an epoll FD with any combination of these as the boost signal.
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 12:38:41PM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 17:46:10 -0800 > Brian Norris wrote: > > > Hi Pekka, > > > > Thanks for the thoughts and review. I've tried to respond below: > > > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris 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 > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > Patch 2 has details. It's not really about precisely how slow PSR is, > > but how much foresight we can gain: in patch 2, I note that with my > > particular user space and system, I can start PSR-exit 50ms earlier than > > I would otherweise. (FWIW, this measurement is exactly the same it was > > with the original version written 4 years ago.) > > > > For how long PSR-exit takes: the measurements I'm able to do (via > > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > > manages to hide nearly 100% of that latency. > > > > Typical use cases where one notices PSR latency (and where this 35-55ms > > matters) involve simply moving a cursor; it's very noticeable when you > > have more than a few frames of latency to "get started". > > Hi Brian, > > that is very interesting, thanks. > > I would never have expected to have userspace take *that* long to > react. But, that sounds like it could be just your userspace software > stack. In the other subthread we're talking about making this more explicit. Maybe we need to combine this with a "I expect to take this many milliseconds to get the first frame out" value. That way compositors which take 50ms (which frankly is shocking slow) can set that, and kms can enable sr exit (since sr exit will actually help here). But other compositors which expect to get the first frame out in maybe 20 can spec that, and then the driver will not sr exit (because too high chances we'll just make shit slower), and instead will only boost render clocks. Thoughts? -Daniel > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > > not that familiar with the user space display stack, but for what I > > know, it's rather custom, developed within the Chromium project. Others > > on CC here could probably give you more detail, if you want specific > > answers, besides docs like this: > > > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > >
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Fri, Nov 19, 2021 at 11:54:19AM +0200, Pekka Paalanen wrote: > On Thu, 18 Nov 2021 15:30:38 -0800 > Rob Clark wrote: > > > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > > Brian Norris wrote: > > > > > > > A variety of applications have found it useful to listen to > > > > user-initiated input events to make decisions within a DRM driver, given > > > > that input events are often the first sign that we're going to start > > > > doing latency-sensitive activities: > > > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > > >with an input_handler boost, that preemptively exits self-refresh > > > >whenever there is input activity. > > > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > > >render new frames immediately after user activity. Powering up the > > > >GPU can take enough time that it is worthwhile to start this process > > > >as soon as there is input activity. Many Chrome OS systems also ship > > > >with an input_handler boost that powers up the GPU. > > > > > > > > This patch provides a small helper library that abstracts some of the > > > > input-subsystem details around picking which devices to listen to, and > > > > some other boilerplate. This will be used in the next patch to implement > > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > > have been carrying for a while. > > > > > > > > Signed-off-by: Brian Norris > > > > --- > > > > > > Thanks Simon for the CC. > > > > > > Hi Brian, > > > > > > while this feature in general makes sense and sounds good, to start > > > warming up display hardware early when something might start to happen, > > > this particular proposal has many problems from UAPI perspective (as it > > > has none). Comments below. > > > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > > from this input event watching? I would imagine the improvement to not > > > be noticeable. > > > > > > I think some numbers about how much this feature helps would be really > > > good, even if they are quite specific use cases. You also need to > > > identify the userspace components, because I think different display > > > servers are very different in their reaction speed. > > > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > > what's the gain. OTOH, if the display server is locking on to vblank, > > > there might be a delay worth avoiding. But then, is it worth > > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > > userspace could hit to start the warming up process? > > > > In my measurements, it takes userspace a frame or two to respond and > > get to the point of starting to build cmdstream (before eventually > > doing atomic/pageflip ioctl).. possibly longer if you don't also have > > a similar boost mechanism to spool up cpufreq > > > > But the important thing, IMO, is that atomic/pageflip ioctl is the > > cumulation of a long sequence of events.. input-boost is letting > > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > > that long sequence. > > Right, exactly. That is why I was musing about a *new* ioctl that > userspace could hit as soon as any input device fd (or network fd!) > shows signs of life. Would that be enough, avoiding all the annoying > questions about which input and DRM devices should participate here > (and what about non-input devices that still want to trigger the > warm-up, like network traffic, e.g. remote control?), or does it really > need to be kernel internal to be fast enough? > > As Brian wrote about his quick hack to test that via debugfs, sounds > like the userspace solution would be totally sufficient. Random idea ... should we perhaps let userspace connect the boosting? I.e. we do a bunch of standardized boost targets (render clocks, display sr exit), and userspace can then connect it to whichever input device it wants to? That also avoids the multi-user lol of us boosting the wrong seat, we could do a drm ioctl where you pass it an eventfd and essentially say "listen to this mkay?" That way the boosting would also neatly get passed along with compositors as we vt switch them, in case you have one that's all tablet, and another one (console emulation) that's kbd only. Also this avoids the latency problem perhaps of a compositor which just dumbly paints every frame because it's VR or something like that, so never any sr exit possible. Just an idea, compositor people pls shred it :-) -Daniel -- Daniel Vetter Software Engineer, Intel
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, 18 Nov 2021 17:46:10 -0800 Brian Norris wrote: > Hi Pekka, > > Thanks for the thoughts and review. I've tried to respond below: > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris 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 > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > Patch 2 has details. It's not really about precisely how slow PSR is, > but how much foresight we can gain: in patch 2, I note that with my > particular user space and system, I can start PSR-exit 50ms earlier than > I would otherweise. (FWIW, this measurement is exactly the same it was > with the original version written 4 years ago.) > > For how long PSR-exit takes: the measurements I'm able to do (via > ftrace) show that drm_self_refresh_transition() takes between 35 and 55 > ms. That's noticeable at 60 fps. And quite conveniently, the input-boost > manages to hide nearly 100% of that latency. > > Typical use cases where one notices PSR latency (and where this 35-55ms > matters) involve simply moving a cursor; it's very noticeable when you > have more than a few frames of latency to "get started". Hi Brian, that is very interesting, thanks. I would never have expected to have userspace take *that* long to react. But, that sounds like it could be just your userspace software stack. > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > If my email address isn't obvious, I'm testing Chrome OS. I'm frankly > not that familiar with the user space display stack, but for what I > know, it's rather custom, developed within the Chromium project. Others > on CC here could probably give you more detail, if you want specific > answers, besides docs like this: > > https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > Rob responded to the first part to some extent (there is definitely gain > to be had). > > To the last part: I wrote a simple debugfs hook to allow user space to > force a PSR exit, and then a simple user space program to read input > events and smash that debugfs file whenever it sees one. Testing in the > same scenarios, this appears to lose less than 100 microseconds versus > the in-kernel approach, which is negligible for this use case. (I'm not > sure about the other use cases.) > > So, this is technically doable in user space. This is crucial information I would like you to include in some commit message. I think it is
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, Nov 18, 2021 at 11:30:43AM -0800, Brian Norris wrote:
> Hi Daniel,
>
> Thanks for the review. Lots to address elsewhere, but I can respond
> here first:
>
> On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote:
> > On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:
> > > --- a/drivers/gpu/drm/Kconfig
> > > +++ b/drivers/gpu/drm/Kconfig
> > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST
> > >
> > > If in doubt, say "N".
> > >
> > > +config DRM_INPUT_HELPER
> > > + def_bool y
> > > + depends on DRM_KMS_HELPER
> > > + depends on INPUT
> >
> > Uh please no configs for each thing, it just makes everything more
> > complex. Do we _really_ need this?
>
> First, it's not a configurable option (a user will never see this nor
> have to answer Y/N to it); it only serves as an intermediary to express
> the CONFIG_INPUT dependency (which is necessary) without making
> DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run
> display stacks without the input subsystem.)
I'm not so much worried about the user cost, but the maintenance cost.
Kbuild config complexity is ridiculous, anything that adds even a bit is
really silly.
> The closest alternative I can think of with fewer Kconfig symbols is to
> just use CONFIG_INPUT directly in the code, to decide whether to provide
> the helpers or else just stub them out. But that has a problem of not
> properly expressing the =m vs. =y necessity: if, for example,
> CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker
> issues.
Usually this is done by providing static inline dummy implementations in
the headers. That avoids having to sprinkle new Kconfig symbols all over.
> In short, yes, I think we really need this. But I'm not a Kbuild expert.
>
> > > diff --git a/include/drm/drm_input_helper.h
> > > b/include/drm/drm_input_helper.h
> > > new file mode 100644
> > > index ..7904f397b934
> > > --- /dev/null
> > > +++ b/include/drm/drm_input_helper.h
> > > @@ -0,0 +1,41 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +/*
> > > + * Copyright (C) 2021 Google, Inc.
> > > + */
> > > +#ifndef __DRM_INPUT_HELPER_H__
> > > +#define __DRM_INPUT_HELPER_H__
> > > +
> > > +#include
> > > +
> > > +struct drm_device;
> > > +
> > > +struct drm_input_handler {
> > > + /*
> > > + * Callback to call for input activity. Will be called in an atomic
> > > + * context.
> >
> > How atomic? Like hardirq, and nasty spinlocks held?
>
> Maybe I should have just cribbed off the doc:
>
> * @event: event handler. This method is being called by input core with
> * interrupts disabled and dev->event_lock spinlock held and so
> * it may not sleep
>
> I probably don't want to propagate the subsystem details about which
> locks, but I guess I can be specific about "interrupts disabled" and
> "don't sleep".
You can also do hyperlinks in the generated htmldocs and just reference
that:
https://dri.freedesktop.org/docs/drm/doc-guide/kernel-doc.html#highlights-and-cross-references
>
> > > + */
> > > + void (*callback)(struct drm_input_handler *handler);
> > > +
> > > + struct input_handler handler;
> > > +};
> > > +
> > > +#if defined(CONFIG_DRM_INPUT_HELPER)
> > > +
> > > +int drm_input_handle_register(struct drm_device *dev,
> > > + struct drm_input_handler *handler);
> > > +void drm_input_handle_unregister(struct drm_input_handler *handler);
> > > +
> > > +#else /* !CONFIG_DRM_INPUT_HELPER */
> > > +
> > > +static inline int drm_input_handle_register(struct drm_device *dev,
> > > + struct drm_input_handler *handler)
> > > +{
> > > + return 0;
> > > +}
> >
> > I guess the reason behind the helper is that you also want to use this in
> > drivers or maybe drm/sched?
>
> I think my reasoning is heavily described in both the cover letter and
> the commit message. If that's not clear, can you point out which part?
> I'd gladly improve it :)
>
> But specifically, see the 2nd bullet from the commit message, which I've
> re-quoted down here:
>
> > > * GPU drivers: on GPU-accelerated desktop systems, we may need to
> > >render new frames immediately after user activity. Powering up the
> > >GPU can take enough time that it is worthwhile to start this process
> > >as soon as there is input activity. Many Chrome OS systems also ship
> > >with an input_handler boost that powers up the GPU.
>
> Rob Clark has patches to drm/msm to boost GPU power-up via a similar
> helper.
Yeah this question was just for confirmation, might be good to include
that other patch set too for the full picture.
> > Anyway I think it looks all reasonable. Definitely need an ack from input
> > people
>
> I realized I failed to carry Dmitry's Ack from version 1 [1]. If this
> has a v3 in similar form, I'll carry it there.
>
> > that the event list you have is a good choice, I have no idea what
> > that all does. Maybe also document that part a bit more.
>
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, 18 Nov 2021 15:30:38 -0800 Rob Clark wrote: > On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > > > On Wed, 17 Nov 2021 14:48:40 -0800 > > Brian Norris wrote: > > > > > A variety of applications have found it useful to listen to > > > user-initiated input events to make decisions within a DRM driver, given > > > that input events are often the first sign that we're going to start > > > doing latency-sensitive activities: > > > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > > >Rockchip eDP) is especially latency sensitive. In some cases, it can > > >take 10s of milliseconds for a panel to exit self-refresh, which can > > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > > >with an input_handler boost, that preemptively exits self-refresh > > >whenever there is input activity. > > > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > > >render new frames immediately after user activity. Powering up the > > >GPU can take enough time that it is worthwhile to start this process > > >as soon as there is input activity. Many Chrome OS systems also ship > > >with an input_handler boost that powers up the GPU. > > > > > > This patch provides a small helper library that abstracts some of the > > > input-subsystem details around picking which devices to listen to, and > > > some other boilerplate. This will be used in the next patch to implement > > > the first bullet: preemptive exit for panel self-refresh. > > > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > > have been carrying for a while. > > > > > > Signed-off-by: Brian Norris > > > --- > > > > Thanks Simon for the CC. > > > > Hi Brian, > > > > while this feature in general makes sense and sounds good, to start > > warming up display hardware early when something might start to happen, > > this particular proposal has many problems from UAPI perspective (as it > > has none). Comments below. > > > > Btw. if PSR is that slow to wake up from, how much do you actually gain > > from this input event watching? I would imagine the improvement to not > > be noticeable. > > > > I think some numbers about how much this feature helps would be really > > good, even if they are quite specific use cases. You also need to > > identify the userspace components, because I think different display > > servers are very different in their reaction speed. > > > > If KMS gets a pageflip or modeset in no time after an input event, then > > what's the gain. OTOH, if the display server is locking on to vblank, > > there might be a delay worth avoiding. But then, is it worth > > short-circuiting the wake-up in kernel vs. adding a new ioctl that > > userspace could hit to start the warming up process? > > In my measurements, it takes userspace a frame or two to respond and > get to the point of starting to build cmdstream (before eventually > doing atomic/pageflip ioctl).. possibly longer if you don't also have > a similar boost mechanism to spool up cpufreq > > But the important thing, IMO, is that atomic/pageflip ioctl is the > cumulation of a long sequence of events.. input-boost is letting > whatever it may be (PSR exit, GPU resume, etc) happen in parallel with > that long sequence. Right, exactly. That is why I was musing about a *new* ioctl that userspace could hit as soon as any input device fd (or network fd!) shows signs of life. Would that be enough, avoiding all the annoying questions about which input and DRM devices should participate here (and what about non-input devices that still want to trigger the warm-up, like network traffic, e.g. remote control?), or does it really need to be kernel internal to be fast enough? As Brian wrote about his quick hack to test that via debugfs, sounds like the userspace solution would be totally sufficient. Thanks, pq pgpzvK14VdrPL.pgp Description: OpenPGP digital signature
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi Pekka, Thanks for the thoughts and review. I've tried to respond below: On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote: > On Wed, 17 Nov 2021 14:48:40 -0800 > Brian Norris 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 > > --- > > Thanks Simon for the CC. > > Hi Brian, > > while this feature in general makes sense and sounds good, to start > warming up display hardware early when something might start to happen, > this particular proposal has many problems from UAPI perspective (as it > has none). Comments below. > > Btw. if PSR is that slow to wake up from, how much do you actually gain > from this input event watching? I would imagine the improvement to not > be noticeable. Patch 2 has details. It's not really about precisely how slow PSR is, but how much foresight we can gain: in patch 2, I note that with my particular user space and system, I can start PSR-exit 50ms earlier than I would otherweise. (FWIW, this measurement is exactly the same it was with the original version written 4 years ago.) For how long PSR-exit takes: the measurements I'm able to do (via ftrace) show that drm_self_refresh_transition() takes between 35 and 55 ms. That's noticeable at 60 fps. And quite conveniently, the input-boost manages to hide nearly 100% of that latency. Typical use cases where one notices PSR latency (and where this 35-55ms matters) involve simply moving a cursor; it's very noticeable when you have more than a few frames of latency to "get started". > I think some numbers about how much this feature helps would be really > good, even if they are quite specific use cases. You also need to > identify the userspace components, because I think different display > servers are very different in their reaction speed. If my email address isn't obvious, I'm testing Chrome OS. I'm frankly not that familiar with the user space display stack, but for what I know, it's rather custom, developed within the Chromium project. Others on CC here could probably give you more detail, if you want specific answers, besides docs like this: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/ozone_overview.md > If KMS gets a pageflip or modeset in no time after an input event, then > what's the gain. OTOH, if the display server is locking on to vblank, > there might be a delay worth avoiding. But then, is it worth > short-circuiting the wake-up in kernel vs. adding a new ioctl that > userspace could hit to start the warming up process? Rob responded to the first part to some extent (there is definitely gain to be had). To the last part: I wrote a simple debugfs hook to allow user space to force a PSR exit, and then a simple user space program to read input events and smash that debugfs file whenever it sees one. Testing in the same scenarios, this appears to lose less than 100 microseconds versus the in-kernel approach, which is negligible for this use case. (I'm not sure about the other use cases.) So, this is technically doable in user space. I can't speak to the ease of _actually_ integrating this into even our own Chrome display manager, but I highly doubt it will get integrated into others. I'd posit this should weigh into the relative worth, but otherwise can't really give you an answer there. I'd also note, software-directed PSR is so far designed to be completely opaque to user space. There's no way to disable it; no way to know it's active; and no way to know anything about the parameters it's computing (like average entry/exit delay). Would you suggest a whole set
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Thu, Nov 18, 2021 at 2:39 AM Pekka Paalanen wrote: > > On Wed, 17 Nov 2021 14:48:40 -0800 > Brian Norris wrote: > > > A variety of applications have found it useful to listen to > > user-initiated input events to make decisions within a DRM driver, given > > that input events are often the first sign that we're going to start > > doing latency-sensitive activities: > > > > * Panel self-refresh: software-directed self-refresh (e.g., with > >Rockchip eDP) is especially latency sensitive. In some cases, it can > >take 10s of milliseconds for a panel to exit self-refresh, which can > >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped > >with an input_handler boost, that preemptively exits self-refresh > >whenever there is input activity. > > > > * GPU drivers: on GPU-accelerated desktop systems, we may need to > >render new frames immediately after user activity. Powering up the > >GPU can take enough time that it is worthwhile to start this process > >as soon as there is input activity. Many Chrome OS systems also ship > >with an input_handler boost that powers up the GPU. > > > > This patch provides a small helper library that abstracts some of the > > input-subsystem details around picking which devices to listen to, and > > some other boilerplate. This will be used in the next patch to implement > > the first bullet: preemptive exit for panel self-refresh. > > > > Bits of this are adapted from code the Android and/or Chrome OS kernels > > have been carrying for a while. > > > > Signed-off-by: Brian Norris > > --- > > Thanks Simon for the CC. > > Hi Brian, > > while this feature in general makes sense and sounds good, to start > warming up display hardware early when something might start to happen, > this particular proposal has many problems from UAPI perspective (as it > has none). Comments below. > > Btw. if PSR is that slow to wake up from, how much do you actually gain > from this input event watching? I would imagine the improvement to not > be noticeable. > > I think some numbers about how much this feature helps would be really > good, even if they are quite specific use cases. You also need to > identify the userspace components, because I think different display > servers are very different in their reaction speed. > > If KMS gets a pageflip or modeset in no time after an input event, then > what's the gain. OTOH, if the display server is locking on to vblank, > there might be a delay worth avoiding. But then, is it worth > short-circuiting the wake-up in kernel vs. adding a new ioctl that > userspace could hit to start the warming up process? In my measurements, it takes userspace a frame or two to respond and get to the point of starting to build cmdstream (before eventually doing atomic/pageflip ioctl).. possibly longer if you don't also have a similar boost mechanism to spool up cpufreq But the important thing, IMO, is that atomic/pageflip ioctl is the cumulation of a long sequence of events.. input-boost is letting whatever it may be (PSR exit, GPU resume, etc) happen in parallel with that long sequence. BR, -R > > > > > Changes in v2: > > - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER > > - Remove void*; users should use container_of() > > - Document the callback context > > > > drivers/gpu/drm/Kconfig| 6 ++ > > drivers/gpu/drm/Makefile | 2 + > > drivers/gpu/drm/drm_input_helper.c | 143 + > > include/drm/drm_input_helper.h | 41 + > > 4 files changed, 192 insertions(+) > > create mode 100644 drivers/gpu/drm/drm_input_helper.c > > create mode 100644 include/drm/drm_input_helper.h > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index fb144617055b..381476b10a9d 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > > > If in doubt, say "N". > > > > +config DRM_INPUT_HELPER > > + def_bool y > > + depends on DRM_KMS_HELPER > > + depends on INPUT > > + > > config DRM_KMS_HELPER > > tristate > > depends on DRM > > + select DRM_INPUT_HELPER if INPUT > > help > > CRTC helpers for KMS drivers. > > > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index 1c41156deb5f..9a6494aa45e6 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o > > drm_crtc_helper.o drm_dp_helper.o \ > > drm_atomic_state_helper.o drm_damage_helper.o \ > > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > > + > > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) +=
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
Hi Daniel,
Thanks for the review. Lots to address elsewhere, but I can respond
here first:
On Thu, Nov 18, 2021 at 10:05:11AM +0100, Daniel Vetter wrote:
> On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:
> > --- a/drivers/gpu/drm/Kconfig
> > +++ b/drivers/gpu/drm/Kconfig
> > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST
> >
> > If in doubt, say "N".
> >
> > +config DRM_INPUT_HELPER
> > + def_bool y
> > + depends on DRM_KMS_HELPER
> > + depends on INPUT
>
> Uh please no configs for each thing, it just makes everything more
> complex. Do we _really_ need this?
First, it's not a configurable option (a user will never see this nor
have to answer Y/N to it); it only serves as an intermediary to express
the CONFIG_INPUT dependency (which is necessary) without making
DRM_KMS_HELPER fully depend on CONFIG_INPUT. (We should be able to run
display stacks without the input subsystem.)
The closest alternative I can think of with fewer Kconfig symbols is to
just use CONFIG_INPUT directly in the code, to decide whether to provide
the helpers or else just stub them out. But that has a problem of not
properly expressing the =m vs. =y necessity: if, for example,
CONFIG_DRM_KMS_HELPER=y and CONFIG_INPUT=m, then we'll have linker
issues.
In short, yes, I think we really need this. But I'm not a Kbuild expert.
> > diff --git a/include/drm/drm_input_helper.h b/include/drm/drm_input_helper.h
> > new file mode 100644
> > index ..7904f397b934
> > --- /dev/null
> > +++ b/include/drm/drm_input_helper.h
> > @@ -0,0 +1,41 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2021 Google, Inc.
> > + */
> > +#ifndef __DRM_INPUT_HELPER_H__
> > +#define __DRM_INPUT_HELPER_H__
> > +
> > +#include
> > +
> > +struct drm_device;
> > +
> > +struct drm_input_handler {
> > + /*
> > +* Callback to call for input activity. Will be called in an atomic
> > +* context.
>
> How atomic? Like hardirq, and nasty spinlocks held?
Maybe I should have just cribbed off the doc:
* @event: event handler. This method is being called by input core with
* interrupts disabled and dev->event_lock spinlock held and so
* it may not sleep
I probably don't want to propagate the subsystem details about which
locks, but I guess I can be specific about "interrupts disabled" and
"don't sleep".
> > +*/
> > + void (*callback)(struct drm_input_handler *handler);
> > +
> > + struct input_handler handler;
> > +};
> > +
> > +#if defined(CONFIG_DRM_INPUT_HELPER)
> > +
> > +int drm_input_handle_register(struct drm_device *dev,
> > + struct drm_input_handler *handler);
> > +void drm_input_handle_unregister(struct drm_input_handler *handler);
> > +
> > +#else /* !CONFIG_DRM_INPUT_HELPER */
> > +
> > +static inline int drm_input_handle_register(struct drm_device *dev,
> > + struct drm_input_handler *handler)
> > +{
> > + return 0;
> > +}
>
> I guess the reason behind the helper is that you also want to use this in
> drivers or maybe drm/sched?
I think my reasoning is heavily described in both the cover letter and
the commit message. If that's not clear, can you point out which part?
I'd gladly improve it :)
But specifically, see the 2nd bullet from the commit message, which I've
re-quoted down here:
> > * GPU drivers: on GPU-accelerated desktop systems, we may need to
> >render new frames immediately after user activity. Powering up the
> >GPU can take enough time that it is worthwhile to start this process
> >as soon as there is input activity. Many Chrome OS systems also ship
> >with an input_handler boost that powers up the GPU.
Rob Clark has patches to drm/msm to boost GPU power-up via a similar
helper.
> Anyway I think it looks all reasonable. Definitely need an ack from input
> people
I realized I failed to carry Dmitry's Ack from version 1 [1]. If this
has a v3 in similar form, I'll carry it there.
> that the event list you have is a good choice, I have no idea what
> that all does. Maybe also document that part a bit more.
I'm admittedly not an expert there, and this is actually one reason why
we hoped to make this a library (that nobody wants to keep figuring out
whether all those flags, etc., are really doing the right thing), but
there are comments about what each entry is _trying_ to do. Are you
suggesting more, as in, why "BTN_LEFT + EV_KEY" means "pointer"? Or why
we match certain devices (because they represent likely user activity
that will affect the display pipeline)? Or both? Anyway, I'll give it a
shot, if we keep this.
Brian
[1] https://lore.kernel.org/all/yyw6fwsenmk25...@google.com/
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Wed, 17 Nov 2021 14:48:40 -0800 Brian Norris wrote: > A variety of applications have found it useful to listen to > user-initiated input events to make decisions within a DRM driver, given > that input events are often the first sign that we're going to start > doing latency-sensitive activities: > > * Panel self-refresh: software-directed self-refresh (e.g., with >Rockchip eDP) is especially latency sensitive. In some cases, it can >take 10s of milliseconds for a panel to exit self-refresh, which can >be noticeable. Rockchip RK3399 Chrome OS systems have always shipped >with an input_handler boost, that preemptively exits self-refresh >whenever there is input activity. > > * GPU drivers: on GPU-accelerated desktop systems, we may need to >render new frames immediately after user activity. Powering up the >GPU can take enough time that it is worthwhile to start this process >as soon as there is input activity. Many Chrome OS systems also ship >with an input_handler boost that powers up the GPU. > > This patch provides a small helper library that abstracts some of the > input-subsystem details around picking which devices to listen to, and > some other boilerplate. This will be used in the next patch to implement > the first bullet: preemptive exit for panel self-refresh. > > Bits of this are adapted from code the Android and/or Chrome OS kernels > have been carrying for a while. > > Signed-off-by: Brian Norris > --- Thanks Simon for the CC. Hi Brian, while this feature in general makes sense and sounds good, to start warming up display hardware early when something might start to happen, this particular proposal has many problems from UAPI perspective (as it has none). Comments below. Btw. if PSR is that slow to wake up from, how much do you actually gain from this input event watching? I would imagine the improvement to not be noticeable. I think some numbers about how much this feature helps would be really good, even if they are quite specific use cases. You also need to identify the userspace components, because I think different display servers are very different in their reaction speed. If KMS gets a pageflip or modeset in no time after an input event, then what's the gain. OTOH, if the display server is locking on to vblank, there might be a delay worth avoiding. But then, is it worth short-circuiting the wake-up in kernel vs. adding a new ioctl that userspace could hit to start the warming up process? > > Changes in v2: > - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER > - Remove void*; users should use container_of() > - Document the callback context > > drivers/gpu/drm/Kconfig| 6 ++ > drivers/gpu/drm/Makefile | 2 + > drivers/gpu/drm/drm_input_helper.c | 143 + > include/drm/drm_input_helper.h | 41 + > 4 files changed, 192 insertions(+) > create mode 100644 drivers/gpu/drm/drm_input_helper.c > create mode 100644 include/drm/drm_input_helper.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index fb144617055b..381476b10a9d 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST > > If in doubt, say "N". > > +config DRM_INPUT_HELPER > + def_bool y > + depends on DRM_KMS_HELPER > + depends on INPUT > + > config DRM_KMS_HELPER > tristate > depends on DRM > + select DRM_INPUT_HELPER if INPUT > help > CRTC helpers for KMS drivers. > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index 1c41156deb5f..9a6494aa45e6 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o > drm_crtc_helper.o drm_dp_helper.o \ > drm_atomic_state_helper.o drm_damage_helper.o \ > drm_format_helper.o drm_self_refresh_helper.o drm_rect.o > > +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o > + > drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > diff --git a/drivers/gpu/drm/drm_input_helper.c > b/drivers/gpu/drm/drm_input_helper.c > new file mode 100644 > index ..470f90865c7c > --- /dev/null > +++ b/drivers/gpu/drm/drm_input_helper.c > @@ -0,0 +1,143 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Copyright (C) 2021 Google, Inc. > + */ > +#include > +#include > + > +#include > +#include > + > +/** > + * 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. > +
Re: [PATCH v2 1/2] drm/input_helper: Add new input-handling helper
On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:
> A variety of applications have found it useful to listen to
> user-initiated input events to make decisions within a DRM driver, given
> that input events are often the first sign that we're going to start
> doing latency-sensitive activities:
>
> * Panel self-refresh: software-directed self-refresh (e.g., with
>Rockchip eDP) is especially latency sensitive. In some cases, it can
>take 10s of milliseconds for a panel to exit self-refresh, which can
>be noticeable. Rockchip RK3399 Chrome OS systems have always shipped
>with an input_handler boost, that preemptively exits self-refresh
>whenever there is input activity.
>
> * GPU drivers: on GPU-accelerated desktop systems, we may need to
>render new frames immediately after user activity. Powering up the
>GPU can take enough time that it is worthwhile to start this process
>as soon as there is input activity. Many Chrome OS systems also ship
>with an input_handler boost that powers up the GPU.
>
> This patch provides a small helper library that abstracts some of the
> input-subsystem details around picking which devices to listen to, and
> some other boilerplate. This will be used in the next patch to implement
> the first bullet: preemptive exit for panel self-refresh.
>
> Bits of this are adapted from code the Android and/or Chrome OS kernels
> have been carrying for a while.
>
> Signed-off-by: Brian Norris
> ---
>
> Changes in v2:
> - Honor CONFIG_INPUT dependency, via new CONFIG_DRM_INPUT_HELPER
> - Remove void*; users should use container_of()
> - Document the callback context
>
> drivers/gpu/drm/Kconfig| 6 ++
> drivers/gpu/drm/Makefile | 2 +
> drivers/gpu/drm/drm_input_helper.c | 143 +
> include/drm/drm_input_helper.h | 41 +
Please add documentation for this and include it under
Documentation/gpu/drm-kms-helpers.rst in a suitable place.
Standards for core code should be overview DOC: with references to key
functions/structs, and all driver visible structs, functions (static
inline in header or exported) fully documented.
> 4 files changed, 192 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_input_helper.c
> create mode 100644 include/drm/drm_input_helper.h
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index fb144617055b..381476b10a9d 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -79,9 +79,15 @@ config DRM_DEBUG_SELFTEST
>
> If in doubt, say "N".
>
> +config DRM_INPUT_HELPER
> + def_bool y
> + depends on DRM_KMS_HELPER
> + depends on INPUT
Uh please no configs for each thing, it just makes everything more
complex. Do we _really_ need this?
> +
> config DRM_KMS_HELPER
> tristate
> depends on DRM
> + select DRM_INPUT_HELPER if INPUT
> help
> CRTC helpers for KMS drivers.
>
> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
> index 1c41156deb5f..9a6494aa45e6 100644
> --- a/drivers/gpu/drm/Makefile
> +++ b/drivers/gpu/drm/Makefile
> @@ -56,6 +56,8 @@ drm_kms_helper-y := drm_bridge_connector.o
> drm_crtc_helper.o drm_dp_helper.o \
> drm_atomic_state_helper.o drm_damage_helper.o \
> drm_format_helper.o drm_self_refresh_helper.o drm_rect.o
>
> +drm_kms_helper-$(CONFIG_DRM_INPUT_HELPER) += drm_input_helper.o
> +
> drm_kms_helper-$(CONFIG_DRM_PANEL_BRIDGE) += bridge/panel.o
> drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o
> drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o
> diff --git a/drivers/gpu/drm/drm_input_helper.c
> b/drivers/gpu/drm/drm_input_helper.c
> new file mode 100644
> index ..470f90865c7c
> --- /dev/null
> +++ b/drivers/gpu/drm/drm_input_helper.c
> @@ -0,0 +1,143 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2021 Google, Inc.
> + */
> +#include
> +#include
> +
> +#include
> +#include
> +
> +/**
> + * DOC: overview
> + *
> + * This helper library provides a thin wrapper around input handles, so that
> + * DRM drivers can easily perform domain-specific actions in response to user
> + * activity. e.g., if someone is moving a mouse, we're likely to want to
> + * display something soon, and we should exit panel self-refresh.
> + */
> +
> +static void drm_input_event(struct input_handle *handle, unsigned int type,
> + unsigned int code, int value)
> +{
> + struct drm_input_handler *handler = handle->handler->private;
> +
> + handler->callback(handler);
> +}
> +
> +static int drm_input_connect(struct input_handler *handler,
> + struct input_dev *dev,
> + const struct input_device_id *id)
> +{
> + struct input_handle *handle;
> + int error;
> +
> + handle = kzalloc(sizeof(struct input_handle), GFP_KERNEL);
> + if (!handle)
> +
