Re: [PATCH libinput 2/2] evdev: ratelimit SYN_DROPPED logging

[Peter Hutterer]

On Tue, Nov 04, 2014 at 03:17:36PM -0800, Bill Spitzak wrote:
> I got the impression the ratelimit object was intended to be a single static
> instance, not per-device. The idea is that if there is a burst of these at
> once they are all from the same device, or due to some interaction between
> them, so you want to limit it all. Also it prevents an implementation detail
> from being in the exposed data structure.

it's fine to be per device. most use-cases I've seen so far only had a
single device triggering this anyway (the touchpad/touchscreen). and the
information that multiple devices are stalling can be quite useful in
debugging.

Cheers,
   Peter

> 
> On 11/04/2014 12:35 AM, David Herrmann wrote:
> >Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we
> >will still receive SYN_DROPPED log-messages after multiple days of
> >runtime, even though there might have been a SYN_DROPPED flood at one
> >point in time.
> >
> >Signed-off-by: David Herrmann 
> >---
> >  src/evdev.c | 14 +++---
> >  src/evdev.h |  4 +---
> >  2 files changed, 8 insertions(+), 10 deletions(-)
> >
> >diff --git a/src/evdev.c b/src/evdev.c
> >index 3aa87a7..836ce56 100644
> >--- a/src/evdev.c
> >+++ b/src/evdev.c
> >@@ -926,16 +926,14 @@ evdev_device_dispatch(void *data)
> > rc = libevdev_next_event(device->evdev,
> >  LIBEVDEV_READ_FLAG_NORMAL, &ev);
> > if (rc == LIBEVDEV_READ_STATUS_SYNC) {
> >-if (device->syn_drops_received < 10) {
> >-device->syn_drops_received++;
> >+if (ratelimit_test(&device->syn_drop_limit))
> > log_info(libinput, "SYN_DROPPED event from "
> >  "\"%s\" - some input events have "
> >  "been lost.\n", device->devname);
> >-if (device->syn_drops_received == 10)
> >-log_info(libinput, "No longer logging "
> >- "SYN_DROPPED events for "
> >- "\"%s\"\n", device->devname);
> >-}
> >+else if (ratelimit_cutoff(&device->syn_drop_limit))
> >+log_info(libinput, "SYN_DROPPED flood "
> >+ "for \"%s\"\n",
> >+ device->devname);
> >
> > /* send one more sync event so we handle all
> >currently pending events before we sync up
> >@@ -1296,6 +1294,8 @@ evdev_device_create(struct libinput_seat *seat,
> > device->scroll.threshold = 5.0; /* Default may be overridden */
> > device->scroll.direction = 0;
> > device->dpi = DEFAULT_MOUSE_DPI;
> >+/* at most 5 SYN_DROPPED log-messages per 30s */
> >+device->syn_drop_limit = RATELIMIT_INIT(30ULL * 1000 * 1000, 5);
> >
> > matrix_init_identity(&device->abs.calibration);
> > matrix_init_identity(&device->abs.usermatrix);
> >diff --git a/src/evdev.h b/src/evdev.h
> >index 666c8dc..eefbb79 100644
> >--- a/src/evdev.h
> >+++ b/src/evdev.h
> >@@ -137,9 +137,7 @@ struct evdev_device {
> > } buttons;
> >
> > int dpi; /* HW resolution */
> >-/* The number of times libevdev processes a SYN_DROPPED, so we can
> >- * stop logging them to avoid flooding the logs. */
> >-int syn_drops_received;
> >+struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */
> >  };
> >
> >  #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 2/2] evdev: ratelimit SYN_DROPPED logging

[Peter Hutterer]

On Tue, Nov 04, 2014 at 09:35:38AM +0100, David Herrmann wrote:
> Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we
> will still receive SYN_DROPPED log-messages after multiple days of
> runtime, even though there might have been a SYN_DROPPED flood at one
> point in time.
> 
> Signed-off-by: David Herrmann 
> ---
>  src/evdev.c | 14 +++---
>  src/evdev.h |  4 +---
>  2 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/src/evdev.c b/src/evdev.c
> index 3aa87a7..836ce56 100644
> --- a/src/evdev.c
> +++ b/src/evdev.c
> @@ -926,16 +926,14 @@ evdev_device_dispatch(void *data)
>   rc = libevdev_next_event(device->evdev,
>LIBEVDEV_READ_FLAG_NORMAL, &ev);
>   if (rc == LIBEVDEV_READ_STATUS_SYNC) {
> - if (device->syn_drops_received < 10) {
> - device->syn_drops_received++;
> + if (ratelimit_test(&device->syn_drop_limit))
>   log_info(libinput, "SYN_DROPPED event from "
>"\"%s\" - some input events have "
>"been lost.\n", device->devname);
> - if (device->syn_drops_received == 10)
> - log_info(libinput, "No longer logging "
> -  "SYN_DROPPED events for "
> -  "\"%s\"\n", device->devname);
> - }
> + else if (ratelimit_cutoff(&device->syn_drop_limit))
> + log_info(libinput, "SYN_DROPPED flood "
> +  "for \"%s\"\n",

s/for/from/ I think.

> +  device->devname);
>  
>   /* send one more sync event so we handle all
>  currently pending events before we sync up
> @@ -1296,6 +1294,8 @@ evdev_device_create(struct libinput_seat *seat,
>   device->scroll.threshold = 5.0; /* Default may be overridden */
>   device->scroll.direction = 0;
>   device->dpi = DEFAULT_MOUSE_DPI;
> + /* at most 5 SYN_DROPPED log-messages per 30s */
> + device->syn_drop_limit = RATELIMIT_INIT(30ULL * 1000 * 1000, 5);

for our use it'd be perfectly fine to have ratelimits defined in
ms rather than µs, would make this code nicer to look at.
  
Cheers,
   Peter

>   matrix_init_identity(&device->abs.calibration);
>   matrix_init_identity(&device->abs.usermatrix);
> diff --git a/src/evdev.h b/src/evdev.h
> index 666c8dc..eefbb79 100644
> --- a/src/evdev.h
> +++ b/src/evdev.h
> @@ -137,9 +137,7 @@ struct evdev_device {
>   } buttons;
>  
>   int dpi; /* HW resolution */
> - /* The number of times libevdev processes a SYN_DROPPED, so we can
> -  * stop logging them to avoid flooding the logs. */
> - int syn_drops_received;
> + struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */
>  };
>  
>  #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
> -- 
> 2.1.3
> 
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 1/2] util: introduce ratelimit helpers

[Peter Hutterer]

On Tue, Nov 04, 2014 at 09:35:37AM +0100, David Herrmann wrote:
> This adds "struct ratelimit" and "ratelimit_test()". It's a very simple
> rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young.
> 
> This comes in handy to limit log-messages in possible busy loops etc..
> 
> Signed-off-by: David Herrmann 
> ---
>  src/libinput-util.c | 48 
>  src/libinput-util.h | 19 +++
>  test/misc.c | 37 +
>  3 files changed, 104 insertions(+)
> 
> diff --git a/src/libinput-util.c b/src/libinput-util.c
> index eeb9786..19594e3 100644
> --- a/src/libinput-util.c
> +++ b/src/libinput-util.c
> @@ -65,3 +65,51 @@ list_empty(const struct list *list)
>  {
>   return list->next == list;
>  }
> +
> +/*
> + * Perform rate-limit test. Returns true if the rate-limited action is still
> + * allowed, false if it should be suppressed.
> + *
> + * The ratelimit object must be initialized via RATELIMIT_INIT().
> + *
> + * Modelled after Linux' lib/ratelimit.c by Dave Young
> + * , which is licensed GPLv2.
> + */
> +bool ratelimit_test(struct ratelimit *r)

libinput style is: return type on a separate line

> +{
> + struct timespec ts;
> + uint64_t utime;
> +
> + if (r->interval <= 0 || r->burst <= 0)
> + return true;
> +
> + clock_gettime(CLOCK_MONOTONIC, &ts);
> + utime = ts.tv_sec * 1000 * 1000 + ts.tv_nsec / 1000;
> +
> + if (r->begin <= 0 || r->begin + r->interval < utime) {
> + /* reset counter */
> + r->begin = utime;
> + r->num = 1;
> + return true;
> + } else if (r->num < r->burst) {
> + /* continue burst */
> + r->num++;
> + return true;
> + }
> +
> + /* rate-limit with overflow check */
> + if (r->num + 1 > r->num)
> + ++r->num;
> +
> + return false;
> +}
> +
> +/*
> + * Return true if the ratelimit counter just crossed the cutoff value. That 
> is,
> + * this function returns true iff the last call to ratelimit_test() was the

s/iff/if/

> + * first call to exceed the burst value in this interval.
> + */
> +bool ratelimit_cutoff(struct ratelimit *r)

bool on separate line please

> +{
> + return r->num == r->burst + 1;
> +}


I'm wondering: why have two separate functions here? 

how about an
enum ratelimit {
RATELIMIT_PASS,
RATELIMIT_THRESHOLD,
RATELIMIT_EXCEEDED,
};

then return that from ratelimit_test and then use the return value to
decide on the rest of the handling?
so the dispatch code would be:
if ((rc = ratelimit_test(...)) != RATELIMIT_EXCEEDED)) {
 log_info("SYN_DROPPED");
 if (rc == RATELIMIT_THRESHOLD) {
log_info("SYN_DROPPED flood");
 }
}

or the same with a switch statement.

> diff --git a/src/libinput-util.h b/src/libinput-util.h
> index 51759e8..8ff8778 100644
> --- a/src/libinput-util.h
> +++ b/src/libinput-util.h
> @@ -25,6 +25,7 @@
>  
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -280,4 +281,22 @@ matrix_to_farray6(const struct matrix *m, float out[6])
>   out[5] = m->val[1][2];
>  }
>  
> +struct ratelimit {
> + uint64_t interval;
> + uint64_t begin;
> + unsigned burst;
> + unsigned num;

unsigned int please

> +} RateLimit;

well, hello. what are you doing here? are you lost? :)

> +
> +#define RATELIMIT_INIT(_interval, _burst)\
> + ((struct ratelimit){\
> + .interval = (_interval),\
> + .begin = 0, \
> + .burst = (_burst),  \
> + .num = 0,   \
> + })

any reason you didn't make this into a function of
void ratelimit_init(struct ratelimit *rl)?
I don't see a lot of benefits having this as a macro given that it's only
called once anyway (per context).

> +
> +bool ratelimit_test(struct ratelimit *r);
> +bool ratelimit_cutoff(struct ratelimit *r);
> +
>  #endif /* LIBINPUT_UTIL_H */
> diff --git a/test/misc.c b/test/misc.c
> index 1512180..70b3e57 100644
> --- a/test/misc.c
> +++ b/test/misc.c
> @@ -508,6 +508,42 @@ START_TEST(matrix_helpers)
>  }
>  END_TEST
>  
> +START_TEST(ratelimit_helpers)
> +{
> + /* 10 attempts every 10ms */
> + struct ratelimit rl = RATELIMIT_INIT(1, 10);
> + unsigned int i, j;
> +
> + for (j = 0; j < 100; ++j) {
> + /* a burst of 10 attempts must succeed */
> + for (i = 0; i < 10; ++i) {
> + ck_assert(ratelimit_test(&rl));
> + ck_assert(!ratelimit_cutoff(&rl));
> + }
> +
> + /* ..then further attempts must fail.. */
> + ck_assert(!ratelimit_test(&rl));
> + ck_assert(ratelimit_cutoff(&rl));

you could just drop the above two lines and merge the comments into one.

> +
> + /* ..re

[PATCH libinput 4/4] test: add the MS surface touch cover device and fake-mt tests

[Peter Hutterer]

In the device description, define the interfaces for touch down/move even
though we technically don't have those interfaces. Makes it easier to test.

The fake-mt tests make sure the device shows up correctly and that no touch
events are being sent for touch events.
This device is a pointer device too, the pointer tests will test it for
correct functionality of the REL_X/Y bits, no special test needed.

Signed-off-by: Peter Hutterer 
---
 test/Makefile.am   |   1 +
 test/litest-ms-surface-cover.c | 386 +
 test/litest.h  |   2 +
 test/touch.c   |  46 +
 4 files changed, 435 insertions(+)
 create mode 100644 test/litest-ms-surface-cover.c

diff --git a/test/Makefile.am b/test/Makefile.am
index 6a68982..d932961 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -20,6 +20,7 @@ liblitest_la_SOURCES = \
litest-generic-singletouch.c \
litest-keyboard.c \
litest-mouse.c \
+   litest-ms-surface-cover.c \
litest-synaptics.c \
litest-synaptics-st.c \
litest-synaptics-t440.c \
diff --git a/test/litest-ms-surface-cover.c b/test/litest-ms-surface-cover.c
new file mode 100644
index 000..bd8b1a1
--- /dev/null
+++ b/test/litest-ms-surface-cover.c
@@ -0,0 +1,386 @@
+/*
+ * Copyright © 2014 Red Hat, Inc.
+ *
+ * Permission to use, copy, modify, distribute, and sell this software and its
+ * documentation for any purpose is hereby granted without fee, provided that
+ * the above copyright notice appear in all copies and that both that copyright
+ * notice and this permission notice appear in supporting documentation, and
+ * that the name of the copyright holders not be used in advertising or
+ * publicity pertaining to distribution of the software without specific,
+ * written prior permission.  The copyright holders make no representations
+ * about the suitability of this software for any purpose.  It is provided "as
+ * is" without express or implied warranty.
+ *
+ * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS SOFTWARE,
+ * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
+ * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
+ * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE,
+ * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+ * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE
+ * OF THIS SOFTWARE.
+ */
+
+#if HAVE_CONFIG_H
+#include "config.h"
+#endif
+
+#include "litest.h"
+#include "litest-int.h"
+
+static void
+litest_ms_surface_cover_setup(void)
+{
+   struct litest_device *d = litest_create_device(LITEST_MS_SURFACE_COVER);
+   litest_set_current_device(d);
+}
+
+static struct input_event down[] = {
+   { .type = EV_ABS, .code = ABS_MT_SLOT, .value = LITEST_AUTO_ASSIGN },
+   { .type = EV_ABS, .code = ABS_MT_TRACKING_ID, .value = 
LITEST_AUTO_ASSIGN },
+   { .type = EV_ABS, .code = ABS_MT_POSITION_X, .value = 
LITEST_AUTO_ASSIGN },
+   { .type = EV_ABS, .code = ABS_MT_POSITION_Y, .value = 
LITEST_AUTO_ASSIGN },
+   { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
+   { .type = -1, .code = -1 },
+};
+
+static struct input_event move[] = {
+   { .type = EV_ABS, .code = ABS_MT_SLOT, .value = LITEST_AUTO_ASSIGN },
+   { .type = EV_ABS, .code = ABS_MT_POSITION_X, .value = 
LITEST_AUTO_ASSIGN },
+   { .type = EV_ABS, .code = ABS_MT_POSITION_Y, .value = 
LITEST_AUTO_ASSIGN },
+   { .type = EV_SYN, .code = SYN_REPORT, .value = 0 },
+   { .type = -1, .code = -1 },
+};
+
+/* We define down/move so that we can emulate fake touches on this device,
+   to make sure nothing crashes. */
+static struct litest_device_interface interface = {
+   .touch_down_events = down,
+   .touch_move_events = move,
+};
+
+static struct input_absinfo absinfo[] = {
+   { ABS_VOLUME, 0, 1023, 0, 0, 0 },
+   { ABS_MISC, 0, 255, 0, 0, 0 },
+   { 41, 0, 255, 0, 0, 0 },
+   { 42, -127, 127, 0, 0, 0 },
+   { 43, -127, 127, 0, 0, 0 },
+   { 44, -127, 127, 0, 0, 0 },
+   { 45, -127, 127, 0, 0, 0 },
+   { 46, -127, 127, 0, 0, 0 },
+   { 47, -127, 127, 0, 0, 0 },
+   /* ABS_MT range overlap starts here */
+   { 48, -127, 127, 0, 0, 0 }, /* ABS_MT_SLOT */
+   { 49, -127, 127, 0, 0, 0 },
+   { 50, -127, 127, 0, 0, 0 },
+   { 51, -127, 127, 0, 0, 0 },
+   { 52, -127, 127, 0, 0, 0 },
+   { 53, -127, 127, 0, 0, 0 },
+   { 54, -127, 127, 0, 0, 0 },
+   { 55, -127, 127, 0, 0, 0 },
+   { 56, -127, 127, 0, 0, 0 },
+   { 57, -127, 127, 0, 0, 0 },
+   { 58, -127, 127, 0, 0, 0 },
+   { 59, -127, 127, 0, 0, 0 },
+   { 60, -127, 127, 0, 0, 0 },
+   { 61, -127, 127, 0, 0, 0 }, /* ABS_MT_TOOL_Y */
+   { 62, -127, 127, 0, 0, 0 },
+   { .value = -1 },
+};
+
+static struct input_id input_id = {
+   .bustype = 0x3,
+  

[PATCH libinput 2/4] evdev: move a comment to where it belongs

[Peter Hutterer]

And s/weston/libinput/ while we're at it

Signed-off-by: Peter Hutterer 
---
 src/evdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index ecf105d..bda6af4 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1096,9 +1096,6 @@ evdev_configure_device(struct evdev_device *device)
device->abs.absinfo_y = absinfo;
has_abs = 1;
}
-/* We only handle the slotted Protocol B in weston.
-   Devices with ABS_MT_POSITION_* but not ABS_MT_SLOT
-   require mtdev for conversion. */
if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) &&
libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) {
absinfo = libevdev_get_abs_info(evdev, 
ABS_MT_POSITION_X);
@@ -1118,6 +1115,9 @@ evdev_configure_device(struct evdev_device *device)
has_touch = 1;
has_mt = 1;
 
+   /* We only handle the slotted Protocol B in libinput.
+  Devices with ABS_MT_POSITION_* but not ABS_MT_SLOT
+  require mtdev for conversion. */
if (evdev_need_mtdev(device)) {
device->mtdev = mtdev_new_open(device->fd);
if (!device->mtdev)
-- 
2.1.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput 1/4] evdev: factor out resolution changing code

[Peter Hutterer]

No functional changes.

Signed-off-by: Peter Hutterer 
---
 src/evdev.c | 57 -
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index 3aa87a7..ecf105d 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1038,13 +1038,31 @@ evdev_tag_device(struct evdev_device *device)
udev_unref(udev);
 }
 
+static inline int
+evdev_fix_abs_resolution(struct libevdev *evdev,
+unsigned int code,
+const struct input_absinfo *absinfo)
+{
+   struct input_absinfo fixed;
+
+   if (absinfo->resolution == 0) {
+   fixed = *absinfo;
+   fixed.resolution = 1;
+   /* libevdev_set_abs_info() changes the absinfo we already
+  have a pointer to, no need to fetch it again */
+   libevdev_set_abs_info(evdev, code, &fixed);
+   return 1;
+   } else {
+   return 0;
+   }
+}
+
 static int
 evdev_configure_device(struct evdev_device *device)
 {
struct libinput *libinput = device->base.seat->libinput;
struct libevdev *evdev = device->evdev;
const struct input_absinfo *absinfo;
-   struct input_absinfo fixed;
int has_abs, has_rel, has_mt;
int has_button, has_keyboard, has_touch;
struct mt_slot *slots;
@@ -1063,22 +1081,18 @@ evdev_configure_device(struct evdev_device *device)
if (libevdev_has_event_type(evdev, EV_ABS)) {
 
if ((absinfo = libevdev_get_abs_info(evdev, ABS_X))) {
-   if (absinfo->resolution == 0) {
-   fixed = *absinfo;
-   fixed.resolution = 1;
-   libevdev_set_abs_info(evdev, ABS_X, &fixed);
+   if (evdev_fix_abs_resolution(evdev,
+ABS_X,
+absinfo))
device->abs.fake_resolution = 1;
-   }
device->abs.absinfo_x = absinfo;
has_abs = 1;
}
if ((absinfo = libevdev_get_abs_info(evdev, ABS_Y))) {
-   if (absinfo->resolution == 0) {
-   fixed = *absinfo;
-   fixed.resolution = 1;
-   libevdev_set_abs_info(evdev, ABS_Y, &fixed);
+   if (evdev_fix_abs_resolution(evdev,
+ABS_Y,
+absinfo))
device->abs.fake_resolution = 1;
-   }
device->abs.absinfo_y = absinfo;
has_abs = 1;
}
@@ -1088,24 +1102,17 @@ evdev_configure_device(struct evdev_device *device)
if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) &&
libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) {
absinfo = libevdev_get_abs_info(evdev, 
ABS_MT_POSITION_X);
-   if (absinfo->resolution == 0) {
-   fixed = *absinfo;
-   fixed.resolution = 1;
-   libevdev_set_abs_info(evdev,
- ABS_MT_POSITION_X,
- &fixed);
+   if (evdev_fix_abs_resolution(evdev,
+ABS_MT_POSITION_X,
+absinfo))
device->abs.fake_resolution = 1;
-   }
device->abs.absinfo_x = absinfo;
+
absinfo = libevdev_get_abs_info(evdev, 
ABS_MT_POSITION_Y);
-   if (absinfo->resolution == 0) {
-   fixed = *absinfo;
-   fixed.resolution = 1;
-   libevdev_set_abs_info(evdev,
- ABS_MT_POSITION_Y,
- &fixed);
+   if (evdev_fix_abs_resolution(evdev,
+ABS_MT_POSITION_Y,
+absinfo))
device->abs.fake_resolution = 1;
-   }
device->abs.absinfo_y = absinfo;
device->is_mt = 1;
has_touch = 1;
-- 
2.1.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput 3/4] evdev: handle fake MT devices

[Peter Hutterer]

The kernel requires absolute axes to fit into the semantic ABS_ naming
scheme but doesn't provide enough free bits unlabelled axes. Devices with many
axes run into the ABS_MT range and look like MT devices when they're not.
See http://www.freedesktop.org/software/libevdev/doc/1.3/group__mt.html

Affected is e.g. the MS Surface 2 touch cover that has codes [41, 62]
set for min/max [-127, 127].

No special handling needed other than forcing has_mt/has_touch to be 0.
ABS_MT_* events from non-touch devices are discarded by libinput.

The has_mt/has_touch = 0 isn't needed, but looks nicer than an empty if
body.

Fixes https://bugs.freedesktop.org/show_bug.cgi?id=85836

Signed-off-by: Peter Hutterer 
CC: Leonid Borisenko 
---
 src/evdev.c | 10 +-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/evdev.c b/src/evdev.c
index bda6af4..6dad32a 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1096,7 +1096,15 @@ evdev_configure_device(struct evdev_device *device)
device->abs.absinfo_y = absinfo;
has_abs = 1;
}
-   if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_X) &&
+
+   /* Fake MT devices have the ABS_MT_SLOT bit set because of
+  the limited ABS_* range - they aren't MT devices, they
+  just have too many ABS_ axes */
+   if (libevdev_has_event_code(evdev, EV_ABS, ABS_MT_SLOT) &&
+   libevdev_get_num_slots(evdev) == -1) {
+   has_mt = 0;
+   has_touch = 0;
+   } else if (libevdev_has_event_code(evdev, EV_ABS, 
ABS_MT_POSITION_X) &&
libevdev_has_event_code(evdev, EV_ABS, ABS_MT_POSITION_Y)) {
absinfo = libevdev_get_abs_info(evdev, 
ABS_MT_POSITION_X);
if (evdev_fix_abs_resolution(evdev,
-- 
2.1.0

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] cosmetic: replace boolean function return values with bool

[Bill Spitzak]

On 11/04/2014 02:48 AM, Pekka Paalanen wrote:


I kind of wonder what including stdbool.h does to C++ sources using
compositor.h, but if that breaks, we'll hear about it.


Looks like there are attempts to make this work in gcc at least. The 
result is that the C++ bool, true, and false are used.


If I understand it right, doing "#include " in C++ will get 
/usr/include/c++/4.6/tr1/stdbool.h.


That file then includes /usr/include/c++/4.6/tr1/cstdbool

That file then includes /usr/lib/gcc/x86_64-linux-gnu/4.6/include/stdbool.h

That file (which would have worked if included directly instead of those 
C++ wrappers) then defines bool=bool, true=true, false=false, and 
_Bool=bool. I guess the reason it does this is so code that does "#ifdef 
bool" will work? I think that is overkill and it should have just done 
nothing.


Typically messy but functional.
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 2/2] evdev: ratelimit SYN_DROPPED logging

[Bill Spitzak]

I got the impression the ratelimit object was intended to be a single 
static instance, not per-device. The idea is that if there is a burst of 
these at once they are all from the same device, or due to some 
interaction between them, so you want to limit it all. Also it prevents 
an implementation detail from being in the exposed data structure.


On 11/04/2014 12:35 AM, David Herrmann wrote:

Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we
will still receive SYN_DROPPED log-messages after multiple days of
runtime, even though there might have been a SYN_DROPPED flood at one
point in time.

Signed-off-by: David Herrmann 
---
  src/evdev.c | 14 +++---
  src/evdev.h |  4 +---
  2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index 3aa87a7..836ce56 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -926,16 +926,14 @@ evdev_device_dispatch(void *data)
rc = libevdev_next_event(device->evdev,
 LIBEVDEV_READ_FLAG_NORMAL, &ev);
if (rc == LIBEVDEV_READ_STATUS_SYNC) {
-   if (device->syn_drops_received < 10) {
-   device->syn_drops_received++;
+   if (ratelimit_test(&device->syn_drop_limit))
log_info(libinput, "SYN_DROPPED event from "
 "\"%s\" - some input events have "
 "been lost.\n", device->devname);
-   if (device->syn_drops_received == 10)
-   log_info(libinput, "No longer logging "
-"SYN_DROPPED events for "
-"\"%s\"\n", device->devname);
-   }
+   else if (ratelimit_cutoff(&device->syn_drop_limit))
+   log_info(libinput, "SYN_DROPPED flood "
+"for \"%s\"\n",
+device->devname);

/* send one more sync event so we handle all
   currently pending events before we sync up
@@ -1296,6 +1294,8 @@ evdev_device_create(struct libinput_seat *seat,
device->scroll.threshold = 5.0; /* Default may be overridden */
device->scroll.direction = 0;
device->dpi = DEFAULT_MOUSE_DPI;
+   /* at most 5 SYN_DROPPED log-messages per 30s */
+   device->syn_drop_limit = RATELIMIT_INIT(30ULL * 1000 * 1000, 5);

matrix_init_identity(&device->abs.calibration);
matrix_init_identity(&device->abs.usermatrix);
diff --git a/src/evdev.h b/src/evdev.h
index 666c8dc..eefbb79 100644
--- a/src/evdev.h
+++ b/src/evdev.h
@@ -137,9 +137,7 @@ struct evdev_device {
} buttons;

int dpi; /* HW resolution */
-   /* The number of times libevdev processes a SYN_DROPPED, so we can
-* stop logging them to avoid flooding the logs. */
-   int syn_drops_received;
+   struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */
  };

  #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)


___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 0/4] some acceleration fixes, mostly for high DPI mice

