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:

quoted

On Wed, Nov 17, 2021 at 02:48:40PM -0800, Brian Norris wrote:

quoted

--- 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.

quoted

quoted

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

quoted

quoted

+	 */
+	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:

quoted

quoted

 * 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.

quoted

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.

quoted

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@google.com/ (local)

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch