[RFC 0/2] Add EVIOC mechanism to extract the MT slot state

[RFC 0/2] Add EVIOC mechanism to extract the MT slot state

[Benjamin Tissoires]

Hi guys,

This patchset is a rework of Henrik's original work.
Henrik, I kept your signed-off-by, please tell me if it's ok with you.
I just changed the method call (and signature) for it to be compliant with the new EVIOC handling.

I'm not very happy with it (for instance, there is a call of an mt stuff in generic input), but it's a start.

Cheers,
Benjamin

[RFC 1/2] input: mt: Add method to extract the MT slot state

[Benjamin Tissoires]

This patch adds the function input_mt_get_abs_value(), which may be
used to extract the current state of the MT slots.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/input/input-mt.c |   32 ++++++++++++++++++++++++++++++++
 include/linux/input/mt.h |    1 +
 2 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
index c48c81f..5b95e38 100644
--- a/drivers/input/input-mt.c
+++ b/drivers/input/input-mt.c
@@ -168,3 +168,35 @@ void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count)
 	}
 }
 EXPORT_SYMBOL(input_mt_report_pointer_emulation);
+
+/**
+ * input_mt_get_absinfo - retrieve MT state variables from a device slot
+ * @dev: input device which state is being queried
+ * @code: ABS code to retrieve
+ * @slot: slot to retrieve from
+ *
+ * Return the ABS state of the given MT slot. If code is not an MT
+ * event, the corresponding ABS event is returned.
+ *
+ * This function may be called by anyone interested in extracting the
+ * current MT state. Used by evdev handlers.
+ */
+struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot)
+{
+	unsigned int mtmap = code > ABS_MT_FIRST ? code - ABS_MT_FIRST : 0;
+	unsigned long flags;
+	struct input_absinfo retval;
+
+	spin_lock_irqsave(&dev->event_lock, flags);
+	if (!mtmap)
+		retval = dev->absinfo[code];
+	else if (slot >= 0 && slot < dev->mtsize) {
+		retval = dev->absinfo[code];
+		retval.value = dev->mt[slot].abs[mtmap];
+	} else
+		retval.value = 0;
+	spin_unlock_irqrestore(&dev->event_lock, flags);
+
+	return retval;
+}
+EXPORT_SYMBOL(input_mt_get_absinfo);
diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
index b3ac06a..a71a27f 100644
--- a/include/linux/input/mt.h
+++ b/include/linux/input/mt.h
@@ -54,4 +54,5 @@ void input_mt_report_slot_state(struct input_dev *dev,
 void input_mt_report_finger_count(struct input_dev *dev, int count);
 void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count);
 
+struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot);
 #endif
-- 
1.7.3.4

[RFC 2/2] input: evdev: Add EVIOC mechanism to extract the MT slot state

[Benjamin Tissoires]

This patch adds the ability to extract the MT slot state sequentially
via EVIOCGABS. The slot parameter is first selected by calling
EVIOCSABS with ABS_MT_SLOT as argument, followed by a set of EVIOCGABS
calls. The slot selection is local to the evdev client handler, and
does not affect the actual input state.

Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
---
 drivers/input/evdev.c |   11 +++++++----
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index c8471a2..a689079 100644
--- a/drivers/input/evdev.c
+++ b/drivers/input/evdev.c
@@ -23,6 +23,7 @@
 #include <linux/input.h>
 #include <linux/major.h>
 #include <linux/device.h>