[Peter Hutterer]

On Tue, Nov 04, 2014 at 09:00:06AM -0600, Derek Foreman wrote:
> On 03/11/14 09:51 PM, Peter Hutterer wrote:
> > On Mon, Nov 03, 2014 at 11:56:59AM +0100, David Herrmann wrote:
> >> Hi
> >>
> >> On Fri, Oct 31, 2014 at 5:33 AM, Peter Hutterer
> >>  wrote:
> >>> On Thu, Oct 30, 2014 at 04:34:12PM -0500, Derek Foreman wrote:
>  The acceleration filter currently isn't particularly pleased with gaming 
>  mice.
> 
>  They generally have high DPI (can be over 8000 DPI) and can have high 
>  update
>  rates (1000+ per second).  This can result in the accel curve being 
>  biased
>  heavily towards the high points on the accel curve.
> 
>  This patch set allows a way to normalize the deltas to 400DPI so when the
>  DPI setting of the mouse is correct, it should feel much the same as a
>  normal mouse.
> 
>  This is, of course, only a partial solution.  Setting a reasonable 
>  default
>  is a massive problem that needs to be addressed in the future - for now
>  we just set it to 400 (which may actually not be that prevalent any more
>  but there doesn't seem to be such thing as a "standard" DPI mouse).
> >>>
> >>> Thanks, I've pushed 1 to 3 (with added comments), not quite happy with
> >>> 4 yet. As a configuration interface, it's mismatched with the usual 
> >>> quartett
> >>> of hooks that we provide for all other config options.
> >>>
> >>> But I think providing a visible API may not be the right approach here
> >>> anyway. One idea I had here was to have this provided through udev, set
> >>> as something like:
> >>> LIBINPUT_MOUSE_DPI="400@80" # dpi @ poll rate
> >>>
> >>> over time, the udev hwdb could add those settings, or we ship them as
> >>> extra rules, or users configure them themselves. this is unfortunately a
> >>> rather nasty task given the amount of HW out there, but I really can't 
> >>> think
> >>> of any other way (short of heuristics which will fail in too many ways).
> >>>
> >>> Any ideas on feasability or better approaches welcome.
> >>
> >> hwdb can be used easily here, but it's indeed a huge amount to gather
> >> all that data. HID doesn't have a way to query this information,
> >> sadly, and I haven't found any generic vendor extensions (maybe
> >> Benjamin knows more).
> >>
> >> We could use the REL_X/REL_Y MAX values to guess the DPI setting and
> >> ignore it if it's out of the expected range (sth. like 100-10k).
> >>
> >> I haven't spent much time thinking it through, but so far I'd prefer a
> >> solid, but basic, heuristic to guess the DPI and then use hwdb for
> >> anything that doesn't fit. This allows us to change the heuristic at
> >> any time and we don't have to introduce any APIs. We can even ship the
> >> hwdb files with libinput.
> > 
> > tbh, I don't think heuristics will work. there is no reference point and
> > when you're looking at relative motion you can't tell if a delta of
> > 10 means the pointer has moved a large distance at low-res or a small
> > distance at high-res.
> >  
> > Anyway, for some data:
> > I recorded my Logitech G500s here with three different dpi settings (400,
> > 800, 2000) and a MS Comfort Optical Mouse 3000 (1000 dpi). A couple of
> > interesting things: regardless of dpi, virtually all events are within [-3,
> > +3] dx/dy and there's nothing over 10. Exception here is the MS mouse which
> > goes up to 78 for a delta, for reasons I can't explain yet.
> 
> Poll rate?  Your Logitech G500s is a Ridiculous Gaming Mouse(TM) with a
> 1000Hz report rate - I suspect the Microsoft mouse is 125Hz - have you
> looked at the time deltas?

Yeah, I suspected as much yesterday but hadn't checked yet. This time with
actual data:
Average delta in µs between two events is 2776.79, 3347.15, 3511.00 for the
G500 recordings (dpi 2000, 400, 800). The MS mouse has 9975.60 µs on
average.

Deltas taken between SYN_REPORT, with all deltas > 50ms dropped for noise
reduction (it was normal desktop usage after all, with many pauses).
Given the remaining noise, that looks close enough to 125Hz on the MS mouse,
and 500Hz on the G500 (which is what the OS X software claims it is set to)

I still think that deltas of 78 seem a bit high, but it's hard to remember
what triggered this (accidental bump of the mouse maybe)

> Unsurprisingly it looks like my results with a mouse set to 1800DPI,
> 1000Hz polling are very similar to your 2000DPI log with no events with
> a delta higher than 11.  I haven't created a nice histogram though.
> 
> This mouse can also be set to 125Hz, where it generates proportionally
> larger deltas.
> 
> We could probably figure out the poll rate heuristically, but I think
> that's useless information since timestamps are already taken into
> account in the acceleration filter.

