[RFCv5 0/2] HID: Add USI support

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [RFCv5 0/2] HID: Add USI support
@ 2021-12-15 13:42 Tero Kristo
  2021-12-15 13:42 ` [RFCv5 1/2] HID: core: Add support for USI style events Tero Kristo
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Tero Kristo @ 2021-12-15 13:42 UTC (permalink / raw)
  To: linux-input, benjamin.tissoires, jikos, tero.kristo
  Cc: linux-kernel, dmitry.torokhov, peter.hutterer

Hi,

These two patches add the missing pieces for HID USI support. First one
adds the HID core changes to support the new Misc events for pen ID,
line color and line style. The second patch adds a BPF program on top of
the HID-BPF driver which adds support for writing the Pen parameters
from userspace, and to add filtering of HID low level events for ELAN
USI controller. The BPF programs are not built by the kernel as of now
(there are no Makefile changes), as there is a plan to most likely
integrate these to a kernel external repository. I have tested these in
my own external build setup though, and I can provide the makefile for
that if needed. Also a sample client program is provided for
communicating with the D-BUS server.

I have also a kernel testing branch available at [1], which contains a
few fix patches on top of Benjamin's HID-BPF driver work, and is rebased
on top of latest hid/for-next. The HID-BPF fixes have been cleaned up a
bit compared to previous setup. There are also a couple of new patches
for adding support for a delayed_work BPF program on top of the
hid-bpf driver; this is used to execute the raw_requests in non-irq
context.

-Tero

[1] https://github.com/t-kristo/linux/tree/usi-5.16-v5-bpf

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [RFCv5 1/2] HID: core: Add support for USI style events
  2021-12-15 13:42 [RFCv5 0/2] HID: Add USI support Tero Kristo
@ 2021-12-15 13:42 ` Tero Kristo
  2021-12-15 13:42 ` [RFCv5 2/2] samples: hid-bpf: add HID USI samples Tero Kristo
  2021-12-16 10:36 ` [RFCv5 0/2] HID: Add USI support Benjamin Tissoires
  2 siblings, 0 replies; 6+ messages in thread
From: Tero Kristo @ 2021-12-15 13:42 UTC (permalink / raw)
  To: linux-input, benjamin.tissoires, jikos, tero.kristo
  Cc: linux-kernel, dmitry.torokhov, peter.hutterer

Add support for Universal Stylus Interface (USI) style events to the HID
core and input layers.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 drivers/hid/hid-debug.c                |  9 +++++-
 drivers/hid/hid-input.c                | 40 ++++++++++++++++++++++++++
 include/linux/mod_devicetable.h        |  2 +-
 include/uapi/linux/input-event-codes.h | 26 +++++++++++------
 4 files changed, 67 insertions(+), 10 deletions(-)

diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index 26c31d759914..18035c63c358 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1025,7 +1025,14 @@ static const char *absolutes[ABS_CNT] = {
 
 static const char *misc[MSC_MAX + 1] = {
 	[MSC_SERIAL] = "Serial",	[MSC_PULSELED] = "Pulseled",
-	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData"
+	[MSC_GESTURE] = "Gesture",	[MSC_RAW] = "RawData",
+	[MSC_PEN_ID] = "PenID",		[MSC_PEN_COLOR] "PenColor",
+	[MSC_PEN_LINE_STYLE_INK] = "PenLineStyleInk",
+	[MSC_PEN_LINE_STYLE_PENCIL] = "PenLineStylePencil",
+	[MSC_PEN_LINE_STYLE_HIGHLIGHTER] = "PenLineStyleHighlighter",
+	[MSC_PEN_LINE_STYLE_CHISEL_MARKER] = "PenLineStyleChiselMarker",
+	[MSC_PEN_LINE_STYLE_BRUSH] = "PenLineStyleBrush",
+	[MSC_PEN_LINE_STYLE_NO_PREFERENCE] = "PenLineStyleNoPreference",
 };
 
 static const char *leds[LED_MAX + 1] = {
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 1ce75e8b49d5..52c581324491 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -833,6 +833,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			}
 			break;
 
+		case 0x38: /* Transducer Index */
+			map_msc(MSC_PEN_ID);
+			goto ignore;
+
 		case 0x3b: /* Battery Strength */
 			hidinput_setup_battery(device, HID_INPUT_REPORT, field, false);
 			usage->type = EV_PWR;
@@ -880,6 +884,42 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			map_msc(MSC_SERIAL);
 			break;
 
+		case 0x5c: /* Digitizer Preferred Color */
+			map_msc(MSC_PEN_COLOR);
+			break;
+
+		case 0x5e: /* Digitizer Preferred Line Width */
+			map_abs_clear(ABS_TOOL_WIDTH);
+			break;
+
+		case 0x70: /* Preferred Line Style -> not an input usage */
+		case 0x71: /* Preferred Line Style is Locked */
+			goto ignore;
+
+		case 0x72: /* Ink */
+			map_msc(MSC_PEN_LINE_STYLE_INK);
+			break;
+
+		case 0x73: /* Pencil */
+			map_msc(MSC_PEN_LINE_STYLE_PENCIL);
+			break;
+
+		case 0x74: /* Highlighter */
+			map_msc(MSC_PEN_LINE_STYLE_HIGHLIGHTER);
+			break;
+
+		case 0x75: /* Chisel Marker */
+			map_msc(MSC_PEN_LINE_STYLE_CHISEL_MARKER);
+			break;
+
+		case 0x76: /* Brush */
+			map_msc(MSC_PEN_LINE_STYLE_BRUSH);
+			break;
+
+		case 0x77: /* No Preference */
+			map_msc(MSC_PEN_LINE_STYLE_NO_PREFERENCE);
+			break;
+
 		default:  goto unknown;
 		}
 		break;
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 4bb71979a8fd..bf2b76a6e7e8 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -322,7 +322,7 @@ struct pcmcia_device_id {
 #define INPUT_DEVICE_ID_KEY_MAX		0x2ff
 #define INPUT_DEVICE_ID_REL_MAX		0x0f
 #define INPUT_DEVICE_ID_ABS_MAX		0x3f
-#define INPUT_DEVICE_ID_MSC_MAX		0x07
+#define INPUT_DEVICE_ID_MSC_MAX		0x0d
 #define INPUT_DEVICE_ID_LED_MAX		0x0f
 #define INPUT_DEVICE_ID_SND_MAX		0x07
 #define INPUT_DEVICE_ID_FF_MAX		0x7f
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 225ec87d4f22..0ebfc92dacf5 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -901,14 +901,24 @@
  * Misc events
  */
 
-#define MSC_SERIAL		0x00
-#define MSC_PULSELED		0x01
-#define MSC_GESTURE		0x02
-#define MSC_RAW			0x03
-#define MSC_SCAN		0x04
-#define MSC_TIMESTAMP		0x05
-#define MSC_MAX			0x07
-#define MSC_CNT			(MSC_MAX+1)
+#define MSC_SERIAL			0x00
+#define MSC_PULSELED			0x01
+#define MSC_GESTURE			0x02
+#define MSC_RAW				0x03
+#define MSC_SCAN			0x04
+#define MSC_TIMESTAMP			0x05
+/* USI Pen events */
+#define MSC_PEN_ID			0x06
+#define MSC_PEN_COLOR			0x07
+#define MSC_PEN_LINE_STYLE_INK			0x08
+#define MSC_PEN_LINE_STYLE_PENCIL		0x09
+#define MSC_PEN_LINE_STYLE_HIGHLIGHTER		0x0a
+#define MSC_PEN_LINE_STYLE_CHISEL_MARKER	0x0b
+#define MSC_PEN_LINE_STYLE_BRUSH		0x0c
+#define MSC_PEN_LINE_STYLE_NO_PREFERENCE	0x0d
+/* TODO: Add USI diagnostic & battery events too */
+#define MSC_MAX				0x0d
+#define MSC_CNT				(MSC_MAX + 1)
 
 /*
  * LEDs
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [RFCv5 2/2] samples: hid-bpf: add HID USI samples
  2021-12-15 13:42 [RFCv5 0/2] HID: Add USI support Tero Kristo
  2021-12-15 13:42 ` [RFCv5 1/2] HID: core: Add support for USI style events Tero Kristo
@ 2021-12-15 13:42 ` Tero Kristo
  2021-12-16 10:36 ` [RFCv5 0/2] HID: Add USI support Benjamin Tissoires
  2 siblings, 0 replies; 6+ messages in thread
From: Tero Kristo @ 2021-12-15 13:42 UTC (permalink / raw)
  To: linux-input, benjamin.tissoires, jikos, tero.kristo
  Cc: linux-kernel, dmitry.torokhov, peter.hutterer

These samples add HID USI client and server code. These are not compiled
within kernel tree and are provided strictly as RFC only; the target for
these is to build them external to the kernel tree. The provided samples
run HID USI client-server communication over D-BUS.

Signed-off-by: Tero Kristo <tero.kristo@linux.intel.com>
---
 samples/bpf/hid_usi.h             |  21 ++
 samples/bpf/hid_usi_client.c      | 154 +++++++++++
 samples/bpf/hid_usi_server.c      | 438 ++++++++++++++++++++++++++++++
 samples/bpf/hid_usi_server_kern.c | 267 ++++++++++++++++++
 4 files changed, 880 insertions(+)
 create mode 100644 samples/bpf/hid_usi.h
 create mode 100644 samples/bpf/hid_usi_client.c
 create mode 100644 samples/bpf/hid_usi_server.c
 create mode 100644 samples/bpf/hid_usi_server_kern.c

diff --git a/samples/bpf/hid_usi.h b/samples/bpf/hid_usi.h
new file mode 100644
index 000000000000..90803375d9c1
--- /dev/null
+++ b/samples/bpf/hid_usi.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * Copyright(c) 2021 Intel Corporation.
+ */
+
+#ifndef HID_USI_H_
+#define HID_USI_H_
+
+#include <linux/bits.h>
+
+enum {
+	USI_PEN_ID = 0,
+	USI_PEN_IN_RANGE,
+	USI_PEN_TOUCHING,
+	USI_PEN_COLOR,
+	USI_PEN_LINE_WIDTH,
+	USI_PEN_LINE_STYLE,
+	USI_NUM_PARAMS
+};
+
+#endif /* HID_USI_H */
diff --git a/samples/bpf/hid_usi_client.c b/samples/bpf/hid_usi_client.c
new file mode 100644
index 000000000000..81a956798f62
--- /dev/null
+++ b/samples/bpf/hid_usi_client.c
@@ -0,0 +1,154 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Intel Corporation
+ */
+
+#include <stdio.h>
+#include <gio/gio.h>
+#include <getopt.h>
+
+static void usi_var_get(GDBusProxy *proxy, const char *var)
+{
+	GVariant *result;
+	unsigned int val;
+	const GVariantType *type;
+	char *str;
+
+	result = g_dbus_proxy_get_cached_property(proxy, var);
+	type = g_variant_get_type(result);
+	if (g_variant_type_equal(type, G_VARIANT_TYPE_UINT32)) {
+		g_variant_get(result, "u", &val);
+		printf("Value for %s (u): %u\n", var, val);
+	} else if (g_variant_type_equal(type, G_VARIANT_TYPE_STRING)) {
+		g_variant_get(result, "&s", &str);
+		printf("Value for %s (s): %s\n", var, str);
+	} else {
+		printf("Unsupported type %s for %s\n",
+		       g_variant_get_type_string(result), var);
+	}
+	g_variant_unref(result);
+}
+
+static void usi_dump_vars(GDBusProxy *proxy)
+{
+	gchar **vars;
+	int i;
+
+	vars = g_dbus_proxy_get_cached_property_names(proxy);
+
+	i = 0;
+	while (vars && vars[i]) {
+		usi_var_get(proxy, vars[i]);
+		i++;
+	}
+
+	g_strfreev(vars);
+}
+
+static void usi_var_set(GDBusProxy *proxy, const char *var, unsigned int value)
+{
+	GError *error = NULL;
+
+	g_dbus_proxy_call_sync(proxy,
+			       "org.freedesktop.DBus.Properties.Set",
+			       g_variant_new("(su)", var, value),
+			       G_DBUS_CALL_FLAGS_NONE, -1, NULL, &error);
+
+	g_assert_no_error(error);
+}
+
+static void usage(void)
+{
+	extern const char *__progname;
+
+	printf("Usage: %s [--help] [--dump] [--color] [--width] [--style] [value]\n",
+	       __progname);
+
+	printf("\nOptions:\n");
+	printf("    --help, -h          this help text\n");
+	printf("    --color [value]     gets/sets stylus color\n");
+	printf("    --width [value]     gets/sets stylus line width\n");
+	printf("    --style [value]     gets/sets stylus line style\n");
+	printf("    --dump              dump all variables\n");
+}
+
+int main(int argc, char *argv[])
+{
+	GDBusProxy *proxy;
+	GDBusConnection *conn;
+	GError *error = NULL;
+	const char *version;
+	GVariant *variant;
+	const char *var = NULL;
+	static struct option options[] = {
+		{ "help", no_argument, NULL, 'h' },
+		{ "color", no_argument, NULL, 'c' },
+		{ "width", no_argument, NULL, 'w' },
+		{ "style", no_argument, NULL, 's' },
+		{ "dump", no_argument, NULL, 'd' },
+	};
+	int opt;
+	int retval = 0;
+	unsigned int value;
+
+	conn = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL, &error);
+	g_assert_no_error(error);
+
+	proxy = g_dbus_proxy_new_sync(conn,
+				      G_DBUS_PROXY_FLAGS_NONE,
+				      NULL,				/* GDBusInterfaceInfo */
+				      "org.universalstylus.PenServer",		/* name */
+				      "/org/universalstylus/Pen",	/* object path */
+				      "org.universalstylus.PenInterface",	/* interface */
+				      NULL,				/* GCancellable */
+				      &error);
+	g_assert_no_error(error);
+
+	/* read the version property of the interface */
+	variant = g_dbus_proxy_get_cached_property(proxy, "Version");
+	g_assert(variant != NULL);
+	g_variant_get(variant, "s", &version);
+	g_variant_unref(variant);
+	printf("Server version v%s\n", version);
+
+	while ((opt = getopt_long(argc, argv, "hcwsd", options, NULL)) != -1) {
+		switch (opt) {
+		case 'c':
+			var = "LineColor";
+			break;
+		case 'w':
+			var = "LineWidth";
+			break;
+		case 's':
+			var = "LineStyle";
+			break;
+		case 'd':
+			usi_dump_vars(proxy);
+			goto exit;
+
+		default:
+			usage();
+			retval = EXIT_FAILURE;
+			goto exit;
+		}
+	}
+
+	if (!var) {
+		usage();
+		retval = EXIT_FAILURE;
+		goto exit;
+	}
+	if (argc == optind) {
+		usi_var_get(proxy, var);
+	} else if (argc == optind + 1) {
+		value = atoi(argv[argc - 1]);
+		usi_var_set(proxy, var, value);
+	} else {
+		usage();
+		retval = EXIT_FAILURE;
+	}
+
+exit:
+	g_object_unref(proxy);
+	g_object_unref(conn);
+	return retval;
+}
diff --git a/samples/bpf/hid_usi_server.c b/samples/bpf/hid_usi_server.c
new file mode 100644
index 000000000000..6d2012b018ac
--- /dev/null
+++ b/samples/bpf/hid_usi_server.c
@@ -0,0 +1,438 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2021, Intel Corporation
+ */
+#include <linux/bpf.h>
+#include <linux/if_link.h>
+#include <assert.h>
+#include <errno.h>
+#include <fcntl.h>
+#include <signal.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+#include <unistd.h>
+#include <libgen.h>
+#include <sys/resource.h>
+#include <getopt.h>
+#include <sys/socket.h>
+#include <sys/un.h>
+#include <dbus/dbus.h>
+#include <dbus/dbus-glib-lowlevel.h>
+#include <glib.h>
+
+#include "bpf_util.h"
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include <sys/stat.h>
+
+#include "hid_usi.h"
+
+static const char *version = "0.5";
+static GMainLoop *mainloop;
+
+static char *sysfs_path;
+static int sysfs_fd;
+static int prog_count;
+static int cache, wr_cache;
+
+static const struct option long_options[] = {
+	{ "help", no_argument, NULL, 'h' },
+};
+
+struct prog {
+	int fd;
+	enum bpf_attach_type type;
+};
+
+static struct prog progs[10];
+
+static const char *server_introspection_xml =
+	DBUS_INTROSPECT_1_0_XML_DOCTYPE_DECL_NODE
+	"<node>\n"
+	"  <interface name='org.freedesktop.DBus.Introspectable'>\n"
+	"    <method name='Introspect'>\n"
+	"      <arg name='data' type='s' direction='out' />\n"
+	"    </method>\n"
+	"  </interface>\n"
+
+	"  <interface name='org.freedesktop.DBus.Properties'>\n"
+	"    <method name='Get'>\n"
+	"      <arg name='property'  type='s' direction='in' />\n"
+	"      <arg name='value'     type='s' direction='out' />\n"
+	"    </method>\n"
+	"    <method name='Set'>\n"
+	"      <arg name='property'  type='s' direction='in' />\n"
+	"      <arg name='value'     type='u' direction='in' />\n"
+	"    </method>\n"
+	"    <method name='GetAll'>\n"
+	"      <arg name='properties' type='a{sv}' direction='out'/>\n"
+	"    </method>\n"
+	"  </interface>\n"
+
+	"  <interface name='org.universalstylus.PenInterface'>\n"
+	"    <property name='Version' type='s' access='read' />\n"
+	"    <property name='LineColor' type='u' access='readwrite' />\n"
+	"    <property name='LineWidth' type='u' access='readwrite' />\n"
+	"    <property name='LineStyle' type='u' access='readwrite' />\n"
+	"  </interface>\n"
+	"</node>\n";
+
+static void int_exit(int sig)
+{
+	int ret;
+
+	for (prog_count--; prog_count >= 0 ; prog_count--) {
+		ret = bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type);
+		if (ret)
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	close(sysfs_fd);
+	exit(0);
+}
+
+static void usage(const char *prog)
+{
+	fprintf(stderr,
+		"usage: %s /sys/bus/hid/devices/<dev>/modalias\n\n",
+		prog);
+}
+
+static int param_to_idx(const char *param)
+{
+	if (!strcmp(param, "LineColor"))
+		return USI_PEN_COLOR;
+	if (!strcmp(param, "LineWidth"))
+		return USI_PEN_LINE_WIDTH;
+	if (!strcmp(param, "LineStyle"))
+		return USI_PEN_LINE_STYLE;
+
+	return -EINVAL;
+}
+
+static int write_value(const char *param, int value)
+{
+	int err;
+	int idx = param_to_idx(param);
+
+	printf("%s: param=%s (%d), value=%d\n", __func__, param, idx, value);
+	err = bpf_map_update_elem(wr_cache, &idx, &value, BPF_ANY);
+	if (err) {
+		printf("Update failed for %d, err=%d\n", idx, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static int read_value(const char *param)
+{
+	int value = -ENOENT;
+	int idx = param_to_idx(param);
+
+	bpf_map_lookup_elem(cache, &idx, &value);
+
+	return value;
+}
+
+static DBusHandlerResult usi_set_prop(DBusConnection *conn, DBusError *err,
+				      DBusMessage *msg, DBusMessage *reply)
+{
+	const char *property;
+	unsigned int value;
+	int ret;
+
+	if (!dbus_message_get_args(msg, err,
+				   DBUS_TYPE_STRING, &property,
+				   DBUS_TYPE_UINT32, &value,
+				   DBUS_TYPE_INVALID))
+		return -1;
+
+	if (write_value(property, value) < 0)
+		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+
+	return DBUS_HANDLER_RESULT_HANDLED;
+}
+
+static DBusHandlerResult usi_get_prop(DBusConnection *conn, DBusError *err,
+				      DBusMessage *msg, DBusMessage *reply)
+{
+	const char *property;
+	int value;
+
+	if (!dbus_message_get_args(msg, err,
+				   DBUS_TYPE_STRING, &property,
+				   DBUS_TYPE_INVALID))
+		return -1;
+
+	if (!strcmp(property, "Version")) {
+		dbus_message_append_args(reply,
+					 DBUS_TYPE_STRING, &version,
+					 DBUS_TYPE_INVALID);
+	} else {
+		value = read_value(property);
+		if (value < 0)
+			return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+
+		dbus_message_append_args(reply,
+					 DBUS_TYPE_UINT32, &value,
+					 DBUS_TYPE_INVALID);
+	}
+
+	return DBUS_HANDLER_RESULT_HANDLED;
+}
+
+static DBusHandlerResult usi_get_all_props(DBusConnection *conn, DBusError *err,
+					   DBusMessage *reply)
+{
+	DBusMessageIter array, dict, iter, variant;
+	const char *props[] = {
+		"Version", "LineColor", "LineWidth", "LineStyle"
+	};
+	const char *types[] = { "s", "u", "u", "u" };
+	int i;
+	unsigned int value;
+	void *ptr;
+	int itype;
+
+	dbus_message_iter_init_append(reply, &iter);
+	dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY, "{sv}", &array);
+
+	for (i = 0; i < ARRAY_SIZE(props); i++) {
+		dbus_message_iter_open_container(&array, DBUS_TYPE_DICT_ENTRY, NULL, &dict);
+		dbus_message_iter_append_basic(&dict, DBUS_TYPE_STRING, &props[i]);
+		dbus_message_iter_open_container(&dict, DBUS_TYPE_VARIANT, types[i], &variant);
+		if (i == 0) {
+			ptr = &version;
+			itype = DBUS_TYPE_STRING;
+		} else {
+			value = read_value(props[i]);
+			ptr = &value;
+			itype = DBUS_TYPE_UINT32;
+		}
+		dbus_message_iter_append_basic(&variant, itype, ptr);
+		dbus_message_iter_close_container(&dict, &variant);
+		dbus_message_iter_close_container(&array, &dict);
+	}
+
+	dbus_message_iter_close_container(&iter, &array);
+
+	return DBUS_HANDLER_RESULT_HANDLED;
+}
+
+static DBusHandlerResult usi_message_handler(DBusConnection *conn,
+					     DBusMessage *message, void *data)
+{
+	DBusHandlerResult result = DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+	DBusMessage *reply;
+	DBusError err;
+	int value;
+
+	fprintf(stderr, "Got D-Bus request: %s.%s on %s\n",
+		dbus_message_get_interface(message),
+		dbus_message_get_member(message),
+		dbus_message_get_path(message));
+
+	dbus_error_init(&err);
+
+	reply = dbus_message_new_method_return(message);
+	if (!reply)
+		return DBUS_HANDLER_RESULT_NEED_MEMORY;
+
+	if (dbus_message_is_method_call(message, DBUS_INTERFACE_INTROSPECTABLE, "Introspect")) {
+		dbus_message_append_args(reply,
+					 DBUS_TYPE_STRING, &server_introspection_xml,
+					 DBUS_TYPE_INVALID);
+
+	} else if (dbus_message_is_method_call(message, DBUS_INTERFACE_PROPERTIES, "Get")) {
+		result = usi_get_prop(conn, &err, message, reply);
+	} else if (dbus_message_is_method_call(message, DBUS_INTERFACE_PROPERTIES, "GetAll")) {
+		result = usi_get_all_props(conn, &err, reply);
+	} else if (dbus_message_is_method_call(message, DBUS_INTERFACE_PROPERTIES, "Set")) {
+		result = usi_set_prop(conn, &err, message, reply);
+	} else {
+		dbus_message_unref(reply);
+		return DBUS_HANDLER_RESULT_NOT_YET_HANDLED;
+	}
+
+	if (dbus_error_is_set(&err)) {
+		dbus_message_unref(reply);
+
+		reply = dbus_message_new_error(message, err.name, err.message);
+		dbus_error_free(&err);
+
+		if (!reply)
+			return DBUS_HANDLER_RESULT_NEED_MEMORY;
+	}
+
+	result = DBUS_HANDLER_RESULT_HANDLED;
+
+	if (!dbus_connection_send(conn, reply, NULL))
+		result = DBUS_HANDLER_RESULT_NEED_MEMORY;
+
+	dbus_message_unref(reply);
+
+	return result;
+}
+
+const DBusObjectPathVTable usi_vtable = {
+	.message_function = usi_message_handler,
+};
+
+static int server_run(void)
+{
+	DBusConnection *conn;
+	DBusError err;
+	int rv;
+
+	dbus_error_init(&err);
+
+	conn = dbus_bus_get(DBUS_BUS_SESSION, &err);
+	if (!conn) {
+		fprintf(stderr, "Failed to get a session DBus connection: %s\n", err.message);
+		goto fail;
+	}
+
+	rv = dbus_bus_request_name(conn, "org.universalstylus.PenServer",
+				   DBUS_NAME_FLAG_REPLACE_EXISTING, &err);
+	if (rv != DBUS_REQUEST_NAME_REPLY_PRIMARY_OWNER) {
+		fprintf(stderr, "Failed to request name on bus: %s\n", err.message);
+		goto fail;
+	}
+
+	if (!dbus_connection_register_object_path(conn,
+						  "/org/universalstylus/Pen",
+						  &usi_vtable, NULL)) {
+		fprintf(stderr, "Failed to register a object path for 'Pen'\n");
+		goto fail;
+	}
+
+	printf("Starting dbus USI server v%s\n", version);
+	mainloop = g_main_loop_new(NULL, false);
+	dbus_connection_setup_with_g_main(conn, NULL);
+	g_main_loop_run(mainloop);
+
+	return 0;
+fail:
+	dbus_error_free(&err);
+	return -1;
+}
+
+static int attach_progs(int argc, char **argv)
+{
+	char filename[256];
+	struct bpf_prog_info info = {};
+	__u32 info_len = sizeof(info);
+	struct bpf_object *obj;
+	struct bpf_program *prog;
+	int err = 0;
+	char buf[BUFSIZ];
+	char param[16];
+	char op[8];
+	int value;
+	int m, n;
+	struct sockaddr_un addr;
+	struct sockaddr_un from;
+	socklen_t from_len = sizeof(from);
+
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	obj = bpf_object__open_file(filename, NULL);
+	err = libbpf_get_error(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: opening BPF object file failed\n");
+		obj = NULL;
+		err = 1;
+		goto cleanup;
+	}
+
+	/* load BPF program */
+	err = bpf_object__load(obj);
+	if (err) {
+		fprintf(stderr, "ERROR: loading BPF object file failed\n");
+		goto cleanup;
+	}
+
+	sysfs_fd = open(sysfs_path, O_RDONLY);
+
+	bpf_object__for_each_program(prog, obj) {
+		progs[prog_count].fd = bpf_program__fd(prog);
+		progs[prog_count].type = bpf_program__get_expected_attach_type(prog);
+
+		err = bpf_prog_attach(progs[prog_count].fd,
+				      sysfs_fd,
+				      progs[prog_count].type,
+				      0);
+		if (err) {
+			fprintf(stderr, "bpf_prog_attach: err=%m\n");
+			progs[prog_count].fd = 0;
+			goto cleanup;
+		}
+		printf("attached BPF program with FD=%d, type=%d\n",
+		       progs[prog_count].fd, progs[prog_count].type);
+		prog_count++;
+	}
+
+	signal(SIGINT, int_exit);
+	signal(SIGTERM, int_exit);
+
+	err = bpf_obj_get_info_by_fd(progs[0].fd, &info, &info_len);
+	if (err) {
+		printf("can't get prog info - %s\n", strerror(errno));
+		goto cleanup;
+	}
+
+	cache = bpf_object__find_map_fd_by_name(obj, "cache");
+	if (cache < 0) {
+		printf("can't get 'cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	wr_cache = bpf_object__find_map_fd_by_name(obj, "wr_cache");
+	if (wr_cache < 0) {
+		printf("can't get 'wr_cache' shared mem from object - %m\n");
+		goto cleanup;
+	}
+
+	server_run();
+
+	return 0;
+
+ cleanup:
+	for (prog_count--; prog_count >= 0; prog_count--) {
+		if (bpf_prog_detach2(progs[prog_count].fd, sysfs_fd, progs[prog_count].type))
+			fprintf(stderr, "bpf_prog_detach2: returned %m\n");
+	}
+
+	bpf_object__close(obj);
+	return err;
+}
+
+int main(int argc, char **argv)
+{
+	int opt;
+
+	while ((opt = getopt_long(argc, argv, "h", long_options,
+				  NULL)) != -1) {
+		switch (opt) {
+		default:
+			usage(basename(argv[0]));
+			return 1;
+		}
+	}
+
+	if (optind == argc) {
+		usage(basename(argv[0]));
+		return 1;
+	}
+
+	sysfs_path = argv[optind];
+	if (!sysfs_path) {
+		perror("hidraw");
+		return 1;
+	}
+
+	printf("sysfs_path: %s\n", sysfs_path);
+
+	return attach_progs(argc, argv);
+}
diff --git a/samples/bpf/hid_usi_server_kern.c b/samples/bpf/hid_usi_server_kern.c
new file mode 100644
index 000000000000..3ecf638fb94d
--- /dev/null
+++ b/samples/bpf/hid_usi_server_kern.c
@@ -0,0 +1,267 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright(c) 2021 Intel Corporation. */
+#include <linux/version.h>
+#include <uapi/linux/bpf.h>
+#include <uapi/linux/hid.h>
+#include <uapi/linux/bpf_hid.h>
+#include <bpf/bpf_helpers.h>
+
+#include "hid_usi.h"
+
+#define MS_TO_JIFFIES(t) ((t) * HZ / 1000)
+
+static const char param_names[USI_NUM_PARAMS][6] = {
+	"id",
+	"range",
+	"touch",
+	"color",
+	"width",
+	"style",
+};
+
+struct {
+	__uint(type, BPF_MAP_TYPE_RINGBUF);
+	__uint(max_entries, 4096);
+} ringbuf SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} cache SEC(".maps");
+
+struct {
+	__uint(type, BPF_MAP_TYPE_ARRAY);
+	__type(key, int);
+	__type(value, int);
+	__uint(max_entries, USI_NUM_PARAMS);
+} wr_cache SEC(".maps");
+
+struct rep_data {
+	int offset;
+	int size;
+	int idx;
+};
+
+static struct rep_data inputs[USI_NUM_PARAMS];
+static struct rep_data features[USI_NUM_PARAMS];
+static bool work_pending;
+static u64 last_pen_event;
+
+SEC("hid/work")
+int hid_work(struct hid_bpf_ctx *ctx)
+{
+	int i, tmp;
+	int *wrc, *c;
+	u8 *buf;
+	bool have_work = false;
+
+	for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+		tmp = i;
+
+		wrc = bpf_map_lookup_elem(&wr_cache, &tmp);
+		c = bpf_map_lookup_elem(&cache, &tmp);
+
+		if (!wrc || !c)
+			continue;
+
+		if (*wrc == -1)
+			continue;
+
+		if (*wrc != *c) {
+			bpf_printk("updating %s to %x", param_names[i],
+				   *wrc);
+			buf = bpf_ringbuf_reserve(&ringbuf, 16, 0);
+			if (!buf)
+				continue;
+
+			buf[0] = features[i].idx;
+			buf[1] = 1;
+			if (i == USI_PEN_LINE_STYLE && *wrc == 6)
+				buf[2] = 255;
+			else
+				buf[2] = *wrc;
+
+			bpf_hid_raw_request(ctx, buf, 3,
+					    HID_FEATURE_REPORT,
+					    HID_REQ_SET_REPORT);
+
+			bpf_ringbuf_discard(buf, 0);
+
+			*c = *wrc;
+
+			*wrc = -1;
+
+			have_work = true;
+			break;
+		}
+	}
+
+	if (have_work)
+		bpf_hid_schedule_work(ctx, MS_TO_JIFFIES(100));
+	else
+		work_pending = false;
+
+	return 0;
+}
+
+SEC("hid/raw_event")
+int hid_raw_event(struct hid_bpf_ctx *ctx)
+{
+	u32 i;
+	u32 tmp;
+	int val, new_val;
+	int *wrc, *c;
+	u8 *buf;
+	u32 flags = 0;
+	int offset;
+	int size;
+	int in_range;
+	int touching;
+	u64 jiffies;
+
+	if (ctx->event.data[0] != inputs[USI_PEN_IN_RANGE].idx)
+		return 0;
+
+	in_range = bpf_hid_get_data(ctx, inputs[USI_PEN_IN_RANGE].offset,
+				    inputs[USI_PEN_IN_RANGE].size);
+	touching = bpf_hid_get_data(ctx, inputs[USI_PEN_TOUCHING].offset,
+				    inputs[USI_PEN_TOUCHING].size);
+
+	if (!touching) {
+		last_pen_event = 0;
+		return 0;
+	}
+
+	jiffies = bpf_jiffies64();
+
+	if (!last_pen_event)
+		last_pen_event = jiffies;
+
+	for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+		offset = inputs[i].offset;
+		size = inputs[i].size;
+		val = bpf_hid_get_data(ctx, offset, size);
+
+		new_val = val;
+		if (i == USI_PEN_LINE_STYLE && new_val == 255)
+			new_val = 6;
+
+		/*
+		 * Make a local copy of 'i' which we can refer via a
+		 * pointer to satisfy BPF verifier.
+		 */
+		tmp = i;
+
+		wrc = bpf_map_lookup_elem(&wr_cache, &tmp);
+		c = bpf_map_lookup_elem(&cache, &tmp);
+		if (!wrc || !c)
+			continue;
+
+		if (*wrc != -1 && *wrc != *c && !work_pending) {
+			bpf_hid_schedule_work(ctx, MS_TO_JIFFIES(200));
+			work_pending = true;
+		}
+
+		if (jiffies < last_pen_event + MS_TO_JIFFIES(200))
+			*c = new_val;
+
+		if (new_val != *c)
+			new_val = *c;
+
+		if (new_val != val)
+			bpf_hid_set_data(ctx, offset, size, new_val);
+	}
+
+	return 0;
+}
+
+static void process_tag(struct hid_bpf_ctx *ctx, struct rep_data *data,
+			struct hid_bpf_parser *parser, u64 *idx)
+{
+	u32 i;
+	int id;
+	u32 offset;
+
+	for (i = 0; i < parser->local.usage_index && i < 16; i++) {
+		offset = parser->report.current_offset +
+			i * parser->global.report_size;
+
+		switch (parser->local.usage[i]) {
+		case HID_DG_PEN_COLOR:
+			id = USI_PEN_COLOR;
+			break;
+		case HID_DG_PEN_LINE_WIDTH:
+			id = USI_PEN_LINE_WIDTH;
+			break;
+		case HID_DG_PEN_LINE_STYLE_INK:
+			id = USI_PEN_LINE_STYLE;
+			break;
+		case HID_DG_INRANGE:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_IN_RANGE;
+			break;
+		case HID_DG_TIPSWITCH:
+			if (parser->local.usage_index == 1)
+				continue;
+
+			id = USI_PEN_TOUCHING;
+			break;
+		default:
+			continue;
+		}
+
+		data[id].offset = offset + 8;
+		data[id].size = parser->global.report_size;
+		data[id].idx = parser->report.id;
+
+		bpf_printk("parsed id=%d, offset=%d, idx=%d",
+			   id, data[id].offset, data[id].idx);
+	}
+}
+
+static u64 process_hid_rdesc_item(struct hid_bpf_ctx *ctx,
+				  struct hid_bpf_parser *parser, u64 *idx,
+				  void *data)
+{
+	struct hid_bpf_item *item = &parser->item;
+
+	switch (item->type) {
+	case HID_ITEM_TYPE_MAIN:
+		if (item->tag == HID_MAIN_ITEM_TAG_INPUT)
+			process_tag(ctx, inputs, parser, idx);
+		if (item->tag == HID_MAIN_ITEM_TAG_FEATURE)
+			process_tag(ctx, features, parser, idx);
+	}
+
+	return 0;
+}
+
+SEC("hid/rdesc_fixup")
+int hid_rdesc_fixup(struct hid_bpf_ctx *ctx)
+{
+	int ret;
+	int *wrc;
+	int i, tmp;
+
+	if (ctx->type != HID_BPF_RDESC_FIXUP)
+		return 0;
+
+	ret = bpf_hid_foreach_rdesc_item(ctx, process_hid_rdesc_item, (void *)0, 0);
+	for (i = USI_PEN_COLOR; i < USI_NUM_PARAMS; i++) {
+		tmp = i;
+		wrc = bpf_map_lookup_elem(&wr_cache, &tmp);
+		if (!wrc)
+			continue;
+		*wrc = -1;
+	}
+
+	bpf_printk("ret: %d", ret);
+	return 0;
+}
+
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFCv5 0/2] HID: Add USI support
  2021-12-15 13:42 [RFCv5 0/2] HID: Add USI support Tero Kristo
  2021-12-15 13:42 ` [RFCv5 1/2] HID: core: Add support for USI style events Tero Kristo
  2021-12-15 13:42 ` [RFCv5 2/2] samples: hid-bpf: add HID USI samples Tero Kristo
@ 2021-12-16 10:36 ` Benjamin Tissoires
  2021-12-16 12:28   ` Tero Kristo
  2 siblings, 1 reply; 6+ messages in thread
From: Benjamin Tissoires @ 2021-12-16 10:36 UTC (permalink / raw)
  To: Tero Kristo
  Cc: open list:HID CORE LAYER, Jiri Kosina, lkml, Dmitry Torokhov,
	Peter Hutterer

Hi Tero,

On Wed, Dec 15, 2021 at 2:42 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi,
>
> These two patches add the missing pieces for HID USI support. First one
> adds the HID core changes to support the new Misc events for pen ID,
> line color and line style. The second patch adds a BPF program on top of
> the HID-BPF driver which adds support for writing the Pen parameters
> from userspace, and to add filtering of HID low level events for ELAN
> USI controller. The BPF programs are not built by the kernel as of now
> (there are no Makefile changes), as there is a plan to most likely
> integrate these to a kernel external repository. I have tested these in
> my own external build setup though, and I can provide the makefile for
> that if needed. Also a sample client program is provided for
> communicating with the D-BUS server.

I had a deeper look at the recordings, and I am very worried in what I
am seeing:
- the USI parameters seems to be transmitted only after the touch
- the USI parameters takes *a lot* of time to be transmitted (2 bytes
every 2 reports)
- the recording of the goodix one starts with a stylus touch without hovering
- the only "reliable" information we get when hovering seems to be the
transducer index

