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 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".
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.
>
> 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.
So maybe this is all very obvious for input folks, and comments about what
each does is overkill.
But I think in the kerneldoc for gfx folks it would be good to explain
what kind of events this listens for, like iirc you listen to key-up not
key-down, since often the boost has expired by the time the key is
actually lifted? Stuff like that I think would be good to explain the why
behind the choice of entries in the list. Or that we try to listen to some
pointer/mouse events (all of them? only "important" ones?)
-Daniel
>
> Brian
>
> [1] https://lore.kernel.org/all/YYW6FwSeNMK25ENm@xxxxxxxxxx/
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
