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 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.

> 
> 
>      >
>      > 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