I agree. It'd be nice to know but not sure what to do with it.

Cheers,
   Peter
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists

Re: Wayland and Weston in Patchwork

[Bryce Harrington]

On Tue, Nov 04, 2014 at 01:14:20PM +0200, Pekka Paalanen wrote:
> Hi all,
> 
> some of you already know that we now have
> http://patchwork.freedesktop.org/project/wayland/list/
> to keep track of the patches sent to wayland-devel mailing list.
> 
> Thanks to Daniel for setting that up. :-)

Ditto thanks Daniel, and thanks pq for this guidance in using it!

> You change the patch status by clicking the patch link in the list, and
> on the following page you should have box titled "Patch Properties"
> near the top. If you don't see that, you don't have permissions to
> change it (e.g. not logged in, or unregistered email address).

There are several defined states:

 New
 Under Review
 Accepted
 Rejected
 RFC
 Not Applicable
 Changes Requested
 Awaiting Upstream
 Superseded
 Deferred

Some of these seem obvious, others less so.  I think it would help to
have a glossary of what they mean, so we use them consistently.  I poked
around but didn't see one.

In particular, if I review someone else's patch and feel it is fine to
be landed, how do I mark it to signal to the Committers "this is ready
to land"?  Should I mark it Accepted?  Do I Delegate it to one of the
Committers?

When is Under Review used?  Looking at the patchwork source this state
and New are the two 'action_required' states, so perhaps Under Review
should be for incoming patches that need someone to review them?

> Shortcomings:
> 
> There are several features we would like to see in Patchwork but AFAIK
> are not there (yet?). Patchwork does not recognize re-submissions so
> that it could automatically set a patch as "Superseded".

Might be nice if it permitted project-specific states, or even just a
richer set of states to track things like if the patch has passed the
testsuite, and to differentiate between okay-in-concept reviews and code
quality reviews.

> It does not maintain patch sets. You can create "Bundles", but so far
> those are just named collections of individual patches, and you need
> to create them manually.

If I create a Bundle of patches (e.g. Derek's large transform patchset),
does that help others?  Or are Bundles just personal?

Bryce
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston v2] compositor: Refactor weston_output_mode_switch()

[Derek Foreman]

This breaks weston_output_mode_switch() into 3 functions:
weston_output_mode_set_native()
weston_output_mode_switch_to_temporary()
weston_output_mode_switch_to_native()

Differences from previous behaviour:
SET_NATIVE didn't set current_scale (now it does)
SET_TEMPORARY could set mode and scale independently - now it can't.

Signed-off-by: Derek Foreman 
---
In previous version weston_output_mode_switch_to_temporary didn't
call weston_mode_switch_finish() correctly

 desktop-shell/shell.c   |  12 +--
 fullscreen-shell/fullscreen-shell.c |  13 +--
 src/compositor-rdp.c|   2 +-
 src/compositor.c| 185 
 src/compositor.h|  17 ++--
 5 files changed, 123 insertions(+), 106 deletions(-)

diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c
index 9fafb39..844a322 100644
--- a/desktop-shell/shell.c
+++ b/desktop-shell/shell.c
@@ -2247,12 +2247,8 @@ shell_surface_set_class(struct wl_client *client,
 static void
 restore_output_mode(struct weston_output *output)
 {
-   if (output->original_mode ||
-   (int32_t)output->current_scale != output->original_scale)
-   weston_output_switch_mode(output,
- output->native_mode,
- output->native_scale,
- WESTON_MODE_SWITCH_RESTORE_NATIVE);
+   if (output->original_mode)
+   weston_output_mode_switch_to_native(output);
 }
 
 static void
@@ -2876,8 +2872,8 @@ shell_configure_fullscreen(struct shell_surface *shsurf)
surf_height * 
surface->buffer_viewport.buffer.scale,
shsurf->fullscreen.framerate};
 