So I am wondering a few things:
- what happens when you switch between pens?
  * Do we immediately get a different transducer index?
  * Are the values right there or do they also take time to be updated?
- on the goodix one, do you still need to issue a get_report on the
feature to get the USI parameters, even when you change the pen?

Could you give me the following recording (with an updated hid-tools
master branch):
- on the Elan:
  * start the recording from a fresh boot (no BPF loaded)
  * hover for a few secs the first USI pen you have
  * touch it for a few secs
  * release, then out of proximity
  * approach the other pen
  * touch
  * release, out of prox
  * then once again with the first pen
  * then once again with the second pen

- on the goodix: same thing

- on the goodix: same thing but with a BPF program to trigger the
GET_REPORT if you can cook one quickly (not a big issue if you can
not).

The reason I am asking about those recordings is because with the 2
logs you kindly provided, there is no way we can forward the raw
information to userspace. So I am slightly tempted to only rely on a
USI manager, in the form of the BPF program in 2/2 to transmit that
information to userspace.

If this is bulky just for the first event, then the input events might
be OK, we can assume when the application needs those events they will
be there.

>
> I have also a kernel testing branch available at [1], which contains a
> few fix patches on top of Benjamin's HID-BPF driver work, and is rebased
> on top of latest hid/for-next. The HID-BPF fixes have been cleaned up a
> bit compared to previous setup. There are also a couple of new patches
> for adding support for a delayed_work BPF program on top of the
> hid-bpf driver; this is used to execute the raw_requests in non-irq
> context.

