Message-ID: <d8734538-81f4-4883-bd29-2f8b682c3d1c@wolfvision.net>
Date: Wed, 10 Jul 2024 14:16:13 +0200
From: Javier Carrasco <javier.carrasco@...fvision.net>
To: Dmitry Torokhov <dmitry.torokhov@...il.com>
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 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.

>>
>> Note that the current implementation allows for multiple definitions
>> of touchscreen areas (regions that report touch events), but only the
>> first one will be used for the touchscreen device that the consumers
>> typically provide.
>> Should the need for multiple touchscreen areas arise, additional
>> touchscreen devices would be required at the consumer side.
>> There is no limitation in the number of touch areas defined as buttons.
>>
>> Reviewed-by: Jeff LaBundy <jeff@...undy.com>
>> Signed-off-by: Javier Carrasco <javier.carrasco@...fvision.net>
> 
>> +int touch_overlay_map(struct list_head *list, struct input_dev *input)
>> +{
>> +	struct fwnode_handle *overlay, *fw_segment;
>> +	struct device *dev = input->dev.parent;
>> +	struct touch_overlay_segment *segment;
>> +	int error = 0;
>> +
>> +	overlay = device_get_named_child_node(dev, "touch-overlay");
> 
> We can annotate this as
> 
> 	struct fwnode_handle *overlay __free(fwnode_handle) = 
> 		device_get_named_child_node(dev, "touch-overlay");
> 

That's right. A scoped version of the loop would have been nice too, but
there is still no such variant for this particular one. I am pushing
that somewhere else, though.

>> +	if (!overlay)
>> +		return 0;
>> +
>> +	fwnode_for_each_available_child_node(overlay, fw_segment) {
>> +		segment = devm_kzalloc(dev, sizeof(*segment), GFP_KERNEL);
>> +		if (!segment) {
>> +			fwnode_handle_put(fw_segment);
>> +			error = -ENOMEM;
> 
> return -ENOMEM;
> 
>> +			break;
>> +		}
>> +		error = touch_overlay_get_segment(fw_segment, segment, input);
>> +		if (error) {
>> +			fwnode_handle_put(fw_segment);
> 
> return error;
> 
>> +			break;
>> +		}
>> +		list_add_tail(&segment->list, list);
>> +	}
>> +	fwnode_handle_put(overlay);
> 
> Drop.
> 
>> +
>> +	return error;
> 
> return 0;
> 

Ack.

>> +}
>> +EXPORT_SYMBOL(touch_overlay_map);
>> +
>> +/**
>> + * touch_overlay_get_touchscreen_abs - get abs size from the touchscreen area.
>> + * @list: pointer to the list that holds the segments
>> + * @x: horizontal abs
>> + * @y: vertical abs
>> + */
>> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y)
>> +{
>> +	struct touch_overlay_segment *segment;
>> +	struct list_head *ptr;
>> +
>> +	list_for_each(ptr, list) {
>> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
>> +		if (!segment->key) {
>> +			*x = segment->x_size - 1;
>> +			*y = segment->y_size - 1;
>> +			break;
>> +		}
>> +	}
>> +}
>> +EXPORT_SYMBOL(touch_overlay_get_touchscreen_abs);
>> +
>> +static bool touch_overlay_segment_event(struct touch_overlay_segment *seg,
>> +					u32 x, u32 y)
>> +{
>> +	if (!seg)
>> +		return false;
> 
> This is a static function in the module, we are not passing NULL
> segments to it ever. Such tests should be done on API boundary.
> 

Ack.

>> +
>> +	if (x >= seg->x_origin && x < (seg->x_origin + seg->x_size) &&
>> +	    y >= seg->y_origin && y < (seg->y_origin + seg->y_size))
>> +		return true;
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * touch_overlay_mapped_touchscreen - check if a touchscreen area is mapped
>> + * @list: pointer to the list that holds the segments
>> + *
>> + * Returns true if a touchscreen area is mapped or false otherwise.
>> + */
>> +bool touch_overlay_mapped_touchscreen(struct list_head *list)
>> +{
>> +	struct touch_overlay_segment *segment;
>> +	struct list_head *ptr;
>> +
>> +	list_for_each(ptr, list) {
>> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
>> +		if (!segment->key)
>> +			return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +EXPORT_SYMBOL(touch_overlay_mapped_touchscreen);
>> +
>> +static bool touch_overlay_event_on_ts(struct list_head *list, u32 *x, u32 *y)
>> +{
>> +	struct touch_overlay_segment *segment;
>> +	struct list_head *ptr;
>> +	bool valid_touch = true;
>> +
>> +	if (!x || !y)
>> +		return false;
>> +
>> +	list_for_each(ptr, list) {
>> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
>> +		if (segment->key)
>> +			continue;
>> +
>> +		if (touch_overlay_segment_event(segment, *x, *y)) {
>> +			*x -= segment->x_origin;
>> +			*y -= segment->y_origin;
>> +			return true;
>> +		}
>> +		/* ignore touch events outside the defined area */
>> +		valid_touch = false;
>> +	}
>> +
>> +	return valid_touch;
>> +}
>> +
>> +static bool touch_overlay_button_event(struct input_dev *input,
>> +				       struct touch_overlay_segment *segment,
>> +				       const u32 *x, const u32 *y, u32 slot)
>> +{
>> +	bool contact = x && y;
>> +
>> +	if (segment->slot == slot && segment->pressed) {
>> +		/* button release */
>> +		if (!contact) {
>> +			segment->pressed = false;
>> +			input_report_key(input, segment->key, false);
>> +			input_sync(input);
> 
> Do we really need to emit sync here? Can we require/rely on the driver
> calling us to emit input_sync() once it's done processing current
> frame/packet?
>

I will test it without it, but that might change anyway depending on the
outcome of your first comment.

>> +			return true;
>> +		}
>> +
>> +		/* sliding out of the button releases it */
>> +		if (!touch_overlay_segment_event(segment, *x, *y)) {
>> +			segment->pressed = false;
>> +			input_report_key(input, segment->key, false);
>> +			input_sync(input);
>> +			/* keep available for a possible touch event */
>> +			return false;
>> +		}
>> +		/* ignore sliding on the button while pressed */
>> +		return true;
>> +	} else if (contact && touch_overlay_segment_event(segment, *x, *y)) {
>> +		segment->pressed = true;
>> +		segment->slot = slot;
>> +		input_report_key(input, segment->key, true);
>> +		input_sync(input);
>> +		return true;
>> +	}
>> +
>> +	return false;
>> +}
>> +
>> +/**
>> + * touch_overlay_process_event - process input events according to the overlay
>> + * mapping. This function acts as a filter to release the calling driver from
>> + * the events that are either related to overlay buttons or out of the overlay
>> + * touchscreen area, if defined.
>> + * @list: pointer to the list that holds the segments
>> + * @input: pointer to the input device associated to the event
>> + * @x: pointer to the x coordinate (NULL if not available - no contact)
>> + * @y: pointer to the y coordinate (NULL if not available - no contact)
> 
> Would it be better to have a separate argument communicating slot state
> (contact/no contact)?
>

Passing NULL would be ok to save one extra argument, but I have no
strong feelings about it.

>> + * @slot: slot associated to the event
> 
> What if we are not dealing with an MT device? Can we say that they
> should use slot 0 or maybe -1?
>

slot 0 would be ok. I will document it.

>> + *
>> + * Returns true if the event was processed (reported for valid key events
>> + * and dropped for events outside the overlay touchscreen area) or false
>> + * if the event must be processed by the caller. In that case this function
>> + * shifts the (x,y) coordinates to the overlay touchscreen axis if required.
>> + */
>> +bool touch_overlay_process_event(struct list_head *list,
>> +				 struct input_dev *input,
>> +				 u32 *x, u32 *y, u32 slot)
>> +{
>> +	struct touch_overlay_segment *segment;
>> +	struct list_head *ptr;
>> +
>> +	/*
>> +	 * buttons must be prioritized over overlay touchscreens to account for
>> +	 * overlappings e.g. a button inside the touchscreen area.
>> +	 */
>> +	list_for_each(ptr, list) {
>> +		segment = list_entry(ptr, struct touch_overlay_segment, list);
>> +		if (segment->key &&
>> +		    touch_overlay_button_event(input, segment, x, y, slot))
>> +			return true;
>> +	}
>> +
>> +	/*
>> +	 * valid touch events on the overlay touchscreen are left for the client
>> +	 * to be processed/reported according to its (possibly) unique features.
>> +	 */
>> +	return !touch_overlay_event_on_ts(list, x, y);
>> +}
>> +EXPORT_SYMBOL(touch_overlay_process_event);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Helper functions for overlay objects on touch devices");
>> diff --git a/include/linux/input/touch-overlay.h b/include/linux/input/touch-overlay.h
>> new file mode 100644
>> index 000000000000..cef05c46000d
>> --- /dev/null
>> +++ b/include/linux/input/touch-overlay.h
>> @@ -0,0 +1,22 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only */
>> +/*
>> + * Copyright (c) 2023 Javier Carrasco <javier.carrasco@...fvision.net>
>> + */
>> +
>> +#ifndef _TOUCH_OVERLAY
>> +#define _TOUCH_OVERLAY
>> +
>> +#include <linux/types.h>
>> +
>> +struct input_dev;
>> +
>> +int touch_overlay_map(struct list_head *list, struct input_dev *input);
>> +
>> +void touch_overlay_get_touchscreen_abs(struct list_head *list, u16 *x, u16 *y);
>> +
>> +bool touch_overlay_mapped_touchscreen(struct list_head *list);
>> +
>> +bool touch_overlay_process_event(struct list_head *list, struct input_dev *input,
>> +				 u32 *x, u32 *y, u32 slot);
>> +
>> +#endif
>>
>> -- 
>> 2.40.1
>>
> 
> Thanks.
> 

Thanks a lot for your feedback.

Best regards,
Javier Carrasco

Powered by blists - more mailing lists