-   if (weston_output_switch_mode(output, &mode, 
surface->buffer_viewport.buffer.scale,
-   WESTON_MODE_SWITCH_SET_TEMPORARY) == 0) 
{
+   if (weston_output_mode_switch_to_temporary(output, 
&mode,
+   surface->buffer_viewport.buffer.scale) 
== 0) {
weston_view_set_position(shsurf->view,
 output->x - surf_x,
 output->y - surf_y);
diff --git a/fullscreen-shell/fullscreen-shell.c 
b/fullscreen-shell/fullscreen-shell.c
index c7950df..35e6d8f 100644
--- a/fullscreen-shell/fullscreen-shell.c
+++ b/fullscreen-shell/fullscreen-shell.c
@@ -284,12 +284,8 @@ fs_output_for_output(struct weston_output *output)
 static void
 restore_output_mode(struct weston_output *output)
 {
-   if (output->original_mode ||
-   (int32_t)output->current_scale != output->original_scale)
-   weston_output_switch_mode(output,
- output->native_mode,
- output->native_scale,
- WESTON_MODE_SWITCH_RESTORE_NATIVE);
+   if (output->original_mode)
+   weston_output_mode_switch_to_native(output);
 }
 
 /*
@@ -472,9 +468,8 @@ fs_output_configure_for_mode(struct fs_output *fsout,
mode.height = surf_height * fsout->output->native_scale;
mode.refresh = fsout->pending.framerate;
 
-   ret = weston_output_switch_mode(fsout->output, &mode,
-   fsout->output->native_scale,
-   WESTON_MODE_SWITCH_SET_TEMPORARY);
+   ret = weston_output_mode_switch_to_temporary(fsout->output, &mode,
+   fsout->output->native_scale);
 
if (ret != 0) {
/* The mode switch failed.  Clear the pending and
diff --git a/src/compositor-rdp.c b/src/compositor-rdp.c
index 9098396..2048f8f 100644
--- a/src/compositor-rdp.c
+++ b/src/compositor-rdp.c
@@ -803,7 +803,7 @@ xf_peer_post_connect(freerdp_peer* client)
weston_log("client mode not found\n");
return FALSE;
}
-   weston_output_switch_mode(&output->base, target_mode, 
1, WESTON_MODE_SWITCH_SET_NATIVE);
+   weston_output_mode_set_native(&output->base, 
target_mode, 1);
output->base.width = new_mode.width;
output->base.height = new_mode.height;
}
diff --git a/src/compositor.c b/src/compositor.c
index 4540911..82e0b6a 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -98,76 +98,13 @@ weston_output_transform_scale_init(struct weston_output 
*output,
 static void
 weston_compositor_build_view_list(struct weston_compositor *compositor);
 
-WL_EXPORT int
-weston_output_switch_mode(struct weston_output *output, struct weston_mode 
*mode,
-   int32_t scale, enum weston_mode_switch_op op)
+static void weston_mode_switch_f

Re: [PATCH weston] compositor: Fix weston_subsurface_is_synchronized() return value.

[Derek Foreman]

On 04/11/14 07:38 AM, Carlos Olmedo Escobar wrote:
> Commit 280e7dd918f1717c7d677676384a9cd991097741 introduced a bug in the
> return value of weston_subsurface_is_synchronized().

Ouch! Nice catch. :(

Reviewed-by: Derek Foreman 

> Signed-off-by: Carlos Olmedo Escobar 
> ---
>  src/compositor.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/compositor.c b/src/compositor.c
> index ac5bda2..ff62b5f 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -2639,7 +2639,7 @@ weston_subsurface_is_synchronized(struct 
> weston_subsurface *sub)
>   sub = weston_surface_to_subsurface(sub->parent);
>   }
>  
> - return true;
> + return false;
>  }
>  
>  static void
> 

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 0/4] some acceleration fixes, mostly for high DPI mice

[Derek Foreman]

On 03/11/14 09:51 PM, Peter Hutterer wrote:
> On Mon, Nov 03, 2014 at 11:56:59AM +0100, David Herrmann wrote:
>> Hi
>>
>> On Fri, Oct 31, 2014 at 5:33 AM, Peter Hutterer
>>  wrote:
>>> On Thu, Oct 30, 2014 at 04:34:12PM -0500, Derek Foreman wrote:
 The acceleration filter currently isn't particularly pleased with gaming 
 mice.

 They generally have high DPI (can be over 8000 DPI) and can have high 
 update
 rates (1000+ per second).  This can result in the accel curve being biased
 heavily towards the high points on the accel curve.

 This patch set allows a way to normalize the deltas to 400DPI so when the
 DPI setting of the mouse is correct, it should feel much the same as a
 normal mouse.

 This is, of course, only a partial solution.  Setting a reasonable default
 is a massive problem that needs to be addressed in the future - for now
 we just set it to 400 (which may actually not be that prevalent any more
 but there doesn't seem to be such thing as a "standard" DPI mouse).
>>>
>>> Thanks, I've pushed 1 to 3 (with added comments), not quite happy with
>>> 4 yet. As a configuration interface, it's mismatched with the usual quartett
>>> of hooks that we provide for all other config options.
>>>
>>> But I think providing a visible API may not be the right approach here
>>> anyway. One idea I had here was to have this provided through udev, set
>>> as something like:
>>> LIBINPUT_MOUSE_DPI="400@80" # dpi @ poll rate
>>>
>>> over time, the udev hwdb could add those settings, or we ship them as
>>> extra rules, or users configure them themselves. this is unfortunately a
>>> rather nasty task given the amount of HW out there, but I really can't think
>>> of any other way (short of heuristics which will fail in too many ways).
>>>
>>> Any ideas on feasability or better approaches welcome.
>>
>> hwdb can be used easily here, but it's indeed a huge amount to gather
>> all that data. HID doesn't have a way to query this information,
>> sadly, and I haven't found any generic vendor extensions (maybe
>> Benjamin knows more).
>>
>> We could use the REL_X/REL_Y MAX values to guess the DPI setting and
>> ignore it if it's out of the expected range (sth. like 100-10k).
>>
>> I haven't spent much time thinking it through, but so far I'd prefer a
>> solid, but basic, heuristic to guess the DPI and then use hwdb for
>> anything that doesn't fit. This allows us to change the heuristic at
>> any time and we don't have to introduce any APIs. We can even ship the
>> hwdb files with libinput.
> 
> tbh, I don't think heuristics will work. there is no reference point and
> when you're looking at relative motion you can't tell if a delta of
> 10 means the pointer has moved a large distance at low-res or a small
> distance at high-res.
>  
> Anyway, for some data:
> I recorded my Logitech G500s here with three different dpi settings (400,
> 800, 2000) and a MS Comfort Optical Mouse 3000 (1000 dpi). A couple of
> interesting things: regardless of dpi, virtually all events are within [-3,
> +3] dx/dy and there's nothing over 10. Exception here is the MS mouse which
> goes up to 78 for a delta, for reasons I can't explain yet.

Poll rate?  Your Logitech G500s is a Ridiculous Gaming Mouse(TM) with a
1000Hz report rate - I suspect the Microsoft mouse is 125Hz - have you
looked at the time deltas?

Unsurprisingly it looks like my results with a mouse set to 1800DPI,
1000Hz polling are very similar to your 2000DPI log with no events with
a delta higher than 11.  I haven't created a nice histogram though.

This mouse can also be set to 125Hz, where it generates proportionally
larger deltas.

We could probably figure out the poll rate heuristically, but I think
that's useless information since timestamps are already taken into
account in the acceleration filter.

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH v1] Added more error checks when strtol function is used

[Imran Zaman]

Signed-off-by: Imran Zaman 
---
 src/scanner.c| 2 +-
 src/wayland-client.c | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/src/scanner.c b/src/scanner.c
index 5e5152b..2ed9775 100644
--- a/src/scanner.c
+++ b/src/scanner.c
@@ -407,7 +407,7 @@ start_element(void *data, const char *element_name, const 
char **atts)
if (since != NULL) {
errno = 0;
version = strtol(since, &end, 0);
-   if (errno == EINVAL || end == since || *end != '\0')
+   if (errno != 0 || end == since || *end != '\0')
fail(&ctx->loc,
 "invalid integer (%s)\n", since);
} else {
diff --git a/src/wayland-client.c b/src/wayland-client.c
index b0f77b9..c99dbeb 100644
--- a/src/wayland-client.c
+++ b/src/wayland-client.c
@@ -829,8 +829,9 @@ wl_display_connect(const char *name)
 
connection = getenv("WAYLAND_SOCKET");
if (connection) {
+   errno = 0;
fd = strtol(connection, &end, 0);
-   if (*end != '\0')
+   if (errno != 0 || connection == end || *end != '\0')
return NULL;
 
flags = fcntl(fd, F_GETFD);
-- 
1.9.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v3] wayland-util: added wl_strtol/wl_strtoul utility functions

[Imran Zaman]

I will push new patch with minor fix to the strtol function in wayland and
move this old patch (after segfault fix) to weston so that it does not end
up in libwayland APIs.
Consequently I changed its property in patchwork

BR
imran

On Wed, Oct 29, 2014 at 10:27 AM, Imran Zaman  wrote:

>
>
> On Wed, Oct 29, 2014 at 10:09 AM, Giulio Camuffo 
> wrote:
>
>> 2014-10-29 8:45 GMT+02:00 Imran Zaman :
>> > Daniel!
>> >
>> > As per your logic, I see wl_list APIs exposed etc, which shouldn't be
>> part
>> > of libwayland as well.
>> > similarly, wl_fixed_to_double and wl_array shouldn't be part of the
>> window
>> > system. Isnt it?
>> > I can make inline functions if that helps.
>>
>> wl_list is used in the server side API, so it's a bit different.
>> However, I'd agree that it'd be better if it wasn't exposed but we
>> can't remove it now. wl_fixed is a wayland specific type so all the
>> wl_fixed_* functions need to be part of the API.
>> On the other hand wl_strtol would just be a function, there are is no
>> other API that depends on it.
>>
>> >
>> > Btw here is an API patch, which has not be reviewed till now.
>> >
>> http://lists.freedesktop.org/archives/wayland-devel/2014-October/017833.html
>>
>> Yes, like there are many other patches waiting for reviews. You need
>> to have patience, it's not like we are ignoring it. But, if I may add,
>> the way you are reacting to a comment to this patch doesn't encourage
>> to review your other ones.
>>
>>
>>
> Neither the random/comments to the patch are encouraging :-) e.g. "AOL.
> We're a window system, not a replacement libc."
> Its your choice to review it or not; I did not put the API patch link here
> just because it has not been reviewed. I have lots of patience but Tizen
> may need something urgent or else we may need to fork wayland/weston in
> Tizen. I put it in the thread because Daniel said that we did not have any
> further progress/discussion on that end.
>
> Anyways I take the patch off, as it does not "sound" like it should be
> here to save everyone's time. If the time allows, I will remove it from
> public APIs in addition to one critical bug fix and resubmit the patch.
>
>
>
>> --
>> Giulio
>>
>> >
>> > BR
>> > imran
>> >
>> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone 
>> wrote:
>> >
>> >> Hi,
>> >>
>> >> On 28 October 2014 15:40, Imran Zaman  wrote:
>> >>>
>> >>> You guys should check the reason why the patch is there rather than
>> >>> throwing out random thoughts or blunt comments.
>> >>>
>> >>> I did this patch mainly because weston/wayland has been using
>> >>> strtol/strtoul functions in number of places with buggy error checks,
>> and
>> >>> duplicate code everywhere. Weston and wayland go together; so in
>> bigger
>> >>> picture, its a very useful patch IMO.. I hardly find any patches with
>> proper
>> >>> tests, but I did it so to make it more effective. But I guess in
>> >>> wayland/weston community, only maintainers are allowed to push patches
>> >>> others are strongly discouraged to do so. I guess its better to
>> encourage
>> >>> people/community for giving helping hand.
>> >>>
>> >>> Anyways we will now only push patches (including multi-seat support)
>> in
>> >>> Tizen weston/wayland rather than wasting time in upstreamn
>> weston/wayland as
>> >>> it seems to be long bureaucratic process to overcome with virtually no
>> >>> success.
>> >>
>> >>
>> >> That's not what we've said. No-one said 'don't take the patch'; all we
>> >> said is 'please don't expose it as part of libwayland-*'s _public_
>> API'.
>> >>
>> >> I like the idea of the patch, I like how it's caught a number of buggy
>> >> spots where we've open-coded the same thing, and I like that it's
>> >> well-tested. For internal usage, it's great! I just don't want to
>> expose
>> >> string manipulation functions in the public API of a window system.
>> >>
>> >> You're right that the test infrastructure is in a pretty bad state.
>> >> Anything which helps that is more than welcome, and you may have seen a
>> >> bunch of patches from Derek Foreman (not a maintainer) on this list,
>> which
>> >> are progressing well and go a long way towards improving our test
>> suite into
>> >> something that will be really useful day to day. Any further
>> contributions
>> >> along those lines - from anyone - are totally welcome.
>> >>
>> >> As for your multiseat patches, the last discussions I remember involved
>> >> some pretty fundamental disagreements about the whole architecture,
>> >> particularly input support. I haven't seen any more patches or
>> discussion
>> >> since the last IRC chat, though.
>> >>
>> >> Cheers,
>> >> Daniel
>> >>
>> >
>> > On Tue, Oct 28, 2014 at 6:36 PM, Daniel Stone 
>> wrote:
>> >>
>> >> Hi,
>> >>
>> >> On 28 October 2014 15:40, Imran Zaman  wrote:
>> >>>
>> >>> You guys should check the reason why the patch is there rather than
>> >>> throwing out random thoughts or blunt comments.
>> >>>
>> >>> I did this patch mainly because weston/wayland has been using

Re: Wayland and Weston in Patchwork

[Imran Zaman]

Thats really good initiative Pekka/Daniel. Thanks.

BR
imran

On Tue, Nov 4, 2014 at 1:14 PM, Pekka Paalanen  wrote:

> Hi all,
>
> some of you already know that we now have
> http://patchwork.freedesktop.org/project/wayland/list/
> to keep track of the patches sent to wayland-devel mailing list.
>
> Thanks to Daniel for setting that up. :-)
>
> We initialized the patch list with my review backlog. If you are
> waiting for review on a patch, that is not for libinput, and you do not
> see it listed in Patchwork at all (try disabling the filters too),
> rebase and re-send it, please.
>
>
> For contributors:
>
> We have a filter, that prevents libinput patches from appearing on
> patchwork, because libinput developers did not feel the need for it. I
> seem to recall the filter works by looking at the email subject if it
> contains "libinput" in the [subject-prefix].
>
> Otherwise, all wayland, weston, and wayland-web patches should appear
> on that list when they hit the mailing list. If you find that a patch
> does not soon appear there while it does appear on the mailing list
> archives[1], please send an email to me and Daniel (cc'd) because
> something is wrong.
>
> When you send patches to wayland-devel@, you can register your email
> address in Pathwork, which allows you to manage the state of your own
> patches. If you re-send or send revised versions of your patches, I
> would hope you also take the time to mark your old patches in Patchwork
> as "superseded" as appropriate.
>
> You change the patch status by clicking the patch link in the list, and
> on the following page you should have box titled "Patch Properties"
> near the top. If you don't see that, you don't have permissions to
> change it (e.g. not logged in, or unregistered email address).
>
> If you want to become a Patchwork maintainer, i.e. have the rights to
> change also other people's patch states, ask me or Daniel.
>
> The old announcement of Patchwork for Mesa contains more information:
> http://lists.freedesktop.org/archives/mesa-dev/2013-November/049293.html
> Just replace the "mesa" project name with "wayland" where needed.
>
> It also includes a command line client for Patchwork.
>
>
> For Wayland/Weston maintainers:
>
> There is a git hook in place on the upstream repositories of Wayland
> and Weston, that will automatically mark a patch in Patchwork as
> "Accepted" if you push it to master unmodified. If you edit anything
> except the commit message, the automatic update will likely fail and
> you need to update the status manually.
>
> When you do a push with successful automatic update, you see something
> like:
>
> $ git push origin master
> Counting objects: 8, done.
> Delta compression using up to 4 threads.
> Compressing objects: 100% (8/8), done.
> Writing objects: 100% (8/8), 1.46 KiB | 0 bytes/s, done.
> Total 8 (delta 7), reused 0 (delta 0)
> remote: Updating patchwork state for
> http://patchwork.freedesktop.org/project/wayland/list/
> remote: I: patch #34560 updated using rev
> 391820b0d6d9fcd99e12cd32623a476da64c89ce
> remote: I: 1 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/wayland/wayland
>4a661c5..391820b  master -> master
>
> When it fails, it will say so.
>
>
> Shortcomings:
>
> There are several features we would like to see in Patchwork but AFAIK
> are not there (yet?). Patchwork does not recognize re-submissions so
> that it could automatically set a patch as "Superseded". It does not
> maintain patch sets. You can create "Bundles", but so far those are
> just named collections of individual patches, and you need to create
> them manually.
>
> If you want to see only Wayland patches, or only Weston patches, etc.,
> you need to set up a filter based on the patch subject, that relies on
> everyone using the subject-prefix properly. Subject-prefix is not
> always set like this, so the filtered lists will miss something. You
> can configure the filters for the list in the blue header box, clicking
> "Filters".
>
> We are just starting to use Patchwork here, so we are still learning
> the process. Even with its shortcomings, I believe Patchwork is an
> improvement to the previous situation. At least the review backlog is
> now public, and if we keep the list in Patchwork maintained, patches
> will not get lost.
>
> Yeah, we also broke the commit announcements through the irc bot while
> getting this up, will see about fixing that.
>
>
> Thanks,
> pq
>
> [1] http://lists.freedesktop.org/archives/wayland-devel/
> ___
> wayland-devel mailing list
> wayland-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston] compositor: Fix weston_subsurface_is_synchronized() return value.

[Carlos Olmedo Escobar]

Commit 280e7dd918f1717c7d677676384a9cd991097741 introduced a bug in the
return value of weston_subsurface_is_synchronized().

Signed-off-by: Carlos Olmedo Escobar 
---
 src/compositor.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/compositor.c b/src/compositor.c
index ac5bda2..ff62b5f 100644
--- a/src/compositor.c
+++ b/src/compositor.c
@@ -2639,7 +2639,7 @@ weston_subsurface_is_synchronized(struct 
weston_subsurface *sub)
sub = weston_surface_to_subsurface(sub->parent);
}
 
-   return true;
+   return false;
 }
 
 static void
-- 
2.1.1

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v6 2/2] window : compare version and call appropriate destructor

[Pekka Paalanen]

On Mon, 20 Oct 2014 11:55:29 +0530
kabeer.k...@samsung.com wrote:

> From: kabeer khan 
> 
> Signed-off-by: kabeer khan 
> ---
>  clients/window.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/clients/window.c b/clients/window.c
> index 139c7f9..c8ed9a2 100644
> --- a/clients/window.c
> +++ b/clients/window.c
> @@ -133,6 +133,7 @@ struct display {
>  
>   int has_rgb565;
>   int seat_version;
> + int data_device_manager_version;
>  };
>  
>  struct window_output {
> @@ -5148,9 +5149,12 @@ input_destroy(struct input *input)
>   if (input->selection_offer)
>   data_offer_destroy(input->selection_offer);
>  
> - if (input->data_device)
> - wl_data_device_destroy(input->data_device);
> -
> + if (input->data_device) {
> + if(input->display->data_device_manager_version >= 2)
> + wl_data_device_release(input->data_device);
> + else
> + wl_data_device_destroy(input->data_device);
> + }
>   if (input->display->seat_version >= 3) {
>   if (input->pointer)
>   wl_pointer_release(input->pointer);
> @@ -5234,9 +5238,10 @@ registry_handle_global(void *data, struct wl_registry 
> *registry, uint32_t id,
>   d->shm = wl_registry_bind(registry, id, &wl_shm_interface, 1);
>   wl_shm_add_listener(d->shm, &shm_listener, d);
>   } else if (strcmp(interface, "wl_data_device_manager") == 0) {
> + d->data_device_manager_version = MIN(version, 2);
>   d->data_device_manager =
> - wl_registry_bind(registry, id,
> -  &wl_data_device_manager_interface, 1);
> + wl_registry_bind(registry, id, 
> +  &wl_data_device_manager_interface, 
> d->data_device_manager_version);

I removed a trailing space and split the long line.

>   } else if (strcmp(interface, "xdg_shell") == 0) {
>   d->xdg_shell = wl_registry_bind(registry, id,
>   &xdg_shell_interface, 1);

Pushed with the above changes.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH v5 1/3] Protocol : Added destructor to wl_data_device interface

[Pekka Paalanen]

On Mon, 13 Oct 2014 10:34:26 +0530
kabeer khan  wrote:

> Fix for Bug# 81745
> 
> Signed-off-by: kabeer khan 
> ---
>  protocol/wayland.xml | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/protocol/wayland.xml b/protocol/wayland.xml
> index 3645208..a129de2 100644
> --- a/protocol/wayland.xml
> +++ b/protocol/wayland.xml
> @@ -517,7 +517,7 @@
>  
>
>  
> -  
> +  
>  
>There is one wl_data_device per seat which can be obtained
>from the global wl_data_device_manager singleton.
> @@ -641,9 +641,17 @@
>
> allow-null="true"/>
>  
> +
> +
> +

There are two lines above that introduce trailing whitespace. I removed
those when applying the patch.

> +
> +  
> + This request destroys the data device
> +  
> +
>
>  
> -  
> +  
>  
>The wl_data_device_manager is a singleton global object that
>provides access to inter-client data transfer mechanisms such as

Pushed.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v6 1/2] data_device : change version while initializing data_device_manager interface and data_device interface

[Pekka Paalanen]

On Mon, 20 Oct 2014 11:47:15 +0530
kabeer.k...@samsung.com wrote:

> From: kabeer khan 
> 
> Signed-off-by: kabeer khan 
> ---
>  src/data-device.c | 15 ++-
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/src/data-device.c b/src/data-device.c
> index 75fc60c..a78ba83 100644
> --- a/src/data-device.c
> +++ b/src/data-device.c
> @@ -761,10 +761,16 @@ data_device_set_selection(struct wl_client *client,
> wl_resource_get_user_data(source_resource),
> serial);
>  }
> +static void
> +data_device_release(struct wl_client *client, struct wl_resource *resource)
> +{
> + wl_resource_destroy(resource);
> +}
>  
>  static const struct wl_data_device_interface data_device_interface = {
>   data_device_start_drag,
>   data_device_set_selection,
> + data_device_release
>  };
>  
>  static void
> @@ -844,7 +850,7 @@ get_data_device(struct wl_client *client,
>   struct wl_resource *resource;
>  
>   resource = wl_resource_create(client,
> -   &wl_data_device_interface, 1, id);
> +   &wl_data_device_interface, 
> wl_resource_get_version(manager_resource), id);

I split the long line.

>   if (resource == NULL) {
>   wl_resource_post_no_memory(manager_resource);
>   return;
> @@ -867,9 +873,8 @@ bind_manager(struct wl_client *client,
>  {
>   struct wl_resource *resource;
>  
> - resource =
> - wl_resource_create(client,
> -&wl_data_device_manager_interface, 1, id);
> + resource = wl_resource_create(client,
> +   &wl_data_device_manager_interface, 
> MIN(version,2), id);

I split the long line, and dropped the MIN, as per rationale in my
earlier reply:
http://lists.freedesktop.org/archives/wayland-devel/2014-November/018023.html

>   if (resource == NULL) {
>   wl_client_post_no_memory(client);
>   return;
> @@ -909,7 +914,7 @@ WL_EXPORT int
>  wl_data_device_manager_init(struct wl_display *display)
>  {
>   if (wl_global_create(display,
> -  &wl_data_device_manager_interface, 1,
> +  &wl_data_device_manager_interface, 2,
>NULL, bind_manager) == NULL)
>   return -1;
>  

Pushed with the mentioned changes.


Thanks,
pq
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston v4] Implement data_device interface destructor

[Pekka Paalanen]

On Fri, 10 Oct 2014 20:57:30 +0300
Giulio Camuffo  wrote:

> A could of comments below, the rest looks good to me.
> 
> 
> 2014-09-23 8:21 GMT+03:00 kabeer khan :
> > window : compare version and call appropriate destructor
> > data-device : change version of creation of data_device_manager and 
> > data_device interfaces
> 
> Maybe it would be better to split these two changes in separate commits.
> 
> >
> > Signed-off-by: kabeer khan 
> > ---
> >  clients/window.c  | 15 ++-
> >  src/data-device.c | 15 ++-
> >  2 files changed, 20 insertions(+), 10 deletions(-)

...

> > @@ -867,9 +873,8 @@ bind_manager(struct wl_client *client,
> >  {
> > struct wl_resource *resource;
> >
> > -   resource =
> > -   wl_resource_create(client,
> > -  &wl_data_device_manager_interface, 1, 
> > id);
> > +   resource = wl_resource_create(client,
> > + &wl_data_device_manager_interface, 
> > version, id);
> 
> If i understand it correctly, the version used here should be
> MIN(version, 2), since the value of 'version' here is just the one the
> client passes to wl_registry.bind.

No, it's the client side code that uses MIN(server_supported,
my_supported).

This is server side code, which either uses the client supplied
version, or raises a protocol error. And I think the code in
libwayland-server already raises the protocol error in registry_bind()
if the client supplied version is not supported by the server.

Server would use MIN(my_supported, libwayland_supported) when creating
the global which is at the root of this protocol object ancestry.


Thanks,
pq


> 
> > if (resource == NULL) {
> > wl_client_post_no_memory(client);
> > return;
> > @@ -909,7 +914,7 @@ WL_EXPORT int
> >  wl_data_device_manager_init(struct wl_display *display)
> >  {
> > if (wl_global_create(display,
> > -&wl_data_device_manager_interface, 1,
> > +&wl_data_device_manager_interface, 2,
> >  NULL, bind_manager) == NULL)
> > return -1;
> >
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Wayland and Weston in Patchwork

[Pekka Paalanen]

Hi all,

some of you already know that we now have
http://patchwork.freedesktop.org/project/wayland/list/
to keep track of the patches sent to wayland-devel mailing list.

Thanks to Daniel for setting that up. :-)

We initialized the patch list with my review backlog. If you are
waiting for review on a patch, that is not for libinput, and you do not
see it listed in Patchwork at all (try disabling the filters too),
rebase and re-send it, please.


For contributors:

We have a filter, that prevents libinput patches from appearing on
patchwork, because libinput developers did not feel the need for it. I
seem to recall the filter works by looking at the email subject if it
contains "libinput" in the [subject-prefix].

Otherwise, all wayland, weston, and wayland-web patches should appear
on that list when they hit the mailing list. If you find that a patch
does not soon appear there while it does appear on the mailing list
archives[1], please send an email to me and Daniel (cc'd) because
something is wrong.

When you send patches to wayland-devel@, you can register your email
address in Pathwork, which allows you to manage the state of your own
patches. If you re-send or send revised versions of your patches, I
would hope you also take the time to mark your old patches in Patchwork
as "superseded" as appropriate.

You change the patch status by clicking the patch link in the list, and
on the following page you should have box titled "Patch Properties"
near the top. If you don't see that, you don't have permissions to
change it (e.g. not logged in, or unregistered email address).

If you want to become a Patchwork maintainer, i.e. have the rights to
change also other people's patch states, ask me or Daniel.

The old announcement of Patchwork for Mesa contains more information:
http://lists.freedesktop.org/archives/mesa-dev/2013-November/049293.html
Just replace the "mesa" project name with "wayland" where needed.

It also includes a command line client for Patchwork.


For Wayland/Weston maintainers:

There is a git hook in place on the upstream repositories of Wayland
and Weston, that will automatically mark a patch in Patchwork as
"Accepted" if you push it to master unmodified. If you edit anything
except the commit message, the automatic update will likely fail and
you need to update the status manually.

When you do a push with successful automatic update, you see something
like:

$ git push origin master
Counting objects: 8, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (8/8), done.
Writing objects: 100% (8/8), 1.46 KiB | 0 bytes/s, done.
Total 8 (delta 7), reused 0 (delta 0)
remote: Updating patchwork state for 
http://patchwork.freedesktop.org/project/wayland/list/
remote: I: patch #34560 updated using rev 
391820b0d6d9fcd99e12cd32623a476da64c89ce
remote: I: 1 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/wayland/wayland
   4a661c5..391820b  master -> master

When it fails, it will say so.


Shortcomings:

There are several features we would like to see in Patchwork but AFAIK
are not there (yet?). Patchwork does not recognize re-submissions so
that it could automatically set a patch as "Superseded". It does not
maintain patch sets. You can create "Bundles", but so far those are
just named collections of individual patches, and you need to create
them manually.

If you want to see only Wayland patches, or only Weston patches, etc.,
you need to set up a filter based on the patch subject, that relies on
everyone using the subject-prefix properly. Subject-prefix is not
always set like this, so the filtered lists will miss something. You
can configure the filters for the list in the blue header box, clicking
"Filters".

We are just starting to use Patchwork here, so we are still learning
the process. Even with its shortcomings, I believe Patchwork is an
improvement to the previous situation. At least the review backlog is
now public, and if we keep the list in Patchwork maintained, patches
will not get lost.

Yeah, we also broke the commit announcements through the irc bot while
getting this up, will see about fixing that.


Thanks,
pq

[1] http://lists.freedesktop.org/archives/wayland-devel/
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH wayland] cosmetic: convert some function returns from int to bool

[Pekka Paalanen]

On Fri,  3 Oct 2014 14:39:59 -0500
Derek Foreman  wrote:

> ---
>  src/scanner.c | 5 +++--
>  src/wayland-shm.c | 9 +
>  2 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/src/scanner.c b/src/scanner.c
> index 809130b..dc113f5 100644
> --- a/src/scanner.c
> +++ b/src/scanner.c
> @@ -21,6 +21,7 @@
>   * OF THIS SOFTWARE.
>   */
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -297,9 +298,9 @@ is_nullable_type(struct arg *arg)

I amended: changed the return type here to 'bool'. :-)


Pushed, thanks,
pq

>   case OBJECT:
>   case NEW_ID:
>   case ARRAY:
> - return 1;
> + return true;
>   default:
> - return 0;
> + return false;
>   }
>  }
>  
> diff --git a/src/wayland-shm.c b/src/wayland-shm.c
> index 04ba4f2..b6b31d6 100644
> --- a/src/wayland-shm.c
> +++ b/src/wayland-shm.c
> @@ -27,6 +27,7 @@
>  
>  #define _GNU_SOURCE
>  
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -99,7 +100,7 @@ static const struct wl_buffer_interface 
> shm_buffer_interface = {
>   shm_buffer_destroy
>  };
>  
> -static int
> +static bool
>  format_is_supported(struct wl_client *client, uint32_t format)
>  {
>   struct wl_display *display = wl_client_get_display(client);
> @@ -109,15 +110,15 @@ format_is_supported(struct wl_client *client, uint32_t 
> format)
>   switch (format) {
>   case WL_SHM_FORMAT_ARGB:
>   case WL_SHM_FORMAT_XRGB:
> - return 1;
> + return true;
>   default:
>   formats = wl_display_get_additional_shm_formats(display);
>   wl_array_for_each(p, formats)
>   if(*p == format)
> - return 1;
> + return true;
>   }
>  
> - return 0;
> + return false;
>  }
>  
>  static void

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH weston] cosmetic: replace boolean function return values with bool

[Pekka Paalanen]

On Mon, 13 Oct 2014 11:41:32 -0700
Bryce Harrington  wrote:

> On Fri, Oct 03, 2014 at 01:13:42PM -0500, Derek Foreman wrote:
> > For functions that test if something is true/valid and return a 1
> > or 0, it makes sense to switch to bool.
> 
> I like it.  Probably many other places this shows up.
> 
> Only question I have is if bool can be used for WL_EXPORT functions?
> bool isn't mentioned in the wire format docs[0], so I'm guessing that
> might not be an allowed type for those routines?

This is a Weston patch and I don't see anything suspicious in using
stdbool.h, so it's ok here.

I kind of wonder what including stdbool.h does to C++ sources using
compositor.h, but if that breaks, we'll hear about it.

Looks good to me, so pushed!


Thanks,
pq

> 
> Assuming that's fine,
> 
> Reviewed-by: Bryce Harrington 
> 
> 0: http://wayland.freedesktop.org/docs/html/sect-Protocol-Wire-Format.html
> 
> > ---
> >  src/cms-colord.c| 10 +-
> >  src/compositor.c| 20 ++--
> >  src/compositor.h|  5 +++--
> >  src/input.c |  8 
> >  src/weston-launch.c | 11 ++-
> >  5 files changed, 28 insertions(+), 26 deletions(-)
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] connection: Leave fd open in wl_connection_destroy