Thanks for that. I had a very quick look. I thought we could directly
use the bpf_timer_* functions instead of having to cook another API.
I'll play around with this, but thanks for pushing forward :)

IIRC you asked me when I was counting on submitting the HID BPF work.

So my answer is that I wanted to submit it by the end of 2021, but it
looks like I have only one week to finalize this :/

The current missing points are:
- add selftests for all the new API introduced
- review the entire API to not have to deal with a mistake forever
- rebase against bpf-next

One part of the API I am wondering is whether it is good or not to
have bpf_hid_foreach_rdesc_item(). This function is complex and we
could in theory parse the report descriptor in userspace, even before
we load the program. So all the parameters you need in the various
raw_event functions could be computed in user space, leading to a much
smaller API. The other benefit would be that the API would only deal
with arrays of bytes, which is a small enough and versatile enough API
:)

Cheers,
Benjamin

>
> -Tero
>
> [1] https://github.com/t-kristo/linux/tree/usi-5.16-v5-bpf
>
>


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFCv5 0/2] HID: Add USI support
  2021-12-16 10:36 ` [RFCv5 0/2] HID: Add USI support Benjamin Tissoires
@ 2021-12-16 12:28   ` Tero Kristo
  2021-12-16 15:52     ` Benjamin Tissoires
  0 siblings, 1 reply; 6+ messages in thread
From: Tero Kristo @ 2021-12-16 12:28 UTC (permalink / raw)
  To: Benjamin Tissoires
  Cc: open list:HID CORE LAYER, Jiri Kosina, lkml, Dmitry Torokhov,
	Peter Hutterer

[-- Attachment #1: Type: text/plain, Size: 6488 bytes --]

Hi Benjamin,

On 16/12/2021 12:36, Benjamin Tissoires wrote:
> Hi Tero,
>
> On Wed, Dec 15, 2021 at 2:42 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>> Hi,
>>
>> These two patches add the missing pieces for HID USI support. First one
>> adds the HID core changes to support the new Misc events for pen ID,
>> line color and line style. The second patch adds a BPF program on top of
>> the HID-BPF driver which adds support for writing the Pen parameters
>> from userspace, and to add filtering of HID low level events for ELAN
>> USI controller. The BPF programs are not built by the kernel as of now
>> (there are no Makefile changes), as there is a plan to most likely
>> integrate these to a kernel external repository. I have tested these in
>> my own external build setup though, and I can provide the makefile for
>> that if needed. Also a sample client program is provided for
>> communicating with the D-BUS server.
> I had a deeper look at the recordings, and I am very worried in what I
> am seeing:
> - the USI parameters seems to be transmitted only after the touch
Yes, they don't get updated before the pen touches the screen.
> - the USI parameters takes *a lot* of time to be transmitted (2 bytes
> every 2 reports)
Yes, there appears to be a very large latency when they are transmitted 
from the pen to controller. Apparently the controller queries each 
parameter from the pen individually and caches the received values 
internally, and then passes these out via input reports.
> - the recording of the goodix one starts with a stylus touch without hovering
This is a glitch I think, I'll capture a new log.
> - the only "reliable" information we get when hovering seems to be the
> transducer index

Yes, however even this one is a static constant of "1" for the current 
controllers.


>
> So I am wondering a few things:
> - what happens when you switch between pens?
>    * Do we immediately get a different transducer index?
No, the transducer index gets re-allocated, effectively the first pen is 
going to use transducer index 1, and the second pen is going to re-use 
that and still report it as 1. This is a highly annoying feature.
>    * Are the values right there or do they also take time to be updated?
They take time to update. If you are using the same pen, the values are 
retained in the cache and report correct values immediately, but if you 
switch pens, they start updating with the same latency as initial pen touch.
> - on the goodix one, do you still need to issue a get_report on the
> feature to get the USI parameters, even when you change the pen?

Yes, goodix reports 0xff,0xff,0x77 for the parameters until a GET_REPORT 
is issued, after which it notices the parameter that was queried has 
changed and updates its cached value. Also, see how it reports 0x77, 
instead of an index to the usages array.


>
> Could you give me the following recording (with an updated hid-tools
> master branch):
> - on the Elan:
>    * start the recording from a fresh boot (no BPF loaded)
>    * hover for a few secs the first USI pen you have
>    * touch it for a few secs
>    * release, then out of proximity
>    * approach the other pen
>    * touch
>    * release, out of prox
>    * then once again with the first pen
>    * then once again with the second pen
>
> - on the goodix: same thing
>
> - on the goodix: same thing but with a BPF program to trigger the
> GET_REPORT if you can cook one quickly (not a big issue if you can
> not).
>
> The reason I am asking about those recordings is because with the 2
> logs you kindly provided, there is no way we can forward the raw
> information to userspace. So I am slightly tempted to only rely on a
> USI manager, in the form of the BPF program in 2/2 to transmit that
> information to userspace.
>
> If this is bulky just for the first event, then the input events might
> be OK, we can assume when the application needs those events they will
> be there.

See attached tarball. All three logs are in it, I crafted a BPF program 
for Goodix controller also. Wasn't that bad actually, a few line change 
so it might be possible to use the same program at least for these two 
controllers.


>
>> I have also a kernel testing branch available at [1], which contains a
>> few fix patches on top of Benjamin's HID-BPF driver work, and is rebased
>> on top of latest hid/for-next. The HID-BPF fixes have been cleaned up a
>> bit compared to previous setup. There are also a couple of new patches
>> for adding support for a delayed_work BPF program on top of the
>> hid-bpf driver; this is used to execute the raw_requests in non-irq
>> context.
> Thanks for that. I had a very quick look. I thought we could directly
> use the bpf_timer_* functions instead of having to cook another API.
> I'll play around with this, but thanks for pushing forward :)

Yeah, I am kicked by someone on this so want to proceed. ;)

According to documentation, bpf_timer callbacks are executed in soft-irq 
context, so we can't use it at least for sending the raw_requests which 
is needed for USI.

>
> IIRC you asked me when I was counting on submitting the HID BPF work.
>
> So my answer is that I wanted to submit it by the end of 2021, but it
> looks like I have only one week to finalize this :/
Ok, sounds like you want to submit it probably early next year then. I 
just wanted to have some sort of ballpark estimate, and this is good 
enough for me, thanks.
>
> The current missing points are:
> - add selftests for all the new API introduced
> - review the entire API to not have to deal with a mistake forever
> - rebase against bpf-next
If you need my help on any of these items, let me know.
>
> One part of the API I am wondering is whether it is good or not to
> have bpf_hid_foreach_rdesc_item(). This function is complex and we
> could in theory parse the report descriptor in userspace, even before
> we load the program. So all the parameters you need in the various
> raw_event functions could be computed in user space, leading to a much
> smaller API. The other benefit would be that the API would only deal
> with arrays of bytes, which is a small enough and versatile enough API
> :)

This might be a good idea. Assuming the BPF programs end up in the 
hid-tools repo, there could be a generic library for parsing the rdescs 
also which the userspace programs could use.

-Tero

>
> Cheers,
> Benjamin
>
>> -Tero
>>
>> [1] https://github.com/t-kristo/linux/tree/usi-5.16-v5-bpf
>>
>>

[-- Attachment #2: usi-logs-2.tar.gz --]
[-- Type: application/gzip, Size: 298541 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFCv5 0/2] HID: Add USI support
  2021-12-16 12:28   ` Tero Kristo
