From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3C63EC433F5 for ; Wed, 9 Mar 2022 10:29:14 +0000 (UTC) Received: from localhost ([::1]:59844 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1nRtZA-0002Hw-UD for qemu-devel@archiver.kernel.org; Wed, 09 Mar 2022 05:29:12 -0500 Received: from eggs.gnu.org ([209.51.188.92]:60074) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1nRtYF-0001SE-5U for qemu-devel@nongnu.org; Wed, 09 Mar 2022 05:28:15 -0500 Received: from [2a00:1450:4864:20::430] (port=40880 helo=mail-wr1-x430.google.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1nRtYC-0007MS-S4 for qemu-devel@nongnu.org; Wed, 09 Mar 2022 05:28:14 -0500 Received: by mail-wr1-x430.google.com with SMTP id k24so2257719wrd.7 for ; Wed, 09 Mar 2022 02:28:12 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=+zhBED1olJtg50Jlh48WTrNishHXIOAOzNFIiFIrDTI=; b=dJeWCCNyt3zs6bG6ApfsH3I0OJsch8KUcyXjvpkIl5UzEB73BtAdPMkkkRO2kMucnP TMCSDHzBeoGGY6XiPxJYjf5uGQeP++nTo3c8ApCZV0fspVSiwYuXiWqVDcQVV/+nRoL4 q+CgP3ANJ4yRyi93PWNnXsRxELJUf2tUNStwbdZQSeFwR63pFN9en4m4h2kvYf3vrArE b1XokAqAtE0QEGKVNpzpvp50lnjT4zdmZHnQhucwa+hOXzkG7FoNN/jhfQyCjN8Zr5T9 EjVyQnyHb+sIYgdAWq3zUS9oEMRNZWJWXgwQhwRt8ZX6SxffK6xYvuhj0N9lA7WkdZpK OgEg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=+zhBED1olJtg50Jlh48WTrNishHXIOAOzNFIiFIrDTI=; b=zDbkAbQzelnb3lF6/YHeI3NGayC+pGjNu7pa9gl8osV5+jf7sSo2z3ZRxZ6zovtxd7 lEqiWlccDAKNMucVC9hfyZryU7+Yja8CiqYd31kcLmlNP1Jxom8AMiA6PHAEMnxtLIRD RAMlY7l4LOZATETnZKfPfdWbJ9/Cr6R5G/vrNbXDi10RroQf6Z3Zb++zr70YhFylt548 82DmrlRE2o2a+3Go9SZ1Xtif7tw+V+kzrtyW8ILzWtvZ0qyjzuU7R8wiEX++bS/QXBXJ i1fdDeitPzjOlEalnDCOlgGRKwM8aZ5pn3G7Fkpbr8s3519byYYF8Y/5M63+5fixJeUg zKDg== X-Gm-Message-State: AOAM530tZKThx/XlCgPxiqXvYhid/TL8n49Zh0Rm8HTn7XbhmXuV0s3h H1bFFAyJ56qYhrKFgL68d9FmEZyHcaCz2NAU0ng= X-Google-Smtp-Source: ABdhPJxJ+lDNhRoFWqpYNopp2WiurtFuqd4NYowQUPUYUW9vvJdb5OKldLJIj4JxOnB2gW2WzxsvOYU5MpbiHutD7rk= X-Received: by 2002:adf:cd02:0:b0:1f0:767d:b39a with SMTP id w2-20020adfcd02000000b001f0767db39amr15161343wrm.529.1646821691345; Wed, 09 Mar 2022 02:28:11 -0800 (PST) MIME-Version: 1.0 References: <1fa142fb-7988-db25-c283-a6b16278f628@gmail.com> <2517a6b9-cc34-3bb1-d17e-d4e30f0e68b7@gmail.com> <76c68a33-b157-f127-36ee-034290bf3e4b@gmail.com> <20220309092605.5izvcbp6pougm6ye@sirius.home.kraxel.org> <5986332a-1f9c-01bf-e3e7-329cf2d04672@gmail.com> <750d1ed6-9c02-bd1b-3988-eb44665e8e5a@gmail.com> <07871c4c-58bf-b0db-b8b2-da6f0f9acfe2@gmail.com> In-Reply-To: <07871c4c-58bf-b0db-b8b2-da6f0f9acfe2@gmail.com> From: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Date: Wed, 9 Mar 2022 14:27:58 +0400 Message-ID: Subject: Re: [PATCH v3 12/12] ui/console: call gfx_switch() even if the current scanout is GL To: Akihiko Odaki Content-Type: multipart/alternative; boundary="000000000000ebd03205d9c68de0" X-Host-Lookup-Failed: Reverse DNS lookup failed for 2a00:1450:4864:20::430 (failed) Received-SPF: pass client-ip=2a00:1450:4864:20::430; envelope-from=marcandre.lureau@gmail.com; helo=mail-wr1-x430.google.com X-Spam_score_int: 3 X-Spam_score: 0.3 X-Spam_bar: / X-Spam_report: (0.3 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, FREEMAIL_REPLY=1, HTML_MESSAGE=0.001, PDS_HP_HELO_NORDNS=0.659, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Gerd Hoffmann , qemu-devel Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --000000000000ebd03205d9c68de0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Hi On Wed, Mar 9, 2022 at 2:20 PM Akihiko Odaki wrote: > On 2022/03/09 19:07, Marc-Andr=C3=A9 Lureau wrote: > > Hi > > > > On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki > > wrote: > > > > On 2022/03/09 18:53, Marc-Andr=C3=A9 Lureau wrote: > > > Hi > > > > > > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki > > > > > > >> 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 vn= c > > > 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 th= e > > 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=C3=A9 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/> > > > > 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. > > > > 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 dynam= ic > > - 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. > - 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. > 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. > - 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. > > > > > 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 an= d > > 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. --000000000000ebd03205d9c68de0 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi

On Wed, Mar 9, 2022 at 2:20 PM Akih= iko Odaki <akihiko.odaki@gmai= l.com> wrote:
On 2022/03/09 19:07, Marc-Andr=C3=A9 Lureau wrote:
> Hi
>
> On Wed, Mar 9, 2022 at 2:01 PM Akihiko Odaki <akihiko.odaki@gmail.com
> <mailto:akihiko.odaki@gmail.com>> wrote:
>
>=C2=A0 =C2=A0 =C2=A0On 2022/03/09 18:53, Marc-Andr=C3=A9 Lureau wrote:<= br> >=C2=A0 =C2=A0 =C2=A0 > Hi
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > On Wed, Mar 9, 2022 at 1:32 PM Akihiko Odaki<= br> >=C2=A0 =C2=A0 =C2=A0<akihiko.odaki@gmail.com <mailto:akihiko.odaki@gmail.com>
>=C2=A0 =C2=A0 =C2=A0 > <mailto:akihiko.odaki@gmail.com
>=C2=A0 =C2=A0 =C2=A0<mailto:akihiko.odaki@gmail.com>>> wrote:
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0On 2022/03/09 18:26, Gerd = Hoffmann wrote:
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 Hi,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> dpy_gfx_switch a= nd dpy_gfx_update need to be called to
>=C2=A0 =C2=A0 =C2=A0finish the
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> initialization o= r switching of the non-OpenGL display.
>=C2=A0 =C2=A0 =C2=A0However,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0the proposed
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> patch only calls= dpy_gfx_switch.
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >>
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> vnc actually doe= s not need dpy_gfx_update because the vnc
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0implementation of
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> dpy_gfx_switch i= mplicitly does the work for
>=C2=A0 =C2=A0 =C2=A0dpy_gfx_update, but
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0the model of
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> ui/console expec= ts the two of dpy_gfx_switch and
>=C2=A0 =C2=A0 =C2=A0dpy_gfx_update
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0is separated
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> and only calling= dpy_gfx_switch violates the model.
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0dpy_gfx_update used to
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >> be called even i= n such a case before and it is a regression.
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > Well, no, the ->d= py_gfx_switch() callback is supposed to do
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0everything
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > needed to bring the = new surface to the screen.=C2=A0 vnc isn't
>=C2=A0 =C2=A0 =C2=A0alone here,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > gtk for example does= the same (see gd_switch()).
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > If dpy_gfx_switch() implies a full dpy_gfx_up= date(), then we
>=C2=A0 =C2=A0 =C2=A0would need
>=C2=A0 =C2=A0 =C2=A0 > another callback to just set the new surface.= This would avoid
>=C2=A0 =C2=A0 =C2=A0 > intermediary and useless switches to 2d/surfa= ce when the scanout
>=C2=A0 =C2=A0 =C2=A0is GL.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > For consistency, we should also declare that = gl_scanout_texture and
>=C2=A0 =C2=A0 =C2=A0 > gl_scanout_dmabuf imply full update as well.<= br> >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > Yes, typically this = is roughly the same an explicit
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0dpy_gfx_update call
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > would do.=C2=A0 So t= his could be changed if it helps making the
>=C2=A0 =C2=A0 =C2=A0opengl
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0code
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > paths less confusing= , but that should be a separate patch
>=C2=A0 =C2=A0 =C2=A0series and
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > separate discussion.=
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 > take care,
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 Gerd >=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Then ui/cocoa is probably = wrong. I don't think it does the
>=C2=A0 =C2=A0 =C2=A0update when
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0dpy_gfx_switch is called.<= br> >=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0Please tell me if you thin= k dpy_gfx_switch shouldn't do the
>=C2=A0 =C2=A0 =C2=A0implicit
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0update in the future. I= 9;ll write a patch to do the update in
>=C2=A0 =C2=A0 =C2=A0cocoa's
>=C2=A0 =C2=A0 =C2=A0 >=C2=A0 =C2=A0 =C2=A0dpy_gfx_switch implementat= ion otherwise.
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > Can we ack this series first and iterate on t= op? It solves a
>=C2=A0 =C2=A0 =C2=A0number of
>=C2=A0 =C2=A0 =C2=A0 > issues already and is a better starting point= .
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > thanks
>=C2=A0 =C2=A0 =C2=A0 >
>=C2=A0 =C2=A0 =C2=A0 > --
>=C2=A0 =C2=A0 =C2=A0 > Marc-Andr=C3=A9 Lureau
>
>=C2=A0 =C2=A0 =C2=A0The call of dpy_gfx_update in displaychangelistener= _display_console
>=C2=A0 =C2=A0 =C2=A0should be removed. It would simplify the patch.
>
>=C2=A0 =C2=A0 =C2=A0Also it is still not shown that the series is a bet= ter alternative to:
>=C2=A0 =C2=A0 =C2=A0https= ://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/
>=C2=A0 =C2=A0 =C2=A0<h= ttps://patchew.org/QEMU/20220213024222.3548-1-akihiko.odaki@gmail.com/&= gt;
>
>=C2=A0 =C2=A0 =C2=A0The series "ui/dbus: Share one listener for a = console" has
>=C2=A0 =C2=A0 =C2=A0significantly
>=C2=A0 =C2=A0 =C2=A0less 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 pr= oportional 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 sche= dule, this is a greater risk.


>
> 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 dyna= mic
> - the GL context split also is a separation of concerns and should hel= p
> 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.
=
=C2=A0
- 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 associat= ed with a window.
=C2=A0
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 a= lso about the type design. It's cleaner to separate DisplayGLCtxt from = DisplayChangeListener.
=C2=A0
- 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.

>
> My understanding of your proposal is that you would rather see all thi= s
> 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 a= nd
> regressions you pointed out that should be fixed. Going back to a
> different design should be done in a next release if sufficiently moti= vated.

Reverting the dbus change is the safest option if it does not settle.

We have a different sense of safety.
<= /div>
--000000000000ebd03205d9c68de0--