+#include <linux/input/mt.h>
 #include "input-compat.h"
 
 struct evdev {
@@ -45,6 +46,7 @@ struct evdev_client {
 	struct fasync_struct *fasync;
 	struct evdev *evdev;
 	struct list_head node;
+	int slot; /* used to extract MT events via EVIOC */
 	int bufsize;
 	struct input_event buffer[];
 };
@@ -741,7 +743,7 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 				return -EINVAL;
 
 			t = _IOC_NR(cmd) & ABS_MAX;
-			abs = dev->absinfo[t];
+			abs = input_mt_get_absinfo(dev, t, client->slot);
 
 			if (copy_to_user(p, &abs, min_t(size_t,
 					size, sizeof(struct input_absinfo))))
@@ -767,9 +769,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			if (size < sizeof(struct input_absinfo))
 				abs.resolution = 0;
 
-			/* We can't change number of reserved MT slots */
-			if (t == ABS_MT_SLOT)
-				return -EINVAL;
+			if (t == ABS_MT_SLOT) {
+				client->slot = abs.value;
+				return 0;
+			}
 
 			/*
 			 * Take event lock to ensure that we are not
-- 
1.7.3.4

Re: [RFC 1/2] input: mt: Add method to extract the MT slot state

[Henrik Rydberg]

Hi Benjamin,

On Thu, Jan 27, 2011 at 11:35:46AM +0100, Benjamin Tissoires wrote:
> This patch adds the function input_mt_get_abs_value(), which may be

Ehm, input_mt_get_absinfo?

> used to extract the current state of the MT slots.
> 
> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>

I had rather re-add this myself, thanks.

> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
> ---
>  drivers/input/input-mt.c |   32 ++++++++++++++++++++++++++++++++
>  include/linux/input/mt.h |    1 +
>  2 files changed, 33 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
> index c48c81f..5b95e38 100644
> --- a/drivers/input/input-mt.c
> +++ b/drivers/input/input-mt.c
> @@ -168,3 +168,35 @@ void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count)
>  	}
>  }
>  EXPORT_SYMBOL(input_mt_report_pointer_emulation);
> +
> +/**
> + * input_mt_get_absinfo - retrieve MT state variables from a device slot
> + * @dev: input device which state is being queried
> + * @code: ABS code to retrieve
> + * @slot: slot to retrieve from
> + *
> + * Return the ABS state of the given MT slot. If code is not an MT
> + * event, the corresponding ABS event is returned.
> + *
> + * This function may be called by anyone interested in extracting the
> + * current MT state. Used by evdev handlers.
> + */
> +struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot)

Why return the struct? Give it a pointer instead, so the return value
can be used for errors.

Also, I wonder about the usefulness of extracting the absinfo here -
only the value depends on the slot argument, and absinfo does not
require the same lock protection. Why not keep the layout from the
original patch?

> +{
> +	unsigned int mtmap = code > ABS_MT_FIRST ? code - ABS_MT_FIRST : 0;

Should rather return -EINVAL on bad argument.

> +	unsigned long flags;
> +	struct input_absinfo retval;
> +
> +	spin_lock_irqsave(&dev->event_lock, flags);
> +	if (!mtmap)
> +		retval = dev->absinfo[code];
> +	else if (slot >= 0 && slot < dev->mtsize) {
> +		retval = dev->absinfo[code];
> +		retval.value = dev->mt[slot].abs[mtmap];

Please use input_mt_get_value().

> +	} else
> +		retval.value = 0;
> +	spin_unlock_irqrestore(&dev->event_lock, flags);
> +
> +	return retval;
> +}
> +EXPORT_SYMBOL(input_mt_get_absinfo);
> diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
> index b3ac06a..a71a27f 100644
> --- a/include/linux/input/mt.h
> +++ b/include/linux/input/mt.h
> @@ -54,4 +54,5 @@ void input_mt_report_slot_state(struct input_dev *dev,
>  void input_mt_report_finger_count(struct input_dev *dev, int count);
>  void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count);
>  
> +struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot);
>  #endif

Thanks,
Henrik

Re: [RFC 2/2] input: evdev: Add EVIOC mechanism to extract the MT slot state

[Henrik Rydberg]

On Thu, Jan 27, 2011 at 11:35:47AM +0100, Benjamin Tissoires wrote:
> This patch adds the ability to extract the MT slot state sequentially
> via EVIOCGABS. The slot parameter is first selected by calling
> EVIOCSABS with ABS_MT_SLOT as argument, followed by a set of EVIOCGABS
> calls. The slot selection is local to the evdev client handler, and
> does not affect the actual input state.

Ok - it seemed like a reasonable idea at the time, but..

> @@ -767,9 +769,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  			if (size < sizeof(struct input_absinfo))
>  				abs.resolution = 0;
>  
> -			/* We can't change number of reserved MT slots */
> -			if (t == ABS_MT_SLOT)
> -				return -EINVAL;
> +			if (t == ABS_MT_SLOT) {
> +				client->slot = abs.value;
> +				return 0;
> +			}

...this just does not look right. Perhaps there should really be a
different ioctl to retrieve the slot values instead. Dmitry?

Thanks,
Henrik

Re: [RFC 1/2] input: mt: Add method to extract the MT slot state

[Benjamin Tissoires]

Hi Henrik,

On Thu, Jan 27, 2011 at 13:06, Henrik Rydberg <rydberg@euromail.se> wrote:
> Hi Benjamin,
>
> On Thu, Jan 27, 2011 at 11:35:46AM +0100, Benjamin Tissoires wrote:
>> This patch adds the function input_mt_get_abs_value(), which may be
>
> Ehm, input_mt_get_absinfo?

