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@xxxxxxxxxxx>
>> Signed-off-by: Javier Carrasco <javier.carrasco@xxxxxxxxxxxxxx>
>
>> +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@xxxxxxxxxxxxxx>
>> + */
>> +
>> +#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
