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

[Akihiko Odaki]

On 2022/03/09 19:45, Marc-André Lureau wrote:
> Hi
>
> On Wed, Mar 9, 2022 at 2:38 PM Akihiko Odaki <akihiko.odaki@gmail.com 
> <mailto:akihiko.odaki@gmail.com>> wrote:
>
>     On 2022/03/09 19:27, Marc-André Lureau wrote:
>      > Hi
>      >
>      > On Wed, Mar 9, 2022 at 2:20 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/09 19:07, Marc-André Lureau wrote:
>      >      > Hi
>      >      >
>      >      > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki
>      >     <akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>> wrote:
>      >      >
>      >      >     On 2022/03/09 18:53, Marc-André Lureau wrote:
>      >      >      > Hi
>      >      >      >
>      >      >      > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki
>      >      >     <akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com> <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com> <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>
>      >      >      > <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>>
>      >      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>
>      >     <mailto:akihiko.odaki@gmail.com
>     <mailto:akihiko.odaki@gmail.com>>>>> wrote:
>      >      >      >
>      >      >      >     On 2022/03/09 18:26, Gerd Hoffmann wrote:
>      >      >      >      >    Hi,
>      >      >      >      >
>      >      >      >      >> dpy_gfx_switch and dpy_gfx_update need to
>     be called to
>      >      >     finish the
>      >      >      >      >> initialization or switching of the
>     non-OpenGL display.
>      >      >     However,
>      >      >      >     the proposed
>      >      >      >      >> patch only calls dpy_gfx_switch.
>      >      >      >      >>
>      >      >      >      >> vnc actually does not need dpy_gfx_update
>     because
>      >     the vnc
>      >      >      >     implementation of
>      >      >      >      >> dpy_gfx_switch implicitly does the work for
>      >      >     dpy_gfx_update, but
>      >      >      >     the model of
>      >      >      >      >> ui/console expects the two of
>     dpy_gfx_switch and
>      >      >     dpy_gfx_update
>      >      >      >     is separated
>      >      >      >      >> and only calling dpy_gfx_switch violates
>     the model.
>      >      >      >     dpy_gfx_update used to
>      >      >      >      >> be called even in such a case before and it
>     is a
>      >     regression.
>      >      >      >      >
>      >      >      >      > Well, no, the ->dpy_gfx_switch() callback is
>      >     supposed to do
>      >      >      >     everything
>      >      >      >      > needed to bring the new surface to the
>     screen.  vnc
>      >     isn't
>      >      >     alone here,
>      >      >      >      > gtk for example does the same (see gd_switch()).
>      >      >      >      >
>      >      >      >
>      >      >      >
>      >      >      > If dpy_gfx_switch() implies a full
>     dpy_gfx_update(), then we
>      >      >     would need
>      >      >      > another callback to just set the new surface. This
>     would avoid
>      >      >      > intermediary and useless switches to 2d/surface
>     when the
>      >     scanout
>      >      >     is GL.
>      >      >      >
>      >      >      > For consistency, we should also declare that
>      >     gl_scanout_texture and
>      >      >      > gl_scanout_dmabuf imply full update as well.
>      >      >      >
>      >      >      >      > Yes, typically this is roughly the same an
>     explicit
>      >      >      >     dpy_gfx_update call
>      >      >      >      > would do.  So this could be changed if it helps
>      >     making the
>      >      >     opengl
>      >      >      >     code
>      >      >      >      > paths less confusing, but that should be a
>     separate
>      >     patch
>      >      >     series and
>      >      >      >      > separate discussion.
>      >      >      >      >
>      >      >      >      > take care,
>      >      >      >      >    Gerd
>      >      >      >      >
>      >      >      >
>      >      >      >     Then ui/cocoa is probably wrong. I don't think
>     it does the
>      >      >     update when
>      >      >      >     dpy_gfx_switch is called.
>      >      >      >
>      >      >      >     Please tell me if you think dpy_gfx_switch
>     shouldn't
>      >     do the
>      >      >     implicit
>      >      >      >     update in the future. I'll write a patch to do the
>      >     update in
>      >      >     cocoa's
>      >      >      >     dpy_gfx_switch implementation otherwise.
>      >      >      >
>      >      >      >
>      >      >      > Can we ack this series first and iterate on top? It
>     solves a
>      >      >     number of
>      >      >      > issues already and is a better starting point.
>      >      >      >
>      >      >      > thanks
>      >      >      >
>      >      >      > --
>      >      >      > Marc-André Lureau
>      >      >
>      >      >     The call of dpy_gfx_update in
>      >     displaychangelistener_display_console
>      >      >     should be removed. It would simplify the patch.
>      >      >
>      >      >     Also it is still not shown that the series is a better
>      >     alternative to:
>      >      >
>      >
>     https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>     <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>
>      >   
>       <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>
>      >      >
>      >     
>       <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/> <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/ <https://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/>>>
>      >      >
>      >      >     The series "ui/dbus: Share one listener for a console" has
>      >      >     significantly
>      >      >     less code than this series and therefore needs some
>     reasoning
>      >     for that.
>      >      >
>      >      >
>      >      > At this point, your change is much larger than the
>     proposed fixes.
>      >
>      >     My change does not touch the common code except reverting and
>     minimizes
>      >     the risk of regression. It also results in the less code when
>      >     applied to
>      >     the tree.
>      >
>      >
>      > The risk of regressions is proportional to the amount of code
>     change.
>      > Your change is larger (and may be even larger when updated and
>     reviewed
>      > properly). At this point in Qemu schedule, this is a greater risk.
>
>     Possibly it can make dbus buggy, but it is not a regression as it is a
>     new feature.
>
>
> A regression is not necessarily against the last stable, but also on the 
> current master which is freezing as we speak.