In fact, the idea was to put this on the table again ;). I have no
idea of what is the right think to do in such case.
So I'll take all comments.

>
>> used to extract the current state of the MT slots.
>>
>> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
>
> I had rather re-add this myself, thanks.

That was my question. Plus I had a subliminal message in which I
intend you to take the drive on this work. ;)

>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@enac.fr>
>> ---
>>  drivers/input/input-mt.c |   32 ++++++++++++++++++++++++++++++++
>>  include/linux/input/mt.h |    1 +
>>  2 files changed, 33 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/input/input-mt.c b/drivers/input/input-mt.c
>> index c48c81f..5b95e38 100644
>> --- a/drivers/input/input-mt.c
>> +++ b/drivers/input/input-mt.c
>> @@ -168,3 +168,35 @@ void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count)
>>       }
>>  }
>>  EXPORT_SYMBOL(input_mt_report_pointer_emulation);
>> +
>> +/**
>> + * input_mt_get_absinfo - retrieve MT state variables from a device slot
>> + * @dev: input device which state is being queried
>> + * @code: ABS code to retrieve
>> + * @slot: slot to retrieve from
>> + *
>> + * Return the ABS state of the given MT slot. If code is not an MT
>> + * event, the corresponding ABS event is returned.
>> + *
>> + * This function may be called by anyone interested in extracting the
>> + * current MT state. Used by evdev handlers.
>> + */
>> +struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot)
>
> Why return the struct? Give it a pointer instead, so the return value
> can be used for errors.

Quick and dirty think. Maybe the old implementation is better.

>
> Also, I wonder about the usefulness of extracting the absinfo here -
> only the value depends on the slot argument, and absinfo does not
> require the same lock protection. Why not keep the layout from the
> original patch?

same above

>
>> +{
>> +     unsigned int mtmap = code > ABS_MT_FIRST ? code - ABS_MT_FIRST : 0;
>
> Should rather return -EINVAL on bad argument.

I did not know how to handle the original code:
"unsigned int mtmap = input_mt_abs_map[code];"

Now I know.

>
>> +     unsigned long flags;
>> +     struct input_absinfo retval;
>> +
>> +     spin_lock_irqsave(&dev->event_lock, flags);
>> +     if (!mtmap)
>> +             retval = dev->absinfo[code];
>> +     else if (slot >= 0 && slot < dev->mtsize) {
>> +             retval = dev->absinfo[code];
>> +             retval.value = dev->mt[slot].abs[mtmap];
>
> Please use input_mt_get_value().

This is a problem when working with code that was send 6 months ago.

>
>> +     } else
>> +             retval.value = 0;
>> +     spin_unlock_irqrestore(&dev->event_lock, flags);
>> +
>> +     return retval;
>> +}
>> +EXPORT_SYMBOL(input_mt_get_absinfo);
>> diff --git a/include/linux/input/mt.h b/include/linux/input/mt.h
>> index b3ac06a..a71a27f 100644
>> --- a/include/linux/input/mt.h
>> +++ b/include/linux/input/mt.h
>> @@ -54,4 +54,5 @@ void input_mt_report_slot_state(struct input_dev *dev,
>>  void input_mt_report_finger_count(struct input_dev *dev, int count);
>>  void input_mt_report_pointer_emulation(struct input_dev *dev, bool use_count);
>>
>> +struct input_absinfo input_mt_get_absinfo(struct input_dev *dev, unsigned int code, int slot);
>>  #endif
>
> Thanks,
> Henrik
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [RFC 1/2] input: mt: Add method to extract the MT slot state

[Henrik Rydberg]

On Thu, Jan 27, 2011 at 01:49:18PM +0100, Benjamin Tissoires wrote:
> Hi Henrik,
> 
> On Thu, Jan 27, 2011 at 13:06, Henrik Rydberg <rydberg@euromail.se> wrote:
> > Hi Benjamin,
> >
> > On Thu, Jan 27, 2011 at 11:35:46AM +0100, Benjamin Tissoires wrote:
> >> This patch adds the function input_mt_get_abs_value(), which may be
> >
> > Ehm, input_mt_get_absinfo?
> 
> In fact, the idea was to put this on the table again ;). I have no
> idea of what is the right think to do in such case.
> So I'll take all comments.
> 
> >
> >> used to extract the current state of the MT slots.
> >>
> >> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
> >
> > I had rather re-add this myself, thanks.
> 
> That was my question. Plus I had a subliminal message in which I
> intend you to take the drive on this work. ;)