[Pekka Paalanen]

On Mon, 27 Oct 2014 09:55:46 +0100
Marek Chalupa  wrote:

> Hi,
> 
> at first glance I didn't like returning fd from wl_connection_destroy, but
> at the other, I did!
> If you think about the connection as a buffer for the fd (and that is
> really the case),
> then it make sense to do something like:
> 
>   create conn  -- destroy conn
> fd --> | connection | > fd
>  --
> 
> The race in threaded environment is real (one thread closes fd, say 6,
> other thread opens new fd
> - it will get the first free value which is 6 - and the first thread close
> the 6 again and we're in trouble..)
> So it's important to close every fd only once.
> 
> For me it's OK and:
> 
> Reviewed-by: Marek Chalupa 
> 
> On 30 September 2014 14:43, Benjamin Herr  wrote:
> 
> > Calling close() on the same file descriptor that a previous call to
> > close() already closed is wrong, and racy if another thread received
> > that same file descriptor as a eg. new socket or actual file.
> >
> > There are two situations where wl_connection_destroy() would close its
> > file descriptor and then another function up in the call chain would
> > close the same file descriptor:
> >
> >   * When wl_client_create() fails after calling wl_connection_create(),
> > it will call wl_connection_destroy() before returning. However, its
> > caller will always close the file descriptor if wl_client_create()
> > fails.
> >
> >   * wl_display_disconnect() unconditionally closes the display file
> > descriptor and also calls wl_connection_destroy().
> >
> > So these two seem to expect wl_connection_destroy() to leave the file
> > descriptor open. The other caller of wl_connection_destroy(),
> > wl_client_destroy(), does however expect wl_connection_destroy() to
> > close its file descriptor, alas.
> >
> > This patch changes wl_connection_destroy() to indulge this majority of
> > two callers by simply not closing the file descriptor. For the benefit
> > of wl_client_destroy(), wl_connection_destroy() then returns the
> > unclosed file descriptor so that wl_client_destroy() can close it
> > itself.
> >
> > Since wl_connection_destroy() is a private function called from few
> > places, changing its semantics seemed like the more expedient way to
> > address the double-close() problem than shuffling around the logic in
> > wl_client_create() to somehow enable it to always avoid calling
> > wl_connection_destroy().
> >
> > Signed-off-by: Benjamin Herr 

Hi,

yeah, I see.

It is not explicitly documented, but it seems that wl_client_create()
takes the ownership of the fd only if it succeeds. That is how it is
being used. Also, wl_client_create() did not close the fd on some error
paths, but did on the others. This patch fixes wl_client_create() to
never close the fd on error.

We cannot comfortably make wl_connection_create() take the fd ownership
when it succeeds, because wl_client_create() can fail other ways later,
which means it would not be able to untake the fd. Or well, maybe we
could, but it doesn't really matter, since this is all internal. Or we
could use dup() which is perhaps what a good public API should do, but
that's overkill here, and wl_client_create does not dup anyway.

Looks good, pushed!


Thanks,
pq

> > ---
> >  src/connection.c| 7 +--
> >  src/wayland-private.h   | 2 +-
> >  src/wayland-server.c| 2 +-
> >  tests/connection-test.c | 8 ++--
> >  4 files changed, 13 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/connection.c b/src/connection.c
> > index f292853..c5daca6 100644
> > --- a/src/connection.c
> > +++ b/src/connection.c
> > @@ -189,13 +189,16 @@ close_fds(struct wl_buffer *buffer, int max)
> > buffer->tail += size;
> >  }
> >
> > -void
> > +int
> >  wl_connection_destroy(struct wl_connection *connection)
> >  {
> > +   int fd = connection->fd;
> > +
> > close_fds(&connection->fds_out, -1);
> > close_fds(&connection->fds_in, -1);
> > -   close(connection->fd);
> > free(connection);
> > +
> > +   return fd;
> >  }
> >
> >  void
> > diff --git a/src/wayland-private.h b/src/wayland-private.h
> > index 67e8783..db76081 100644
> > --- a/src/wayland-private.h
> > +++ b/src/wayland-private.h
> > @@ -86,7 +86,7 @@ int wl_interface_equal(const struct wl_interface *iface1,
> >const struct wl_interface *iface2);
> >
> >  struct wl_connection *wl_connection_create(int fd);
> > -void wl_connection_destroy(struct wl_connection *connection);
> > +int wl_connection_destroy(struct wl_connection *connection);
> >  void wl_connection_copy(struct wl_connection *connection, void *data,
> > size_t size);
> >  void wl_connection_consume(struct wl_connection *connection, size_t size);
> >
> > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > index 674aeca..7caeb30 100644
> > --- a/src/wayland-server.c
> > +++ b/src/wayland-se

Re: [PATCH libinput v2] evdev: Log evdev event queue overflows

[David Herrmann]

Hi

On Tue, Nov 4, 2014 at 12:17 AM, Peter Hutterer
 wrote:
