Hi On Wed, Mar 9, 2022 at 2:38 PM Akihiko Odaki <akihiko.od...@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.od...@gmail.com > > <mailto:akihiko.od...@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.od...@gmail.com <mailto:akihiko.od...@gmail.com> > > > <mailto:akihiko.od...@gmail.com > > <mailto:akihiko.od...@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.od...@gmail.com <mailto:akihiko.od...@gmail.com> > > <mailto:akihiko.od...@gmail.com <mailto:akihiko.od...@gmail.com>> > > > > <mailto:akihiko.od...@gmail.com > > <mailto:akihiko.od...@gmail.com> > > > <mailto:akihiko.od...@gmail.com > > <mailto:akihiko.od...@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.od...@gmail.com/ > > < > https://patchew.org/QEMU/20220213024222.3548-1-akihiko.od...@gmail.com/> > > > > > < > https://patchew.org/QEMU/20220213024222.3548-1-akihiko.od...@gmail.com/ < > https://patchew.org/QEMU/20220213024222.3548-1-akihiko.od...@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. thanks again for helping so far -- Marc-André Lureau
