[PATCH libinput] evdev: improve default scroll button detection

2017-02-27 Thread Peter Hutterer
Try to guess the default scroll buttons a bit better. Right now we default to
scroll button 0 (disabled) whenever a device doesn't have a middle button but
we might as well cast a wider net here as setting a scroll button only has a
direct effect when button scrolling is enabled.

Use the first extra button we find or fall back onto the right button if we
don't have any extra buttons.

Signed-off-by: Peter Hutterer 
---
 src/evdev.c | 9 +
 test/test-pointer.c | 4 
 2 files changed, 13 insertions(+)

diff --git a/src/evdev.c b/src/evdev.c
index 2a57b25..e6ae1a3 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1536,10 +1536,19 @@ static uint32_t
 evdev_scroll_get_default_button(struct libinput_device *device)
 {
struct evdev_device *evdev = evdev_device(device);
+   unsigned int code;
 
if (libevdev_has_event_code(evdev->evdev, EV_KEY, BTN_MIDDLE))
return BTN_MIDDLE;
 
+   for (code = BTN_SIDE; code <= BTN_TASK; code++) {
+   if (libevdev_has_event_code(evdev->evdev, EV_KEY, code))
+   return code;
+   }
+
+   if (libevdev_has_event_code(evdev->evdev, EV_KEY, BTN_RIGHT))
+   return BTN_RIGHT;
+
return 0;
 }
 
diff --git a/test/test-pointer.c b/test/test-pointer.c
index 06c45b2..e09f8f8 100644
--- a/test/test-pointer.c
+++ b/test/test-pointer.c
@@ -1176,11 +1176,15 @@ START_TEST(pointer_scroll_defaults_logitech_marble)
struct litest_device *dev = litest_current_device();
struct libinput_device *device = dev->libinput_device;
enum libinput_config_scroll_method method;
+   uint32_t button;
 
method = libinput_device_config_scroll_get_method(device);
ck_assert_int_eq(method, LIBINPUT_CONFIG_SCROLL_NO_SCROLL);
method = libinput_device_config_scroll_get_default_method(device);
ck_assert_int_eq(method, LIBINPUT_CONFIG_SCROLL_NO_SCROLL);
+
+   button = libinput_device_config_scroll_get_button(device);
+   ck_assert_int_eq(button, BTN_SIDE);
 }
 END_TEST
 
-- 
2.9.3

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


Re: weston-simple-egl fullscreen broken?

2017-02-27 Thread Daniel Stone
Hi Arnaud,

On 27 February 2017 at 23:12, Arnaud Vrac  wrote:
> On Mon, Feb 27, 2017 at 11:18 PM Daniel Stone  wrote:
>> On 6 February 2017 at 16:56, Fabien DESSENNE  wrote:
>> > I remember I used to get « weston-simple-egl -f » working fine.
>> > But it does not work anymore : nothing is displayed. From the logs I can 
>> > see
>> > (among others) zxdg_toplevel_v6.configure and wl_surface.commit
>> > Testing with another client works fine: weston-terminal -f -> OK
>> > There may be something wrong with my environment since I am testing with 
>> > the
>> > Daniel’s atomic version (forked from weston-1.12.0+), but I guess this is
>> > broken also with the official version.
>> > If anyone can make a quick test and let me know.
>>
>> This also works fine for me, both upstream and on the atomic branch;
>> it's one of the main tests I do.
>
> I remember that weston-simple-egl -f was broken when using software
> rendering in mesa. From my irc log:
>
> 21:56:01< rawoul> 'weston-simple-egl -f' is also broken with
> LIBGL_ALWAYS_SOFTWARE=1 on libweston-desktop... That's a mesa bug
> though, the first commited buffer does not use the size sent in the
> configure event
> 22:02:04< SardemFF7> not sure it’s a mesa bug
> 22:04:32< SardemFF7> mesa doesn’t know about the shell protocol, and
> that’s a requirement from xdg_shell_v6
> 22:05:16< rawoul> SardemFF7: the client does properly calls
> wl_egl_window_resize with the correct size before the first call to
> eglSwapBuffers. It actually works with dri2, but it seems swrast
> defers the resize

I'm pretty sure that Fabien is using a real GL driver; software GL
would never make it to scanout as it's a SHM buffer. Either way, I'm
pretty sure this bug is fixed in Mesa 17.0. :)

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: weston-simple-egl fullscreen broken?

2017-02-27 Thread Arnaud Vrac
On Mon, Feb 27, 2017 at 11:18 PM Daniel Stone  wrote:
>
> Hi Fabien,
>
> On 6 February 2017 at 16:56, Fabien DESSENNE  wrote:
> > I remember I used to get « weston-simple-egl -f » working fine.
> > But it does not work anymore : nothing is displayed. From the logs I can see
> > (among others) zxdg_toplevel_v6.configure and wl_surface.commit
> > Testing with another client works fine: weston-terminal -f -> OK
> > There may be something wrong with my environment since I am testing with the
> > Daniel’s atomic version (forked from weston-1.12.0+), but I guess this is
> > broken also with the official version.
> > If anyone can make a quick test and let me know.
>
> This also works fine for me, both upstream and on the atomic branch;
> it's one of the main tests I do.

I remember that weston-simple-egl -f was broken when using software
rendering in mesa. From my irc log:

21:56:01< rawoul> 'weston-simple-egl -f' is also broken with
LIBGL_ALWAYS_SOFTWARE=1 on libweston-desktop... That's a mesa bug
though, the first commited buffer does not use the size sent in the
configure event
22:02:04< SardemFF7> not sure it’s a mesa bug
22:04:32< SardemFF7> mesa doesn’t know about the shell protocol, and
that’s a requirement from xdg_shell_v6
22:05:16< rawoul> SardemFF7: the client does properly calls
wl_egl_window_resize with the correct size before the first call to
eglSwapBuffers. It actually works with dri2, but it seems swrast
defers the resize

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


Re: [PATCH weston 21/68] compositor-drm: Use drm_fb for cursor buffers

2017-02-27 Thread Daniel Stone
Hi,

On 22 February 2017 at 14:08, Pekka Paalanen  wrote:
> On Fri,  9 Dec 2016 19:57:36 + Daniel Stone  wrote:
> looks like this patch causes an FB to be created from cursor bo-s, and
> this was not done before and the FBs are not used yet. I think this
> requires an explanation in the commit message: what happens and why it
> will be useful later.
>
> I assume the purpose is to use them with universal planes, right?

Exactly right; added.

>> @@ -183,11 +184,12 @@ struct drm_output {
>>   int disable_pending;
>>
>>   struct gbm_surface *gbm_surface;
>> - struct gbm_bo *gbm_cursor_bo[2];
>> + struct drm_fb *gbm_cursor_fb[2];
>>   struct weston_plane cursor_plane;
>> - struct weston_plane fb_plane;
>>   struct weston_view *cursor_view;
>>   int current_cursor;
>> +
>> + struct weston_plane fb_plane;
>
> What is fb_plane doing here?

Oops.

>> @@ -1867,6 +1871,48 @@ find_crtc_for_connector(struct drm_backend *b,
>>   return -1;
>>  }
>>
>> +static void drm_output_fini_cursor_egl(struct drm_output *output)
>> +{
>> + unsigned int i;
>> +
>> + for (i = 0; i < ARRAY_LENGTH(output->gbm_cursor_fb); i++) {
>> + drm_fb_unref(output->gbm_cursor_fb[i]);
>
> Oh this is why drm_fb_unref() silently accepts NULL. I thought we had a
> habit of avoiding that, or are destructors an exception?

It's a reason I suppose, but not _the_ reason. The reason is that by
the culmination of the atomic series, there are a number of points
where the argument could legitimately be valid or NULL, and which one
it is makes no difference to logic or consistency. Same rationale as
why free() accepts NULL: because the number of calling points where
adding assert(arg) makes sense would be fewer than the number of
calling points where you'd need to wrap it in if (arg) for entirely
legitimate reasons.

I will grant you that there are fewer of these callsites now that
everything has progressively moved into plane/output/pending (surprise
sneak peek!) states, but there are still a few.

I'm still inclined to treat them like free() though, and am on the
side that adding assert(fb) where necessary would make more sense.

>> @@ -1925,6 +1957,7 @@ drm_output_fini_egl(struct drm_output *output)
>>  {
>>   gl_renderer->output_destroy(&output->base);
>>   gbm_surface_destroy(output->gbm_surface);
>> + drm_output_fini_cursor_egl(output);
>>  }
>>
>>  static int
>
> Anyway, with fb_plane dropped and commit message improved:
> Reviewed-by: Pekka Paalanen 

Thanks!

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 19/68] compositor-drm: Refcount drm_fb

2017-02-27 Thread Daniel Stone
Hi Pekka,

On 21 February 2017 at 15:19, Pekka Paalanen  wrote:
> On Fri,  9 Dec 2016 19:57:34 + Daniel Stone  wrote:
>> @@ -312,6 +316,7 @@ drm_fb_create_dumb(struct drm_backend *b, int width, int 
>> height,
>>   if (!fb->format->depth || !fb->format->bpp) {
>>   weston_log("format 0x%lx is not compatible with dumb 
>> buffers\n",
>>  (unsigned long) format);
>> + goto err_fb;
>
> this hunk belongs in an earlier patch.

As you already pointed out whilst reviewing that patch. :)

>> @@ -392,13 +404,14 @@ drm_fb_get_from_bo(struct gbm_bo *bo, struct 
>> drm_backend *backend,
>>   int ret;
>>
>>   if (fb)
>> - return fb;
>> + return drm_fb_ref(fb);
>
> This path is now different from before, and requires adding an
> equivalent drm_fb_unref() call somewhere, but I don't see it.
> Previously unref would have released it immediately, now the first call
> will be a no-op.
>
> Or, if this is a bug fix, it requires an explanation in the commit
> message how in some cases drm_fb_unref() got called twice.
>
> Maybe this hunk belongs in a different patch instead?

It's indeed almost certainly a bugfix: if you have one client buffer
with multiple views, both of which could be promoted to scanout,
you'll get the same user data (drm_fb) handed back for the same BO,
and could attempt to destroy it twice. Previously you'd probably
mostly luck out due to the memory not yet being reused, but now you'd
be near-guaranteed to hit the assert in drm_fb_unref, which seemed
harsh. So I squashed it in here.

drm_fb_ref doesn't exist before this patch, so the split would have to
be this in a later patch. Would that work for you? I don't mind either
way.

>> @@ -472,6 +485,10 @@ drm_fb_unref(struct drm_fb *fb)
>>   if (!fb)
>>   return;
>>
>> + assert(fb->refcnt > 0);
>> + if (--fb->refcnt > 0)
>> + return;
>> +
>>   switch (fb->type) {
>>   case BUFFER_PIXMAN_DUMB:
>>   /* nothing: pixman buffers are destroyed manually */
>
> It took a while to see the paths:
>
> drm_fb_unref -> gbm_bo_destroy -> drm_fb_destroy_gbm
>
> drm_fb_unref -> gbm_surface_release_buffer
> gbm_surface_destroy -> drm_fb_destroy_gbm
>
> drm_output_fini_pixman -> drm_fb_destroy_dumb
>
> The goal is to solidify drm_fb_unref() as the main entry point for
> destruction, but it's not quite there yet. Very good.

Slowly but surely ...

> If you move both of the hunks I pointed out into other patches, then
> this patch gets:
> Reviewed-by: Pekka Paalanen 
> If that's not the right thing to do, I'll review the next revision.

I'll leave you to review the next revision and/or just continue this thread on.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 17/68] compositor-drm: Refactor destroy drm_fb function

2017-02-27 Thread Daniel Stone
Hi,

On 21 February 2017 at 13:58, Pekka Paalanen  wrote:
> On Fri,  9 Dec 2016 19:57:32 + Daniel Stone  wrote:
>> diff --git a/libweston/compositor-drm.c b/libweston/compositor-drm.c
>> index af43a15..7dbfc6b 100644
>> --- a/libweston/compositor-drm.c
>> +++ b/libweston/compositor-drm.c
>> @@ -251,16 +251,39 @@ drm_sprite_crtc_supported(struct drm_output *output, 
>> struct drm_sprite *sprite)
>>  }
>>
>>  static void
>> -drm_fb_destroy_callback(struct gbm_bo *bo, void *data)
>> +drm_fb_destroy(struct drm_fb *fb)
>>  {
>> - struct drm_fb *fb = data;
>> + drmModeRmFB(fb->fd, fb->fb_id);
>
> fb_id cannot be zero, even thought earlier there was a check. Ok.
>
>> + weston_buffer_reference(&fb->buffer_ref, NULL);
>> + free(fb);
>> +}
>> +
>> +static void
>> +drm_fb_destroy_dumb(struct drm_fb *fb)
>> +{
>> + struct drm_mode_destroy_dumb destroy_arg;
>>
>> - if (fb->fb_id)
>> - drmModeRmFB(fb->fd, fb->fb_id);
>> + if (!fb)
>> + return;
>
> Silent acceptance of NULL argument, really?

I think this was just misreading the callsites, thinking that some
called it with possibly-NULL. Inspection shows that to be incorrect,
so, fixed.

>> - weston_buffer_reference(&fb->buffer_ref, NULL);
>> + assert(fb->type == BUFFER_PIXMAN_DUMB);
>
> fb->map cannot be NULL, even thought earlier there was a check. Ok.

Both that and fb_id fixed; the error would've been swallowed either
way, but better not to generate errors in the first.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston 15/68] compositor-drm: Add explicit type member to drm_fb

2017-02-27 Thread Daniel Stone
Hi Pekka,
Thanks for the thoughtful review, as ever.

On 22 February 2017 at 13:45, Pekka Paalanen  wrote:
> On Tue, 21 Feb 2017 15:29:09 +0200 Pekka Paalanen  wrote:
>> On Fri,  9 Dec 2016 19:57:30 + Daniel Stone  
>> wrote:
>> > @@ -452,15 +461,19 @@ drm_output_release_fb(struct drm_output *output, 
>> > struct drm_fb *fb)
>> > if (!fb)
>> > return;
>> >
>> > -   if (fb->map &&
>> > -(fb != output->dumb[0] && fb != output->dumb[1])) {
>> > -   drm_fb_destroy_dumb(fb);
>>
>> This piece sent me into a recursive "well, actually..." loop.
>>
>> It looked like dead code at first hand, as this gets hit when
>> output->dumb and fb don't match. When would that be? I would guess when
>> video mode changed.
>>
>> I think changing resolutions would hit this path, when flipping to a
>> new dumb buffer of a different size than one coming out of scanout
>> which cannot be destroyed until pageflip completed.
>>
>> Except I am wrong in a couple of ways: destroying the buffer is fine,
>> the kernel will keep it referenced as long as necessary anyway. And,
>> drm_output_switch_mode() does destroy everything immediately.
>>
>> So this bit is ok. Unless there is a third well-actually.
>
> And there is it:
>
> < MrCooper> pq: IME KMS framebuffers aren't reference-counted in the
> kernel; destroying an fb which is still being scanned out by any CRTCs
> turns off those CRTCs
>
> Is it then so, that drm_output_switch_mode() is wrong to destroy the FB
> immediately before even flipping a new one, but as it does so, there is
> no leak introduced by removing the above drm_fb_destroy_dumb() call?
>
> I.e. the patch does not regress anything because of a bug elsewhere?
>
> If that is so, my R-b stands, as fixing switch_mode is a whole another
> matter. Also the refcounting introduced later in this series offer a
> much better basis for fixing switch_mode.

Correct on all counts, especially on switch_mode. ;) I wrote up my
findings but decided not to conflate that and atomic:
https://phabricator.freedesktop.org/T7621 - if anyone else wants to
pursue that, I'm more than happy to help out and talk you through it.
I think it'd be a really interesting task to attack, and it'd
definitely be really helpful.

Michel is indeed right that calling drmModeRmFB will disable any CRTC
currently scanning out of that buffer. I don't think your analysis is
quite correct though: drm_output_release_fb() gets called from
switch_mode, but _before_ the drm_output_fini_pixman() ->
drm_output_init_pixman() cycle, which is what will set new
output->dumb pointers. So I believe this call was a no-op. Even if it
wasn't a no-op, drm_output_fini_pixman() will destroy those buffers
anyway; if the release_fb() call _wasn't_ a no-op, then
drm_output_fini_pixman() would crash because you'd destroy the buffers
twice. Great.

tl;dr: I think it's fine, you think it's fine, so I'll take the
asserts and your R-b, thankyou ;)

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: weston-simple-egl fullscreen broken?

2017-02-27 Thread Daniel Stone
Hi Fabien,

On 6 February 2017 at 16:56, Fabien DESSENNE  wrote:
> I remember I used to get « weston-simple-egl -f » working fine.
> But it does not work anymore : nothing is displayed. From the logs I can see
> (among others) zxdg_toplevel_v6.configure and wl_surface.commit
> Testing with another client works fine: weston-terminal -f -> OK
> There may be something wrong with my environment since I am testing with the
> Daniel’s atomic version (forked from weston-1.12.0+), but I guess this is
> broken also with the official version.
> If anyone can make a quick test and let me know.

This also works fine for me, both upstream and on the atomic branch;
it's one of the main tests I do.

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: [PATCH weston] Update COPYING

2017-02-27 Thread Daniel Stone
Hi Bryce,
Sorry, totally neglected this whilst travelling.

On 17 February 2017 at 18:09, Bryce Harrington  wrote:
> On Fri, Feb 17, 2017 at 05:19:55PM +, Daniel Stone wrote:
>> On 17 February 2017 at 17:12, Bryce Harrington  wrote:
>> > It seems atypical amongst open source projects to have an exhaustive
>> > (and duplicative as pointed out) listing of copyright statements.  Has
>> > there been an issue raised from outside the project that this listing
>> > would solve?
>>
>> The X projects all have it, at least.
>
> I only spot checked, and you would know better than me, but the ones I
> looked at appeared to just list key/major copyright holders (which does
> seem sensible).  "Vague indications of copyrights", as you mention below.

Hm, xserver I thought was at least exhaustive.

>> Everyone who distributes it
>> (distributions, companies building products, etc) need to have
>> something that at least minimally conforms to the Mesa licensing
>> document: a full statement of the license, and at least a vague
>> indication of the copyrights. Depending on the legal department
>> involved, they may end up compiling this exact list for their own use.
>
> I've done a couple such conformance checks in the past, and indeed I had
> to compile such a list, so you're certainly right.  But as pq showed,
> it's a straightforward set of shell commands to do it.  And actually, if
> I were doing a conformance check, I wouldn't trust that the COPYING file
> was being kept up to date so would do that scan regardless.  Indeed, if
> there were any descrepancies that showed up I would feel compelled to
> investigate each of them.  IOW rather than saving time the COPYING file
> might actually create an bit of extra work for compliance checker.
> Frankly, the script itself might be more valuable in this regard, so
> maybe that's what should be included in the tree?
>
> If the ultimate goal is to help make compliance checking easier, I would
> suggest focusing on fixing any irregularities in the files themselves -
> e.g. continuing to ensuring dates and copyright formats are correct,
> that boilerplate licensing text is consistent across files, etc., as
> folks have been doing, so that running a scan is clean and reliable.
>
> OTOH, if the goal is about giving recognition to contributors, an
> AUTHORS or CONTRIBUTORS file seems to be more conventionally used
> approaches.

Fair enough. FWIW, that wasn't my goal: if I was after more credit,
I'd probably do it in a way which was visible to more than just distro
maintainers and lawyers. :)

>> I'd be fine to reduce it to the minimal license text, but that doesn't
>> free us up from needing to check incoming source to make sure it
>> conforms to the same license. We should really also merge data/COPYING
>> into the core COPYING.
>
> Obviously checking licenses on incoming code is always extremely
> important. :-)
>
> I'm not sure what you're suggesting by reducing it to the minimal
> license, the file only includes one license statement so appears to be
> minimal already; I'm not suggesting copyrights *shouldn't* be present,
> or that any of the existing ones should be removed.  AIUI it's required
> to have at least one copyright statement, and seems pretty standard to
> list the major copyright holders (esp. any companies/individuals with a
> legal interest.)  The main purpose of COPYING, though, is the licensing,
> to document how the codebase can be shared and reused.
>
> You're probably right that merging data/COPYING and COPYING makes
> sense, but I've seen enough other projects that had subdir-specific
> licensing gunk that I'm not really worried about it.  I'd be fine
> either way.

Yeah, I don't think we're big enough that having separate files makes
much sense. What I'm mostly just stuck with is the copyright
statements: at the moment, we list a few but don't go on to list any
others. I'd suggest an incomplete statement is the worst of both
worlds: should we maybe just list the applicable licenses with a
'Copyright © 2008-2017 multiple authors' and the license text, with a
note to check the individual files to determine who owns copyright
over which part?

Cheers,
Daniel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel


Re: libwayland-server ABI change: [PATCH] wayland-server: Destroy Clients upon wl_display_destroy()

2017-02-27 Thread Pekka Paalanen
On Wed, 25 Jan 2017 13:29:08 +0200
Pekka Paalanen  wrote:

> On Wed, 25 Jan 2017 11:08:26 +0800
> Jonas Ådahl  wrote:
> 
> > On Mon, Jan 23, 2017 at 03:40:11PM +0200, Pekka Paalanen wrote:  
> > > On Tue, 20 Dec 2016 18:40:13 +0530
> > > viveka  wrote:
> > > 
> > > > From: "Vivek.A" 
> > > > 
> > > > Without this, client socket file descriptors are being kept open 
> > > > until the process
> > > > hosting the compositor gets terminated / exit. This causes 
> > > > compositor termination
> > > > goes undetected through its fd. This could be reproduced by having 
> > > > compositor as
> > > > one of the threads in a process.
> > > > 
> > > > https://bugs.freedesktop.org/show_bug.cgi?id=99142
> > > > 
> > > > Signed-off-by: Vivek.A 
> > > > ---
> > > >  src/wayland-server.c | 5 +
> > > >  1 file changed, 5 insertions(+)
> > > > 
> > > > diff --git a/src/wayland-server.c b/src/wayland-server.c
> > > > index 9d7d9c1..8195064 100644
> > > > --- a/src/wayland-server.c
> > > > +++ b/src/wayland-server.c
> > > > @@ -979,11 +979,16 @@ wl_socket_alloc(void)
> > > >  WL_EXPORT void
> > > >  wl_display_destroy(struct wl_display *display)
> > > >  {
> > > > +   struct wl_client *c, *cnext;
> > > > struct wl_socket *s, *next;
> > > > struct wl_global *global, *gnext;
> > > >  
> > > > wl_signal_emit(&display->destroy_signal, display);
> > > >  
> > > > +   wl_list_for_each_safe(c, cnext, &display->client_list, link) {
> > > > +   wl_client_destroy(c);
> > > > +   }
> > > > +
> > > > wl_list_for_each_safe(s, next, &display->socket_list, link) {
> > > > wl_socket_destroy(s);
> > > > }
> > > 
> > > Hi,
> > > 
> > > this needs an update on the documentation to say that existing clients
> > > are destroyed.
> > > 
> > > Previously it has been illegal to have existing clients when destroying
> > > a wl_display, because destroying a wl_client afterwards would have
> > > potentially accessed freed memory: wl_display::client_list list head.
> > > However, we failed to document this.
> > > 
> > > As far as I can tell, Weston also got it wrong, and never destroyed the
> > > clients before the wl_display.
> > > 
> > > Now starts the self-rant, which could be just TL;DR and conclude as: a
> > > good patch, fine by me if you add docs, but Weston is broken anyway.
> > > 
> > > ---
> > > 
> > > (A counter-proposition to this patch could be to just unlink the client
> > > list head, and leave the clients to be destroyed by the compositor. I
> > > have hard time imagining when that would be useful (Graceful shutdown
> > > waiting for clients to exit themselves? This is not a web server.) and
> > > it would be awkward to use, too. Also, destroying the wl_display
> > > removes the only supported way of polling and servicing the clients.
> > > Therefore I reject the counter-proposition.)
> > > 
> > > So I started wondering what happens in Weston, as I should really see
> > > the access to freed memory in valgrind, but I don't. Then I realized
> > > that nothing frees the remaining clients in Weston on shutdown. Ok, I
> > > should see a reduction in leaked memory then. But the results are
> > > depressing.
> > > 
> > > Start weston on x11:
> > > 
> > > $ valgrind -v --leak-check=full --show-reachable=yes --track-origins=yes 
> > > --num-callers=30 weston --use-pixman
> > > 
> > > Then click (twice... why twice...) the icon to start weston_terminal,
> > > and then close weston while the terminal window is open. This is needed
> > > to have a client remain, because the shell helper clients get torn down
> > > anyway.
> > > 
> > > 
> > > Before patch:
> > > 
> > > ==23476== LEAK SUMMARY:
> > > ==23476==definitely lost: 232 bytes in 3 blocks
> > > ==23476==indirectly lost: 22,890 bytes in 47 blocks
> > > ==23476==  possibly lost: 163 bytes in 4 blocks
> > > ==23476==still reachable: 72,450 bytes in 181 blocks
> > > ==23476== suppressed: 0 bytes in 0 blocks
> > > ==23476== 
> > > ==23476== ERROR SUMMARY: 7 errors from 7 contexts (suppressed: 0 from 0)
> > > 
> > > After patch:
> > > 
> > > ==26025== LEAK SUMMARY:
> > > ==26025==definitely lost: 968 bytes in 4 blocks
> > > ==26025==indirectly lost: 1,584 bytes in 6 blocks
> > > ==26025==  possibly lost: 163 bytes in 4 blocks
> > > ==26025==still reachable: 72,450 bytes in 181 blocks
> > > ==26025== suppressed: 0 bytes in 0 blocks
> > > ==26025== 
> > > ==26025== ERROR SUMMARY: 23 errors from 21 contexts (suppressed: 0 from 0)
> > > 
> > > The indirectly lost amount goes down as expected, but the number of
> > > errors grows badly.
> > > 
> > > I suppose the thing is that weston has already pretty much shut down by
> > > the time it calls wl_display_destroy(). It's not prepared to resources
> > > still existing after shutdown, so when wl_display_destroy() goes
> > > destroying the remaining clients, their resources get destroyed, which
>

Re: [PATCH libinput] touchpad: reduce minimum height for horiz edge scrolling to 40mm

2017-02-27 Thread Hans de Goede

Hi,

On 27-02-17 02:02, Peter Hutterer wrote:

Introduced in commit 8e7f99c27ab39 we only allowed horizontal edge scrolling
on devices larger than 50mm to leave enough reactive space on the touchpad.
Looking at a ruler, a 50mm high touchpad is still large enough to leave the
bottom 7mm as an horizontal edge scroll area. Reduce the minimum size to 40mm
instead, that's closer to where it starts to get a bit iffy.

https://bugzilla.redhat.com/show_bug.cgi?id=141

Signed-off-by: Peter Hutterer 


Patch looks good to me:

Reviewed-by: Hans de Goede 

Regards,

Hans




---
 src/evdev-mt-touchpad-edge-scroll.c | 4 ++--
 test/test-touchpad.c| 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/evdev-mt-touchpad-edge-scroll.c 
b/src/evdev-mt-touchpad-edge-scroll.c
index 1d30bca..5551a8d 100644
--- a/src/evdev-mt-touchpad-edge-scroll.c
+++ b/src/evdev-mt-touchpad-edge-scroll.c
@@ -291,14 +291,14 @@ tp_edge_scroll_init(struct tp_dispatch *tp, struct 
evdev_device *device)
struct phys_coords mm = { 0.0, 0.0 };

evdev_device_get_size(device, &width, &height);
-   /* Touchpads smaller than 50mm are not tall enough to have a
+   /* Touchpads smaller than 40mm are not tall enough to have a
   horizontal scroll area, it takes too much space away. But
   clickpads have enough space here anyway because of the
   software button area (and all these tiny clickpads were built
   when software buttons were a thing, e.g. Lenovo *20 series)
 */
if (!tp->buttons.is_clickpad)
-   want_horiz_scroll = (height >= 50);
+   want_horiz_scroll = (height >= 40);

/* 7mm edge size */
mm.x = width - 7;
diff --git a/test/test-touchpad.c b/test/test-touchpad.c
index 4656443..29039b3 100644
--- a/test/test-touchpad.c
+++ b/test/test-touchpad.c
@@ -462,7 +462,7 @@ touchpad_has_horiz_edge_scroll_size(struct litest_device 
*dev)

rc = libinput_device_get_size(dev->libinput_device, &width, &height);

-   return rc == 0 && height >= 50;
+   return rc == 0 && height >= 40;
 }

 START_TEST(touchpad_edge_scroll_horiz)


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