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