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 000000000000..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 <linux/input.h>
> > +
> > +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 <linux/input.h> 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/YYW6FwSeNMK25ENm@xxxxxxxxxx/
