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>> 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>>> 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>>>> 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/>>
> >
> > 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.
>
>
> >
> > 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.
>
> - 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)
>
> 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?
>
> - 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;
..
>
>
> >
> > 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.