[PATCH v4 0/24] MR6323: Draft: winewayland: Support GL/VK rendering in child windows (alt).

[Rémi Bernon (＠rbernon)]

--
  v4: winewayland: Use subsurfaces for unmanaged windows.
      winewayland: Introduce a new update_wayland_surface_state_toplevel helper.
      winewayland: Introduce a new wayland_surface_reconfigure_xdg helper.
      winewayland: Introduce a new wayland_surface role enumeration.
      winewayland: Call wayland_surface_clear_role in wayland_surface_destroy.
      winewayland: Move surface title change to wayland_surface_make_toplevel.
      winewayland: Attach client client surfaces to their toplevel surface.
      winewayland: Use WindowPosChanged swp_flags to decide if a window is visible.
      winewayland: Use the wayland_win_data client rect for child GL windows.
      winewayland: Split wayland_win_data_update_wayland_surface helper.
      winewayland: Call ensure_window_surface_contents with the toplevel window.
      win32u: Update children window state when the parent state changes.
      winewayland: Get rid of window_surface reference from wayland_win_data.
      winewayland: Move client surface to wayland_win_data struct.
      winewayland: Get rid of the window surface individual locks.
      winewayland: Introduce a new wayland_client_surface_create helper.
      winewayland: Get rid of wayland_surface reference from window_surface.
      winewayland: Move window contents buffer to wayland_win_data struct.
      winewayland: Reset the buffer damage region immediately after copy.
      winewayland: Introduce a new get_window_surface_contents helper.
      winewayland: Introduce a new set_window_surface_contents helper.
      winewayland: Introduce a new ensure_window_surface_contents helper.
      winewayland: Post WM_WAYLAND_CONFIGURE outside of the surface lock.
      winewayland: Store all window rects in wayland_win_data.

This merge request has too many patches to be relayed via email.
Please visit the URL below to see the contents of the merge request.
https://gitlab.winehq.org/wine/wine/-/merge_requests/6323

[Rémi Bernon (＠rbernon)]

Another couple of tweaks because some optimizations wrt. skipping wayland_surface_clear_role were wrong.

[Alexandros Frantzis (＠afrantzis)]

