Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL

[Akihiko Odaki <akihiko.odaki@gmail.com>]

On 2022/03/09 17:11, Marc-André Lureau wrote:
> Hi
> 
> On Wed, Mar 9, 2022 at 12:05 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
> 
>     On 2022/03/09 17:02, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Tue, Mar 8, 2022 at 6:43 PM Akihiko Odaki
>     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>> wrote:
>      >
>      >     On 2022/03/08 23:26, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Mon, Mar 7, 2022 at 4:49 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>>
>     wrote:
>      >      >>
>      >      >
>      >      > (taking notes, mostly for myself)
>      >      >
>      >      >>> Could you provide a failing test case or a more concrete
>      >     suggestion on
>      >      >>> what to do instead? I am all ears :)
>      >      >>>
>      >      >>> thanks
>      >      >>>
>      >      >>
>      >      >> Let me describe a more concrete case. Think that egl-headless
>      >     and vnc
>      >      >> are enabled. The guest devices are serial virtio-gpu-gl.
>     vnc selects
>      >      >> serial at first.
>      >      >
>      >      > (I am not sure how you would select serial first, but let's
>      >     assume you did)
>      >      >
>      >      >>
>      >      >> vnc uses register_displaychangelistener and
>     console_select to switch
>      >      >> consoles.
>      >      >>
>      >      >> displaychangelistener_display_console does the actual
>     switching, and
>      >      >> calls dpy_gfx_switch and dpy_gfx_update if
>     con->scanout.kind is
>      >      >> SCANOUT_SURFACE. It calls dpy_gl_scanout_texture or
>      >      >> dpy_gl_scanout_dmabuf if con->scanout.kind is
>     SCANOUT_TEXTURE or
>      >      >> SCANOUT_DMABUF. It works for a OpenGL display, but it
>     doesn't vnc in
>      >      >> combination with egl-headless.
>      >      >
>      >      > (hmm, what doesn't work? With this patch, the
>     DisplaySurface is
>      >     always
>      >      > switched, no matter what con->scanout.kind is)
>      >      >
>      >      >>
>      >      >> virtio-gpu-gl starts scanning out texture. It would
>     happen in the
>      >      >> following sequence:
>      >      >> 1. virtio-gpu-gl calls qemu_console_resize. con->scanout.kind
>      >     will be
>      >      >> SCANOUT_SURFACE.
>      >      >> 2. qemu_console_resize calls dpy_gfx_switch, delivering
>      >     DisplaySurface
>      >      >> to egl-headless. con->scanout.kind will be SCANOUT_TEXTURE.
>      >      >
>      >      > (qemu_console_resize calls dpy_gfx_replace_surface.
>     con.scanout.kind
>      >      > is still SCANOUT_SURFACE)
>      >      >
>      >      > (It calls displaychangelistener_gfx_switch() which will call
>      >      > egl_gfx_switch() and update the current surface)
>      >      >
>      >      >> 3. virtio-gpu-gl calls dpy_gl_scanout_texture.
>     egl-headless starts
>      >      >> rendering the texture to the DisplaySurface.
>      >      >
>      >      > (now con.scanout.kind becomes SCANOUT_TEXTURE)
>      >      >
>      >      >>
>      >      >> In the end, the DisplaySurface will have the image
>     rendered, and
>      >      >> con->scanout.kind will be SCANOUT_TEXTURE.
>      >      >
>      >      > (so far it works as expected, right?)
>      >      >
>      >      >
>      >      >
>      >      >>
>      >      >> Now vnc switches to virtio-gpu-gl.
>      >      >>
>      >      >> 4. vnc calls console_select
>      >      >> 5. displaychangelistener_display_console finds
>     con->scanout.kind is
>      >      >> SCANOUT_TEXTURE so it tries to scanout texture, but vnc
>     is not
>      >     an OpenGL
>      >      >> display. It essentially becomes no-op and the display
>     would be
>      >     blank.
>      >      >>
>      >      >> This patch will change it to call dpy_gfx_switch but not
>     to call
>      >      >> dpy_gfx_update.
>      >      >
>      >      > Alright, I think I see what you mean. egl-headless is
>     associated with
>      >      > a specific con, and thus is not involved during vnc console
>      >     switching.
>      >      >
>      >      > However, dpy_gfx_switch on vnc is not a no-op. It updates the
>      >     surface,
>      >      > resize the client and mark the area dirty. Because the
>     con.surface is
>      >      > kept updated by egl-headless, the client gets the updated
>     content.
>      >      > Iow, it still works.
>      >      >
>      >      > If we always called dpy_gfx_update() during
>      >      > displaychangelistener_display_console(), it would mean for the
>      >      > listener to display the surface content, even when the scanout
>      >     kind is
>      >      > set for a texture. Or we need to change the behaviour of
>      >      > dpy_gfx_update() to depend on the current scanout kind.
>      >
>      >     vnc has to display the surface content so dpy_gfx_update
>     should be
>      >     called for vnc, but not for OpenGL displays.
>      >
>      >
>      > VNC already marks the whole surface as dirty during
>     vnc_dpy_switch().
>      > This is like calling dpy_gfx_update().
> 
>     Then why does it call dpy_gfx_update for SCANOUT_SURFACE first
>     place? It
>     shouldn't call the function if it is not required to display the
>     surface
>     content.
> 
> 
> This is a pre-existing discrepancy. vnc handles switch with an implicit 
> update. Imho, we need to improve the code so that updates/display become 
> explicit.
> 
> But this is extra, not necessary to fix the regressions you pointed out. 
> I'd rather not mix concerns, and proceed step by step.
> 
> -- 
> Marc-André Lureau

If it is expected that dpy_gfx_update is required, it should call 
dpy_gfx_update. I agree it is not a right timing to fix vnc to remove 
the implicit update as it is pre-existing.
However the lack of dpy_gfx_update call is a regression and should be fixed.

Regards,
Akihiko Odaki