> On Mon, Nov 03, 2014 at 11:49:12AM +0100, David Herrmann wrote:
>> Hi
>>
>> On Wed, Oct 29, 2014 at 3:56 PM, Derek Foreman  
>> wrote:
>> > Log a message when the kernel event queue overflows and events are dropped.
>> > After 10 messages logging stops to avoid flooding the logs if the condition
>> > is persistent.
>> > ---
>> >  src/evdev.c | 11 +++
>> >  src/evdev.h |  4 
>> >  2 files changed, 15 insertions(+)
>> >
>> > diff --git a/src/evdev.c b/src/evdev.c
>> > index 1b4ce10..9026f5c 100644
>> > --- a/src/evdev.c
>> > +++ b/src/evdev.c
>> > @@ -924,6 +924,17 @@ evdev_device_dispatch(void *data)
>> > rc = libevdev_next_event(device->evdev,
>> >  LIBEVDEV_READ_FLAG_NORMAL, &ev);
>> > if (rc == LIBEVDEV_READ_STATUS_SYNC) {
>> > +   if (device->syn_drops_received < 10) {
>> > +   device->syn_drops_received++;
>> > +   log_info(libinput, "SYN_DROPPED event from 
>> > "
>> > +"\"%s\" - some input events have "
>> > +"been lost.\n", device->devname);
>> > +   if (device->syn_drops_received == 10)
>> > +   log_info(libinput, "No longer 
>> > logging "
>> > +"SYN_DROPPED events for "
>> > +"\"%s\"\n", 
>> > device->devname);
>> > +   }
>> > +
>>
>> I really appreciate those log-messages, but can we use rate-limiting
>> here, rather than a per-device counter? I mean, my compositor usually
>> runs for multiple days, or even weeks. I really don't want it to stop
>> reporting SYN_DROPPED events just because it got 100 of those during
>> startup (or something similar).
>>
>> Something like this:
>> http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/lib/ratelimit.c
>>
>> I can prep a patch if you want.
>
> good idea, let's do that.

I sent 2 patches to wayland-devel.

Thanks
David
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 0/4] some acceleration fixes, mostly for high DPI mice

[David Herrmann]

Hi

On Tue, Nov 4, 2014 at 4:51 AM, Peter Hutterer  wrote:
> On Mon, Nov 03, 2014 at 11:56:59AM +0100, David Herrmann wrote:
>> I haven't spent much time thinking it through, but so far I'd prefer a
>> solid, but basic, heuristic to guess the DPI and then use hwdb for
>> anything that doesn't fit. This allows us to change the heuristic at
>> any time and we don't have to introduce any APIs. We can even ship the
>> hwdb files with libinput.
>
> tbh, I don't think heuristics will work. there is no reference point and
> when you're looking at relative motion you can't tell if a delta of
> 10 means the pointer has moved a large distance at low-res or a small
> distance at high-res.

I was more talking about the MIN/MAX values reported by the device. I
assumed that high-dpi devices just reported higher precision for the
same interval as low-dpi devices. But as your data shows, this turns
out to be not true... what a bummer!

> Anyway, for some data:
> I recorded my Logitech G500s here with three different dpi settings (400,
> 800, 2000) and a MS Comfort Optical Mouse 3000 (1000 dpi). A couple of
> interesting things: regardless of dpi, virtually all events are within [-3,
> +3] dx/dy and there's nothing over 10. Exception here is the MS mouse which
> goes up to 78 for a delta, for reasons I can't explain yet.

Interesting. This is probably to throttle HID throughput and use
report-on-change, instead of continuous-reporting.

Anyway, if you want, I can come up with a hwdb format to retrieve DPI
settings. Just lemme know what you want to match on for the devices.

Thanks
David
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput 2/2] evdev: ratelimit SYN_DROPPED logging

[David Herrmann]

Use the ratelimit helpers for SYN_DROPPED logging. This guarantees that we
will still receive SYN_DROPPED log-messages after multiple days of
runtime, even though there might have been a SYN_DROPPED flood at one
point in time.

Signed-off-by: David Herrmann 
---
 src/evdev.c | 14 +++---
 src/evdev.h |  4 +---
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/src/evdev.c b/src/evdev.c
index 3aa87a7..836ce56 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -926,16 +926,14 @@ evdev_device_dispatch(void *data)
rc = libevdev_next_event(device->evdev,
 LIBEVDEV_READ_FLAG_NORMAL, &ev);
if (rc == LIBEVDEV_READ_STATUS_SYNC) {
-   if (device->syn_drops_received < 10) {
-   device->syn_drops_received++;
+   if (ratelimit_test(&device->syn_drop_limit))
log_info(libinput, "SYN_DROPPED event from "
 "\"%s\" - some input events have "
 "been lost.\n", device->devname);
-   if (device->syn_drops_received == 10)
-   log_info(libinput, "No longer logging "
-"SYN_DROPPED events for "
-"\"%s\"\n", device->devname);
-   }
+   else if (ratelimit_cutoff(&device->syn_drop_limit))
+   log_info(libinput, "SYN_DROPPED flood "
+"for \"%s\"\n",
+device->devname);
 
/* send one more sync event so we handle all
   currently pending events before we sync up
@@ -1296,6 +1294,8 @@ evdev_device_create(struct libinput_seat *seat,
device->scroll.threshold = 5.0; /* Default may be overridden */
device->scroll.direction = 0;
device->dpi = DEFAULT_MOUSE_DPI;
+   /* at most 5 SYN_DROPPED log-messages per 30s */
+   device->syn_drop_limit = RATELIMIT_INIT(30ULL * 1000 * 1000, 5);
 
matrix_init_identity(&device->abs.calibration);
matrix_init_identity(&device->abs.usermatrix);
diff --git a/src/evdev.h b/src/evdev.h
index 666c8dc..eefbb79 100644
--- a/src/evdev.h
+++ b/src/evdev.h
@@ -137,9 +137,7 @@ struct evdev_device {
} buttons;
 
int dpi; /* HW resolution */
-   /* The number of times libevdev processes a SYN_DROPPED, so we can
-* stop logging them to avoid flooding the logs. */
-   int syn_drops_received;
+   struct ratelimit syn_drop_limit; /* ratelimit for SYN_DROPPED logging */
 };
 
 #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
-- 
2.1.3

___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH libinput 1/2] util: introduce ratelimit helpers

[David Herrmann]

This adds "struct ratelimit" and "ratelimit_test()". It's a very simple
rate-limit helper modeled after Linux' lib/ratelimit.c by Dave Young.

This comes in handy to limit log-messages in possible busy loops etc..

Signed-off-by: David Herrmann 
---
 src/libinput-util.c | 48 
 src/libinput-util.h | 19 +++
 test/misc.c | 37 +
 3 files changed, 104 insertions(+)

diff --git a/src/libinput-util.c b/src/libinput-util.c
index eeb9786..19594e3 100644
--- a/src/libinput-util.c
+++ b/src/libinput-util.c
@@ -65,3 +65,51 @@ list_empty(const struct list *list)
 {
return list->next == list;
 }
+
+/*
+ * Perform rate-limit test. Returns true if the rate-limited action is still
+ * allowed, false if it should be suppressed.
+ *
+ * The ratelimit object must be initialized via RATELIMIT_INIT().
+ *
+ * Modelled after Linux' lib/ratelimit.c by Dave Young
+ * , which is licensed GPLv2.
+ */
+bool ratelimit_test(struct ratelimit *r)
+{
+   struct timespec ts;
+   uint64_t utime;
+
+   if (r->interval <= 0 || r->burst <= 0)
+   return true;
+
+   clock_gettime(CLOCK_MONOTONIC, &ts);
+   utime = ts.tv_sec * 1000 * 1000 + ts.tv_nsec / 1000;
+
+   if (r->begin <= 0 || r->begin + r->interval < utime) {
+   /* reset counter */
+   r->begin = utime;
+   r->num = 1;
+   return true;
+   } else if (r->num < r->burst) {
+   /* continue burst */
+   r->num++;
+   return true;
+   }
+
+   /* rate-limit with overflow check */
+   if (r->num + 1 > r->num)
+   ++r->num;
+
+   return false;
+}
+
+/*
+ * Return true if the ratelimit counter just crossed the cutoff value. That is,
+ * this function returns true iff the last call to ratelimit_test() was the
+ * first call to exceed the burst value in this interval.
+ */
+bool ratelimit_cutoff(struct ratelimit *r)
+{
+   return r->num == r->burst + 1;
+}
diff --git a/src/libinput-util.h b/src/libinput-util.h
index 51759e8..8ff8778 100644
--- a/src/libinput-util.h
+++ b/src/libinput-util.h
@@ -25,6 +25,7 @@
 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -280,4 +281,22 @@ matrix_to_farray6(const struct matrix *m, float out[6])
out[5] = m->val[1][2];
 }
 
+struct ratelimit {
+   uint64_t interval;
+   uint64_t begin;
+   unsigned burst;
+   unsigned num;
+} RateLimit;
+
+#define RATELIMIT_INIT(_interval, _burst)  \
+   ((struct ratelimit){\
+   .interval = (_interval),\
+   .begin = 0, \
+   .burst = (_burst),  \
+   .num = 0,   \
+   })
+
+bool ratelimit_test(struct ratelimit *r);
+bool ratelimit_cutoff(struct ratelimit *r);
+
 #endif /* LIBINPUT_UTIL_H */
diff --git a/test/misc.c b/test/misc.c
index 1512180..70b3e57 100644
--- a/test/misc.c
+++ b/test/misc.c
@@ -508,6 +508,42 @@ START_TEST(matrix_helpers)
 }
 END_TEST
 
+START_TEST(ratelimit_helpers)
+{
+   /* 10 attempts every 10ms */
+   struct ratelimit rl = RATELIMIT_INIT(1, 10);
+   unsigned int i, j;
+
+   for (j = 0; j < 100; ++j) {
+   /* a burst of 10 attempts must succeed */
+   for (i = 0; i < 10; ++i) {
+   ck_assert(ratelimit_test(&rl));
+   ck_assert(!ratelimit_cutoff(&rl));
+   }
+
+   /* ..then further attempts must fail.. */
+   ck_assert(!ratelimit_test(&rl));
+   ck_assert(ratelimit_cutoff(&rl));
+
+   /* ..regardless of how often we try. */
+   for (i = 0; i < 100; ++i) {
+   ck_assert(!ratelimit_test(&rl));
+   ck_assert(!ratelimit_cutoff(&rl));
+   }
+
+   /* ..even after waiting 5ms */
+   usleep(5000);
+   for (i = 0; i < 100; ++i) {
+   ck_assert(!ratelimit_test(&rl));
+   ck_assert(!ratelimit_cutoff(&rl));
+   }
+
+   /* but after 10ms the counter is reset */
+   usleep(6000); /* +1ms to account for time drifts */
+   }
+}
+END_TEST
+
 int main (int argc, char **argv) {
litest_add_no_device("events:conversion", 
event_conversion_device_notify);
litest_add_no_device("events:conversion", event_conversion_pointer);
@@ -519,5 +555,6 @@ int main (int argc, char **argv) {
litest_add_no_device("config:status string", config_status_string);
 
litest_add_no_device("misc:matrix", matrix_helpers);
+   litest_add_no_device("misc:ratelimit", ratelimit_helpers);
return litest_run(argc, argv);
 }
-- 
2.1.3

___
wayland-devel mailing list
wa