@ 2021-12-16 15:52     ` Benjamin Tissoires
  0 siblings, 0 replies; 6+ messages in thread
From: Benjamin Tissoires @ 2021-12-16 15:52 UTC (permalink / raw)
  To: Tero Kristo
  Cc: open list:HID CORE LAYER, Jiri Kosina, lkml, Dmitry Torokhov,
	Peter Hutterer

On Thu, Dec 16, 2021 at 1:28 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
>
> Hi Benjamin,
>
> On 16/12/2021 12:36, Benjamin Tissoires wrote:
> > Hi Tero,
> >
> > On Wed, Dec 15, 2021 at 2:42 PM Tero Kristo <tero.kristo@linux.intel.com> wrote:
> >> Hi,
> >>
> >> These two patches add the missing pieces for HID USI support. First one
> >> adds the HID core changes to support the new Misc events for pen ID,
> >> line color and line style. The second patch adds a BPF program on top of
> >> the HID-BPF driver which adds support for writing the Pen parameters
> >> from userspace, and to add filtering of HID low level events for ELAN
> >> USI controller. The BPF programs are not built by the kernel as of now
> >> (there are no Makefile changes), as there is a plan to most likely
> >> integrate these to a kernel external repository. I have tested these in
> >> my own external build setup though, and I can provide the makefile for
> >> that if needed. Also a sample client program is provided for
> >> communicating with the D-BUS server.
> > I had a deeper look at the recordings, and I am very worried in what I
> > am seeing:
> > - the USI parameters seems to be transmitted only after the touch
> Yes, they don't get updated before the pen touches the screen.

Looking at the logs you provided, and when you switch pens, it seems
the serial number in the Elan case is updated while hovering (but it
takes 3 seconds to get to that point).
Also, the first time you hovered the pen, the inrange slot was not
long enough to be 3 seconds, so maybe that's the time the controller
needs to manually update the information.

OTOH, on the Goodix one, if you initiated the GET_REPORT after the
first touch, the information came in a few ms.

So I wonder, what if you take the BPF program from the goodix and
start querying the information as soon as the pen is hovering in the
Elan case. Would that accelerate the time it fetches them?

> > - the USI parameters takes *a lot* of time to be transmitted (2 bytes
> > every 2 reports)
> Yes, there appears to be a very large latency when they are transmitted
> from the pen to controller. Apparently the controller queries each
> parameter from the pen individually and caches the received values
> internally, and then passes these out via input reports.

And of course, Goodix doesn't even bother with the serial number.

> > - the recording of the goodix one starts with a stylus touch without hovering
> This is a glitch I think, I'll capture a new log.
> > - the only "reliable" information we get when hovering seems to be the
> > transducer index
>
> Yes, however even this one is a static constant of "1" for the current
> controllers.

Ouch. Well, that also means that this is used as an index and if the
controller were to support more than a pen, the index would be 2 for
the second pen when the first is touching...
So this is not reliable for now, but is consistent with a multi-pen scenario.

>
>
> >
> > So I am wondering a few things:
> > - what happens when you switch between pens?
> >    * Do we immediately get a different transducer index?
> No, the transducer index gets re-allocated, effectively the first pen is
> going to use transducer index 1, and the second pen is going to re-use
> that and still report it as 1. This is a highly annoying feature.

Well, that also means that we can ignore this field until we see
controllers capable of handling more than one pen.

> >    * Are the values right there or do they also take time to be updated?
> They take time to update. If you are using the same pen, the values are
> retained in the cache and report correct values immediately, but if you
> switch pens, they start updating with the same latency as initial pen touch.

That is worrying. The serial number is also cached and we don't know
if we are using the same pen or not way too long after the pen touched
the surface.


> > - on the goodix one, do you still need to issue a get_report on the
> > feature to get the USI parameters, even when you change the pen?
>
> Yes, goodix reports 0xff,0xff,0x77 for the parameters until a GET_REPORT
> is issued, after which it notices the parameter that was queried has
> changed and updates its cached value. Also, see how it reports 0x77,
> instead of an index to the usages array.

The 0x77 doesn't bother me too much. This should not trigger an input
event because we are out of the usage range.

>
>
> >
> > Could you give me the following recording (with an updated hid-tools
> > master branch):
> > - on the Elan:
> >    * start the recording from a fresh boot (no BPF loaded)
> >    * hover for a few secs the first USI pen you have
> >    * touch it for a few secs
> >    * release, then out of proximity
> >    * approach the other pen
> >    * touch
> >    * release, out of prox
> >    * then once again with the first pen
> >    * then once again with the second pen
> >
> > - on the goodix: same thing
> >
> > - on the goodix: same thing but with a BPF program to trigger the
> > GET_REPORT if you can cook one quickly (not a big issue if you can
> > not).
> >
> > The reason I am asking about those recordings is because with the 2
> > logs you kindly provided, there is no way we can forward the raw
> > information to userspace. So I am slightly tempted to only rely on a
> > USI manager, in the form of the BPF program in 2/2 to transmit that
> > information to userspace.
> >
> > If this is bulky just for the first event, then the input events might
> > be OK, we can assume when the application needs those events they will
> > be there.
>
> See attached tarball. All three logs are in it, I crafted a BPF program
> for Goodix controller also. Wasn't that bad actually, a few line change
> so it might be possible to use the same program at least for these two
> controllers.

\o/

>
>
> >
> >> I have also a kernel testing branch available at [1], which contains a
> >> few fix patches on top of Benjamin's HID-BPF driver work, and is rebased
> >> on top of latest hid/for-next. The HID-BPF fixes have been cleaned up a
> >> bit compared to previous setup. There are also a couple of new patches
> >> for adding support for a delayed_work BPF program on top of the
> >> hid-bpf driver; this is used to execute the raw_requests in non-irq
> >> context.
> > Thanks for that. I had a very quick look. I thought we could directly
> > use the bpf_timer_* functions instead of having to cook another API.
> > I'll play around with this, but thanks for pushing forward :)
>
> Yeah, I am kicked by someone on this so want to proceed. ;)
>
> According to documentation, bpf_timer callbacks are executed in soft-irq
> context, so we can't use it at least for sending the raw_requests which
> is needed for USI.

I might be wrong, but I am not sure if soft IRQs are incompatible with
hid_hw_raw_request.
Anyway, I gave it a shot today and I am hitting the fact that we lose
the BPF context in the timer callback. So we might actually need a new
API :/

>
> >
> > IIRC you asked me when I was counting on submitting the HID BPF work.
> >
> > So my answer is that I wanted to submit it by the end of 2021, but it
> > looks like I have only one week to finalize this :/
> Ok, sounds like you want to submit it probably early next year then. I
> just wanted to have some sort of ballpark estimate, and this is good
> enough for me, thanks.

Yeah, sorry for the delay.

> >
> > The current missing points are:
> > - add selftests for all the new API introduced
> > - review the entire API to not have to deal with a mistake forever
> > - rebase against bpf-next
> If you need my help on any of these items, let me know.
> >
> > One part of the API I am wondering is whether it is good or not to
> > have bpf_hid_foreach_rdesc_item(). This function is complex and we
> > could in theory parse the report descriptor in userspace, even before
> > we load the program. So all the parameters you need in the various
> > raw_event functions could be computed in user space, leading to a much
> > smaller API. The other benefit would be that the API would only deal
> > with arrays of bytes, which is a small enough and versatile enough API
> > :)
>
> This might be a good idea. Assuming the BPF programs end up in the
> hid-tools repo, there could be a generic library for parsing the rdescs
> also which the userspace programs could use.

Oh. Glad to see that this is something you find interesting. Peter
Hutterer mentioned that idea and I am still not 100% sure about it,
but if you also think it'll make things easier, then why not :)

Cheers,
Benjamin

>
> -Tero
>
> >
> > Cheers,
> > Benjamin
> >
> >> -Tero
> >>
> >> [1] https://github.com/t-kristo/linux/tree/usi-5.16-v5-bpf
> >>
> >>


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2021-12-16 15:52 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2021-12-15 13:42 [RFCv5 0/2] HID: Add USI support Tero Kristo
2021-12-15 13:42 ` [RFCv5 1/2] HID: core: Add support for USI style events Tero Kristo
2021-12-15 13:42 ` [RFCv5 2/2] samples: hid-bpf: add HID USI samples Tero Kristo
2021-12-16 10:36 ` [RFCv5 0/2] HID: Add USI support Benjamin Tissoires
2021-12-16 12:28   ` Tero Kristo
2021-12-16 15:52     ` Benjamin Tissoires

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox