Re: [PATCH weston 1/2] clients: consolidate timer code part 1

[Daniel Stone]

Hi,

On 9 March 2018 at 17:32, Derek Foreman  wrote:
> On 2018-03-09 05:44 AM, Pekka Paalanen wrote:
>> There are multiple copies for the timerfd handling code, and I need a
>> timer in one more app. Consolidate all the timerfd code into window.c to
>> reduce the duplication. Many of the copies were also flawed against the
>> race mentioned in toytimer_fire().
>>
>> This patch handles clickdot and window.c's tooltip timer and cursor
>> timer.
>
> Just out of curiosity, how'd you decide to break it down this way?
>
> Both patches look good to me.  I hate that reset/fire race and wasted a bit
> of time understanding it the first time I hit it.  I'm really happy to see
> it handled consistently in one place.
>
> Reviewed-by: Derek Foreman 
> For both.
>
> I think they look safe to land now, as it's mostly harmless refactor, and
> the rest is actual bug fix, as abort()ing on the read fail was wrong.
>
> I'll let you make the call on landing it. :)

I don't know timerfd well enough to comment on whether all the usage
is correct. But I like the idea of centralising error-prone things in
one place. It makes it much harder for people to get it wrong, and
does serve our thriving community of people who copy Weston code into
their own projects.

Given that, I'd be in favour of landing for release, and the series is:
Acked-by: Daniel Stone 

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

Re: [PATCH weston 1/2] clients: consolidate timer code part 1

[Derek Foreman]

On 2018-03-09 05:44 AM, Pekka Paalanen wrote:

From: Pekka Paalanen 

There are multiple copies for the timerfd handling code, and I need a
timer in one more app. Consolidate all the timerfd code into window.c to
reduce the duplication. Many of the copies were also flawed against the
race mentioned in toytimer_fire().

This patch handles clickdot and window.c's tooltip timer and cursor
timer.

Signed-off-by: Pekka Paalanen 


Just out of curiosity, how'd you decide to break it down this way?

Both patches look good to me.  I hate that reset/fire race and wasted a 
bit of time understanding it the first time I hit it.  I'm really happy 
to see it handled consistently in one place.


Reviewed-by: Derek Foreman 
For both.

I think they look safe to land now, as it's mostly harmless refactor, 
and the rest is actual bug fix, as abort()ing on the read fail was wrong.


I'll let you make the call on landing it. :)

Thanks,
Derek


---
  clients/clickdot.c |  36 +++--
  clients/window.c   | 150 +++--
  clients/window.h   |  27 ++
  3 files changed, 135 insertions(+), 78 deletions(-)

diff --git a/clients/clickdot.c b/clients/clickdot.c
index f52fbf04..f9e6e640 100644
--- a/clients/clickdot.c
+++ b/clients/clickdot.c
@@ -32,9 +32,8 @@
  #include 
  #include 
  #include 
-#include 
-#include 
  #include 
+#include 
  
  #include 

  #include 
@@ -62,8 +61,7 @@ struct clickdot {
int reset;
  
  	struct input *cursor_timeout_input;

-   int cursor_timeout_fd;
-   struct task cursor_timeout_task;
+   struct toytimer cursor_timeout;
  };
  
  static void