Thanks, the overall approach looks good (note: I haven't done a detailed review). I like both this and !6107 in different ways, so if this is the direction you prefer, I am happy to move forward with it.

I have pushed some potential improvements to https://gitlab.winehq.org/afrantzis/wine/-/commits/wip/wayland-child-surface... (rebased on latest version).

Relatedly, I also cleaned up some changes to glmark2, which I have been using to manually test some more exotic nested/reparenting scenarios, and pushed them to https://github.com/glmark2/glmark2/tree/win32-nested (please read https://github.com/glmark2/glmark2/blob/win32-nested/README.win32-nested), in case it's useful for your testing.

I have found that on some compositors (e.g. kwin) this MR leads to issues with some of the glmark2 scenarios (whereas !6107 works fine), but I think it's actually the compositor's fault not dealing well with some of the subsurface detachment/attachments in this MR. Experimenting a bit, I found that if we ensure that the client surface does not have an attached buffer before creating a new wl_subsurface for it, then the issues go away, but this shouldn't be needed (Weston works fine for example). FWIW, last time I tried mutter it also didn't behave well with more complex subsurfaces scenarios, so I haven't been using it much for testing these MRs.

[Alexandros Frantzis (＠afrantzis)]

Alexandros Frantzis (@afrantzis) commented about dlls/winewayland.drv/wayland_surface.c:
> - int local_x, local_y, x, y;
> -
> - if (surface->processing.serial && surface->processing.processed &&
> -
>    (toplevel_data = wayland_win_data_get_nolock(surface->toplevel_hwnd)) &&
> -
>    (toplevel_surface = toplevel_data->wayland_surface))
> - {
> -
>    local_x = surface->window.rect.left - toplevel_surface->window.rect.left;
> -
>    local_y = surface->window.rect.top - toplevel_surface->window.rect.top;
> -
> -
>    wayland_surface_coords_from_window(surface, local_x, local_y, &x, &y);
> -
> -
>    TRACE("hwnd=%p pos=%d,%d\n", surface->hwnd, x, y);
> -
> -
>    wl_subsurface_set_position(surface->wl_subsurface, x, y);
> -
>    if (toplevel_data->client_surface)
> -
>        wl_subsurface_place_above(surface->wl_subsurface, toplevel_data->client_surface->wl_surface);

Ideally we should do the same for (child) `client_surface`s themselves, since they could also be obscured by their toplevel parent's `client_surface`.

[Rémi Bernon (＠rbernon)]

On Thu Aug 22 16:28:24 2024 +0000, Alexandros Frantzis wrote:
> Ideally we should do the same for (child) `client_surface`s themselves,
> since they could also be obscured by their toplevel parent's `client_surface`.

Sure, but TBH I think we will have compositing issues with every driver when accelerated content is used both on toplevel and child -or overlapping siblings- windows and it will be better solved with a general solution, ie: a Wine-side compositor for cases where it is needed.

[Rémi Bernon (＠rbernon)]

> Experimenting a bit, I found that if we ensure that the client surface does not have an attached buffer before creating a new wl_subsurface for it, then the issues go away, but this shouldn't be needed (Weston works fine for example). FWIW, last time I tried mutter it also didn't behave well with more complex subsurfaces scenarios, so I haven't been using it much for testing these MRs.

Maybe it's a matter of avoiding unnecesary client window attach/detach operations, I didn't try to be very careful about that. Or can we also maybe detach buffers from the surface ourselves?
> I am thinking that a possible approach here is to avoid all commits on `client->wl_surface` (i.e., in reconfigure_client) and let all changes be applied when EGL/VK performs the next commit itself.

That sounds completely reasonable to me.

[Alexandros Frantzis (＠afrantzis)]

> Maybe it's a matter of avoiding unnecesary client window attach/detach operations, I didn't try to be very careful about that.

I guess it could help, but even if it does, it would just be a workaround for (what seems to me to be) a compositor issue. That being said, we may still want to do reduce attach/detach operations for efficiency rather than correctness reasons. In any case, I will try to chase this with upstream kwin.
> Or can we also maybe detach buffers from the surface ourselves?

This is what I did to test my hypothesis for kwin, but I wouldn't want to have it there normally, because, like the `client->wl_surface` commit that we want to avoid, it can also lead to interleaving problems with GL/VK. If the worst case for this was just a transient visual glitch, perhaps it would be acceptable, but there are in fact scenarios where the compositor would disconnect us with a protocol error (see https://gitlab.freedesktop.org/mesa/mesa/-/blob/421c42170e1abda849ea19c07f61... and 
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/blob/d1d185c61f68...).

[Rémi Bernon (＠rbernon)]

Should I rebase this? Otherwise please feel free to update https://gitlab.winehq.org/wine/wine/-/merge_requests/6107 if you have a better version that addresses the issues you mentioned.

[Alexandros Frantzis (＠afrantzis)]

On Thu Sep  5 12:40:07 2024 +0000, Rémi Bernon wrote:
> Should I rebase this? Otherwise please feel free to update
> https://gitlab.winehq.org/wine/wine/-/merge_requests/6107 if you have a
> better version that addresses the issues you mentioned.

Please rebase, since it's my understanding that this is your preferred approach and I am also fine with it. Concerning the issues:
* Avoiding all explicit commits on client->wl_surface should be good enough to not trigger bad interactions between GL/VK and other threads.
* I still think the kwin behavior is a compositor problem, although I haven't yet created a minimum reproducer to move forward with a bug report and see what upstream says about this.

[Rémi Bernon (＠rbernon)]

> Concerning the child. accel design proposed, I am not sure if I find it less complex, but rather differently complex. !6107 centralizes the complexity in the `wayland_surface` update logic, but then other parts of the code can just use it without caring about most of the details. This MR has simpler parts, but then spreads out the complexity, since other parts need to care about `wayland_surface` vs `wayland_client_surface` etc. I am sure this perception about complexity is affected by my mind being used to the other design, though, and there are certainly benefits to this MR (e.g., avoiding an intermediate `wl_surface` when doing child accel. rendering).

Fwiw, the rationale behind keeping client surfaces better separated from window surfaces, is that I believe it will make it easier to move move more code into win32u. Right now the driver responsibility is fairly large, and they can manage and organize their surfaces pretty much the way they like. I think it makes it however difficult to factor things together between drivers. Other drivers have this same window vs client surface model, I think it's good to keep it the same.

What I ambition then, is to move as much of the "client surface" (GL/VK) code as possible to win32u. This is already almost the case for Vulkan, a bit less for GL. Then, win32u would also be in charge of managing these client surfaces the way it likes, compositing them or sharing them with other processes in a common way.

For the "window surface", I would like to find a way to split it a bit more than it is currently. We could have pure windowing on one side, which would be only about positioning/organizing window frames. And the GDI drawing logic on another side. Ideally the latter would be using the same primitives as the "client surface" code, and use similar surfaces that could be presented directly or fed into our compositor, to be later presented onto the window frames we have set up.

It's actually quite similar to the wayland protocol BTW, with surfaces and buffers, but would be done in win32u, with our primitives, and for all the drivers.