Re: [PATCH v2 00/12] GL & D-Bus display related fixes

[Akihiko Odaki]

On Fri, Feb 18, 2022 at 2:07 AM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi
>
> On Thu, Feb 17, 2022 at 8:39 PM Akihiko Odaki <akihiko.odaki@gmail.com> wrote:
>>
>> On Fri, Feb 18, 2022 at 1:12 AM Marc-André Lureau
>> <marcandre.lureau@gmail.com> wrote:
>> >
>> > Hi
>> >
>> > On Thu, Feb 17, 2022 at 5:09 PM Akihiko Odaki <akihiko.odaki@gmail.com> 
>> > wrote:
>> >>
>> >> On Thu, Feb 17, 2022 at 8:58 PM <marcandre.lureau@redhat.com> wrote:
>> >> >
>> >> > From: Marc-André Lureau <marcandre.lureau@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