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.
>
> 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.
- 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.
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.
- 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.
>
> 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?
Regards,
Akihiko Odaki