On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau <marcandre.lur...@gmail.com> wrote: > > Hi Akihiko > > On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.od...@gmail.com> wrote: >> >> ui/dbus required to have multiple DisplayChangeListeners (possibly with >> OpenGL) >> for a console but that caused several problems: >> - It broke egl-headless, an unusual display which implements OpenGL rendering >> for non-OpenGL displays. The code to support multiple >> DisplayChangeListeners >> does not consider the case where non-OpenGL displays listens OpenGL >> consoles. > > > Can you provide instructions on what broke? Even better write a test, please.
The following command segfaults. Adding a test would be nice, but it would need a binary which uses OpenGL. qemu-system-x86_64 -device virtio-gpu-gl-pci -display egl-headless -vnc :0 -m 8G -cdrom Fedora-Workstation-Live-x86_64-34-1.2.iso -accel kvm > > "make check-avocado AVOCADO_TESTS=tests/avocado/virtio-gpu.py", which covers > egl-headless usage, works. > >> >> - Multiple OpenGL DisplayChangeListeners of dbus shares one DisplaySurface >> and >> modifies its texture field, causing OpenGL texture leak and use-after-free. > > > Again, please provide instructions. I have regularly run -display dbus with > multiple clients and qemu compiled with sanitizers. I don't see any leak or > use after free. I doubt sanitizers can find this because it is an OpenGL texture. You may add a probe around surface_gl_create_texture and surface_gl_destroy_texture. > >> >> - Introduced extra code to check the compatibility of OpenGL context >> providers >> and OpenGL texture renderers where those are often inherently tightly >> coupled >> since they must share the same hardware. > > > So code checks are meant to prevent misusage. They might be too limited or > broken in some ways, but reverting is likely going to introduce other > regressions I was trying to fix. The misuse will not occur because DisplayChangeListeners will be merged with OpenGL context providers. > >> - Needed extra work to broadcast the same change to multiple dbus listeners. >> > > Compared to what? Compared to sharing one DisplayChangeListener for multiple dbus listeners. > >> >> This series solve them by implementing the change broadcast in ui/dbus, which >> knows exactly what is needed. Changes for the common code to support multiple >> OpenGL DisplayChangeListeners were reverted to fix egl-headless and reduce >> the code size. > > > Thanks a lot for your work, I am going to take a look at your approach. But > please help us understand better what the problem actually is, by giving > examples & tests to avoid future regressions and document the expected > behaviour. The thing is really complicated and I may miss details so please feel free to ask questions or provide suggestions. Regards, Akihiko Odaki > >> >> Akihiko Odaki (6): >> ui/dbus: Share one listener for a console >> Revert "console: save current scanout details" >> Revert "ui: split the GL context in a different object" >> Revert "ui: dispatch GL events to all listeners" >> Revert "ui: associate GL context outside of display listener >> registration" >> Revert "ui: factor out qemu_console_set_display_gl_ctx()" >> >> include/ui/console.h | 60 +----- >> include/ui/egl-context.h | 6 +- >> include/ui/gtk.h | 11 +- >> include/ui/sdl2.h | 7 +- >> include/ui/spice-display.h | 1 - >> ui/console.c | 258 +++++++---------------- >> ui/dbus-console.c | 109 ++-------- >> ui/dbus-listener.c | 417 +++++++++++++++++++++++++++---------- >> ui/dbus.c | 22 -- >> ui/dbus.h | 36 +++- >> ui/egl-context.c | 6 +- >> ui/egl-headless.c | 20 +- >> ui/gtk-egl.c | 10 +- >> ui/gtk-gl-area.c | 8 +- >> ui/gtk.c | 25 +-- >> ui/sdl2-gl.c | 10 +- >> ui/sdl2.c | 14 +- >> ui/spice-display.c | 19 +- >> 18 files changed, 498 insertions(+), 541 deletions(-) >> >> -- >> 2.32.0 (Apple Git-132) >> >> > > > -- > Marc-André Lureau
