On Mon, Feb 14, 2022 at 9:07 PM Marc-André Lureau
<marcandre.lureau@gmail.com> wrote:
>
> Hi Akihiko
>
> On Sun, Feb 13, 2022 at 6:44 AM Akihiko Odaki <akihiko.odaki@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
Thanks!
This is clearly a mistake from commit 7cc712e98 ("ui: dispatch GL events to all listener").
It should have taken into account that some listeners do not have GL callbacks, and guard the call.
We should wrap the missing ops->dpy_gl_call() with a if (ops->dpy_gl_call) ? I'll send a patch for that.
>
> "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.
Indeed, a surface is created on each frame, because we create a 2d surface on each qemu_console_resize(), which is called at each virgl scanout. This is a regression introduced by commit ebced09185 ("console: save current scanout details"). I can propose an easy fix, please check it.
And the root of the leak is actually surface_gl_create_texutre(), it should be idempotent, just like destroy().
>
>>
>> - 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.
Ok, but aren't the checks enough to prevent it already? I have to check the use cases and differences of design, but you might be right that we don't need such a split after all.
>
>> - Needed extra work to broadcast the same change to multiple dbus listeners.
>>
>
> Compared to what?
Compared to sharing one DisplayChangeListener for multiple dbus listeners.
Well, you just moved the problem to the dbus display, not removed any work.
What we have currently is more generic, you should be able to add/remove various listeners (in theory, we only really do it for dbus at this point).
>
>>
>> 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.
Reverting changes and proposing an alternative implementation requires detailed explanation and convincing arguments. It may take a while, but we will try to get through the problems and evaluate the alternative designs. Thanks again for your help!
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