On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau <marcandre.lur...@gmail.com> wrote: > > Hi > > On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.od...@gmail.com> wrote: >> >> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau >> <marcandre.lur...@gmail.com> wrote: >> > >> > Hi >> > >> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.od...@gmail.com> >> > wrote: >> >> >> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lur...@redhat.com> wrote: >> >> > >> >> > From: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> > >> >> > Hi, >> >> > >> >> > In the thread "[PATCH 0/6] ui/dbus: Share one listener for a console", >> >> > Akihiko >> >> > Odaki reported a number of issues with the GL and D-Bus display. His >> >> > series >> >> > propose a different design, and reverting some of my previous generic >> >> > console >> >> > changes to fix those issues. >> >> > >> >> > However, as I work through the issue so far, they can be solved by >> >> > reasonable >> >> > simple fixes while keeping the console changes generic (not specific to >> >> > the >> >> > D-Bus display backend). I belive a shared infrastructure is more >> >> > beneficial long >> >> > term than having GL-specific code in the DBus code (in particular, the >> >> > egl-headless & VNC combination could potentially use it) >> >> > >> >> > Thanks a lot Akihiko for reporting the issues proposing a different >> >> > approach! >> >> > Please test this alternative series and let me know of any further >> >> > issues. My >> >> > understanding is that you are mainly concerned with the Cocoa backend, >> >> > and I >> >> > don't have a way to test it, so please check it. If necessary, we may >> >> > well have >> >> > to revert my earlier changes and go your way, eventually. >> >> > >> >> > Marc-André Lureau (12): >> >> > ui/console: fix crash when using gl context with non-gl listeners >> >> > ui/console: fix texture leak when calling surface_gl_create_texture() >> >> > ui: do not create a surface when resizing a GL scanout >> >> > ui/console: move check for compatible GL context >> >> > ui/console: move dcl compatiblity check to a callback >> >> > ui/console: egl-headless is compatible with non-gl listeners >> >> > ui/dbus: associate the DBusDisplayConsole listener with the given >> >> > console >> >> > ui/console: move console compatibility check to dcl_display_console() >> >> > ui/shader: fix potential leak of shader on error >> >> > ui/shader: free associated programs >> >> > ui/console: add a dpy_gfx_switch callback helper >> >> > ui/dbus: fix texture sharing >> >> > >> >> > include/ui/console.h | 19 ++++--- >> >> > ui/dbus.h | 3 ++ >> >> > ui/console-gl.c | 4 ++ >> >> > ui/console.c | 119 ++++++++++++++++++++++++++----------------- >> >> > ui/dbus-console.c | 27 +++++----- >> >> > ui/dbus-listener.c | 11 ---- >> >> > ui/dbus.c | 33 +++++++++++- >> >> > ui/egl-headless.c | 17 ++++++- >> >> > ui/gtk.c | 18 ++++++- >> >> > ui/sdl2.c | 9 +++- >> >> > ui/shader.c | 9 +++- >> >> > ui/spice-display.c | 9 +++- >> >> > 12 files changed, 192 insertions(+), 86 deletions(-) >> >> > >> >> > -- >> >> > 2.34.1.428.gdcc0cd074f0c >> >> > >> >> > >> >> >> >> You missed only one thing: >> >> >- that console_select and register_displaychangelistener may not call >> >> > dpy_gfx_switch and call dpy_gl_scanout_texture instead. It is >> >> > incompatible with non-OpenGL displays >> >> >> >> displaychangelistener_display_console always has to call >> >> dpy_gfx_switch for non-OpenGL displays, but it still doesn't. >> > >> > >> > Ok, would that be what you have in mind? >> > >> > --- a/ui/console.c >> > +++ b/ui/console.c >> > @@ -1122,6 +1122,10 @@ static void >> > displaychangelistener_display_console(DisplayChangeListener *dcl, >> > } else if (con->scanout.kind == SCANOUT_SURFACE) { >> > dpy_gfx_create_texture(con, con->surface); >> > displaychangelistener_gfx_switch(dcl, con->surface); >> > + } else { >> > + /* use the fallback surface, egl-headless keeps it updated */ >> > + assert(con->surface); >> > + displaychangelistener_gfx_switch(dcl, con->surface); >> > } >> >> It should call displaychangelistener_gfx_switch even when e.g. >> con->scanout.kind == SCANOUT_TEXTURE. egl-headless renders the content >> to the last DisplaySurface it received while con->scanout.kind == >> SCANOUT_TEXTURE. > > > I see, egl-headless is really not a "listener". > >> >> > >> > I wish such egl-headless specific code would be there, but we would need >> > more refactoring. >> > >> > I think I would rather have a backend split for GL context, like "-object >> > egl-context". egl-headless-specific copy code would be handled by >> > common/util code for anything that wants a pixman surface (VNC, screen >> > capture, non-GL display etc). >> > >> > This split would allow sharing the context code, and introduce other >> > system specific GL initialization, such as WGL etc. Right now, I doubt the >> > EGL code works on anything but Linux. >> >> Sharing the context code is unlikely to happen. Usually the toolkit >> (GTK, SDL, or Cocoa in my fork) knows what graphics accelerator should >> be used and how the context should be created for a particular window. >> The context sharing can be achieved only for headless displays, namely >> dbus, egl-headless and spice. Few people would want to use them in >> combination. > > > Ok for toolkits, they usually have their own context. But ideally, qemu > should be "headless". And the GL contexts should be working on other systems > than EGL Linux. > > Any of the spice, vnc, dbus display etc may legitimately be fixed to work > with WGL etc. Doing this repeatedly on the various display backends would be > bad design.
We already have ui/egl-context.c to share the code for EGL. We can have ui/headless-context.c or something which creates a context for headless but the implementation can be anything proper there. It doesn't require modifying ui/console.c or adding something like "-object egl-context". > > Although my idea is that display servers (spice, vnc, rdp, etc) & various UI > (gtk, cocoa, sdl, etc) should be outside of qemu. The display would use IPC, > based on DBus if it fits the job, or something else if necessary. Obviously, > there is still a lot of work to do to improve surface & texture sharing and > portability, but that should be possible... Maybe we can rework the present UIs of QEMU to make them compatible with both in-process communication and D-Bus inter-process communication. If the user has a requirement incompatible with IPC (e.g. OpenGL on macOS), the user can opt for in-process communication. D-Bus would be used otherwise. (Of course that would require substantial effort.) > >> >> >> > >> >> >> >> Anything else should be addressed with this patch series. (And it also >> >> has nice fixes for shader leaks.) >> > >> > >> > thanks >> > >> >> >> >> >> >> cocoa doesn't have OpenGL output and egl-headless, the cause of many >> >> pains addressed here, does not work on macOS so you need little >> >> attention. I have an out-of-tree OpenGL support for cocoa but it is >> >> out-of-tree anyway and I can fix it anytime. >> > >> > >> > Great! >> > >> > btw, I suppose you checked your DBus changes against the WIP >> > "qemu-display" project. What was your experience? I don't think many >> > people have tried it yet. Do you think this could be made to work on >> > macOS? at least the non-dmabuf support should work, as long as Gtk4 has >> > good macOS support. I don't know if dmabuf or similar exist there, any >> > idea? >> >> I tested it on Fedora. I think it would probably work on macOS but >> maybe require some tweaks. IOSurface is a counterpart of DMA-BUF in >> macOS but its situation is bad; it must be delivered via macOS's own >> IPC mechanisms (Mach port and XPC), but they are for server-client >> model and not for P2P. fileport mechanism allows to convert Mach port >> to file descriptor, but it is not documented. (In reality, I think all >> of the major browsers, Chromium, Firefox and Safari use fileport for >> this purpose. Apple should really document it if they use it for their >> app.) It is also possible to share IOSurface with a global number, but >> it can be brute-forced and is insecure. >> > > Thanks, the Gtk developers might have some clue. They have been working on > improving macOS support, and it can use opengl now > (https://blogs.gnome.org/chergert/2020/12/15/gtk-4-got-a-new-macos-backend-now-with-opengl/). They don't need IPC for passing textures so that is a different story. > >> >> Regards, >> Akihiko Odaki >> >> > >> > >> > -- >> > Marc-André Lureau > > > > -- > Marc-André Lureau