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

[Peter Hutterer <peter.hutterer@who-t.net>]

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 <briannorris@chromium.org> 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 <briannorris@chromium.org> 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-MkPkNdQ@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) 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/.

you can also unbind devices and use hidraw directly.

> And even if they register as input devices, do they match the ID list in
> this patch?

device type assignment is problematic, but i think in this case it doesn't
matter if the associations are a bit rough. You don't care about the type of
device, you merely care about "is this likely to be used". And I think for
that the list is good enough.

though tbh, having this policy in userspace would still be better IMO.

Cheers,
  Peter

> > Even audio sockets are input devices for detecting whether a plug has
> > been plugged in, but those probably wouldn't flood anything.
> 
> They also won't match the input_handler ID list, because they won't
> support the key or position combinations in the heuristic.
> 
> > > > Yet another problem here is that this completely ignores the concept of
> > > > physical seats. Of course it does so, because seats are a pure
> > > > userspace concept.
> > > > 
> > > > The kernel VT console already has problems because the kernel has no
> > > > concept of seats, which means that if there is a second seat defined and
> > > > a desktop running on it, while the first seat is in the normal VT text
> > > > mode, then everything typed in the desktop will be delivered to the VT
> > > > shell as well! (This has a possible workaround in userspace [1], by opening
> > > > the evdev input devices in some kind of exclusive mode - which is not
> > > > common practise AFAIK.)  
> > > 
> > > Sure.
> > > 
> > > I'd bet the intersection of systems that use SW-directed PSR and
> > > "multi-seat" is negligibly close to zero, but I can't guarantee that.
> > > Chalk one up for a user space policy.
> > 
> > Your cover letter has also the other bullet point: ramping up GPUs.
> > That applies to a lot more systems than PSR, right?
> > 
> > Maybe that is an acceptable trade-off: be 100 µs faster (your
> > measurement) by ramping up all GPUs in a system instead of only the
> > relevant ones?
> > 
> > Or maybe that will hurt normal gaming computers by ramping up the iGPU
> > when the OS and game only uses the dGPU, which makes iGPU eat away the
> > CPU power budget, causing the CPU to slow down? I suppose that would be
> > handled by ramping up only GPUs that userspace has opened.
> 
> FWIW, the current work we have out-of-tree involves only select GPU
> drivers that know they are slow to ramp up. If this were generalized,
> then yes, it could potentially have undesireable side effects. I'm
> certainly not an expert on Rob's work though, so I can't speak to this
> very much, but I imagine we could resolve the {d,i}GPU problem easily.
> 
> Brian