Message-ID: <1283293796.2255.164.camel@mini>
Date:	Tue, 31 Aug 2010 18:29:56 -0400
From:	Chase Douglas <chase.douglas@...onical.com>
To:	Henrik Rydberg <rydberg@...omail.se>
Cc:	Jiri Kosina <jkosina@...e.cz>, Michael Poole <mdpoole@...ilus.org>,
	Tejun Heo <tj@...nel.org>, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/6 v2] HID: magicmouse: remove axis data filtering

On Wed, 2010-09-01 at 00:05 +0200, Henrik Rydberg wrote:
> On 08/31/2010 11:56 PM, Chase Douglas wrote:
> 
> > On Tue, 2010-08-31 at 17:51 -0400, Chase Douglas wrote:
> >> On Tue, 2010-08-31 at 23:39 +0200, Henrik Rydberg wrote:
> >>> On 08/31/2010 11:27 PM, Chase Douglas wrote:
> >>>
> >>>> On Tue, 2010-08-31 at 23:18 +0200, Henrik Rydberg wrote:
> >>>>> On 08/31/2010 11:16 PM, Chase Douglas wrote:
> >>>>>
> >>>>>> On Tue, 2010-08-31 at 23:06 +0200, Henrik Rydberg wrote:
> >>>>>>> On 08/31/2010 10:58 PM, Chase Douglas wrote:
> >>>>>>>
> >>>>>>>> On Tue, 2010-08-31 at 22:34 +0200, Henrik Rydberg wrote:
> >>>>>>>>> On 08/31/2010 08:41 PM, Chase Douglas wrote:
> >>>>>>>>>
> >>>>>>>>>> The Magic Mouse device is very precise. No driver filtering of input
> >>>>>>>>>> data needs to be performed.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Chase Douglas <chase.douglas@...onical.com>
> >>>>>>>>>> Acked-by: Michael Poole <mdpoole@...ilus.org>
> >>>>>>>>>> ---
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> I am still not sure this is a good idea. Bandwidth from MT devices is a big
> >>>>>>>>> deal. A statement roughly how much data comes out of mtdev (which does the
> >>>>>>>>> filtering for type A devices) before and after this change would be reassuring.
> >>>>>>>>
> >>>>>>>> As it is right now, hid-magicmouse doesn't support MT slots. I think all
> >>>>>>>> the fuzz code ends up comparing in the MT case is between one touch and
> >>>>>>>> another touch, not between one touch's current location and its previous
> >>>>>>>> location. If I'm correct, then it means a fuzz > 0 is incorrect for
> >>>>>>>> non-slotted MT devices.
> >>>>>>>>
> >>>>>>>> In fact, the code in drivers/input/input.c around line 194 looks like it
> >>>>>>>> discards defuzzing in this case, so one could say this patch is making
> >>>>>>>> things more correct :).
> >>>>>>>
> >>>>>>>
> >>>>>>> For type A devices, the filtering is performed in userspace, in mtdev, in the
> >>>>>>> same manner as it would have been performed in the kernel in the MT slot case.
> >>>>>>> Therefore, knowing the amount of messages coming out of mtdev is a direct
> >>>>>>> measurement of the effect of filtering.
> >>>>>>
> >>>>>> Yes, but we're only interested in the kernel driver when reviewing this
> >>>>>> patch. Leaving the fuzz in as it is has no effect right now on ABS_MT_*
> >>>>>> axes. On the other axes, such as the touch orientation, it's probably
> >>>>>> more harmful than good.
> >>>>>
> >>>>>
> >>>>> "We" are interested in knowing if this patch makes any sense, given that
> >>>>> filtering is in actuality performed in userspace, and thus modifying this code
> >>>>> changes the stream rate into userspace applications, thank you.
> >>>>
> >>>> Because in-kernel defuzzing is turned off for ABS_MT_* axes on
> >>>> non-slotted MT devices, there will be no change in the number of events
> >>>> sent to the userspace due to this patch.
> >>>>
> >>>> Maybe I'm missing something more fundamental. In which case, I'll need
> >>>> more details to get it through my dense skull :).
> >>>
> >>>
> >>> All events passes through to mtdev, yes, but mtdev filters a considerable amount
> >>> of events from passing through to X drivers like evdev. Thus, the fuzz values
> >>> reported in the kernel driver impacts the performance in userspace, even if
> >>> filtering is not done in the kernel.
> >>
> >> My disconnect was that I didn't understand that the fuzz value in the
> >> kernel is exported to userspace. I thought the defuzzing in mtdev was
> >> independent of the defuzzing in the kernel.
> >>
> >> Basically, I don't feel I have the time to do the analysis you want
> >> right now. If you really want, I can just remove this change.
> > 
> > On second thought, if there's no jitter from the device, the only reason
> > to filter is to save bandwidth. The magicmouse devices are very verbose
> > strictly because they have a higher resolution than most devices. If
> > that's an issue for userspace, then the filtering should be based on the
> > resolution. In fact, I see that's what you do in mtdev when the
> > in-kernel fuzz is 0.
> > 
> > This separates the kernel fuzz into a removal of jitter, and the mtdev
> > fuzz into a dampening of the number of events passed to clients. Since
> > magicmouse devices have no discernable jitter, shouldn't we set the
> > in-kernel fuzz to 0 and let mtdev do its thing as appropriate?
> 
> 
> Per-driver data is more precise than a generic value for all devices. This
> thread has documented the details of why there is an interest in the fuzz
> values. Skipping the patch until MT slots are implemented or measurements can be
> done might be the most sensible thing to do.

Yes, there's still per-driver fuzz data. If a device is jittery it makes
sense to set the in-kernel fuzz factor to a reasonable value. If a
device is not jittery, I don't see a need to do any defuzzing of the
values in the driver itself. If we think it's necessary to reduce
bandwidth, then the kernel should be defuzzing uniformly across devices
to compensate. mtdev could do this defuzzing for bandwidth uniformly
too.

As it is, this driver just has an arbitrary value that is not used in
the kernel and likely hasn't ever been used anywhere outside of mtdev. I
think it makes sense to reset the fuzz to the baseline of 0 and fix it
later if we find it's necessary. Otherwise we're just codifying an
arbitrary value.

-- Chase

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists