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

[Pekka Paalanen]

On Fri, 19 Nov 2021 16:56:01 +0100
Daniel Vetter <daniel@xxxxxxxx> 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 <briannorris@xxxxxxxxxxxx> wrote:
> >   
> > > Hi Pekka,
> > > 
> > > Thanks for the thoughts and review. I've tried to respond below:
> > > 
> > > On Thu, Nov 18, 2021 at 12:39:28PM +0200, Pekka Paalanen wrote:  
> > > > On Wed, 17 Nov 2021 14:48:40 -0800
> > > > Brian Norris <briannorris@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > > > ---    
> > > > 
> > > > 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

Attachment: pgp0yWC4Yw4Kd.pgp
Description: OpenPGP digital signature