lkml.org 
[lkml]   [2012]   [Aug]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH 06/19] Input: Send events one packet at a time
    Hi Daniel,

    > On Mon, Aug 13, 2012 at 5:42 AM, Henrik Rydberg <rydberg@euromail.se> wrote:
    > > On heavy event loads, such as a multitouch driver, the irqsoff latency
    > > can be as high as 250 us. By accumulating a frame worth of data
    > > before passing it on, the latency can be dramatically reduced. As a
    > > side effect, the special EV_SYN handling can be removed, since the
    > > frame is now atomic.
    >
    > This patch(set) is very interesting and exciting. Thanks!
    > Some questions and comments inline...
    >
    > >
    > > This patch adds the events() handler callback and uses it if it
    > > exists. The latency is improved by 50 us even without the callback.
    >
    > How much of the savings is just from reducing the number of
    > add_input_randomness() calls from 1-per-input_value to 1-per-frame?

    Some, but the largest saving comes from calling down to evdev more sparsely.

    > Could you achieve similar savings by only calling add_input_randomness
    > on the first input_value after each EV_SYN/SYN_REPORT (ie "when sync =
    > true")?

    It might make a bit of a difference, because of the additional locks,
    but I have not tried it explicitly.

    > > @@ -90,46 +90,81 @@ static void input_stop_autorepeat(struct input_dev *dev)
    > > * filtered out, through all open handles. This function is called with
    > > * dev->event_lock held and interrupts disabled.
    > > */
    > > -static void input_pass_event(struct input_dev *dev,
    > > - unsigned int type, unsigned int code, int value)
    > > +static size_t input_to_handler(struct input_handle *handle,
    > > + struct input_value *vals, size_t count)
    >
    > The only thing that is a little strange with this function is that it
    > actually changes the 'vals' array due to in-place filtering.

    Hm, yes, I did not want to allocate additional memory for the
    filtering stuff. It is only used in a few (one?) place, and TBH, it is
    not on my list of favorite pieces of code. I would rather see that
    api modified than working towards more elaborate filtering schemes.

    > It means
    > that input_to_handler can't handle const arrays of vals, which may
    > have a performance impact in some cases (like key repeat). You are
    > relying on this behavior since you want to pass the final filtered
    > input_value array to ->events() without copying, but this seems to be
    > optimizing the 'filtered' case relative to the (normal?) unfiltered
    > behavior. Probably not worth changing, though.

    Right.

    >
    > > {
    > > - struct input_handler *handler;
    > > - struct input_handle *handle;
    > > + struct input_handler *handler = handle->handler;
    > > + struct input_value *end = vals;
    > > + struct input_value *v;
    > >
    > > - rcu_read_lock();
    > > + for (v = vals; v != vals + count; v++) {
    > > + if (handler->filter &&
    >
    > if (handler->filter == false), you could skip the whole loop and just
    > assign end = vals + count.

    True - in principle, but currently a suboptimization.

    > Also, the original version assumed that if a handler had the grab, it
    > couldn't be a filter, and would skip filtering entirely.
    >
    > > + handler->filter(handle, v->type, v->code, v->value))
    >
    > Maybe we can have a handler->filter_events(handle, vals, count) that
    > returns the number of events left after filtering.
    > This would allow more sophisticated filtering that could inspect an
    > entire frame.

    Possibly. Still, the notion of filtering as information-sharing
    between drivers on the input bus is not one of my favorites. IMHO,
    focus should be on getting the data out of the kernel as quickly as
    possible.

    >
    > > + continue;
    > > + if (end != v)
    > > + *end = *v;
    > > + end++;
    > > + }
    > >
    > > - handle = rcu_dereference(dev->grab);
    > > - if (handle)
    > > - handle->handler->event(handle, type, code, value);
    > > - else {
    > > - bool filtered = false;
    > > + count = end - vals;
    > > + if (!count)
    > > + return 0;
    > >
    > > - list_for_each_entry_rcu(handle, &dev->h_list, d_node) {
    > > - if (!handle->open)
    > > - continue;
    >
    > In the original version, one handler would not call both ->filter()
    > and ->event().
    > I'm not sure if that was a bug or a feature. But, you now make it possible.
    > However, this opens up the possibility of a filter handler processing
    > events via its ->event that would get filtered out by a later
    > handler's filter.

    True, but it does not change any of the existing usages of filtering.

    > In sum, I think if we assume a handler only has either ->filter or
    > ->event (->events), then we can split this function into two, one that
    > only does filtering on filters, and one that only passes the resulting
    > filtered events.
    >
    > > + if (handler->events)
    > > + handler->events(handle, vals, count);
    > > + else
    > > + for (v = vals; v != end; v++)
    > > + handler->event(handle, v->type, v->code, v->value);
    > > +
    > > + return count;
    > > +}

    My standpoint is clear by now, so I shall not repeat myself. :-)

    > > @@ -326,14 +331,35 @@ static void input_handle_event(struct input_dev *dev,
    > > break;
    > > }
    > >
    > > - if (disposition != INPUT_IGNORE_EVENT && type != EV_SYN)
    > > - dev->sync = false;
    > > + return disposition;
    > > +}
    > > +
    > > +static void input_handle_event(struct input_dev *dev,
    > > + unsigned int type, unsigned int code, int value)
    > > +{
    > > + struct input_value *v;
    > > + int disp;
    > > +
    > > + disp = input_get_disposition(dev, type, code, value);
    > >
    > > - if ((disposition & INPUT_PASS_TO_DEVICE) && dev->event)
    > > + if ((disp & INPUT_PASS_TO_DEVICE) && dev->event)
    > > dev->event(dev, type, code, value);
    > >
    > > - if (disposition & INPUT_PASS_TO_HANDLERS)
    > > - input_pass_event(dev, type, code, value);
    > > + if (!dev->vals)
    > > + return;
    > > +
    > > + if (disp & INPUT_PASS_TO_HANDLERS) {
    > > + v = &dev->vals[dev->num_vals++];
    > > + v->type = type;
    > > + v->code = code;
    > > + v->value = value;
    > > + }
    > > +
    > > + if ((disp & INPUT_FLUSH) || (dev->num_vals >= dev->max_vals)) {
    > > + if (dev->num_vals >= 2)
    >
    > I'm not sure about this check. What if the previous "frame" had
    > dev->max_vals + 1 events, and so dev->max_vals of them (all but the
    > SYN_REPORT) were already passed.
    > We would not get that frame's SYN_REPORT all by itself, so "disp &
    > INPUT_FLUSH" is true, but dev->num_vals == 1. We still want to pass
    > the SYN_REPORT immediately, and not save until we get another full
    > frame.
    >
    > Is this even possible?

    Yes, it is possible, if the driver is misconfigured with respect to
    the input buffer size. I have ignored that possibility in a few other
    places as well (keyboard repeat for one). You are probably right in
    that it should be handled somehow, but I would rather make sure the
    buffer is always large enough. The atomicity of the frame is really
    what makes things go faster.

    >
    > > + input_pass_values(dev, dev->vals, dev->num_vals);
    > > + dev->num_vals = 0;
    > > + }
    > > }
    > >
    > > /**
    > > @@ -361,7 +387,6 @@ void input_event(struct input_dev *dev,
    > > if (is_event_supported(type, dev->evbit, EV_MAX)) {
    > >
    > > spin_lock_irqsave(&dev->event_lock, flags);
    > > - add_input_randomness(type, code, value);
    > > input_handle_event(dev, type, code, value);
    > > spin_unlock_irqrestore(&dev->event_lock, flags);
    > > }
    > > @@ -842,8 +867,7 @@ int input_set_keycode(struct input_dev *dev,
    > > __test_and_clear_bit(old_keycode, dev->key)) {
    > >
    > > input_pass_event(dev, EV_KEY, old_keycode, 0);
    > > - if (dev->sync)
    > > - input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
    > > + input_pass_event(dev, EV_SYN, SYN_REPORT, 1);
    > > }
    > >
    > > out:
    > > @@ -1425,6 +1449,7 @@ static void input_dev_release(struct device *device)
    > > input_ff_destroy(dev);
    > > input_mt_destroy_slots(dev);
    > > kfree(dev->absinfo);
    > > + kfree(dev->vals);
    > > kfree(dev);
    > >
    > > module_put(THIS_MODULE);
    > > @@ -1845,6 +1870,14 @@ int input_register_device(struct input_dev *dev)
    > > if (dev->hint_events_per_packet < packet_size)
    > > dev->hint_events_per_packet = packet_size;
    > >
    > > + dev->num_vals = 0;
    > > + dev->max_vals = max(dev->hint_events_per_packet, packet_size);
    > > +
    > > + kfree(dev->vals);
    > How could this already be non-NULL? Is it possible to re-register a device?

    Uhm, you are probably right.

    > A huge optimization to input event processing is pretty exciting!

    Thanks for the review, I will send out a new patchset taking all the
    response so far into account.


    \
     
     \ /
      Last update: 2012-08-25 22:01    [W:0.042 / U:93.424 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site