Thanks for the patches, Benjamin, perhaps they do need a bit of
rewrite or a new ioctl. We will see what happens.

Thanks,
Henrik

Re: [RFC 2/2] input: evdev: Add EVIOC mechanism to extract the MT slot state

[Dmitry Torokhov]

On Thu, Jan 27, 2011 at 01:09:45PM +0100, Henrik Rydberg wrote:
> On Thu, Jan 27, 2011 at 11:35:47AM +0100, Benjamin Tissoires wrote:
> > This patch adds the ability to extract the MT slot state sequentially
> > via EVIOCGABS. The slot parameter is first selected by calling
> > EVIOCSABS with ABS_MT_SLOT as argument, followed by a set of EVIOCGABS
> > calls. The slot selection is local to the evdev client handler, and
> > does not affect the actual input state.
> 
> Ok - it seemed like a reasonable idea at the time, but..
> 
> > @@ -767,9 +769,10 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> >  			if (size < sizeof(struct input_absinfo))
> >  				abs.resolution = 0;
> >  
> > -			/* We can't change number of reserved MT slots */
> > -			if (t == ABS_MT_SLOT)
> > -				return -EINVAL;
> > +			if (t == ABS_MT_SLOT) {
> > +				client->slot = abs.value;
> > +				return 0;
> > +			}
> 
> ...this just does not look right. Perhaps there should really be a
> different ioctl to retrieve the slot values instead. Dmitry?
> 

Yes, a separate ioctl with explicit slot number is much better option.
Then it is automatically multi-thread safe and just does not look
ugly as hell.

Thanks.

-- 
Dmitry

Re: [RFC 1/2] input: mt: Add method to extract the MT slot state

[Benjamin Tissoires]

On Fri, Jan 28, 2011 at 18:33, Henrik Rydberg <rydberg@euromail.se> wrote:
> On Thu, Jan 27, 2011 at 01:49:18PM +0100, Benjamin Tissoires wrote:
>> Hi Henrik,
>>
>> On Thu, Jan 27, 2011 at 13:06, Henrik Rydberg <rydberg@euromail.se> wrote:
>> > Hi Benjamin,
>> >
>> > On Thu, Jan 27, 2011 at 11:35:46AM +0100, Benjamin Tissoires wrote:
>> >> This patch adds the function input_mt_get_abs_value(), which may be
>> >
>> > Ehm, input_mt_get_absinfo?
>>
>> In fact, the idea was to put this on the table again ;). I have no
>> idea of what is the right think to do in such case.
>> So I'll take all comments.
>>
>> >
>> >> used to extract the current state of the MT slots.
>> >>
>> >> Signed-off-by: Henrik Rydberg <rydberg@euromail.se>
>> >
>> > I had rather re-add this myself, thanks.
>>
>> That was my question. Plus I had a subliminal message in which I
>> intend you to take the drive on this work. ;)
>
> Thanks for the patches, Benjamin, perhaps they do need a bit of
> rewrite or a new ioctl. We will see what happens.
>

Today  I found a bug in these 2 patches: they send garbage with
devices that use protocol A.
This is definitely a bad idea to send the input_absinfo.

In addition to that, using the original behavior (which seems better)
does not seems to add sth in regard to input_mt_get_value.
It would just add some guards that can be resumed in just a test:
(code >= ABS_MT_FIRST && code <= ABS_MT_LAST && dev->mt &&
	slot >= 0 && slot < dev->mtsize)

Henrik, if you want to introduce the new IOCTL, feel free, I don't
need it right now.

Cheers,
Benjamin

Re: [RFC 1/2] input: mt: Add method to extract the MT slot state

[Henrik Rydberg]

> > Thanks for the patches, Benjamin, perhaps they do need a bit of
> > rewrite or a new ioctl. We will see what happens.
> >
> 
> Today  I found a bug in these 2 patches: they send garbage with
> devices that use protocol A.
> This is definitely a bad idea to send the input_absinfo.
> 
> In addition to that, using the original behavior (which seems better)
> does not seems to add sth in regard to input_mt_get_value.
> It would just add some guards that can be resumed in just a test:
> (code >= ABS_MT_FIRST && code <= ABS_MT_LAST && dev->mt &&
> 	slot >= 0 && slot < dev->mtsize)
> 
> Henrik, if you want to introduce the new IOCTL, feel free, I don't
> need it right now.

Ok - for the record, neither of us actually re-initiated this thing,
so it still falls into the nice-to-have category. The good thing is
there is now a clear path on how to proceed.

Thanks Benjamin.

Henrik