On Wed Jun 7 20:54:07 2023 +0000, Rémi Bernon wrote:
What about something like that?
static pthread_mutex_t xdg_data_mutex = PTHREAD_MUTEX_INITIALIZER; static struct wayland_surface *wayland_surface_lock_xdg(struct xdg_surface *xdg_surface) { struct wayland_surface *surface; pthread_mutex_lock(&xdg_data_mutex); surface = xdg_surface_get_user_data(xdg_surface); if (surface) pthread_mutex_lock(&surface->mutex); pthread_mutex_unlock(&xdg_data_mutex); return surface; } static void xdg_surface_handle_configure(void *data, struct xdg_surface *xdg_surface, uint32_t serial) { struct wayland_surface *surface; BOOL initial_configure; TRACE("serial=%u\n", serial); if (!(surface = wayland_surface_lock_xdg(xdg_surface))) return; initial_configure = surface->pending_serial == 0 && surface->current_serial == 0; surface->pending_serial = serial; if (initial_configure) pthread_cond_signal(&surface->configured_cond); pthread_mutex_unlock(&surface->mutex); } /* [...] */ void wayland_surface_destroy(struct wayland_surface *surface) { pthread_mutex_lock(&xdg_data_mutex); pthread_mutex_lock(&surface->mutex); if (surface->xdg_toplevel) { xdg_toplevel_destroy(surface->xdg_toplevel); surface->xdg_toplevel = NULL; } if (surface->xdg_surface) { xdg_surface_set_user_data(surface->xdg_surface, NULL); xdg_surface_destroy(surface->xdg_surface); surface->xdg_surface = NULL; } if (surface->wl_surface) { wl_surface_destroy(surface->wl_surface); surface->wl_surface = NULL; } pthread_mutex_unlock(&surface->mutex); pthread_mutex_unlock(&xdg_data_mutex); wl_display_flush(process_wayland.wl_display); pthread_cond_destroy(&surface->configured_cond); pthread_mutex_destroy(&surface->mutex); free(surface); } /* And something similar in wayland_surface_clear_role */I can also imagine something more generic, with refcounted objects for anything that is used as wl_proxy_user_data, and references held as long as they are in user_data, or used in a callback, instead of this double locking which is slightly ugly. This approach would also avoid unnecessary synchronization between the dispatcher and threads destroying objects. And like I mentioned above, I believe thread synchronization is best avoided in the drivers.
Thanks @rbernon, I experimented with what you propose and I think it is a good alternative. I am not sure I would consider it simpler than the current proposal (I guess it's complicated in a different way, the implementation details and locking requirements leak into the affected code a bit more), but since it makes trade-offs that seem to be more appropriate for the driver (no thread sync etc), I am happy to go with it in the next iteration of this MR.
I also think (but need to ponder on a bit more) we can avoid changing `wayland_surface_clear_role` at all if we keep the `if (surface->xdg_surface == xdg_surface)` in `xdg_surface_handle_configure` (like we are doing in the current approach). I prefer this compared to introducing the extra xdg user data locking in `wayland_surface_clear_role`, if possible.
I haven't experimented with the refcount approach, but I find that the double locking is contained enough to not be a problem, for now at least. If we get more cases that can benefit from a more general refcount based pattern, we can revisit (unless you feel strongly about exploring this more now).