linux-kernel - Re: [PATCH v10 2/4] Input: touch-overlay

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <ZpAT01jr9rS55t_B@google.com>
Date: Thu, 11 Jul 2024 10:18:11 -0700
From: Dmitry Torokhov <dmitry.torokhov@...il.com>
To: Javier Carrasco <javier.carrasco@...fvision.net>
Cc: Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Bastian Hecht <hechtb@...il.com>,
	Michael Riesch <michael.riesch@...fvision.net>,
	linux-input@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jeff LaBundy <jeff@...undy.com>
Subject: Re: [PATCH v10 2/4] Input: touch-overlay - Add touchscreen overlay
 handling

On Wed, Jul 10, 2024 at 02:16:13PM +0200, Javier Carrasco wrote:
> On 09/07/2024 02:08, Dmitry Torokhov wrote:
> > Hi Javier,
> > 
> > On Wed, Jun 26, 2024 at 11:56:14AM +0200, Javier Carrasco wrote:
> >> Some touch devices provide mechanical overlays with different objects
> >> like buttons or clipped touchscreen surfaces.
> > 
> > Thank you for your work. I think it is pretty much ready to be merged,
> > just a few small comments:
> > 
> >>
> >> In order to support these objects, add a series of helper functions
> >> to the input subsystem to transform them into overlay objects via
> >> device tree nodes.
> >>
> >> These overlay objects consume the raw touch events and report the
> >> expected input events depending on the object properties.
> > 
> > So if we have overlays and also want to invert/swap axis then the
> > overlays should be processed first and only then
> > touchscreen_set_mt_pos() or touchscreen_report_pos() should be called?
> > 
> > But then it will not work if we need help frm the input core to assign
> > slots in cases when touch controller does not implement [reliable]
> > contact tracing/identification.
> > 
> > I think this all needs to be clarified.
> > 
> 
> I think this is the most critical point from your feedback.
> 
> So far, touch-overlay processes the coordinates of input_mt_pos elements
> before passing them to touchscreen_set_mt_pos(). As you pointed out,
> that means that it does not act on the slots i.e. it assumes that the
> controller does the contact tracing and pos[i] is assigned to slot[i],
> which is wrong.
> 
> Current status:
> [Sensor]->[touch-overlay(filter + button
> events)]->[touchscreen_set_mt_pos()]->[input_mt_assign_slots()]->[report
> MT events]
> 
> If I am not mistaken, I would need a solution that processes the
> coordinates associated to assigned slots via input_mt_assign_slots().
> Then I would have to avoid generating MT events from slots whose
> coordinates are outside of the overlay frame (ignored) or on overlay
> buttons (generating button press/release events instead).
> 
> But once input_mt_assign_slots() is called, it is already too late for
> that processing, isn't it? Unless assigned slots filtered by
> touch-overlay are kept from generating MT events somehow, but still used
> to keep contact tracing.
> 
> Any suggestion on this respect would be more than welcome.

The driver is in control which slots to emit the events for, so it can
skip reporting if it wants. Consider the st1232 for which you are making
these adjustments:

	for (i = 0; i < ts->chip_info->max_fingers; i++) {
		u8 *buf = &ts->read_buf[i * 4];

		if (buf[0] & BIT(7)) {
			unsigned int x = ((buf[0] & 0x70) << 4) | buf[1];
			unsigned int y = ((buf[0] & 0x07) << 8) | buf[2];

			touchscreen_set_mt_pos(&pos[n_contacts],
					       &ts->prop, x, y);

			/* st1232 includes a z-axis / touch strength */
			if (ts->chip_info->have_z)
				z[n_contacts] = ts->read_buf[i + 6];

			n_contacts++;
		}
	}

	input_mt_assign_slots(input, slots, pos, n_contacts, 0);
	for (i = 0; i < n_contacts; i++) {
		input_mt_slot(input, slots[i]);
		input_mt_report_slot_state(input, MT_TOOL_FINGER, true);
		input_report_abs(input, ABS_MT_POSITION_X, pos[i].x);
		input_report_abs(input, ABS_MT_POSITION_Y, pos[i].y);
		if (ts->chip_info->have_z)
			input_report_abs(input, ABS_MT_TOUCH_MAJOR, z[i]);
	}

Can you call touch_overlay_process_event() from the 2nd for() loop,
something like this:

	for (i = 0; i < n_contacts; i++) {
		if (touch_overlay_process_event(&ts->touch_overlay_list,
						input,
						&pos[i].x, &pos[i].y))
			continue;

		input_mt_slot(input, slots[i]);
		...
	}

Note that you will no longer able to rely on the input core to drop
"unused" slots because you may need to generate "key up" events for
them, but you do have access to input_mt_is_active() and
input_mt_is_used() so you can implement what you need.

Thanks.

-- 
Dmitry