In that sense, yes, my series could have more regressions. The premise 
of the less regression only applies to the difference between releases.

>      >
>      >
>      >      >
>      >      > I already discussed the rationale for the current design. To
>      >     summarize:
>      >      > - dispatching DCL in the common code allows for greater
>     reuse if an
>      >      > alternative to dbus emerges, and should help making the
>     code more
>      >     dynamic
>      >      > - the GL context split also is a separation of concerns and
>      >     should help
>      >      > for alternatives to EGL
>      >      > - dbus code only handles dbus specifics
>      >
>      >     Let me summarize my counterargument:
>      >     - The suggested reuse case is not emerged yet.
>      >
>      >
>      > It doesn't mean the design isn't superior and wanted.
>
>     It doesn't, but it does not mean the design is superior and wanted
>     either.
>
>
> But your suggestion would not help in this regard.

It doesn't, but it is not shown that allowing another display to 
dispatch the GL output to multiple listeners in the same manner as dbus 
does would help in the future.

>      >
>      >     - The GL context split is not aligned with the reality where the
>      >     display
>      >     knows the graphics accelerator where the window resides and
>     the context
>      >     should be created. The alternative to EGL can be introduced in a
>      >     similar
>      >
>      >
>      > A GL context is not necessarily associated with a window.
>
>     It still can happen. Even if it is not associated with a window, it
>     still requires some code to know that and the user must be aware of
>     that.
>
>
> That's why we have compatibility checks now (which also help in other cases)

Can you elaborate the other cases?

>      >
>      >     manner with ui/egl-context.c and ui/egl-helpers.c. If several
>     context
>      >     providers need to be supported, the selection should be
>     passed as a
>      >     parameter, just as the current code does for EGL rendernode.
>      >
>      >
>      > It's not just about where the code resides, but also about the type
>      > design. It's cleaner to separate DisplayGLCtxt from
>     DisplayChangeListener.
>
>     It would add a new failure possibility where the compatibility check
>     between DisplayGLCtx and DisplayChangeListener is flawed, which
>     happened
>     with egl-headless. Unified DisplayChangeListener is a cleaner approach
>     to describe the compatibility.
>
>
> Care to describe the flaw in detail?

egl-headless requires to be compatible with any displays without GL 
handlers, but that case was not considered and required a patch.

Regards,
Akihiko Odaki

>      >
>      >     - implementing the dispatching would allow dbus to share more
>     things
>      >     like e.g. textures converted from DisplaySurface and
>     GunixFDList for
>      >     DMA-BUF. They are not present in all displays and some are
>     completely
>      >     specific to dbus.
>      >
>      >
>      > And the dbus specific code is within dbus modules.
>
>     The code to share e.g. GunixFDList are not yet.
>
>
>   ~/src/qemu   master  git grep UnixFD
> audio/dbusaudio.c:                             GUnixFDList *fd_list,
> audio/dbusaudio.c:                                 GUnixFDList *fd_list,
> audio/dbusaudio.c:                                GUnixFDList *fd_list,
> tests/qtest/dbus-display-test.c:    g_autoptr(GUnixFDList) fd_list = NULL;
> ui/dbus-chardev.c:    GUnixFDList *fd_list,
> ui/dbus-console.c:                               GUnixFDList *fd_list,
> ui/dbus-listener.c:    g_autoptr(GUnixFDList) fd_list = NULL; > ..

I meant "shared code" but "code to share GunixFDList". GunixFDLists are 
created for each listeners and not shared.

Regards,
Akihiko Odaki

>      >
>      >
>      >      >
>      >      > My understanding of your proposal is that you would rather see
>      >     all this
>      >      > done within the dbus code. I disagree for the reasons above. I
>      >     may be
>      >      > proven wrong, but so far, this works as expected minor the
>      >     left-over and
>      >      > regressions you pointed out that should be fixed. Going
>     back to a
>      >      > different design should be done in a next release if
>     sufficiently
>      >     motivated.
>      >
>      >     Reverting the dbus change is the safest option if it does not
>     settle.
>      >
>      >
>      > We have a different sense of safety.
>
>     Can you elaborate?
>
>
> See above.
>
> Sorry, I'll slow down my reply, as I think we have made enough rumble 
> and not much progress.
>
> thanks again for helping so far
>
>
> --
> Marc-André Lureau