@@ -224,14 +222,7 @@ button_handler(struct widget *widget,
  static void
  cursor_timeout_reset(struct clickdot *clickdot)
  {
-   const long cursor_timeout = 500;
-   struct itimerspec its;
-
-   its.it_interval.tv_sec = 0;
-   its.it_interval.tv_nsec = 0;
-   its.it_value.tv_sec = cursor_timeout / 1000;
-   its.it_value.tv_nsec = (cursor_timeout % 1000) * 1000 * 1000;
-   timerfd_settime(clickdot->cursor_timeout_fd, 0, , NULL);
+   toytimer_arm_once_usec(>cursor_timeout, 500 * 1000);
  }
  
  static int

@@ -271,15 +262,10 @@ leave_handler(struct widget *widget,
  }
  
  static void

-cursor_timeout_func(struct task *task, uint32_t events)
+cursor_timeout_func(struct toytimer *tt)
  {
struct clickdot *clickdot =
-   container_of(task, struct clickdot, cursor_timeout_task);
-   uint64_t exp;
-
-   if (read(clickdot->cursor_timeout_fd, , sizeof (uint64_t)) !=
-   sizeof(uint64_t))
-   abort();
+   container_of(tt, struct clickdot, cursor_timeout);
  
  	input_set_pointer_image(clickdot->cursor_timeout_input,

CURSOR_LEFT_PTR);
@@ -317,12 +303,8 @@ clickdot_create(struct display *display)
clickdot->line.old_y = -1;
clickdot->reset = 0;
  
-	clickdot->cursor_timeout_fd =

-   timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
-   clickdot->cursor_timeout_task.run = cursor_timeout_func;
-   display_watch_fd(window_get_display(clickdot->window),
-clickdot->cursor_timeout_fd,
-EPOLLIN, >cursor_timeout_task);
+   toytimer_init(>cursor_timeout, CLOCK_MONOTONIC,
+ display, cursor_timeout_func);
  
  	return clickdot;

  }
@@ -330,9 +312,7 @@ clickdot_create(struct display *display)
  static void
  clickdot_destroy(struct clickdot *clickdot)
  {
-   display_unwatch_fd(window_get_display(clickdot->window),
-  clickdot->cursor_timeout_fd);
-   close(clickdot->cursor_timeout_fd);
+   toytimer_fini(>cursor_timeout);
if (clickdot->buffer)
cairo_surface_destroy(clickdot->buffer);
widget_destroy(clickdot->widget);
diff --git a/clients/window.c b/clients/window.c
index a94cee2c..e1c9de71 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -349,9 +349,8 @@ struct input {
struct wl_callback *cursor_frame_cb;
uint32_t cursor_timer_start;
uint32_t cursor_anim_current;
-   int cursor_delay_fd;
+   struct toytimer cursor_timer;
bool cursor_timer_running;
-   struct task cursor_task;
struct wl_surface *pointer_surface;
uint32_t modifiers;
uint32_t pointer_enter_serial;
@@ -440,8 +439,7 @@ struct tooltip {
struct widget *parent;
struct widget *widget;
char *entry;
-   struct task tooltip_task;
-   int tooltip_fd;
+   struct toytimer timer;
float x, y;
  };
  
@@ -2122,21 +2120,17 @@ widget_destroy_tooltip(struct widget *parent)

tooltip->widget = NULL;
}
  
-	close(tooltip->tooltip_fd);

+   toytimer_fini(>timer);
free(tooltip->entry);
free(tooltip);
parent->tooltip = 

Re: [PATCH] compositor-wayland: Ignore pointer enter on destroyed surface

[Pekka Paalanen]

On Fri, 9 Mar 2018 14:14:11 +
Daniel Stone  wrote:

> Hi,
> 
> On 9 March 2018 at 14:08, Pekka Paalanen  wrote:
> > On Fri,  9 Mar 2018 10:08:37 +
> > Daniel Stone  wrote:  
> >> + if (!surface) {
> >> + input->output = NULL;
> >> + input->has_focus = false;
> >> + notify_pointer_focus(>base, NULL, 0, 0);
> >> + input_set_cursor(input);
> >> + return;
> >> + }
> >> +
> >>   x = wl_fixed_to_double(fixed_x);
> >>   y = wl_fixed_to_double(fixed_y);
> >>  
> >
> > Hi Daniel,
> >
> > looks good, but what is the input_set_cursor() call doing there?  
> 
> Pure cargo-cult from the bottom of the function here. I guess it's
> probably unlikely to be helpful: if we've already destroyed the
> surface, there's little to no point setting a cursor on it. So I think
> it can, in fact, be removed.

Cool, I made that change, and pushed:
   72e183bd..3f839374  master -> master


Thanks,
pq


pgpH9r7YdEfUJ.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH] compositor-wayland: Ignore pointer enter on destroyed surface

[Daniel Stone]

Hi,

On 9 March 2018 at 14:08, Pekka Paalanen  wrote:
> On Fri,  9 Mar 2018 10:08:37 +
> Daniel Stone  wrote:
>> + if (!surface) {
>> + input->output = NULL;
>> + input->has_focus = false;
>> + notify_pointer_focus(>base, NULL, 0, 0);
>> + input_set_cursor(input);
>> + return;
>> + }
>> +
>>   x = wl_fixed_to_double(fixed_x);
>>   y = wl_fixed_to_double(fixed_y);
>>
>
> Hi Daniel,
>
> looks good, but what is the input_set_cursor() call doing there?

Pure cargo-cult from the bottom of the function here. I guess it's
probably unlikely to be helpful: if we've already destroyed the
surface, there's little to no point setting a cursor on it. So I think
it can, in fact, be removed.

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

Re: [PATCH] compositor-wayland: Ignore pointer enter on destroyed surface

[Pekka Paalanen]

On Fri,  9 Mar 2018 10:08:37 +
Daniel Stone  wrote:

> Due to race conditions, it is (vanishingly unlikely but) possible to
> receive a wl_pointer.enter event referring to a wl_surface we have just
> destroyed. If this happens, wl_surface will be NULL. Detect this, clear
> out our focus, and return.
> 
> Other pointer and keyboard events are robust against destroyed surfaces.
> 
> Signed-off-by: Daniel Stone 
> Cc: Pekka Paalanen 
> ---
>  libweston/compositor-wayland.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
> index 9c401d2e6..964b84e4b 100644
> --- a/libweston/compositor-wayland.c
> +++ b/libweston/compositor-wayland.c
> @@ -1525,6 +1525,14 @@ input_handle_pointer_enter(void *data, struct 
> wl_pointer *pointer,
>   enum theme_location location;
>   double x, y;
>  
> + if (!surface) {
> + input->output = NULL;
> + input->has_focus = false;
> + notify_pointer_focus(>base, NULL, 0, 0);
> + input_set_cursor(input);
> + return;
> + }
> +
>   x = wl_fixed_to_double(fixed_x);
>   y = wl_fixed_to_double(fixed_y);
>  

Hi Daniel,

looks good, but what is the input_set_cursor() call doing there?


Thanks,
pq


pgpil9dRbPPBf.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel

[PATCH weston 1/2] clients: consolidate timer code part 1

[Pekka Paalanen]

From: Pekka Paalanen 

There are multiple copies for the timerfd handling code, and I need a
timer in one more app. Consolidate all the timerfd code into window.c to
reduce the duplication. Many of the copies were also flawed against the
race mentioned in toytimer_fire().

This patch handles clickdot and window.c's tooltip timer and cursor
timer.

Signed-off-by: Pekka Paalanen 
---
 clients/clickdot.c |  36 +++--
 clients/window.c   | 150 +++--
 clients/window.h   |  27 ++
 3 files changed, 135 insertions(+), 78 deletions(-)

diff --git a/clients/clickdot.c b/clients/clickdot.c
index f52fbf04..f9e6e640 100644
--- a/clients/clickdot.c
+++ b/clients/clickdot.c
@@ -32,9 +32,8 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
+#include 
 
 #include 
 #include 
@@ -62,8 +61,7 @@ struct clickdot {
int reset;
 
struct input *cursor_timeout_input;
-   int cursor_timeout_fd;
-   struct task cursor_timeout_task;
+   struct toytimer cursor_timeout;
 };
 
 static void
@@ -224,14 +222,7 @@ button_handler(struct widget *widget,
 static void
 cursor_timeout_reset(struct clickdot *clickdot)
 {
-   const long cursor_timeout = 500;
-   struct itimerspec its;
-
-   its.it_interval.tv_sec = 0;
-   its.it_interval.tv_nsec = 0;
-   its.it_value.tv_sec = cursor_timeout / 1000;
-   its.it_value.tv_nsec = (cursor_timeout % 1000) * 1000 * 1000;
-   timerfd_settime(clickdot->cursor_timeout_fd, 0, , NULL);
+   toytimer_arm_once_usec(>cursor_timeout, 500 * 1000);
 }
 
 static int
@@ -271,15 +262,10 @@ leave_handler(struct widget *widget,
 }
 
 static void
-cursor_timeout_func(struct task *task, uint32_t events)
+cursor_timeout_func(struct toytimer *tt)
 {
struct clickdot *clickdot =
-   container_of(task, struct clickdot, cursor_timeout_task);
-   uint64_t exp;
-
-   if (read(clickdot->cursor_timeout_fd, , sizeof (uint64_t)) !=
-   sizeof(uint64_t))
-   abort();
+   container_of(tt, struct clickdot, cursor_timeout);
 
input_set_pointer_image(clickdot->cursor_timeout_input,
CURSOR_LEFT_PTR);
@@ -317,12 +303,8 @@ clickdot_create(struct display *display)
clickdot->line.old_y = -1;
clickdot->reset = 0;
 
-   clickdot->cursor_timeout_fd =
-   timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
-   clickdot->cursor_timeout_task.run = cursor_timeout_func;
-   display_watch_fd(window_get_display(clickdot->window),
-clickdot->cursor_timeout_fd,
-EPOLLIN, >cursor_timeout_task);
+   toytimer_init(>cursor_timeout, CLOCK_MONOTONIC,
+ display, cursor_timeout_func);
 
return clickdot;
 }
@@ -330,9 +312,7 @@ clickdot_create(struct display *display)
 static void
 clickdot_destroy(struct clickdot *clickdot)
 {
-   display_unwatch_fd(window_get_display(clickdot->window),
-  clickdot->cursor_timeout_fd);
-   close(clickdot->cursor_timeout_fd);
+   toytimer_fini(>cursor_timeout);
if (clickdot->buffer)
cairo_surface_destroy(clickdot->buffer);
widget_destroy(clickdot->widget);
diff --git a/clients/window.c b/clients/window.c
index a94cee2c..e1c9de71 100644
--- a/clients/window.c
+++ b/clients/window.c
@@ -349,9 +349,8 @@ struct input {
struct wl_callback *cursor_frame_cb;
uint32_t cursor_timer_start;
uint32_t cursor_anim_current;
-   int cursor_delay_fd;
+   struct toytimer cursor_timer;
bool cursor_timer_running;
-   struct task cursor_task;
struct wl_surface *pointer_surface;
uint32_t modifiers;
uint32_t pointer_enter_serial;
@@ -440,8 +439,7 @@ struct tooltip {
struct widget *parent;
struct widget *widget;
char *entry;
-   struct task tooltip_task;
-   int tooltip_fd;
+   struct toytimer timer;
float x, y;
 };
 
@@ -2122,21 +2120,17 @@ widget_destroy_tooltip(struct widget *parent)
tooltip->widget = NULL;
}
 
-   close(tooltip->tooltip_fd);
+   toytimer_fini(>timer);
free(tooltip->entry);
free(tooltip);
parent->tooltip = NULL;
 }
 
 static void
-tooltip_func(struct task *task, uint32_t events)
+tooltip_func(struct toytimer *tt)
 {
-   struct tooltip *tooltip =
-   container_of(task, struct tooltip, tooltip_task);
-   uint64_t exp;
+   struct tooltip *tooltip = container_of(tt, struct tooltip, timer);
 
-   if (read(tooltip->tooltip_fd, , sizeof (uint64_t)) != sizeof 
(uint64_t))
-   abort();
window_create_tooltip(tooltip);
 }
 
@@ -2144,16 +2138,7 @@ tooltip_func(struct task *task, uint32_t events)
 static int
 tooltip_timer_reset(struct tooltip *tooltip)
 {
-

[PATCH weston 2/2] clients: consolidate timer code part 2

[Pekka Paalanen]

From: Pekka Paalanen 

Continue moving bits to use toytimer instead of carelessly open-coded
equivalent. Many of the copies were flawed against the race mentioned
in toytimer_fire().

This patch handles window.c's key repeat, confine demo, and
desktop-shell panel clock.

Signed-off-by: Pekka Paalanen 
---
 clients/confine.c   | 35 +++
 clients/desktop-shell.c | 35 +++
 clients/window.c| 41 ++---
 3 files changed, 24 insertions(+), 87 deletions(-)

diff --git a/clients/confine.c b/clients/confine.c
index c0d908fb..255236a3 100644
--- a/clients/confine.c
+++ b/clients/confine.c
@@ -33,8 +33,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 
 #include 
@@ -64,8 +62,7 @@ struct confine {
int reset;
 
struct input *cursor_timeout_input;
-   int cursor_timeout_fd;
-   struct task cursor_timeout_task;
+   struct toytimer cursor_timeout;
 
bool pointer_confined;
 
@@ -347,14 +344,7 @@ button_handler(struct widget *widget,
 static void
 cursor_timeout_reset(struct confine *confine)
 {
-   const long cursor_timeout = 500;
-   struct itimerspec its;
-
-   its.it_interval.tv_sec = 0;
-   its.it_interval.tv_nsec = 0;
-   its.it_value.tv_sec = cursor_timeout / 1000;
-   its.it_value.tv_nsec = (cursor_timeout % 1000) * 1000 * 1000;
-   timerfd_settime(confine->cursor_timeout_fd, 0, , NULL);
+   toytimer_arm_once_usec(>cursor_timeout, 500 * 1000);
 }
 
 static int
@@ -406,15 +396,10 @@ leave_handler(struct widget *widget,
 }
 
 static void
-cursor_timeout_func(struct task *task, uint32_t events)
+cursor_timeout_func(struct toytimer *tt)
 {
struct confine *confine =
-   container_of(task, struct confine, cursor_timeout_task);
-   uint64_t exp;
-
-   if (read(confine->cursor_timeout_fd, , sizeof (uint64_t)) !=
-   sizeof(uint64_t))
-   abort();
+   container_of(tt, struct confine, cursor_timeout);
 
input_set_pointer_image(confine->cursor_timeout_input,
CURSOR_LEFT_PTR);
@@ -461,12 +446,8 @@ confine_create(struct display *display)
confine->line.old_y = -1;
confine->reset = 0;
 
-   confine->cursor_timeout_fd =
-   timerfd_create(CLOCK_MONOTONIC, TFD_CLOEXEC);
-   confine->cursor_timeout_task.run = cursor_timeout_func;
-   display_watch_fd(window_get_display(confine->window),
-confine->cursor_timeout_fd,
-EPOLLIN, >cursor_timeout_task);
+   toytimer_init(>cursor_timeout, CLOCK_MONOTONIC,
+ display, cursor_timeout_func);
 
return confine;
 }
@@ -474,9 +455,7 @@ confine_create(struct display *display)
 static void
 confine_destroy(struct confine *confine)
 {
-   display_unwatch_fd(window_get_display(confine->window),
-  confine->cursor_timeout_fd);
-   close(confine->cursor_timeout_fd);
+   toytimer_fini(>cursor_timeout);
if (confine->buffer)
cairo_surface_destroy(confine->buffer);
widget_destroy(confine->widget);
diff --git a/clients/desktop-shell.c b/clients/desktop-shell.c
index cabe851f..6d19d029 100644
--- a/clients/desktop-shell.c
+++ b/clients/desktop-shell.c
@@ -34,8 +34,6 @@
 #include 
 #include 
 #include 
-#include 
-#include 
 #include 
 #include 
 #include 
@@ -148,8 +146,7 @@ struct panel_launcher {
 struct panel_clock {
struct widget *widget;
struct panel *panel;
-   struct task clock_task;
-   int clock_fd;
+   struct toytimer timer;
char *format_string;
time_t refresh_timer;
 };
@@ -365,14 +362,10 @@ panel_launcher_touch_up_handler(struct widget *widget, 
struct input *input,
 }
 
 static void
-clock_func(struct task *task, uint32_t events)
+clock_func(struct toytimer *tt)
 {
-   struct panel_clock *clock =
-   container_of(task, struct panel_clock, clock_task);
-   uint64_t exp;
+   struct panel_clock *clock = container_of(tt, struct panel_clock, timer);
 
-   if (read(clock->clock_fd, , sizeof exp) != sizeof exp)
-   abort();
widget_schedule_redraw(clock->widget);
 }
 
@@ -423,10 +416,7 @@ clock_timer_reset(struct panel_clock *clock)
its.it_interval.tv_nsec = 0;
its.it_value.tv_sec = clock->refresh_timer;
its.it_value.tv_nsec = 0;
-   if (timerfd_settime(clock->clock_fd, 0, , NULL) < 0) {
-   fprintf(stderr, "could not set timerfd\n: %m");
-   return -1;
-   }
+   toytimer_arm(>timer, );
 
return 0;
 }
@@ -435,9 +425,7 @@ static void
 panel_destroy_clock(struct panel_clock *clock)
 {
widget_destroy(clock->widget);
-
-   close(clock->clock_fd);
-
+   toytimer_fini(>timer);

Re: [PATCH weston] libweston/compositor-drm: Reset repaint scheduled status when setting DPMS level to off

[Marius-cristian Vlad]

On Fri, 2018-03-09 at 09:35 +, Daniel Stone wrote:
> Hi Marius,
> 
> On 7 March 2018 at 17:36, Marius Vlad 
> wrote:
> > Otherwise when setting dpms level DPMS_ON,
> > weston_output_schedule_repaint()
> > will bail out early and never get a chance to wake up the output.
> > 
> > Arguably this could also be done in drm_set_dpms() when setting
> > dpms_off_pending
> > but I figure it better to do it when deferred.
> 
> Thanks, that's a good catch, but I do wonder how it wasn't getting
> tripped up before. In previous releases, we'd call drm_set_dpms()
> from
> anywhere, which would block on the update completing and then set the
> DPMS level. I wonder if it's because we would receive a flip-done
> event anyway and call weston_output_finish_frame() after the DPMS had
> completed, which would drive us into repaint-idle.

Indeed, I've submitted a previous patch because for timeline
instrumentation because of this. weston_output_finish_frame() is being
called with a NULL timestamp.

> 
> I suspect we should be calling finish_frame() to match that
> behaviour,
> to indicate that the previous update has completed, and then
> synchronously applying DPMS state. Pretending that
> disable/destroy/DPMS-off can be called at any time is really biting
> though. :(

I don't know enough about the repaint machinery, should this be done
from drm_set_dpms() or from drm_output_update_complete()?. I'm hitting
some asserts in both cases, maybe I'm doing it wrong :/.



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

Re: [PATCH wayland v2 1/4] Revert "wayland-egl-symbols-check: pass the DSO name via the build system"

[Daniel Stone]

Hi Emil,

On 9 March 2018 at 10:59, Emil Velikov  wrote:
> On 9 March 2018 at 09:47, Daniel Stone  wrote:
>> Patches 2-4 look fine and I'm happy to merge them with my review, but
>> could you please explain some more about this patch? I very much like
>> keeping details of the build system (specifically its internal build
>> paths) in the build system itself and not in the script. I was
>> assuming something in 2-4 needed this revert to be applied, but
>> couldn't see anything. Is there something I'm missing?
>
> There is one word to describe it - compromise:
>
>  - above all, the internal path is a 'dummy' fallback. anyone can
> provide the binary name as an argument
> $ .../wayland-egl-symbols-check .../libwayland-egl.so
>  - since we have a fallback - a plain .../wayland-egl-symbols-check
> will work most of the time

That makes sense, running it from the build root. Is that just because
'make check' is slow, or? (sanity-test is really slow.)

>  - handling env. variables (as opposed to arguments) is a pain with meson

Hm, not really. You just add an 'env' argument when declaring the test, e.g.:
egl_sym_check = find_program('wayland-egl-symbols-check')
test_egl_syms = test('egl-symbols', egl_sym_check, env: [
'WAYLAND_EGL_LIB=@0@'.format(lib_wayland_egl) ])

>  - handling arguments (as opposed to env. variable) is a pain with
> current testing scheme

Yeah, that doesn't work.

>  - the original code is shorter
>
> Hope you find at least some of those reasonable.

It's fair enough. I'm just trying to find out the balance of these:
for instance, if it's no problem to add environment variables with
Meson, do you still want to push it for reason #1, or?

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

Re: [PATCH wayland v2 1/4] Revert "wayland-egl-symbols-check: pass the DSO name via the build system"

[Emil Velikov]

On 9 March 2018 at 09:47, Daniel Stone  wrote:
> Hi Emil,
>
> On 8 March 2018 at 18:32, Emil Velikov  wrote:
>> On 28 February 2018 at 16:38, Emil Velikov  wrote:
>>> This reverts commit 85cb5ed64aa8246f4da93fc5b76dfc34096bf803.
>>>
>>> It seems like we've misread the existing code - the DSO name can be
>>> propagated via the build-system. The one available in the script was a
>>> simple fall-back.
>>
>> Humble ping on the series?
>> It should be dead trivial although do ask for details if anything is unclear.
>
> Patches 2-4 look fine and I'm happy to merge them with my review, but
> could you please explain some more about this patch? I very much like
> keeping details of the build system (specifically its internal build
> paths) in the build system itself and not in the script. I was
> assuming something in 2-4 needed this revert to be applied, but
> couldn't see anything. Is there something I'm missing?
>
There is one word to describe it - compromise:

 - above all, the internal path is a 'dummy' fallback. anyone can
provide the binary name as an argument
$ .../wayland-egl-symbols-check .../libwayland-egl.so
 - since we have a fallback - a plain .../wayland-egl-symbols-check
will work most of the time
 - handling env. variables (as opposed to arguments) is a pain with meson
 - handling arguments (as opposed to env. variable) is a pain with
current testing scheme
 - the original code is shorter

Hope you find at least some of those reasonable.

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

Re: [PATCH weston] compositor-wayland: handle wl_keyboard.enter(NULL)

[Daniel Stone]

Hi Pekka,

On 22 February 2018 at 14:54, Pekka Paalanen  wrote:
> Destroying an output (wl_surface) can race against the parent compositor
> sending wl_keyboard.enter. When this race is lost, wayland-backend
> receives wl_keyboard.enter with a NULL wl_surface for the surface it
> just destroyed.
>
> Handle this case by ignoring such enter events. Since it is
> theoretically possible to follow enter with key events, drop those too.
>
> The modifiers event is sent before enter, so we cannot drop that on the
> same condition.
>
> wl_keyboard.leave handler seems to already handle the NULL focus case,
> but there is a question if the notify_keyboard_focus_out() call should
> be avoided.
>
> This patch fixes a hard to reproduce crash. I was running weston/x11
> with two outputs, and weston/wayland --sprawl inside that, then closing
> the parent compositor windows one by one. Sometimes it would trigger
> this crash.

Thanks! This looks good to me. Whilst looking, I noticed pointer had a
similar issue, so I've just sent a patch for that now.

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

[PATCH] compositor-wayland: Ignore pointer enter on destroyed surface

[Daniel Stone]

Due to race conditions, it is (vanishingly unlikely but) possible to
receive a wl_pointer.enter event referring to a wl_surface we have just
destroyed. If this happens, wl_surface will be NULL. Detect this, clear
out our focus, and return.

Other pointer and keyboard events are robust against destroyed surfaces.

Signed-off-by: Daniel Stone 
Cc: Pekka Paalanen 
---
 libweston/compositor-wayland.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/libweston/compositor-wayland.c b/libweston/compositor-wayland.c
index 9c401d2e6..964b84e4b 100644
--- a/libweston/compositor-wayland.c
+++ b/libweston/compositor-wayland.c
@@ -1525,6 +1525,14 @@ input_handle_pointer_enter(void *data, struct wl_pointer 
*pointer,
enum theme_location location;
double x, y;
 
+   if (!surface) {
+   input->output = NULL;
+   input->has_focus = false;
+   notify_pointer_focus(>base, NULL, 0, 0);
+   input_set_cursor(input);
+   return;
+   }
+
x = wl_fixed_to_double(fixed_x);
y = wl_fixed_to_double(fixed_y);
 
-- 
2.16.2

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

Re: [PATCH weston] libweston/compositor-drm: Reset repaint scheduled status when setting DPMS level to off

[Daniel Stone]

Hi Marius,

On 7 March 2018 at 17:36, Marius Vlad  wrote:
> Otherwise when setting dpms level DPMS_ON, weston_output_schedule_repaint()
> will bail out early and never get a chance to wake up the output.
>
> Arguably this could also be done in drm_set_dpms() when setting 
> dpms_off_pending
> but I figure it better to do it when deferred.

Thanks, that's a good catch, but I do wonder how it wasn't getting
tripped up before. In previous releases, we'd call drm_set_dpms() from
anywhere, which would block on the update completing and then set the
DPMS level. I wonder if it's because we would receive a flip-done
event anyway and call weston_output_finish_frame() after the DPMS had
completed, which would drive us into repaint-idle.

I suspect we should be calling finish_frame() to match that behaviour,
to indicate that the previous update has completed, and then
synchronously applying DPMS state. Pretending that
disable/destroy/DPMS-off can be called at any time is really biting
though. :(

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