Re: [RFC PATCH 2/4] ui/egl-helpers: Consolidates create-sync and create-fence

[Kim, Dongwon]

Hi Marc-André,

I will come up with a separate patch for this.

On 7/2/2024 1:32 AM, Marc-André Lureau wrote:

Hi

On Fri, Jun 21, 2024 at 3:19 AM > wrote:


From: Dongwon Kim mailto:dongwon@intel.com>>

There is no reason to split those two operations so combining
two functions - egl_dmabuf_create_sync and egl_dmabuf_create_fence.

Cc: Gerd Hoffmann mailto:kra...@redhat.com>>
Cc: Marc-André Lureau mailto:marcandre.lur...@redhat.com>>
Cc: Vivek Kasireddy mailto:vivek.kasire...@intel.com>>
Signed-off-by: Dongwon Kim mailto:dongwon@intel.com>>


Can you send this change as a different patch series, or earlier in the 
series?


You should also remove qemu_dmabuf_{set,get}_sync() and associated fields

---
  include/ui/egl-helpers.h |  3 +--
  ui/egl-helpers.c         | 27 +++
  ui/gtk-egl.c             | 19 +++
  ui/gtk-gl-area.c         | 10 ++
  4 files changed, 21 insertions(+), 38 deletions(-)

diff --git a/include/ui/egl-helpers.h b/include/ui/egl-helpers.h
index 4b8c0d2281..606d6c8288 100644
--- a/include/ui/egl-helpers.h
+++ b/include/ui/egl-helpers.h
@@ -51,8 +51,7 @@ int egl_get_fd_for_texture(uint32_t tex_id, EGLint
*stride, EGLint *fourcc,

  void egl_dmabuf_import_texture(QemuDmaBuf *dmabuf);
  void egl_dmabuf_release_texture(QemuDmaBuf *dmabuf);
-void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf);
-void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf);
+int egl_dmabuf_create_fence(QemuDmaBuf *dmabuf);

  #endif

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 99b2ebbe23..e16f2cb23d 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -371,34 +371,29 @@ void egl_dmabuf_release_texture(QemuDmaBuf
*dmabuf)
      qemu_dmabuf_set_texture(dmabuf, 0);
  }

-void egl_dmabuf_create_sync(QemuDmaBuf *dmabuf)
+int egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
  {
      EGLSyncKHR sync;
+    int fence_fd = -1;

      if (epoxy_has_egl_extension(qemu_egl_display,
                                  "EGL_KHR_fence_sync") &&
          epoxy_has_egl_extension(qemu_egl_display,
-                                "EGL_ANDROID_native_fence_sync")) {
+                                "EGL_ANDROID_native_fence_sync") &&
+        qemu_dmabuf_get_fence_fd(dmabuf) == -1) {
          sync = eglCreateSyncKHR(qemu_egl_display,
                                  EGL_SYNC_NATIVE_FENCE_ANDROID, NULL);
          if (sync != EGL_NO_SYNC_KHR) {
-            qemu_dmabuf_set_sync(dmabuf, sync);
+            fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
+                                                  sync);
+            if (fence_fd >= 0) {
+                qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
+            }
+            eglDestroySyncKHR(qemu_egl_display, sync);
          }
      }
-}
-
-void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
-{
-    void *sync = qemu_dmabuf_get_sync(dmabuf);
-    int fence_fd;

-    if (sync) {
-        fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
-                                              sync);
-        qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
-        eglDestroySyncKHR(qemu_egl_display, sync);
-        qemu_dmabuf_set_sync(dmabuf, NULL);
-    }
+    return fence_fd;


It should probably return qemu_dmabuf_get_fence_fd() instead. The 
function should be renamed with _fd postfix since you make it return the fd.


  }

  #endif /* CONFIG_GBM */
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 9831c10e1b..55199f8659 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -68,7 +68,6 @@ void gd_egl_draw(VirtualConsole *vc)
      GdkWindow *window;
  #ifdef CONFIG_GBM
      QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
-    int fence_fd;
  #endif
      int ww, wh, ws;

@@ -99,13 +98,12 @@ void gd_egl_draw(VirtualConsole *vc)
          glFlush();
  #ifdef CONFIG_GBM
          if (dmabuf) {
-            egl_dmabuf_create_fence(dmabuf);
-            fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
+            fence_fd = egl_dmabuf_create_fence(dmabuf);
              if (fence_fd >= 0) {
                  qemu_set_fd_handler(fence_fd, gd_hw_gl_flushed,
NULL, vc);
-                return;
+            } else {
+                graphic_hw_gl_block(vc->gfx.dcl.con, false);
              }
-            graphic_hw_gl_block(vc->gfx.dcl.con, false);
          }
  #endif
      } else {
@@ -336,7 +334,11 @@ void gd_egl_scanout_flush(DisplayChangeListener
*dcl,
  {
      VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
      GdkWindow *window;
+#ifdef CONFIG_GBM
+    

Re: [RFC PATCH 0/4] hw/display/virtio-gpu: Introducing asynchronous guest display render

[Kim, Dongwon]

Hi Marc-André,

On 7/2/2024 3:29 AM, Marc-André Lureau wrote:

Hi

On Fri, Jun 21, 2024 at 3:20 AM > wrote:


From: Dongwon Kim mailto:dongwon@intel.com>>

Introducing new virtio-gpu param, 'render_sync' when guest scanout blob
is used (blob=true). The new param is used to specify when to start
rendering a guest scanout frame.

By default (and so far) rendering of the guest frame is started in
the draw event to make sure guest display update is sychronized with
host's vsync. But this method inevitably brings some extra wait because
most of time, the draw event is not happening right after the guest
scanout frame is flushed.


This delay often makes the guest stuck at certain frame for too long and
causes general performance degradation of graphic workloads on the
guest's
side especially when the display update rate is high. This unwanted perf
drop can be reduced if the guest scanout frame is rendered as soon
as it is
flushed through 'VIRTIO_GPU_CMD_RESOURCE_FLUSH' msg. The gl display
pipeline can be unblocked a lot earlier in this case so that guest can
move to the next display frame right away.

However, this "asynchrounous" render mode may cause some waste of
resources
as the guest could produce more frames than what are actually displayed
on the host display. This is similar as running rendering apps with
no vblank
or vsync option. This is why this feature should stay as optional.


Indeed, I don't see much point in doing so, it's pure waste. If you want 
to benchmark something perhaps. But then, why not simply run a benchmark 
offscreen?


Benchmark score is not the primary reason. The problem we want to avoid 
is the laggy display update due to too many asynchronous plane updates 
happening in the guest in certain situations like when moving SW mouse 
cursor rigorously on the guest. This is because the fence (as well as 
response for the resource-flush cmd) is signaled in the draw event.





The param 'render_sync' is set to 'true' by default and this is in line
with traditional way while setting it to 'false' is basically enabling
this asynchronouse mode.


As it stands now, the option should actually be on the display backend 
(gtk/gtk-egl), it's not implemented for other backends.


We can help to deploy this concept to other backends if needed.



I am not convinced this is generally useful to be an extra option though.


I initially thought this should be the default because the virtio-gpu 
guest should not be hold by the host for too long in any cases. And this 
new approach is comparable to the default mode with blob=false where the 
guest is released as soon as the resource flush is done. Only difference 
is the resource synchronization using the fence.




Dongwon Kim (4):
   hw/display/virtio-gpu: Introducing render_sync param
   ui/egl-helpers: Consolidates create-sync and create-fence
   ui/gtk-egl: Start rendering of guest blob scanout if render_sync is
     off
   ui/gtk-gl-draw: Start rendering of guest blob scanout if render_sync
     is off

  include/hw/virtio/virtio-gpu.h  |  3 ++
  include/ui/dmabuf.h             |  4 +-
  include/ui/egl-helpers.h        |  3 +-
  hw/display/vhost-user-gpu.c     |  3 +-
  hw/display/virtio-gpu-udmabuf.c |  3 +-
  hw/display/virtio-gpu.c         |  2 +
  hw/vfio/display.c               |  3 +-
  ui/dbus-listener.c              |  2 +-
  ui/dmabuf.c                     | 11 +++-
  ui/egl-helpers.c                | 27 --
  ui/gtk-egl.c                    | 93 ++---
  ui/gtk-gl-area.c                | 90 +++
  12 files changed, 146 insertions(+), 98 deletions(-)

-- 
2.34.1





--
Marc-André Lureau

Re: [PATCH] ui/gtk: Negative Page number is not valid

[Kim, Dongwon]

On 6/26/2024 10:06 AM, Philippe Mathieu-Daudé wrote:

Hi Dongwon,

On 26/6/24 02:08, dongwon@intel.com wrote:

From: Dongwon Kim 

Negative page number means the page with that number does not
belong to the notebook so it shouldn't be used as a valid page
number in gd_vc_find_by_page. This function should just return
null in such case.

This change, however, will cause a segfault during detaching
/untabifying process in gtk_release_modifiers because the
current VC's page number suddenly becomes '-1' as soon as
the VC is detached, which makes gd_vc_find_by_page return
null. So gtk_release_modifiers should do the null check on
VC returned from gd_vc_find_by_page.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Signed-off-by: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
  ui/gtk.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk.c b/ui/gtk.c
index 93b13b7a30..1f8523fd81 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -164,7 +164,7 @@ static VirtualConsole 
*gd_vc_find_by_page(GtkDisplayState *s, gint page)


The caller should check gtk_notebook_get_current_page() != -1.

We might assert(page >= 0) here.


We could do that but it means there should be more checks in
other functions where gd_vc_find_by_page is called, like
gd_vc_find_current. And we just can't do assert in\
gd_vc_find_current because detached VC has the page number of -1.




  for (i = 0; i < s->nb_vcs; i++) {
  vc = >vc[i];
  p = gtk_notebook_page_num(GTK_NOTEBOOK(s->notebook), 
vc->tab_item);

-    if (p == page) {
+    if (p > -1 && p == page) {


Then this is not necessary.


  return vc;
  }
  }

     return NULL;

I wonder about returning NULL, maybe just

    g_assert_not_reached();


@@ -357,7 +357,7 @@ static void gtk_release_modifiers(GtkDisplayState *s)
  {
  VirtualConsole *vc = gd_vc_find_current(s);
-    if (vc->type != GD_VC_GFX ||
+    if (!vc || vc->type != GD_VC_GFX ||


Then this is not necessary.


  !qemu_console_is_graphic(vc->gfx.dcl.con)) {
  return;
  }

Re: [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors

[Kim, Dongwon]

Hi Marc-André,

Ok, it is good to know dmabuf is used for sharing the scanout between 
processes when using dbus or virt-viewer.


BTW,

I have some more comments on your previous answers. I just copied ans 
pasted your previous comments. Sorry about any inconvenience caused by 
this delayed response.


[Marc-André] The plan is there, GNOME has made bold moves in the past. 
There is not much left in the TODO. But yes, it takes a bit longer than 
expected.


[Dongwon] If Gnome abandons Xorg, then users would find other distros or 
other desktop environments that support Xorg. My point is why should 
Qemu GTK UI not support those distros/environments that support Xorg 
today with new features? If maintainability is your concern, again we 
can offer supporting this feature as long as there is a distro out there 
that supports Xorg.


[Marc-André] Intuitive, perhaps. Discoverable and portable?

[Dongwon] We thought about doing the multi monitor mapping similar to 
how virt-viewer is doing it but we chose monitor labels because they 
uniquely identify the monitors even if they are repeatedly 
unplugged/plugged which may not be possible if we use integer IDs to 
represent monitors. For example, if virt-viewer client identifies a DP-1 
with monitor ID 1 and a HDMI-1 with ID 2 and they are both unplugged and 
HDMI-1 is hotplugged first, I don't think it would probably ID 1 with 
HDMI-1 as there is no way for it to know without the label.


[Marc-André] Interesting case that could be added to virt-viewer if it's 
necessary.


[Dongwon] The implementation might become very complex if we were to add 
the "hotplug" functionality as opposed to the simplicity with which it 
is done with Qemu GTK UI. And, it appears there is no flexibility to 
make policy changes such as using monitor labels instead of monitor IDs. 
And, btw, from a quick look, virt-viewer appears to use the same 
API(gtk_window_move()) that we are relying on, so I guess a similar fate 
awaits it if Xorg is gone or it switches to GTK4.


[Marc-André] Honestly, I don't support the idea of duplicating this 
effort in QEMU.


[Dongwon] That is unfortunate. It is a similar, but a different feature 
with "hotplug" and labels as its core as described earlier. I think it 
may not be a great idea to force users who are already using Qemu GTK UI 
given their use-cases, to use virt-viewer (which adds another layer of

complexity) just to use this feature.

[Dongwon]
Thanks!

On 6/14/2024 1:41 AM, Marc-André Lureau wrote:

Hi

On Thu, Jun 13, 2024 at 9:08 PM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


 >     "hotplug" functionality where a Guest display/window is
deeply tied to a
 >     physical monitor to make it appear to the guest that it is
dealing with
 >     a real physical monitor.
 >
 >     In other words, when the physical monitor is unplugged, the
associated
 >     guest display/window gets destroyed/hidden and gets
recreated/shown
 >     when
 >     the monitor is hotplugged again.
 >
 >
 > Interesting case that could be added to virt-viewer if it's
necessary.
 >
 > The subject is sufficiently complex that there is already additional
 > documentation/specification in:
 >
https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads 
<https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads> 
<https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads 
<https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads>>
 >
 > Honestly, I don't support the idea of duplicating this effort in
QEMU.

Marc-André,

My assumption is virt-viewer might not be able to completely replace
GTK-UI path in terms of performance and smoothness of display update as
(I think) frame copy between processes is implied, which is same as


There is no frame copy when using DMABUF scanouts between qemu and client.
Iow, the performance difference is negligible / noise level.

spice-remote viewer. What about display-bus that you have been working
on? Would it be a good alternative w.r.t perf concern that I specified
above?


There shouldn't be much difference for the local DMABUF display case.


 >
 > --
 > Marc-André Lureau



--
Marc-André Lureau

Re: [PATCH] ui/gtk: Wait until the current guest frame is rendered before switching to RUN_STATE_SAVE_VM

[Kim, Dongwon]

Hi Marc-André,

On 6/13/2024 6:16 AM, Marc-André Lureau wrote:

Hi

On Wed, Jun 12, 2024 at 10:50 PM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


On 6/11/2024 10:44 PM, Marc-André Lureau wrote:
 > Hi
 >
 > On Wed, Jun 12, 2024 at 5:29 AM Kim, Dongwon
mailto:dongwon@intel.com>
 > <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>> wrote:
 >
 >     Hi,
 >
 >     From: Marc-André Lureau mailto:marcandre.lur...@gmail.com>
 >     <mailto:marcandre.lur...@gmail.com
<mailto:marcandre.lur...@gmail.com>>>
 >     Sent: Wednesday, June 5, 2024 12:56 AM
 >     To: Kim, Dongwon mailto:dongwon@intel.com> <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>>>
 >     Cc: qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>
<mailto:qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>>; Peter Xu
 >     mailto:pet...@redhat.com>
<mailto:pet...@redhat.com <mailto:pet...@redhat.com>>>
 >     Subject: Re: [PATCH] ui/gtk: Wait until the current guest
    frame is
 >     rendered before switching to RUN_STATE_SAVE_VM
 >
 >     Hi
 >
 >     On Tue, Jun 4, 2024 at 9:49 PM Kim, Dongwon
 >     <mailto:dongwon@intel.com <mailto:dongwon@intel.com>
<mailto:dongwon@intel.com <mailto:dongwon@intel.com>>> wrote:
 >     On 6/4/2024 4:12 AM, Marc-André Lureau wrote:
 >      > Hi
 >      >
 >      > On Thu, May 30, 2024 at 2:44 AM
<mailto:dongwon@intel.com <mailto:dongwon@intel.com>
 >     <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>
 >      > <mailto:mailto <mailto:mailto> <mailto:mailto
<mailto:mailto>>:dongwon@intel.com <mailto:dongwon@intel.com>
 >     <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>>>> wrote:
 >      >
 >      >     From: Dongwon <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>
 >     <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>
<mailto:mailto <mailto:mailto>
 >     <mailto:mailto <mailto:mailto>>:dongwon@intel.com
<mailto:dongwon@intel.com> <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>>>>
 >      >
 >      >     Make sure rendering of the current frame is finished
before
 >     switching
 >      >     the run state to RUN_STATE_SAVE_VM by waiting for egl-sync
 >     object to be
 >      >     signaled.
 >      >
 >      >
 >      > Can you expand on what this solves?
 >
 >     In current scheme, guest waits for the fence to be signaled
for each
 >     frame it submits before moving to the next frame. If the
guest’s state
 >     is saved while it is still waiting for the fence, The guest will
 >     continue to  wait for the fence that was signaled while ago
when it is
 >     restored to the point. One way to prevent it is to get it
finish the
 >     current frame before changing the state.
 >
 >     After the UI sets a fence, hw_ops->gl_block(true) gets
called, which
 >     will block virtio-gpu/virgl from processing commands (until the
 >     fence is signaled and gl_block/false called again).
 >
 >     But this "blocking" state is not saved. So how does this affect
 >     save/restore? Please give more details, thanks
 >
 >     Yeah sure. "Blocking" state is not saved but guest's state is
saved
 >     while it was still waiting for the response for its last
 >     resource-flush virtio msg. This virtio response, by the way
is set
 >     to be sent to the guest when the pipeline is unblocked (and
when the
 >     fence is signaled.). Once the guest's state is saved, current
 >     instance of guest will be continued and receives the response as
 >     usual. The problem is happening when we restore the saved guest's
 >     state again because what guest does will be waiting for the
response
 >     that was sent a while ago to the original instance.
 >
 >
 > Where is the pending response saved? Can you detail how you test
this?
 >

There is no pending response for the guest's restored point, which is a
problem. The response is sent out after saving is done.

Normal cycle :

resource-flush (scanout flush) -> gl block -> render -> gl unblock
(after fence is sig

Re: [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors

[Kim, Dongwon]

Hi Marc-André,

On 6/12/2024 11:56 PM, Marc-André Lureau wrote:

Hi

On Thu, Jun 13, 2024 at 3:34 AM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


On 6/11/2024 11:42 PM, Marc-André Lureau wrote:
 > Hi
 >
 > On Tue, Jun 11, 2024 at 10:28 PM Kim, Dongwon
mailto:dongwon@intel.com>
 > <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>> wrote:
 >
 >     Hi Marc-André,
 >
 >     On 6/5/2024 12:26 AM, Marc-André Lureau wrote:
 >      > Hi
     >      >
 >      > On Tue, Jun 4, 2024 at 9:59 PM Kim, Dongwon
 >     mailto:dongwon@intel.com>
<mailto:dongwon@intel.com <mailto:dongwon@intel.com>>
 >      > <mailto:dongwon@intel.com
<mailto:dongwon@intel.com> <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>>>> wrote:
 >
 >      > Xorg may not be going away soon, but it's used less and
less. As
 >     one of
 >      > the developers, I am no longer running/testing it for a
long time. I
 >      > wish we would just drop its support tbh.
 >
 >     There are features offered by Xorg that are not offered by
Wayland
 >     compositors and again, we have customers that rely on these
features.
 >     One of them is the ability to position the window via
 >     gtk_window_set_position(). There are strong arguments
 >     made on either side when it comes to window positioning:
 >
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247 
<https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247> 
<https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247 
<https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247>>
 >
 >     Until there is a way to do this with Wayland compositors, we
have to
 >     unfortunately rely on Gnome + Xorg.
 >
 >
 > It's a smaller and smaller number of users. The potential/future
users
 > are greater if we focus on Wayland.

Right, but until Gtk + Wayland offers the same feature parity and
customization as that of Gtk + Xorg, there will be distros/users that
will keep it alive.
 >
 > Fwiw, GNOME (and RHEL) is set to drop Xorg support
 > (https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98
<https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98>
 > <https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98
<https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98>>)

Doesn't look like it is going to happen anytime soon given the massive
pushback.


The plan is there, GNOME has made bold moves in the past. There is not 
much left in the TODO. But yes, it takes a bit longer than expected.



 >
 > Btw, there is a similar monitor-mapping functionality implemented in
 > virt-viewer/remote-viewer:
 > https://www.mankier.com/1/virt-viewer#Configuration
<https://www.mankier.com/1/virt-viewer#Configuration>
 > <https://www.mankier.com/1/virt-viewer#Configuration
<https://www.mankier.com/1/virt-viewer#Configuration>>. Is this
something
 > that those users could use instead?

It looks a bit similar and interesting but one difference is that our
feature uses monitor labels such as DP-1, HDMI-2 which is a bit more
intuitive. And, the other key difference is that our feature includes


Intuitive, perhaps. Discoverable and portable?

"hotplug" functionality where a Guest display/window is deeply tied to a
physical monitor to make it appear to the guest that it is dealing with
a real physical monitor.

In other words, when the physical monitor is unplugged, the associated
guest display/window gets destroyed/hidden and gets recreated/shown
when
the monitor is hotplugged again.


Interesting case that could be added to virt-viewer if it's necessary.

The subject is sufficiently complex that there is already additional 
documentation/specification in:

https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads 
<https://gitlab.com/virt-viewer/virt-viewer/-/tree/master/docs?ref_type=heads>

Honestly, I don't support the idea of duplicating this effort in QEMU.


Marc-André,

My assumption is virt-viewer might not be able to completely replace 
GTK-UI path in terms of performance and smoothness of display update as 
(I think) frame copy between processes is implied, which is same as 
spice-remote viewer. What about display-bus that you have been working 
on? Would it be a good alternative w.r.t perf concern that I specified 
above?




--
Marc-André Lureau

Re: [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors

[Kim, Dongwon]

On 6/11/2024 11:42 PM, Marc-André Lureau wrote:

Hi

On Tue, Jun 11, 2024 at 10:28 PM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


Hi Marc-André,

On 6/5/2024 12:26 AM, Marc-André Lureau wrote:
 > Hi
 >
 > On Tue, Jun 4, 2024 at 9:59 PM Kim, Dongwon
mailto:dongwon@intel.com>
 > <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>> wrote:

 > Xorg may not be going away soon, but it's used less and less. As
one of
 > the developers, I am no longer running/testing it for a long time. I
 > wish we would just drop its support tbh.

There are features offered by Xorg that are not offered by Wayland
compositors and again, we have customers that rely on these features.
One of them is the ability to position the window via
gtk_window_set_position(). There are strong arguments
made on either side when it comes to window positioning:
https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247 
<https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/247>

Until there is a way to do this with Wayland compositors, we have to
unfortunately rely on Gnome + Xorg.


It's a smaller and smaller number of users. The potential/future users 
are greater if we focus on Wayland.


Right, but until Gtk + Wayland offers the same feature parity and 
customization as that of Gtk + Xorg, there will be distros/users that 
will keep it alive.


Fwiw, GNOME (and RHEL) is set to drop Xorg support 
(https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98 
<https://gitlab.gnome.org/GNOME/gnome-session/-/merge_requests/98>)


Doesn't look like it is going to happen anytime soon given the massive 
pushback.




Btw, there is a similar monitor-mapping functionality implemented in 
virt-viewer/remote-viewer: 
https://www.mankier.com/1/virt-viewer#Configuration 
<https://www.mankier.com/1/virt-viewer#Configuration>. Is this something 
that those users could use instead?


It looks a bit similar and interesting but one difference is that our 
feature uses monitor labels such as DP-1, HDMI-2 which is a bit more 
intuitive. And, the other key difference is that our feature includes 
"hotplug" functionality where a Guest display/window is deeply tied to a
physical monitor to make it appear to the guest that it is dealing with 
a real physical monitor.


In other words, when the physical monitor is unplugged, the associated 
guest display/window gets destroyed/hidden and gets recreated/shown when 
the monitor is hotplugged again.





--
Marc-André Lureau

Re: [PATCH] ui/gtk: Wait until the current guest frame is rendered before switching to RUN_STATE_SAVE_VM

[Kim, Dongwon]

On 6/11/2024 10:44 PM, Marc-André Lureau wrote:

Hi

On Wed, Jun 12, 2024 at 5:29 AM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


Hi,

From: Marc-André Lureau mailto:marcandre.lur...@gmail.com>>
Sent: Wednesday, June 5, 2024 12:56 AM
To: Kim, Dongwon mailto:dongwon@intel.com>>
Cc: qemu-devel@nongnu.org <mailto:qemu-devel@nongnu.org>; Peter Xu
mailto:pet...@redhat.com>>
Subject: Re: [PATCH] ui/gtk: Wait until the current guest frame is
rendered before switching to RUN_STATE_SAVE_VM

Hi

    On Tue, Jun 4, 2024 at 9:49 PM Kim, Dongwon
<mailto:dongwon@intel.com <mailto:dongwon@intel.com>> wrote:
On 6/4/2024 4:12 AM, Marc-André Lureau wrote:
 > Hi
 >
 > On Thu, May 30, 2024 at 2:44 AM <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>
 > <mailto:mailto <mailto:mailto>:dongwon@intel.com
<mailto:dongwon@intel.com>>> wrote:
 >
 >     From: Dongwon <mailto:dongwon@intel.com
<mailto:dongwon@intel.com> <mailto:mailto
<mailto:mailto>:dongwon@intel.com <mailto:dongwon@intel.com>>>
 >
 >     Make sure rendering of the current frame is finished before
switching
 >     the run state to RUN_STATE_SAVE_VM by waiting for egl-sync
object to be
 >     signaled.
 >
 >
 > Can you expand on what this solves?

In current scheme, guest waits for the fence to be signaled for each
frame it submits before moving to the next frame. If the guest’s state
is saved while it is still waiting for the fence, The guest will
continue to  wait for the fence that was signaled while ago when it is
restored to the point. One way to prevent it is to get it finish the
current frame before changing the state.

After the UI sets a fence, hw_ops->gl_block(true) gets called, which
will block virtio-gpu/virgl from processing commands (until the
fence is signaled and gl_block/false called again).

But this "blocking" state is not saved. So how does this affect
save/restore? Please give more details, thanks

Yeah sure. "Blocking" state is not saved but guest's state is saved
while it was still waiting for the response for its last
resource-flush virtio msg. This virtio response, by the way is set
to be sent to the guest when the pipeline is unblocked (and when the
fence is signaled.). Once the guest's state is saved, current
instance of guest will be continued and receives the response as
usual. The problem is happening when we restore the saved guest's
state again because what guest does will be waiting for the response
that was sent a while ago to the original instance.


Where is the pending response saved? Can you detail how you test this?



There is no pending response for the guest's restored point, which is a 
problem. The response is sent out after saving is done.


Normal cycle :

resource-flush (scanout flush) -> gl block -> render -> gl unblock 
(after fence is signaled) -> pending response sent out to the guest -> 
guest (virtio-gpu drv) processes the next scanout frame -> (next cycle) 
resource-flush -> gl block ..


When vm state is saved in the middle :

resource-flush (scanout-flush) -> gl block -> saving vm-state -> render 
-> gl unblock -> pending response (resp #1) sent out to the guest -> 
guest (virtio-gpu drv) processes the next scanout frame -> (next cycle) 
resource-flush -> gl block ..


Now, we restore the vm-state we saved

vm-state is restored -> guest (virtio-gpu drv) can't move on as this 
state is still waiting for the response (resp #1)


So we need to make sure vm-state is saved after the cycle is completed.

This situation would be only happening if you use blob=true with 
virtio-gpu drv as KMS on the linux guest. Do you have any similar setup?



thanks

--
Marc-André Lureau

RE: [PATCH] ui/gtk: Wait until the current guest frame is rendered before switching to RUN_STATE_SAVE_VM

[Kim, Dongwon]

Hi, 

From: Marc-André Lureau  
Sent: Wednesday, June 5, 2024 12:56 AM
To: Kim, Dongwon 
Cc: qemu-devel@nongnu.org; Peter Xu 
Subject: Re: [PATCH] ui/gtk: Wait until the current guest frame is rendered 
before switching to RUN_STATE_SAVE_VM

Hi

On Tue, Jun 4, 2024 at 9:49 PM Kim, Dongwon <mailto:dongwon@intel.com> 
wrote:
On 6/4/2024 4:12 AM, Marc-André Lureau wrote:
> Hi
> 
> On Thu, May 30, 2024 at 2:44 AM <mailto:dongwon@intel.com 
> <mailto:mailto:dongwon@intel.com>> wrote:
> 
>     From: Dongwon <mailto:dongwon@intel.com 
><mailto:mailto:dongwon@intel.com>>
> 
>     Make sure rendering of the current frame is finished before switching
>     the run state to RUN_STATE_SAVE_VM by waiting for egl-sync object to be
>     signaled.
> 
> 
> Can you expand on what this solves?

In current scheme, guest waits for the fence to be signaled for each 
frame it submits before moving to the next frame. If the guest’s state 
is saved while it is still waiting for the fence, The guest will 
continue to  wait for the fence that was signaled while ago when it is 
restored to the point. One way to prevent it is to get it finish the 
current frame before changing the state.

After the UI sets a fence, hw_ops->gl_block(true) gets called, which will block 
virtio-gpu/virgl from processing commands (until the fence is signaled and 
gl_block/false called again).

But this "blocking" state is not saved. So how does this affect save/restore? 
Please give more details, thanks

Yeah sure. "Blocking" state is not saved but guest's state is saved while it 
was still waiting for the response for its last resource-flush virtio msg. This 
virtio response, by the way is set to be sent to the guest when the pipeline is 
unblocked (and when the fence is signaled.). Once the guest's state is saved, 
current instance of guest will be continued and receives the response as usual. 
The problem is happening when we restore the saved guest's state again because 
what guest does will be waiting for the response that was sent a while ago to 
the original instance.

> 
> 
>     Cc: Marc-André Lureau <mailto:marcandre.lur...@redhat.com
>     <mailto:mailto:marcandre.lur...@redhat.com>>
>     Cc: Vivek Kasireddy <mailto:vivek.kasire...@intel.com
>     <mailto:mailto:vivek.kasire...@intel.com>>
>     Signed-off-by: Dongwon Kim <mailto:dongwon@intel.com
>     <mailto:mailto:dongwon@intel.com>>
>     ---
>       ui/egl-helpers.c |  2 --
>       ui/gtk.c         | 19 +++
>       2 files changed, 19 insertions(+), 2 deletions(-)
> 
>     diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
>     index 99b2ebbe23..dafeb36074 100644
>     --- a/ui/egl-helpers.c
>     +++ b/ui/egl-helpers.c
>     @@ -396,8 +396,6 @@ void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
>               fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
>                                                     sync);
>               qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
>     -        eglDestroySyncKHR(qemu_egl_display, sync);
>     -        qemu_dmabuf_set_sync(dmabuf, NULL);
> 
> 
> If this function is called multiple times, it will now set a new 
> fence_fd each time, and potentially leak older fd. Maybe it could first 
> check if a fence_fd exists instead.

We can make that change.

> 
>           }
>       }
> 
>     diff --git a/ui/gtk.c b/ui/gtk.c
>     index 93b13b7a30..cf0dd6abed 100644
>     --- a/ui/gtk.c
>     +++ b/ui/gtk.c
>     @@ -600,9 +600,12 @@ void gd_hw_gl_flushed(void *vcon)
> 
>           fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
>           if (fence_fd >= 0) {
>     +        void *sync = qemu_dmabuf_get_sync(dmabuf);
>               qemu_set_fd_handler(fence_fd, NULL, NULL, NULL);
>               close(fence_fd);
>               qemu_dmabuf_set_fence_fd(dmabuf, -1);
>     +        eglDestroySyncKHR(qemu_egl_display, sync);
>     +        qemu_dmabuf_set_sync(dmabuf, NULL);
>               graphic_hw_gl_block(vc->gfx.dcl.con, false);
>           }
>       }
>     @@ -682,6 +685,22 @@ static const DisplayGLCtxOps egl_ctx_ops = {
>       static void gd_change_runstate(void *opaque, bool running,
>     RunState state)
>       {
>           GtkDisplayState *s = opaque;
>     +    QemuDmaBuf *dmabuf;
>     +    int i;
>     +
>     +    if (state == RUN_STATE_SAVE_VM) {
>     +        for (i = 0; i < s->nb_vcs; i++) {
>     +            VirtualConsole *vc = >vc[i];
>     +            dmabuf = vc->gfx.guest_fb.dmabuf;
>     +            if (dmabuf && qemu_dmabuf_get_fence_fd(dmabuf) >= 0) {
>     +                /* wait for the rendering to be completed */
>    

Re: [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors

[Kim, Dongwon]

Hi Marc-André,

On 6/5/2024 12:26 AM, Marc-André Lureau wrote:

Hi

On Tue, Jun 4, 2024 at 9:59 PM Kim, Dongwon <mailto:dongwon@intel.com>> wrote:


Hi Marc-André,

On 6/4/2024 3:37 AM, Marc-André Lureau wrote:
 > Hi
 >
 > On Fri, May 31, 2024 at 11:00 PM mailto:dongwon@intel.com>
 > <mailto:dongwon@intel.com <mailto:dongwon@intel.com>>> wrote:
 >
 >     From: Dongwon Kim mailto:dongwon@intel.com> <mailto:dongwon@intel.com
<mailto:dongwon@intel.com>>>
 >
 >     This patch series is a replacement of
 >
https://mail.gnu.org/archive/html/qemu-devel/2023-06/msg03989.html
<https://mail.gnu.org/archive/html/qemu-devel/2023-06/msg03989.html>
 >   
  <https://mail.gnu.org/archive/html/qemu-devel/2023-06/msg03989.html <https://mail.gnu.org/archive/html/qemu-devel/2023-06/msg03989.html>>

 >
 >     There is a need, expressed by several users, to assign
ownership of one
 >     or more physical monitors/connectors to individual guests.
This creates
 >     a clear notion of which guest's contents are being displayed
on any
 >     given
 >     monitor. Given that there is always a display
server/compositor running
 >     on the host, monitor ownership can never truly be transferred
to guests.
 >     However, the closest approximation is to request the host
compositor to
 >     fullscreen the guest's windows on individual monitors. This
allows for
 >     various configurations, such as displaying four different guests'
 >     windows
 >     on four different monitors, a single guest's windows (or virtual
 >     consoles)
 >     on four monitors, or any similar combination.
 >
 >     This patch series attempts to accomplish this by introducing
a new
 >     parameter named "connector" to assign monitors to the GFX VCs
associated
 >     with a guest. If the assigned monitor is not connected, the
guest's
 >     window
 >     will not be displayed, similar to how a host compositor
behaves when
 >     connectors are not connected. Once the monitor is
hot-plugged, the
 >     guest's
 >     window(s) will be positioned on the assigned monitor.
 >
 >     Usage example:
 >
 >     -display gtk,gl=on,connectors=DP-1:eDP-1:HDMI-2...
 >
 >     In this example, the first graphics virtual console will be
placed
 >     on the
 >     DP-1 display, the second on eDP-1, and the third on HDMI-2.
 >
 >
 > Unfortunately, this approach with GTK is doomed. gtk4 dropped the
 > gtk_window_set_position() altogether.

Do you mean we have a plan to lift GTK version in QEMU? Are we going to
lose all GTK3 specific features?


No concrete plan, no. But eventually GTK3 will go away some day.


There are users who still rely on features provided by GTK3 and we also 
have customers who are moving from VMware, virtualbox that have 
requested for this feature. Their use-cases are current and active. If 
windows repositioning won't be supported someday, then we would need to 
make this feature obsolete but many users/customers would benefit from 
it until then.




fwiw, I wish QEMU wouldn't have N built-in UIs/Spice/VNC, but different 
projects elsewhere using -display dbus. There is 
https://gitlab.gnome.org/GNOME/libmks 
<https://gitlab.gnome.org/GNOME/libmks> or 
https://gitlab.com/marcandre.lureau/qemu-display 
<https://gitlab.com/marcandre.lureau/qemu-display> gtk4 efforts.


As you know, there cannot be a one size fits all solution that would 
work for all the users, which is probably why there are many Qemu UIs.





 >
 > It's not even clear how the different monitors/outputs/connectors
are
 > actually named, whether they are stable etc (not mentioning the
 > portability).
 >
 > Window placement & geometry is a job for the compositor. Can you
discuss
 > this issue with GTK devs & the compositor you are targeting?

I guess you are talking about wayland compositor. We are mainly using
Xorg on the host and this feature works pretty good on it. I am


Xorg may not be going away soon, but it's used less and less. As one of 
the developers, I am no longer running/testing it for a long time. I 
wish we would just drop its support tbh.


There are features offered by Xorg that are not offered by Wayland 
compositors and again, we have customers that rely on these features.
One of them is the ability to position the window via 
gtk_window_set_position(). There are strong arguments

made on either side when it comes to window positioning:
https://gitlab.freedesktop.org/wayland/wayla

Re: [RFC PATCH v2 0/2] ui/gtk: Introduce new param - Connectors

[Kim, Dongwon]

Hi Marc-André,

On 6/4/2024 3:37 AM, Marc-André Lureau wrote:

Hi

On Fri, May 31, 2024 at 11:00 PM > wrote:


From: Dongwon Kim mailto:dongwon@intel.com>>

This patch series is a replacement of
https://mail.gnu.org/archive/html/qemu-devel/2023-06/msg03989.html


There is a need, expressed by several users, to assign ownership of one
or more physical monitors/connectors to individual guests. This creates
a clear notion of which guest's contents are being displayed on any
given
monitor. Given that there is always a display server/compositor running
on the host, monitor ownership can never truly be transferred to guests.
However, the closest approximation is to request the host compositor to
fullscreen the guest's windows on individual monitors. This allows for
various configurations, such as displaying four different guests'
windows
on four different monitors, a single guest's windows (or virtual
consoles)
on four monitors, or any similar combination.

This patch series attempts to accomplish this by introducing a new
parameter named "connector" to assign monitors to the GFX VCs associated
with a guest. If the assigned monitor is not connected, the guest's
window
will not be displayed, similar to how a host compositor behaves when
connectors are not connected. Once the monitor is hot-plugged, the
guest's
window(s) will be positioned on the assigned monitor.

Usage example:

-display gtk,gl=on,connectors=DP-1:eDP-1:HDMI-2...

In this example, the first graphics virtual console will be placed
on the
DP-1 display, the second on eDP-1, and the third on HDMI-2.


Unfortunately, this approach with GTK is doomed. gtk4 dropped the 
gtk_window_set_position() altogether.


Do you mean we have a plan to lift GTK version in QEMU? Are we going to 
lose all GTK3 specific features?




It's not even clear how the different monitors/outputs/connectors are 
actually named, whether they are stable etc (not mentioning the 
portability).


Window placement & geometry is a job for the compositor. Can you discuss 
this issue with GTK devs & the compositor you are targeting?


I guess you are talking about wayland compositor. We are mainly using 
Xorg on the host and this feature works pretty good on it. I am 
wondering if we limit the feature to Xorg case or adding some warning 
messages with error return in case any of parts is not working?

(like the warning message I added

+model = gdk_monitor_get_model(monitor);
+if (!model) {
+g_warning("retrieving connector name using\n"
+  "gdk_monitor_get_model isn't supported\n"
+  "please do not use connectors param in\n"
+  "current environment\n");
+return -1;
+}
)



--
Marc-André Lureau

Re: [PATCH] ui/gtk: Wait until the current guest frame is rendered before switching to RUN_STATE_SAVE_VM

[Kim, Dongwon]

On 6/4/2024 4:12 AM, Marc-André Lureau wrote:

Hi

On Thu, May 30, 2024 at 2:44 AM > wrote:


From: Dongwon mailto:dongwon@intel.com>>

Make sure rendering of the current frame is finished before switching
the run state to RUN_STATE_SAVE_VM by waiting for egl-sync object to be
signaled.


Can you expand on what this solves?


In current scheme, guest waits for the fence to be signaled for each 
frame it submits before moving to the next frame. If the guest’s state 
is saved while it is still waiting for the fence, The guest will 
continue to  wait for the fence that was signaled while ago when it is 
restored to the point. One way to prevent it is to get it finish the 
current frame before changing the state.





Cc: Marc-André Lureau mailto:marcandre.lur...@redhat.com>>
Cc: Vivek Kasireddy mailto:vivek.kasire...@intel.com>>
Signed-off-by: Dongwon Kim mailto:dongwon@intel.com>>
---
  ui/egl-helpers.c |  2 --
  ui/gtk.c         | 19 +++
  2 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 99b2ebbe23..dafeb36074 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -396,8 +396,6 @@ void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
          fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
                                                sync);
          qemu_dmabuf_set_fence_fd(dmabuf, fence_fd);
-        eglDestroySyncKHR(qemu_egl_display, sync);
-        qemu_dmabuf_set_sync(dmabuf, NULL);


If this function is called multiple times, it will now set a new 
fence_fd each time, and potentially leak older fd. Maybe it could first 
check if a fence_fd exists instead.


We can make that change.



      }
  }

diff --git a/ui/gtk.c b/ui/gtk.c
index 93b13b7a30..cf0dd6abed 100644
--- a/ui/gtk.c
+++ b/ui/gtk.c
@@ -600,9 +600,12 @@ void gd_hw_gl_flushed(void *vcon)

      fence_fd = qemu_dmabuf_get_fence_fd(dmabuf);
      if (fence_fd >= 0) {
+        void *sync = qemu_dmabuf_get_sync(dmabuf);
          qemu_set_fd_handler(fence_fd, NULL, NULL, NULL);
          close(fence_fd);
          qemu_dmabuf_set_fence_fd(dmabuf, -1);
+        eglDestroySyncKHR(qemu_egl_display, sync);
+        qemu_dmabuf_set_sync(dmabuf, NULL);
          graphic_hw_gl_block(vc->gfx.dcl.con, false);
      }
  }
@@ -682,6 +685,22 @@ static const DisplayGLCtxOps egl_ctx_ops = {
  static void gd_change_runstate(void *opaque, bool running,
RunState state)
  {
      GtkDisplayState *s = opaque;
+    QemuDmaBuf *dmabuf;
+    int i;
+
+    if (state == RUN_STATE_SAVE_VM) {
+        for (i = 0; i < s->nb_vcs; i++) {
+            VirtualConsole *vc = >vc[i];
+            dmabuf = vc->gfx.guest_fb.dmabuf;
+            if (dmabuf && qemu_dmabuf_get_fence_fd(dmabuf) >= 0) {
+                /* wait for the rendering to be completed */
+                eglClientWaitSync(qemu_egl_display,
+                                  qemu_dmabuf_get_sync(dmabuf),
+                                  EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
+                                  10);


  I don't think adding waiting points in the migration path is 
appropriate. Perhaps once you explain the actual issue, it will be 
easier to help.


+            }
+        }
+    }

      gd_update_caption(s);
  }
-- 
2.34.1





--
Marc-André Lureau

RE: [PULL 1/5] ui/console: Only declare variable fence_fd when CONFIG_GBM is defined

[Kim, Dongwon]

Thanks and sorry for missing this in the original commit.

Acked-by: Dongwon Kim 

> -Original Message-
> From: Philippe Mathieu-Daudé 
> Sent: Friday, May 17, 2024 8:02 AM
> To: qemu-devel@nongnu.org
> Cc: Cédric Le Goater ; Kim, Dongwon
> ; Marc-André Lureau
> ; Philippe Mathieu-Daudé
> 
> Subject: [PULL 1/5] ui/console: Only declare variable fence_fd when
> CONFIG_GBM is defined
> 
> From: Cédric Le Goater 
> 
> This to avoid a build breakage :
> 
> ../ui/gtk-egl.c: In function ‘gd_egl_draw’:
> ../ui/gtk-egl.c:73:9: error: unused variable ‘fence_fd’ 
> [-Werror=unused-variable]
>73 | int fence_fd;
>   | ^~~~
> 
> Fixes: fa6426805b12 ("ui/console: Use qemu_dmabuf_set_..() helpers instead")
> Cc: Dongwon Kim 
> Cc: Marc-André Lureau 
> Signed-off-by: Cédric Le Goater 
> Reviewed-by: Philippe Mathieu-Daudé 
> Message-ID: <20240515100520.574383-1-...@redhat.com>
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
>  ui/gtk-egl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 0473f689c9..9831c10e1b 100644
> --- a/ui/gtk-egl.c
> +++ b/ui/gtk-egl.c
> @@ -68,9 +68,9 @@ void gd_egl_draw(VirtualConsole *vc)
>  GdkWindow *window;
>  #ifdef CONFIG_GBM
>  QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> +int fence_fd;
>  #endif
>  int ww, wh, ws;
> -int fence_fd;
> 
>  if (!vc->gfx.gls) {
>  return;
> --
> 2.41.0

RE: [PULL 00/11] Ui patches

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Wednesday, May 15, 2024 3:44 AM
> To: Michael Tokarev 
> Cc: qemu-devel@nongnu.org; qemu-stable ;
> hikalium ; Kim, Dongwon 
> Subject: Re: [PULL 00/11] Ui patches
> 
> Hi
> 
> On Wed, May 15, 2024 at 2:29 PM Michael Tokarev  wrote:
> >
> > 14.05.2024 16:17, marcandre.lur...@redhat.com wrote:
> > > 
> > > UI: small fixes and improvements
> > >
> > > 
> > >
> > > Bernhard Beschow (1):
> > >ui/sdl2: Allow host to power down screen
> > >
> > > Dongwon Kim (7):
> > >ui/gtk: Draw guest frame at refresh cycle
> > >ui/gtk: Check if fence_fd is equal to or greater than 0
> > >ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and
> > >  helpers
> > >ui/console: Use qemu_dmabuf_get_..() helpers instead
> > >ui/console: Use qemu_dmabuf_set_..() helpers instead
> > >ui/console: Use qemu_dmabuf_new() and free() helpers instead
> > >ui/console: move QemuDmaBuf struct def to dmabuf.c
> > >
> > > Sergii Zasenko (1):
> > >Allow UNIX socket option for VNC websocket
> > >
> > > hikalium (2):
> > >ui/gtk: Add gd_motion_event trace event
> > >ui/gtk: Fix mouse/motion event scaling issue with GTK display
> > > backend
> >
> >  From this list, it looks like
> >
> >ui/gtk: Draw guest frame at refresh cycle
> 
> I would allow a bit more time for this to be actually more widely tested.
> 
> Dongwon, wdyt?
[Kim, Dongwon] Ok, that sounds good to me.

> 
> >ui/gtk: Check if fence_fd is equal to or greater than 0
> > (questionable, minor issue)
> 
> minor, but fine in stable too.
> 
> >ui/gtk: Fix mouse/motion event scaling issue with GTK display
> > backend
> 
> ok for stable imho (even though I don't like that we don't support hidpi
> correctly, as I described in the patch review)

RE: [PATCH v2] vhost-user-gpu: fix import of DMABUF

[Kim, Dongwon]

Hi Marc-André,

This commit looks good but are you planning to merge this before "ui/console: 
Private QemuDmaBuf struct"?
It will cause some conflict. Let me know if rebasing is needed on "ui/console: 
Private QemuDmaBuf struct".

> -Original Message-
> From: marcandre.lur...@redhat.com 
> Sent: Monday, May 13, 2024 4:13 AM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon ; Marc-André Lureau
> ; Gerd Hoffmann ;
> Michael S. Tsirkin 
> Subject: [PATCH v2] vhost-user-gpu: fix import of DMABUF
> 
> From: Marc-André Lureau 
> 
> When using vhost-user-gpu with GL, qemu -display gtk doesn't show output
> and prints: qemu: eglCreateImageKHR failed
> 
> Since commit 9ac06df8b ("virtio-gpu-udmabuf: correct naming of
> QemuDmaBuf size properties"), egl_dmabuf_import_texture() uses
> backing_{width,height} for the texture dimension.
> 
> Fixes: commit 9ac06df8b ("virtio-gpu-udmabuf: correct naming of
> QemuDmaBuf size properties")
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/display/vhost-user-gpu.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c index
> 709c8a02a1..96743aba8a 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -273,8 +273,10 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> VhostUserGpuMsg *msg)
>  }
>  *dmabuf = (QemuDmaBuf) {
>  .fd = fd,
> -.width = m->fd_width,
> -.height = m->fd_height,
> +.width = m->width,
> +.height = m->height,
> +.backing_width = m->fd_width,
> +.backing_height = m->fd_height,
>  .stride = m->fd_stride,
>  .fourcc = m->fd_drm_fourcc,
>  .y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
> --
> 2.41.0.28.gd7d8841f67

RE: [PATCH] ui/gtk: Explicitly set the default size of new window when untabifying

[Kim, Dongwon]

Hi Marc-André,

I found that the problem is actually due to scaling factor of 0.25 
(VC_SCALE_MIN).

static void gd_update_geometry_hints(VirtualConsole *vc)
{
GtkDisplayState *s = vc->s;
GdkWindowHints mask = 0;
GdkGeometry geo = {};
GtkWidget *geo_widget = NULL;
GtkWindow *geo_window;

if (vc->type == GD_VC_GFX) {
if (!vc->gfx.ds) {
return;
}
if (s->free_scale) {
geo.min_width  = surface_width(vc->gfx.ds) * VC_SCALE_MIN;
geo.min_height = surface_height(vc->gfx.ds) * VC_SCALE_MIN;
mask |= GDK_HINT_MIN_SIZE;
} else {
geo.min_width  = surface_width(vc->gfx.ds) * vc->gfx.scale_x;
geo.min_height = surface_height(vc->gfx.ds) * vc->gfx.scale_y;
mask |= GDK_HINT_MIN_SIZE;
}
geo_widget = vc->gfx.drawing_area;
gtk_widget_set_size_request(geo_widget, geo.min_width, geo.min_height);

s->free_scale is set when 'zoom_to_fit' is set. This 'zoom_to_fit' is normally 
set via param but it is unconditionally set when
ui_info is supported like when some display device is enabled like virtio-vga.

So here free_scale is true so min_width and min_height are set to 1/4 of 
original width/height of the surface (640x480). That is totally fine.
But the real problem is at 

gtk_widget_set_size_request(geo_widget, geo.min_width, geo.min_height);

I do not understand why we are making set size request with these "min" values.

This commit from Gerd originally introduced the 0.25 scaling factor but not 
sure what is intention there.
(I hope Gerd can comment on this.)

commit 82fc18099aa8ee2533add523cc0069f26a83e7b6
Author: Gerd Hoffmann 
Date:   Fri May 16 12:26:12 2014 +0200

gtk: window sizing overhaul

Major overhaul for window size handling.  This basically switches qemu
over to use geometry hints for the window manager instead of trying to
get the job done with widget resize requests.  This allows to specify
better what we need and also avoids window resizes.

FIXME: on gtk2 someone overwrites the geometry hints :(

Signed-off-by: Gerd Hoffmann 

I am wondering if we could just set geo.base_width and height to the normal 
size then use these instead when making size request there.

Can you share your thought?

Thanks,
DW

> -Original Message-
> From: qemu-devel-bounces+dongwon.kim=intel@nongnu.org  devel-bounces+dongwon.kim=intel@nongnu.org> On Behalf Of Kim,
> Dongwon
> Sent: Tuesday, May 7, 2024 9:06 AM
> To: Marc-André Lureau 
> Cc: qemu-devel@nongnu.org; kra...@redhat.com
> Subject: RE: [PATCH] ui/gtk: Explicitly set the default size of new window 
> when
> untabifying
> 
> Hi Marc-André,
> 
> > -Original Message-----
> > From: Marc-André Lureau 
> > Sent: Tuesday, May 7, 2024 8:10 AM
> > To: Kim, Dongwon 
> > Cc: qemu-devel@nongnu.org; kra...@redhat.com
> > Subject: Re: [PATCH] ui/gtk: Explicitly set the default size of new
> > window when untabifying
> >
> > Hi
> >
> > On Wed, May 1, 2024 at 7:47 AM  wrote:
> > >
> > > From: Dongwon Kim 
> > >
> > > When untabifying, the default size of the new window was
> > > inadvertently set to the size smaller than quarter of the primary
> > > window size due to lack of explicit configuration. This commit
> > > addresses the issue by ensuring that the size of untabified windows
> > > is set to match the surface size.
> >
> > From a quick test, I don't see a difference of behaviour after the
> > patch. Could you help me reproduce the issue?
> >
> > I also don't think it is correct for two reasons:
> > - the inner display widget should cause a window size reconfiguration
> > - the window size != display size
> 
> [Kim, Dongwon] Ok, I see this is happening only when virtio-vga device is used
> like
> qemu-system-x86_64 -m 2048 -enable-kvm -cpu host -smp cores=2 -drive
> file=./OVMF.fd,format=raw,if=pflash -device virtio-vga -display gtk,gl=on 
> Maybe
> some setting of dimensions is missing in there? I will take a look.
> 
> >
> > thanks
> >
> > > Cc: Gerd Hoffmann 
> > > Cc: Marc-André Lureau 
> > > Cc: Vivek Kasireddy 
> > > Signed-off-by: Dongwon Kim 
> > > ---
> > >  ui/gtk.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index 810d7fc796..269b8207d7 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -1395,6 +1395,9 @@ static void gd_menu_untabify(GtkMenuItem
> > > *item,
> > void *opaque)
> > >  if (!vc->window) {
> > >  gtk_widget_set_sensitive(vc-&g

RE: [PATCH] ui/gtk: Explicitly set the default size of new window when untabifying

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Tuesday, May 7, 2024 8:10 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; kra...@redhat.com
> Subject: Re: [PATCH] ui/gtk: Explicitly set the default size of new window 
> when
> untabifying
> 
> Hi
> 
> On Wed, May 1, 2024 at 7:47 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > When untabifying, the default size of the new window was inadvertently
> > set to the size smaller than quarter of the primary window size due to
> > lack of explicit configuration. This commit addresses the issue by
> > ensuring that the size of untabified windows is set to match the
> > surface size.
> 
> From a quick test, I don't see a difference of behaviour after the patch. 
> Could
> you help me reproduce the issue?
> 
> I also don't think it is correct for two reasons:
> - the inner display widget should cause a window size reconfiguration
> - the window size != display size

[Kim, Dongwon] Ok, I see this is happening only when virtio-vga device is used 
like
qemu-system-x86_64 -m 2048 -enable-kvm -cpu host -smp cores=2 -drive 
file=./OVMF.fd,format=raw,if=pflash -device virtio-vga -display gtk,gl=on
Maybe some setting of dimensions is missing in there? I will take a look.

> 
> thanks
> 
> > Cc: Gerd Hoffmann 
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/gtk.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..269b8207d7 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1395,6 +1395,9 @@ static void gd_menu_untabify(GtkMenuItem *item,
> void *opaque)
> >  if (!vc->window) {
> >  gtk_widget_set_sensitive(vc->menu_item, false);
> >  vc->window = gtk_window_new(GTK_WINDOW_TOPLEVEL);
> > +gtk_window_set_default_size(GTK_WINDOW(vc->window),
> > +surface_width(vc->gfx.ds),
> > +surface_height(vc->gfx.ds));
> >  #if defined(CONFIG_OPENGL)
> >  if (vc->gfx.esurface) {
> >  eglDestroySurface(qemu_egl_display, vc->gfx.esurface);
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau

RE: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Tuesday, April 23, 2024 7:07 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; marcandre.lur...@redhat.com;
> phi...@linaro.org
> Subject: Re: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for
> QemuDmaBuf struct and helpers
> 
> On Mon, Apr 22, 2024 at 07:22:49PM -0700, dongwon@intel.com wrote:
> > From: Dongwon Kim 
> >
> > New header and source files are added for containing QemuDmaBuf struct
> > definition and newly introduced helpers for creating/freeing the
> > struct and accessing its data.
> >
> > v10: Change the license type for both dmabuf.h and dmabuf.c from MIT to
> >  GPL to be in line with QEMU's default license
> >  (Daniel P. Berrangé )
> >
> > Suggested-by: Marc-André Lureau 
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Daniel P. Berrangé 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/console.h |  20 +
> >  include/ui/dmabuf.h  |  64 +++
> >  ui/dmabuf.c  | 189 +++
> >  ui/meson.build   |   1 +
> >  4 files changed, 255 insertions(+), 19 deletions(-)  create mode
> > 100644 include/ui/dmabuf.h  create mode 100644 ui/dmabuf.c
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..a208a68b88 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -7,6 +7,7 @@
> >  #include "qapi/qapi-types-ui.h"
> >  #include "ui/input.h"
> >  #include "ui/surface.h"
> > +#include "ui/dmabuf.h"
> >
> >  #define TYPE_QEMU_CONSOLE "qemu-console"
> >  OBJECT_DECLARE_TYPE(QemuConsole, QemuConsoleClass, QEMU_CONSOLE)
> @@
> > -185,25 +186,6 @@ struct QEMUGLParams {
> >  int minor_ver;
> >  };
> >
> > -typedef struct QemuDmaBuf {
> > -int   fd;
> > -uint32_t  width;
> > -uint32_t  height;
> > -uint32_t  stride;
> > -uint32_t  fourcc;
> > -uint64_t  modifier;
> > -uint32_t  texture;
> > -uint32_t  x;
> > -uint32_t  y;
> > -uint32_t  backing_width;
> > -uint32_t  backing_height;
> > -bool  y0_top;
> > -void  *sync;
> > -int   fence_fd;
> > -bool  allow_fences;
> > -bool  draw_submitted;
> > -} QemuDmaBuf;
> > -
> >  enum display_scanout {
> >  SCANOUT_NONE,
> >  SCANOUT_SURFACE,
> > diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h new file mode
> > 100644 index 00..7a60116ee6
> > --- /dev/null
> > +++ b/include/ui/dmabuf.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * QemuDmaBuf struct and helpers used for accessing its data
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef DMABUF_H
> > +#define DMABUF_H
> > +
> > +typedef struct QemuDmaBuf {
> > +int   fd;
> > +uint32_t  width;
> > +uint32_t  height;
> > +uint32_t  stride;
> > +uint32_t  fourcc;
> > +uint64_t  modifier;
> > +uint32_t  texture;
> > +uint32_t  x;
> > +uint32_t  y;
> > +uint32_t  backing_width;
> > +uint32_t  backing_height;
> > +bool  y0_top;
> > +void  *sync;
> > +int   fence_fd;
> > +bool  allow_fences;
> > +bool  draw_submitted;
> > +} QemuDmaBuf;
> > +
> > +QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > +   uint32_t stride, uint32_t x,
> > +   uint32_t y, uint32_t backing_width,
> > +   uint32_t backing_height, uint32_t 
> > fourcc,
> > +   uint64_t modifier, int32_t
> > +dmabuf_fd,
> 
> Should be 'int' not 'int32_t' for FDs.
> 
> > +   bool allow_fences, bool y0_top);
> > +void qemu_dmabuf_free(QemuDmaBuf *dmabuf);
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuDmaBuf, qemu_dmabuf_free);
> > +
> > +int32_t qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
> 
> Again should be 'int' not 'int42_t'
> 
> I think there ought to also be a
> 
>   int qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf);
> 
> to do th

RE: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for QemuDmaBuf struct and helpers

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Tuesday, April 23, 2024 7:07 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; marcandre.lur...@redhat.com;
> phi...@linaro.org
> Subject: Re: [PATCH v10 2/6] ui/console: new dmabuf.h and dmabuf.c for
> QemuDmaBuf struct and helpers
> 
> On Mon, Apr 22, 2024 at 07:22:49PM -0700, dongwon@intel.com wrote:
> > From: Dongwon Kim 
> >
> > New header and source files are added for containing QemuDmaBuf struct
> > definition and newly introduced helpers for creating/freeing the
> > struct and accessing its data.
> >
> > v10: Change the license type for both dmabuf.h and dmabuf.c from MIT to
> >  GPL to be in line with QEMU's default license
> >  (Daniel P. Berrangé )
> >
> > Suggested-by: Marc-André Lureau 
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Daniel P. Berrangé 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/console.h |  20 +
> >  include/ui/dmabuf.h  |  64 +++
> >  ui/dmabuf.c  | 189
> +++
> >  ui/meson.build   |   1 +
> >  4 files changed, 255 insertions(+), 19 deletions(-)  create mode
> > 100644 include/ui/dmabuf.h  create mode 100644 ui/dmabuf.c
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..a208a68b88 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -7,6 +7,7 @@
> >  #include "qapi/qapi-types-ui.h"
> >  #include "ui/input.h"
> >  #include "ui/surface.h"
> > +#include "ui/dmabuf.h"
> >
> >  #define TYPE_QEMU_CONSOLE "qemu-console"
> >  OBJECT_DECLARE_TYPE(QemuConsole, QemuConsoleClass,
> QEMU_CONSOLE) @@
> > -185,25 +186,6 @@ struct QEMUGLParams {
> >  int minor_ver;
> >  };
> >
> > -typedef struct QemuDmaBuf {
> > -int   fd;
> > -uint32_t  width;
> > -uint32_t  height;
> > -uint32_t  stride;
> > -uint32_t  fourcc;
> > -uint64_t  modifier;
> > -uint32_t  texture;
> > -uint32_t  x;
> > -uint32_t  y;
> > -uint32_t  backing_width;
> > -uint32_t  backing_height;
> > -bool  y0_top;
> > -void  *sync;
> > -int   fence_fd;
> > -bool  allow_fences;
> > -bool  draw_submitted;
> > -} QemuDmaBuf;
> > -
> >  enum display_scanout {
> >  SCANOUT_NONE,
> >  SCANOUT_SURFACE,
> > diff --git a/include/ui/dmabuf.h b/include/ui/dmabuf.h new file mode
> > 100644 index 00..7a60116ee6
> > --- /dev/null
> > +++ b/include/ui/dmabuf.h
> > @@ -0,0 +1,64 @@
> > +/*
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + *
> > + * QemuDmaBuf struct and helpers used for accessing its data
> > + *
> > + * This work is licensed under the terms of the GNU GPL, version 2 or 
> > later.
> > + * See the COPYING file in the top-level directory.
> > + */
> > +
> > +#ifndef DMABUF_H
> > +#define DMABUF_H
> > +
> > +typedef struct QemuDmaBuf {
> > +int   fd;
> > +uint32_t  width;
> > +uint32_t  height;
> > +uint32_t  stride;
> > +uint32_t  fourcc;
> > +uint64_t  modifier;
> > +uint32_t  texture;
> > +uint32_t  x;
> > +uint32_t  y;
> > +uint32_t  backing_width;
> > +uint32_t  backing_height;
> > +bool  y0_top;
> > +void  *sync;
> > +int   fence_fd;
> > +bool  allow_fences;
> > +bool  draw_submitted;
> > +} QemuDmaBuf;
> > +
> > +QemuDmaBuf *qemu_dmabuf_new(uint32_t width, uint32_t height,
> > +   uint32_t stride, uint32_t x,
> > +   uint32_t y, uint32_t backing_width,
> > +   uint32_t backing_height, uint32_t 
> > fourcc,
> > +   uint64_t modifier, int32_t
> > +dmabuf_fd,
> 
> Should be 'int' not 'int32_t' for FDs.
> 
> > +   bool allow_fences, bool y0_top);
> > +void qemu_dmabuf_free(QemuDmaBuf *dmabuf);
> > +
> > +G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuDmaBuf,
> qemu_dmabuf_free);
> > +
> > +int32_t qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf);
> 
> Again should be 'int' not 'int42_t'
> 
> I think there ought to also be a
> 
>   int qemu_dmabuf_dup_fd(QemuDmaBuf *dmabuf);
> 
>

RE: [PATCH v6 1/3] ui/console: Introduce dpy_gl_qemu_dmabuf_get_..() helpers

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Wednesday, April 17, 2024 4:05 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; marcandre.lur...@redhat.com
> Subject: Re: [PATCH v6 1/3] ui/console: Introduce
> dpy_gl_qemu_dmabuf_get_..() helpers
> 
> On Tue, Apr 16, 2024 at 09:09:52PM -0700, dongwon@intel.com wrote:
> > From: Dongwon Kim 
> >
> > This commit introduces dpy_gl_qemu_dmabuf_get_... helpers to extract
> > specific fields from the QemuDmaBuf struct. It also updates all
> > instances where fields within the QemuDmaBuf struct are directly
> > accessed, replacing them with calls to these new helper functions.
> >
> > v6: fix typos in helper names in ui/spice-display.c
> >
> > Suggested-by: Marc-André Lureau 
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/console.h|  17 +
> >  hw/display/vhost-user-gpu.c |   6 +-
> >  hw/display/virtio-gpu-udmabuf.c |   7 +-
> >  hw/vfio/display.c   |  15 +++--
> >  ui/console.c| 116 +++-
> >  ui/dbus-console.c   |   9 ++-
> >  ui/dbus-listener.c  |  43 +++-
> >  ui/egl-headless.c   |  23 +--
> >  ui/egl-helpers.c|  47 +++--
> >  ui/gtk-egl.c|  48 -
> >  ui/gtk-gl-area.c|  37 ++
> >  ui/gtk.c|   6 +-
> >  ui/spice-display.c  |  50 --
> >  13 files changed, 316 insertions(+), 108 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..6292943a82 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -358,6 +358,23 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con,
> QemuDmaBuf *dmabuf,
> >bool have_hot, uint32_t hot_x, uint32_t
> > hot_y);  void dpy_gl_cursor_position(QemuConsole *con,
> >  uint32_t pos_x, uint32_t pos_y);
> > +
> > +int32_t dpy_gl_qemu_dmabuf_get_fd(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_width(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_height(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_stride(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_fourcc(QemuDmaBuf *dmabuf); uint64_t
> > +dpy_gl_qemu_dmabuf_get_modifier(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_texture(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_x(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_y(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_qemu_dmabuf_get_backing_width(QemuDmaBuf *dmabuf);
> uint32_t
> > +dpy_gl_qemu_dmabuf_get_backing_height(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_y0_top(QemuDmaBuf *dmabuf); void
> > +*dpy_gl_qemu_dmabuf_get_sync(QemuDmaBuf *dmabuf); int32_t
> > +dpy_gl_qemu_dmabuf_get_fence_fd(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_allow_fences(QemuDmaBuf *dmabuf); bool
> > +dpy_gl_qemu_dmabuf_get_draw_submitted(QemuDmaBuf *dmabuf);
> 
> IMHO these method names don't need a "dpy_gl_" prefix on them. Since
> they're accessors for the "QemuDmaBuf" struct, I think its sufficient to just
> have "qemu_dmabuf_" as the name prefix, making names more compact.
> 
> The console.{h,c} files are a bit of a dumping ground for UI code. While
> QemuDmaBuf was just a struct with direct field access that's OK.
> 
> With turning this into a more of an object with accessors, I think it would 
> also
> be desirable to move the struct definition and all its methods into separate
> ui/dmabuf.{c,h} files, so its fully self-contained.
 
[Kim, Dongwon] I am ok with changing function names and create
separate c and h for dmabuf helpers as you suggested. But I would
like to hear Marc-André's opinion about this suggestion before I make
such changes.

Marc-André, do you have any thought on Daniel's suggestion?

> 
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

RE: [PATCH] vhost-user-gpu: fix import of DMABUF

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: marcandre.lur...@redhat.com 
> Sent: Monday, April 15, 2024 4:16 AM
> To: qemu-devel@nongnu.org
> Cc: Kim, Dongwon ; dbas...@redhat.com; Marc-
> André Lureau ; Michael S. Tsirkin
> ; Gerd Hoffmann 
> Subject: [PATCH] vhost-user-gpu: fix import of DMABUF
> 
> From: Marc-André Lureau 
> 
> When using vhost-user-gpu with GL, qemu -display gtk doesn't show output and
> prints: qemu: eglCreateImageKHR failed
> 
> Since commit 9ac06df8b ("virtio-gpu-udmabuf: correct naming of QemuDmaBuf
> size properties"), egl_dmabuf_import_texture() uses backing_{width,height} for
> the texture dimension.
> 
> Fixes: commit 9ac06df8b ("virtio-gpu-udmabuf: correct naming of QemuDmaBuf
> size properties")
> Signed-off-by: Marc-André Lureau 
> ---
>  hw/display/vhost-user-gpu.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c index
> 709c8a02a1..baffb1c2d4 100644
> --- a/hw/display/vhost-user-gpu.c
> +++ b/hw/display/vhost-user-gpu.c
> @@ -273,8 +273,8 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> VhostUserGpuMsg *msg)
>  }
>  *dmabuf = (QemuDmaBuf) {
>  .fd = fd,
> -.width = m->fd_width,
> -.height = m->fd_height,
[Kim, Dongwon]  I think we could just leave .width/.height setting here 
although nothing will go wrong in any cases. Did you have any specific reason 
why leaving these uninitialized?

> +.backing_width = m->fd_width,
> +.backing_height = m->fd_height,
>  .stride = m->fd_stride,
>  .fourcc = m->fd_drm_fourcc,
>  .y0_top = m->fd_flags & VIRTIO_GPU_RESOURCE_FLAG_Y_0_TOP,
> --
> 2.41.0.28.gd7d8841f67

RE: [PATCH v4 1/3] ui/console: Introduce dpy_gl_dmabuf_get_height/width() helpers

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Friday, March 22, 2024 2:06 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; phi...@linaro.org
> Subject: Re: [PATCH v4 1/3] ui/console: Introduce
> dpy_gl_dmabuf_get_height/width() helpers
> 
> Hi Kim
> 
> On Fri, Mar 22, 2024 at 3:45 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > dpy_gl_dmabuf_get_height() and dpy_gl_dmabuf_get_width() are helpers
> > for retrieving width and height fields from QemuDmaBuf struct.
> >
> 
> There are many places left where width/height fields are still accessed 
> directly.
> 
> If we want to make the whole structure private, we will probably need setters.
[Kim, Dongwon]  I am wondering if you are saying we need setters and getters 
for all individual fields and use those new functions in any places in QEMU 
where any of those fields are accessed including ui/* (e.g. gtk-egl.c)? 

> 
> I don't see why the function should silently return 0 when given NULL.
> Imho an assert(dmabuf != NULL) is appropriate (or g_return_val_if_fail).
[Kim, Dongwon] Yeah I can do that. I will update that part.

> 
> 
> 
> 
> 
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/console.h|  2 ++
> >  hw/display/virtio-gpu-udmabuf.c |  7 ---
> >  hw/vfio/display.c   |  9 ++---
> >  ui/console.c| 18 ++
> >  4 files changed, 30 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > 0bc7a00ac0..6064487fc4 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -358,6 +358,8 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con,
> QemuDmaBuf *dmabuf,
> >bool have_hot, uint32_t hot_x, uint32_t
> > hot_y);  void dpy_gl_cursor_position(QemuConsole *con,
> >  uint32_t pos_x, uint32_t pos_y);
> > +uint32_t dpy_gl_dmabuf_get_width(QemuDmaBuf *dmabuf); uint32_t
> > +dpy_gl_dmabuf_get_height(QemuDmaBuf *dmabuf);
> >  void dpy_gl_release_dmabuf(QemuConsole *con,
> > QemuDmaBuf *dmabuf);  void
> > dpy_gl_update(QemuConsole *con, diff --git
> > a/hw/display/virtio-gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c
> > index d51184d658..a4ebf828ec 100644
> > --- a/hw/display/virtio-gpu-udmabuf.c
> > +++ b/hw/display/virtio-gpu-udmabuf.c
> > @@ -206,6 +206,7 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,  {
> >  struct virtio_gpu_scanout *scanout = 
> > >parent_obj.scanout[scanout_id];
> >  VGPUDMABuf *new_primary, *old_primary = NULL;
> > +uint32_t width, height;
> >
> >  new_primary = virtio_gpu_create_dmabuf(g, scanout_id, res, fb, r);
> >  if (!new_primary) {
> > @@ -216,10 +217,10 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,
> >  old_primary = g->dmabuf.primary[scanout_id];
> >  }
> >
> > +width = dpy_gl_dmabuf_get_width(_primary->buf);
> > +height = dpy_gl_dmabuf_get_height(_primary->buf);
> >  g->dmabuf.primary[scanout_id] = new_primary;
> > -qemu_console_resize(scanout->con,
> > -new_primary->buf.width,
> > -new_primary->buf.height);
> > +qemu_console_resize(scanout->con, width, height);
> >  dpy_gl_scanout_dmabuf(scanout->con, _primary->buf);
> >
> >  if (old_primary) {
> > diff --git a/hw/vfio/display.c b/hw/vfio/display.c
> > index 1aa440c663..c962e5f88f 100644
> > --- a/hw/vfio/display.c
> > +++ b/hw/vfio/display.c
> > @@ -286,6 +286,7 @@ static void vfio_display_dmabuf_update(void
> *opaque)
> >  VFIOPCIDevice *vdev = opaque;
> >  VFIODisplay *dpy = vdev->dpy;
> >  VFIODMABuf *primary, *cursor;
> > +uint32_t width, height;
> >  bool free_bufs = false, new_cursor = false;
> >
> >  primary = vfio_display_get_dmabuf(vdev, DRM_PLANE_TYPE_PRIMARY);
> > @@ -296,10 +297,12 @@ static void vfio_display_dmabuf_update(void
> *opaque)
> >  return;
> >  }
> >
> > +width = dpy_gl_dmabuf_get_width(>buf);
> > +height = dpy_gl_dmabuf_get_height(>buf);
> > +
> >  if (dpy->dmabuf.primary != primary) {
> >  dpy->dmabuf.primary = primary;
> > -qemu_console_resize(dpy->con,
> > -primary->buf.width, primary->buf.height);
> > +qemu_consol

RE: [PATCH v4 3/3] ui/console: Introduce dpy_gl_create_dmabuf() helper

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Friday, March 22, 2024 2:06 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; phi...@linaro.org
> Subject: Re: [PATCH v4 3/3] ui/console: Introduce dpy_gl_create_dmabuf()
> helper
> 
> Hi
> 
> On Fri, Mar 22, 2024 at 3:45 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > dpy_gl_create_dmabuf() allocates QemuDmaBuf and initialize fields.
> > hw/display modules, hw/vfio and ui/dbus-listener now use this method
> > to create QemuDmaBuf instead of declaring and initializing it on their
> > own.
> >
> > Cc: Philippe Mathieu-Daudé 
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/hw/vfio/vfio-common.h   |  2 +-
> >  include/hw/virtio/virtio-gpu.h  |  4 ++--
> >  include/ui/console.h|  6 ++
> >  hw/display/vhost-user-gpu.c | 33 ++---
> >  hw/display/virtio-gpu-udmabuf.c | 23 ---
> >  hw/vfio/display.c   | 26 +++---
> >  ui/console.c| 28 
> >  ui/dbus-listener.c  | 28 
> >  8 files changed, 86 insertions(+), 64 deletions(-)
> >
> > diff --git a/include/hw/vfio/vfio-common.h
> > b/include/hw/vfio/vfio-common.h index b9da6c08ef..d66e27db02 100644
> > --- a/include/hw/vfio/vfio-common.h
> > +++ b/include/hw/vfio/vfio-common.h
> > @@ -148,7 +148,7 @@ typedef struct VFIOGroup {  } VFIOGroup;
> >
> >  typedef struct VFIODMABuf {
> > -QemuDmaBuf buf;
> > +QemuDmaBuf *buf;
> >  uint32_t pos_x, pos_y, pos_updates;
> >  uint32_t hot_x, hot_y, hot_updates;
> >  int dmabuf_id;
> > diff --git a/include/hw/virtio/virtio-gpu.h
> > b/include/hw/virtio/virtio-gpu.h index ed44cdad6b..56d6e821bf 100644
> > --- a/include/hw/virtio/virtio-gpu.h
> > +++ b/include/hw/virtio/virtio-gpu.h
> > @@ -169,7 +169,7 @@ struct VirtIOGPUBaseClass {
> >  DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
> >
> >  typedef struct VGPUDMABuf {
> > -QemuDmaBuf buf;
> > +QemuDmaBuf *buf;
> >  uint32_t scanout_id;
> >  QTAILQ_ENTRY(VGPUDMABuf) next;
> >  } VGPUDMABuf;
> > @@ -238,7 +238,7 @@ struct VhostUserGPU {
> >  VhostUserBackend *vhost;
> >  int vhost_gpu_fd; /* closed by the chardev */
> >  CharBackend vhost_chr;
> > -QemuDmaBuf dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
> > +QemuDmaBuf *dmabuf[VIRTIO_GPU_MAX_SCANOUTS];
> >  bool backend_blocked;
> >  };
> >
> > diff --git a/include/ui/console.h b/include/ui/console.h index
> > d5334a806c..01e998264b 100644
> > --- a/include/ui/console.h
> > +++ b/include/ui/console.h
> > @@ -358,6 +358,12 @@ void dpy_gl_cursor_dmabuf(QemuConsole *con,
> QemuDmaBuf *dmabuf,
> >bool have_hot, uint32_t hot_x, uint32_t
> > hot_y);  void dpy_gl_cursor_position(QemuConsole *con,
> >  uint32_t pos_x, uint32_t pos_y);
> > +QemuDmaBuf *dpy_gl_create_dmabuf(uint32_t width, uint32_t height,
> > + uint32_t stride, uint32_t x,
> > + uint32_t y, uint32_t backing_width,
> > + uint32_t backing_height, uint32_t fourcc,
> > + uint64_t modifier, uint32_t dmabuf_fd,
> > + bool allow_fences, bool y0_top);
> >  uint32_t dpy_gl_dmabuf_get_width(QemuDmaBuf *dmabuf);  uint32_t
> > dpy_gl_dmabuf_get_height(QemuDmaBuf *dmabuf);  int32_t
> > dpy_gl_dmabuf_get_fd(QemuDmaBuf *dmabuf); diff --git
> > a/hw/display/vhost-user-gpu.c b/hw/display/vhost-user-gpu.c index
> > 709c8a02a1..0e49a934ed 100644
> > --- a/hw/display/vhost-user-gpu.c
> > +++ b/hw/display/vhost-user-gpu.c
> > @@ -249,6 +249,7 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> VhostUserGpuMsg *msg)
> >  case VHOST_USER_GPU_DMABUF_SCANOUT: {
> >  VhostUserGpuDMABUFScanout *m = >payload.dmabuf_scanout;
> >  int fd = qemu_chr_fe_get_msgfd(>vhost_chr);
> > +uint64_t modifier = 0;
> >  QemuDmaBuf *dmabuf;
> >
> >  if (m->scanout_id >= g->parent_obj.conf.max_outputs) { @@
> > -261,30 +262,32 @@ vhost_user_gpu_handle_display(VhostUserGPU *g,
> > VhostUserGpuMsg *msg)
> >
> >  g->parent_obj.enable = 1;
> >  con = g->paren

RE: [INCOMPLETE PATCH v3 3/3] ui/console: Introduce dpy_gl_create_dmabuf() helper

[Kim, Dongwon]

Hi Phil,

I submitted revised series (v4) based on your v3 work. I tried to address 2 or 
3 major issues you mentioned there and some minor corrections.
Please check it. I appreciate your help and feedback.

[PATCH v4 0/3] ui/console: Introduce helpers for creating and

> -Original Message-
> From: Philippe Mathieu-Daudé 
> Sent: Thursday, March 21, 2024 3:07 AM
> To: Kim, Dongwon ; qemu-devel@nongnu.org
> Cc: Alex Williamson ; Cédric Le Goater
> ; Marc-André Lureau ;
> Kasireddy, Vivek ; Michael S. Tsirkin
> ; Gerd Hoffmann ; Philippe Mathieu-
> Daudé 
> Subject: [INCOMPLETE PATCH v3 3/3] ui/console: Introduce
> dpy_gl_create_dmabuf() helper
> 
> From: Dongwon Kim 
> 
> It is safer to create, initialize, and access all the parameters in QemuDmaBuf
> from a central location, ui/console, instead of hw/virtio-gpu or hw/vfio 
> modules.
> 
> Signed-off-by: Dongwon Kim 
> Message-Id: <20240320034229.3347130-1-dongwon@intel.com>
> [PMD: Split patch in 3, part 3/3]
> Signed-off-by: Philippe Mathieu-Daudé 
> ---
> Incomplete... VhostUserGPU doesn't allocate,
> vhost_user_gpu_handle_display() crashes.
> ---
>  include/hw/vfio/vfio-common.h   |  2 +-
>  include/hw/virtio/virtio-gpu.h  |  2 +-
>  include/ui/console.h|  7 +++
>  hw/display/virtio-gpu-udmabuf.c | 23 ---
>  hw/vfio/display.c   | 24 ++--
>  ui/console.c| 28 
>  6 files changed, 55 insertions(+), 31 deletions(-)
> 
> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
> index b9da6c08ef..d66e27db02 100644
> --- a/include/hw/vfio/vfio-common.h
> +++ b/include/hw/vfio/vfio-common.h
> @@ -148,7 +148,7 @@ typedef struct VFIOGroup {  } VFIOGroup;
> 
>  typedef struct VFIODMABuf {
> -QemuDmaBuf buf;
> +QemuDmaBuf *buf;
>  uint32_t pos_x, pos_y, pos_updates;
>  uint32_t hot_x, hot_y, hot_updates;
>  int dmabuf_id;
> diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h 
> index
> ed44cdad6b..010083e8e3 100644
> --- a/include/hw/virtio/virtio-gpu.h
> +++ b/include/hw/virtio/virtio-gpu.h
> @@ -169,7 +169,7 @@ struct VirtIOGPUBaseClass {
>  DEFINE_PROP_UINT32("yres", _state, _conf.yres, 800)
> 
>  typedef struct VGPUDMABuf {
> -QemuDmaBuf buf;
> +QemuDmaBuf *buf;
>  uint32_t scanout_id;
>  QTAILQ_ENTRY(VGPUDMABuf) next;
>  } VGPUDMABuf;
> diff --git a/include/ui/console.h b/include/ui/console.h index
> 1f3d025548..0b823efb2e 100644
> --- a/include/ui/console.h
> +++ b/include/ui/console.h
> @@ -279,6 +279,7 @@ typedef struct DisplayChangeListenerOps {
>  /* optional */
>  void (*dpy_gl_cursor_position)(DisplayChangeListener *dcl,
> uint32_t pos_x, uint32_t pos_y);
> +
>  /* optional */
>  void (*dpy_gl_release_dmabuf)(DisplayChangeListener *dcl,
>QemuDmaBuf *dmabuf); @@ -358,6 +359,12 @@ 
> void
> dpy_gl_cursor_dmabuf(QemuConsole *con, QemuDmaBuf *dmabuf,
>bool have_hot, uint32_t hot_x, uint32_t hot_y);  
> void
> dpy_gl_cursor_position(QemuConsole *con,
>  uint32_t pos_x, uint32_t pos_y);
> +QemuDmaBuf *dpy_gl_create_dmabuf(uint32_t width, uint32_t height,
> + uint32_t stride, uint32_t x,
> + uint32_t y, uint32_t backing_width,
> + uint32_t backing_height, uint32_t fourcc,
> + uint32_t modifier, uint32_t dmabuf_fd,
> + bool allow_fences);
>  uint32_t dpy_gl_dmabuf_get_width(QemuDmaBuf *dmabuf);  uint32_t
> dpy_gl_dmabuf_get_height(QemuDmaBuf *dmabuf);  int32_t
> dpy_gl_dmabuf_get_fd(QemuDmaBuf *dmabuf); diff --git a/hw/display/virtio-
> gpu-udmabuf.c b/hw/display/virtio-gpu-udmabuf.c index
> d680e871c1..dde6c8e9d9 100644
> --- a/hw/display/virtio-gpu-udmabuf.c
> +++ b/hw/display/virtio-gpu-udmabuf.c
> @@ -162,7 +162,7 @@ static void virtio_gpu_free_dmabuf(VirtIOGPU *g,
> VGPUDMABuf *dmabuf)
>  struct virtio_gpu_scanout *scanout;
> 
>  scanout = >parent_obj.scanout[dmabuf->scanout_id];
> -dpy_gl_release_dmabuf(scanout->con, >buf);
> +dpy_gl_release_dmabuf(scanout->con, dmabuf->buf);
>  QTAILQ_REMOVE(>dmabuf.bufs, dmabuf, next);
>  g_free(dmabuf);
>  }
> @@ -181,17 +181,10 @@ static VGPUDMABuf
>  }
> 
>  dmabuf = g_new0(VGPUDMABuf, 1);
> -dmabuf->buf.width = r->width;
> -dmabuf->buf.height = r->height;
> -dmabuf->buf.strid

RE: [PATCH 0/3] ui/console: initialize QemuDmaBuf in ui/console

[Kim, Dongwon]

Hi Phlippe,

> -Original Message-
> From: Philippe Mathieu-Daudé 
> Sent: Wednesday, March 20, 2024 11:57 PM
> To: Kim, Dongwon ; qemu-devel@nongnu.org
> Cc: marcandre.lur...@redhat.com
> Subject: Re: [PATCH 0/3] ui/console: initialize QemuDmaBuf in ui/console
> 
> Hi Dongwon,
> 
> On 20/3/24 21:50, dongwon@intel.com wrote:
> > From: Dongwon Kim 
> >
> > QemuDmaBuf struct is defined and primarily used by ui/console/gl so it
> > is better to handle its creation, initialization and access within
> > ui/console rather than within hw modules such as hw/display/virtio-gpu
> > and hw/vfio/display.
> >
> > To achieve this, new methods for allocating, initializing the struct,
> > and accessing certain fields necessary for hardware modules have been
> > introduced in ui/console.c.
> > (3rd patch)
> >
> > Furthermore, modifications have been made to hw/display/virtio-gpu and
> > hw/vfio/display to utilize these new methods instead of setting up the
> > struct independently.
> > (1st and 2nd patches)
> 
> Thanks for splitting, unfortunately the series isn't buildable / bisectable 
> since the
> methods use in patches 1&2 are only introduced in patch 3 :/
[Kim, Dongwon]  Maybe changing order of patches like 3-1-2 would be acceptable?

> 
> > Dongwon Kim (3):
> >hw/virtio: intialize QemuDmaBuf using the function from ui/console
> >hw/vfio: intialize QemuDmaBuf using the function from ui/console
> >ui/console: add methods for allocating, intializing and accessing
> >  QemuDmaBuf

RE: [PATCH v3] ui/gtk: flush display pipeline before saving vmstate when blob=true

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Tuesday, March 12, 2024 4:27 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH v3] ui/gtk: flush display pipeline before saving vmstate 
> when
> blob=true
> 
> Hi
> 
> On Thu, Mar 7, 2024 at 2:27 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > If the guest state is paused before it gets a response for the current
> > scanout frame submission (resource-flush), it won't flush new frames
> > after being restored as it still waits for the old response, which is
> > accepted as a scanout render done signal. So it's needed to unblock
> > the current scanout render pipeline before the run state is changed to
> > make sure the guest receives the response for the current frame
> > submission.
> >
> > v2: Giving some time for the fence to be signaled before flushing
> > the pipeline
> >
> > v3: Prevent redundant call of gd_hw_gl_flushed by checking dmabuf
> > and fence_fd >= 0 in it (e.g. during and after eglClientWaitSync
> > in gd_change_runstate).
> >
> > Destroy sync object later in gd_hw_fl_flushed
> >
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/egl-helpers.c |  2 --
> >  ui/gtk.c | 31 +++
> >  2 files changed, 27 insertions(+), 6 deletions(-)
> >
> > diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index
> > 3d19dbe382..a77f9e57d9 100644
> > --- a/ui/egl-helpers.c
> > +++ b/ui/egl-helpers.c
> > @@ -385,8 +385,6 @@ void egl_dmabuf_create_fence(QemuDmaBuf
> *dmabuf)
> >  if (dmabuf->sync) {
> 
> We may want to check that no fence_fd exists before, to avoid leaks.

[Kim, Dongwon]  OK
> 
> I also notice that fence_fd is initialized with 0 in 
> vfio_display_get_dmabuf(). It
> would probably make sense to introduce functions to allocate, set and get 
> fields
> from QemuDmaBuf and make the struct private, as it is too easy to do a wrong
> initialization...
[Kim, Dongwon] Yeah I will look into this.
> 
> 
> >  dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
> >dmabuf->sync);
> > -eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> > -dmabuf->sync = NULL;
> >  }
> >  }
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..eaca890cba 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -597,10 +597,14 @@ void gd_hw_gl_flushed(void *vcon)
> >  VirtualConsole *vc = vcon;
> >  QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> >
> > -qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > -close(dmabuf->fence_fd);
> > -dmabuf->fence_fd = -1;
> > -graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +if (dmabuf && dmabuf->fence_fd >= 0) {
> 
> It may have failed to create the fence_fd, but succeeded in creating the 
> sync, in
> which case it will leak the sync.
> 
> Btw, can't the fence_fd be created at the same time as the sync instead of
> having two functions?
[Kim, Dongwon] I will take a look. 
> 
> I also noticed that fenced_fd is incorrectly checked for > 0 instead of >= 0 
> in gtk-
> egl.c and gtk-gl-area.c. Can you fix that as well?
[Kim, Dongwon] Sure I will work on v2 with suggested changes.

Thanks,
DW

> 
> > +qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > +close(dmabuf->fence_fd);
> > +dmabuf->fence_fd = -1;
> > +eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> > +dmabuf->sync = NULL;
> > +graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +}
> >  }
> >
> >  /** DisplayState Callbacks (opengl version) **/ @@ -678,6 +682,25 @@
> > static const DisplayGLCtxOps egl_ctx_ops = {  static void
> > gd_change_runstate(void *opaque, bool running, RunState state)  {
> >  GtkDisplayState *s = opaque;
> > +int i;
> > +
> > +if (state == RUN_STATE_SAVE_VM) {
> > +for (i = 0; i < s->nb_vcs; i++) {
> > +VirtualConsole *vc = >vc[i];
> > +
> > +if (vc->gfx.guest_fb.dmabuf &&
> > +vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
> > +eglClientWaitSync(qemu_egl_display,
> > +  vc->gfx.guest_fb.dmabuf->sync,
> > +  EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
> > +  1);
> > +
> > +/* force flushing current scanout blob rendering process
> > + * just in case the fence is still not signaled */
> > +gd_hw_gl_flushed(vc);
> > +}
> > +}
> > +}
> >
> >  gd_update_caption(s);
> >  }
> > --
> > 2.34.1
> >
> >
> 
> thanks
> 
> 
> --
> Marc-André Lureau

RE: [PATCH v2] virtio-gpu: first surface update with blob scanout after resumed

[Kim, Dongwon]

Hi Marc-André,

Ok, seems like your commit would fix the problem I was trying to solve. I will 
test it and let you know the results.

> -Original Message-
> From: Marc-André Lureau 
> Sent: Tuesday, March 12, 2024 4:35 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH v2] virtio-gpu: first surface update with blob scanout 
> after
> resumed
> 
> Hi
> 
> On Thu, Mar 7, 2024 at 2:27 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > The guest surface needs to be updated with a blob scanout after
> > resumed from saved vm state if blob is enabled.
> >
> > v2: Rebased
> >
> 
> This patch conflicts with the already reviewed (and almost in queue) patch 
> from
> https://patchew.org/QEMU/20240228122323.962826-1-
> marcandre.lur...@redhat.com/20240228122323.962826-3-
> marcandre.lur...@redhat.com/.
> 
> There are chances that it solves your problem, since it calls into
> virtio_gpu_do_set_scanout() which handles blob resources.
> 
> Could you check? And if it doesn't fix it, can you apply your solution on top 
> of it?
> 
> thanks
> 
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  hw/display/virtio-gpu.c | 21 ++---
> >  1 file changed, 14 insertions(+), 7 deletions(-)
> >
> > diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index
> > 1c1ee230b3..01bc4f9565 100644
> > --- a/hw/display/virtio-gpu.c
> > +++ b/hw/display/virtio-gpu.c
> > @@ -1422,16 +1422,23 @@ static int virtio_gpu_post_load(void *opaque, int
> version_id)
> >  if (!res) {
> >  return -EINVAL;
> >  }
> > -scanout->ds = qemu_create_displaysurface_pixman(res->image);
> > -if (!scanout->ds) {
> > -return -EINVAL;
> > -}
> > +
> > +if (res->blob_size) {
> > +assert(g->dmabuf.primary[i] != NULL);
> > +g->dmabuf.primary[i]->buf.fd = res->dmabuf_fd;
> > +dpy_gl_scanout_dmabuf(scanout->con, 
> > >dmabuf.primary[i]->buf);
> > +} else {
> > +scanout->ds = qemu_create_displaysurface_pixman(res->image);
> > +if (!scanout->ds) {
> > +return -EINVAL;
> > +}
> >  #ifdef WIN32
> > -qemu_displaysurface_win32_set_handle(scanout->ds, res->handle, 0);
> > +qemu_displaysurface_win32_set_handle(scanout->ds,
> > + res->handle, 0);
> >  #endif
> > +dpy_gfx_replace_surface(scanout->con, scanout->ds);
> > +dpy_gfx_update_full(scanout->con);
> > +}
> >
> > -dpy_gfx_replace_surface(scanout->con, scanout->ds);
> > -dpy_gfx_update_full(scanout->con);
> >  if (scanout->cursor.resource_id) {
> >  update_cursor(g, >cursor);
> >  }
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau

RE: [PATCH 0/3] ui/gtk: introducing vc->visible

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Tuesday, March 5, 2024 4:18 AM
> To: Kim, Dongwon ; P. Berrange, Daniel
> 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 0/3] ui/gtk: introducing vc->visible
> 
> Hi Kim
> 
> I am uncomfortable with the series in general.
> 
> Not only we don't have the means to draw dmabuf/scanout "when required", so
> resuming drawing won't work until the guest draws (this is already a problem 
> but
[Kim, Dongwon] Yes, this is right. The display won't be updated until there is 
a new frame submitted.
> you are only making it worse). And I also think reconfiguring the guest by 
> merely
> minimizing or switching window/tabs isn't what most users would expect.
[Kim, Dongwon] I understand your concern. Then what do you suggest I need to do 
or look into to avoid the situation where the host rendering of the guest frame 
is scheduled but pending due to tab switching or minimization of window as the 
guest (virtio-gpu drv) is waiting for the response to move on to the next 
frame? Do you think the frame update should just continue on the host side 
regardless of visibility of the window? (If this is the standard expectation, 
then one of our Linux configuration - Yocto + ICE WM has some bug in it.)

> 
> (fwiw, my personal opinion is that QEMU shouldn't provide UIs and different
> clients should be able to implement different behaviours, out of process.. 
> that
> makes me relatively less motivated to break things and be responsible)
> 
> Daniel, could you have a look too?
> 
> thanks
> 
> On Fri, Mar 1, 2024 at 4:05 AM Kim, Dongwon 
> wrote:
> >
> > Hi Marc-André Lureau,
> >
> > Just a reminder.. I need your help reviewing this patch series. Please
> > take a look at my messages at
> > https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg06636.html
> > and
> > https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg06637.html
> >
> > Thanks!!
> > DW
> >
> > > -Original Message-
> > > From: qemu-devel-bounces+dongwon.kim=intel@nongnu.org  > > devel-bounces+dongwon.kim=intel@nongnu.org> On Behalf Of
> > > dongwon@intel.com
> > > Sent: Tuesday, January 30, 2024 3:49 PM
> > > To: qemu-devel@nongnu.org
> > > Subject: [PATCH 0/3] ui/gtk: introducing vc->visible
> > >
> > > From: Dongwon Kim 
> > >
> > > Drawing guest display frames can't be completed while the VC is not
> > > in visible state, which could result in timeout in both the host and
> > > the guest especially when using blob scanout. Therefore it is needed
> > > to update and track the visiblity status of the VC and unblock the 
> > > pipeline in
> case when VC becomes invisible (e.g.
> > > windows minimization, switching among tabs) while processing a guest
> frame.
> > >
> > > First patch (0001-ui-gtk-skip...) is introducing a flag 'visible' to
> > > VirtualConsole struct then set it only if the VC and its window is 
> > > visible.
> > >
> > > Second patch (0002-ui-gtk-set-...) sets the ui size to 0 when VC is
> > > invisible when the tab is closed or deactivated. This notifies the
> > > guest that the associated guest display is not active anymore.
> > >
> > > Third patch (0003-ui-gtk-reset-visible...) adds a callback for GTK
> > > window-state- event. The flag, 'visible' is updated based on the 
> > > minization
> status of the window.
> > >
> > > Dongwon Kim (3):
> > >   ui/gtk: skip drawing guest scanout when associated VC is invisible
> > >   ui/gtk: set the ui size to 0 when invisible
> > >   ui/gtk: reset visible flag when window is minimized
> > >
> > >  include/ui/gtk.h |  1 +
> > >  ui/gtk-egl.c |  8 +++
> > >  ui/gtk-gl-area.c |  8 +++
> > >  ui/gtk.c | 62 ++--
> > >  4 files changed, 77 insertions(+), 2 deletions(-)
> > >
> > > --
> > > 2.34.1
> > >
> >

RE: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated VC is invisible

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Thursday, March 7, 2024 10:01 AM
> To: Kim, Dongwon 
> Cc: Marc-André Lureau ; qemu-
> de...@nongnu.org
> Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated
> VC is invisible
> 
> On Thu, Mar 07, 2024 at 05:53:24PM +, Kim, Dongwon wrote:
> > Hi Daniel,
> >
> > > -Original Message-
> > > From: Daniel P. Berrangé 
> > > Sent: Thursday, March 7, 2024 1:46 AM
> > > To: Kim, Dongwon 
> > > Cc: Marc-André Lureau ; qemu-
> > > de...@nongnu.org
> > > Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when
> > > associated VC is invisible
> > >
> > > On Thu, Feb 01, 2024 at 06:48:58PM +, Kim, Dongwon wrote:
> > > > Hi Marc-André,
> > > >
> > > > Thanks for your feedback. Yes, you are right, rendering doesn't
> > > > stop on Ubuntu system as it has preview even after the window is
> > > > minimized. But
> > > this is not always the case.
> > > > Some simple windows managers don't have this preview (thumbnail)
> > > > feature and this visible flag is not toggled. And the rendering
> > > > stops right away there when the window is minimized.
> > >
> > > This makes me pretty uncomfortable. This is proposing changing QEMU
> > > behaviour to workaround a problem whose behaviour varies based on
> > > what 3rd party software QEMU is running on
> > >
> > > What you say "window managers" are you referring to a traditional
> > > X11 based host display only, or does the problem also exist on
> > > modern Wayland host display ?
> >
> > [Kim, Dongwon]  I didn't mean anything about the compositor/display
> > server itself. And I am not sure about the general behavior of Wayland
> > compositors but the point here is the rendering while the app is being
> > iconized (minimized) is not always the case. For example, ICE-WM on
> > Yocto Linux doesn't have any preview for the iconized or minimized
> > applications, which means all drawing activities on the minimized app
> > are paused. This is the problem in case blob scanout is used with
> > virtio-gpu on the guest because the guest won't receive the response
> > for the frame submission until the frame is fully rendered on the
> > host. This is causing timeout and further issue on the display
> > pipeline in virtio-gpu driver in the guest.
> 
> I guess I'm wondering why we should consider this a bug in QEMU rather than
> a bug in either the toolkit or host rendering stack ?
> 
> Lets say there was no guest OS here, just a regular host app issuing drawing
> requests that were equivalent to the workload the guest is issuing.  If that
> applications' execution got blocked because its drawing requests are not
> getting processed when iconified, I'd be inclined to call it a bug.
> 
> Or are we perhaps handling drawing in the wrong way in QEMU ?
[Kim, Dongwon] Hmm.. Yeah that is a good point.. If non-rendering workload is 
blocked in the same way, that would be a problem. 
> 
> If the problem is with drawing to a iconified windows, is there an 
> intermediate
> target buffer we should be drawing to, instead of directly to the window. 
> There
> must be some supported way to have drawing requests fully processed in the
> background indepent of having a visible window, surely ?
[Kim, Dongwon]  I will check what other options that don't look like WAs are 
available.

> 
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

RE: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated VC is invisible

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Thursday, March 7, 2024 1:46 AM
> To: Kim, Dongwon 
> Cc: Marc-André Lureau ; qemu-
> de...@nongnu.org
> Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated
> VC is invisible
> 
> On Thu, Feb 01, 2024 at 06:48:58PM +, Kim, Dongwon wrote:
> > Hi Marc-André,
> >
> > Thanks for your feedback. Yes, you are right, rendering doesn't stop
> > on Ubuntu system as it has preview even after the window is minimized. But
> this is not always the case.
> > Some simple windows managers don't have this preview (thumbnail)
> > feature and this visible flag is not toggled. And the rendering stops
> > right away there when the window is minimized.
> 
> This makes me pretty uncomfortable. This is proposing changing QEMU
> behaviour to workaround a problem whose behaviour varies based on what 3rd
> party software QEMU is running on
> 
> What you say "window managers" are you referring to a traditional
> X11 based host display only, or does the problem also exist on modern
> Wayland host display ?

[Kim, Dongwon]  I didn't mean anything about the compositor/display server 
itself. And I am not sure about the general behavior of Wayland compositors but 
the point here is the rendering while the app is being iconized (minimized) is 
not always the case. For example, ICE-WM on Yocto Linux doesn't have any 
preview for the iconized or minimized applications, which means all drawing 
activities on the minimized app are paused. This is the problem in case blob 
scanout is used with virtio-gpu on the guest because the guest won't receive 
the response for the frame submission until the frame is fully rendered on the 
host. This is causing timeout and further issue on the display pipeline in 
virtio-gpu driver in the guest.

> 
> If the problem is confined to X11, that would steer towards saying we 
> shouldn't
> try to workaround it given that X11 is very much obsolete at this point.

[Kim, Dongwon]  I think I should try Wayland too but I guess it depends on the 
compositor and the shell design. Of course I need to test but what if it works 
ok on the Ubuntu running with wayland backend but not with Weston?

> 
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

RE: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated VC is invisible

[Kim, Dongwon]

Hi Daniel,

> -Original Message-
> From: Daniel P. Berrangé 
> Sent: Thursday, March 7, 2024 1:41 AM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated
> VC is invisible
> 
> On Tue, Jan 30, 2024 at 03:48:38PM -0800, dongwon@intel.com wrote:
> > From: Dongwon Kim 
> >
> > A new flag "visible" is added to show visibility status of the gfx console.
> > The flag is set to 'true' when the VC is visible but set to 'false'
> > when it is hidden or closed. When the VC is invisible, drawing guest
> > frames should be skipped as it will never be completed and it would
> > potentially lock up the guest display especially when blob scanout is used.
> >
> > Cc: Marc-André Lureau 
> > Cc: Gerd Hoffmann 
> > Cc: Vivek Kasireddy 
> >
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/gtk.h |  1 +
> >  ui/gtk-egl.c |  8 
> >  ui/gtk-gl-area.c |  8 
> >  ui/gtk.c | 10 +-
> >  4 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/ui/gtk.h b/include/ui/gtk.h index
> > aa3d637029..2de38e5724 100644
> > --- a/include/ui/gtk.h
> > +++ b/include/ui/gtk.h
> > @@ -57,6 +57,7 @@ typedef struct VirtualGfxConsole {
> >  bool y0_top;
> >  bool scanout_mode;
> >  bool has_dmabuf;
> > +bool visible;
> >  #endif
> >  } VirtualGfxConsole;
> >
> > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 3af5ac5bcf..993c283191
> > 100644
> > --- a/ui/gtk-egl.c
> > +++ b/ui/gtk-egl.c
> > @@ -265,6 +265,10 @@ void
> gd_egl_scanout_dmabuf(DisplayChangeListener
> > *dcl,  #ifdef CONFIG_GBM
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> > vc->gfx.esurface, vc->gfx.ectx);
> >
> > @@ -363,6 +367,10 @@ void gd_egl_flush(DisplayChangeListener *dcl,
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >  GtkWidget *area = vc->gfx.drawing_area;
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  if (vc->gfx.guest_fb.dmabuf && !vc->gfx.guest_fb.dmabuf-
> >draw_submitted) {
> >  graphic_hw_gl_block(vc->gfx.dcl.con, true);
> >  vc->gfx.guest_fb.dmabuf->draw_submitted = true; diff --git
> > a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c index 52dcac161e..04e07bd7ee
> > 100644
> > --- a/ui/gtk-gl-area.c
> > +++ b/ui/gtk-gl-area.c
> > @@ -285,6 +285,10 @@ void
> > gd_gl_area_scanout_flush(DisplayChangeListener *dcl,  {
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  if (vc->gfx.guest_fb.dmabuf && !vc->gfx.guest_fb.dmabuf-
> >draw_submitted) {
> >  graphic_hw_gl_block(vc->gfx.dcl.con, true);
> >  vc->gfx.guest_fb.dmabuf->draw_submitted = true; @@ -299,6
> > +303,10 @@ void gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,
> > #ifdef CONFIG_GBM
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
> >  egl_dmabuf_import_texture(dmabuf);
> >  if (!dmabuf->texture) {
> 
> If we skip processing these requests, what happens when the QEMU windows is
> then re-displayed. Won't it now be missing updates to various regions of the
> guest display ?
 
[Kim, Dongwon]  Scanout blob is continuously submitted by the guest even when 
the vc->visible is false. So it will just display the current guest frame right 
away when the window is redisplayed again.

> 
> >
> With regards,
> Daniel
> --
> |: https://berrange.com  -o-https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o-https://fstop138.berrange.com :|
> |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|

RE: [PATCH v3 1/2] ui/gtk: flush display pipeline before saving vmstate when blob=true

[Kim, Dongwon]

Hi Marc-André,

Can you take a look at this new version and also 
https://lists.gnu.org/archive/html/qemu-devel/2023-12/msg01828.html?

Thanks!
DW

> -Original Message-
> From: qemu-devel-bounces+dongwon.kim=intel@nongnu.org  devel-bounces+dongwon.kim=intel@nongnu.org> On Behalf Of
> dongwon@intel.com
> Sent: Wednesday, September 20, 2023 4:24 PM
> To: qemu-devel@nongnu.org
> Subject: [PATCH v3 1/2] ui/gtk: flush display pipeline before saving vmstate
> when blob=true
> 
> From: Dongwon Kim 
> 
> If the guest state is paused before it gets a response for the current scanout
> frame submission (resource-flush), it won't flush new frames after being
> restored as it still waits for the old response, which is accepted as a 
> scanout
> render done signal. So it's needed to unblock the current scanout render 
> pipeline
> before the run state is changed to make sure the guest receives the response 
> for
> the current frame submission.
> 
> v2: Giving some time for the fence to be signaled before flushing
> the pipeline
> 
> v3: Prevent redundant call of gd_hw_gl_flushed by checking dmabuf
> and fence_fd >= 0 in it (e.g. during and after eglClientWaitSync
> in gd_change_runstate).
> 
> Destroy sync object later in gd_hw_fl_flushed since it is needed
> by eglClientWaitSync.
> 
> Cc: Marc-André Lureau 
> Cc: Vivek Kasireddy 
> Signed-off-by: Dongwon Kim 
> ---
>  ui/egl-helpers.c |  2 --
>  ui/gtk.c | 31 +++
>  2 files changed, 27 insertions(+), 6 deletions(-)
> 
> diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c index 3d19dbe382..a77f9e57d9
> 100644
> --- a/ui/egl-helpers.c
> +++ b/ui/egl-helpers.c
> @@ -385,8 +385,6 @@ void egl_dmabuf_create_fence(QemuDmaBuf *dmabuf)
>  if (dmabuf->sync) {
>  dmabuf->fence_fd = eglDupNativeFenceFDANDROID(qemu_egl_display,
>dmabuf->sync);
> -eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> -dmabuf->sync = NULL;
>  }
>  }
> 
> diff --git a/ui/gtk.c b/ui/gtk.c
> index 810d7fc796..eaca890cba 100644
> --- a/ui/gtk.c
> +++ b/ui/gtk.c
> @@ -597,10 +597,14 @@ void gd_hw_gl_flushed(void *vcon)
>  VirtualConsole *vc = vcon;
>  QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> 
> -qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> -close(dmabuf->fence_fd);
> -dmabuf->fence_fd = -1;
> -graphic_hw_gl_block(vc->gfx.dcl.con, false);
> +if (dmabuf && dmabuf->fence_fd >= 0) {
> +qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> +close(dmabuf->fence_fd);
> +dmabuf->fence_fd = -1;
> +eglDestroySyncKHR(qemu_egl_display, dmabuf->sync);
> +dmabuf->sync = NULL;
> +graphic_hw_gl_block(vc->gfx.dcl.con, false);
> +}
>  }
> 
>  /** DisplayState Callbacks (opengl version) **/ @@ -678,6 +682,25 @@ static
> const DisplayGLCtxOps egl_ctx_ops = {  static void gd_change_runstate(void
> *opaque, bool running, RunState state)  {
>  GtkDisplayState *s = opaque;
> +int i;
> +
> +if (state == RUN_STATE_SAVE_VM) {
> +for (i = 0; i < s->nb_vcs; i++) {
> +VirtualConsole *vc = >vc[i];
> +
> +if (vc->gfx.guest_fb.dmabuf &&
> +vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
> +eglClientWaitSync(qemu_egl_display,
> +  vc->gfx.guest_fb.dmabuf->sync,
> +  EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
> +  1);
> +
> +/* force flushing current scanout blob rendering process
> + * just in case the fence is still not signaled */
> +gd_hw_gl_flushed(vc);
> +}
> +}
> +}
> 
>  gd_update_caption(s);
>  }
> --
> 2.34.1
> 

RE: [PATCH 0/3] ui/gtk: introducing vc->visible

[Kim, Dongwon]

Hi Marc-André Lureau,

Just a reminder.. I need your help reviewing this patch series. Please take a 
look at my messages at
https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg06636.html and
https://lists.gnu.org/archive/html/qemu-devel/2024-01/msg06637.html

Thanks!!
DW

> -Original Message-
> From: qemu-devel-bounces+dongwon.kim=intel@nongnu.org  devel-bounces+dongwon.kim=intel@nongnu.org> On Behalf Of
> dongwon@intel.com
> Sent: Tuesday, January 30, 2024 3:49 PM
> To: qemu-devel@nongnu.org
> Subject: [PATCH 0/3] ui/gtk: introducing vc->visible
> 
> From: Dongwon Kim 
> 
> Drawing guest display frames can't be completed while the VC is not in visible
> state, which could result in timeout in both the host and the guest especially
> when using blob scanout. Therefore it is needed to update and track the 
> visiblity
> status of the VC and unblock the pipeline in case when VC becomes invisible 
> (e.g.
> windows minimization, switching among tabs) while processing a guest frame.
> 
> First patch (0001-ui-gtk-skip...) is introducing a flag 'visible' to 
> VirtualConsole
> struct then set it only if the VC and its window is visible.
> 
> Second patch (0002-ui-gtk-set-...) sets the ui size to 0 when VC is invisible 
> when
> the tab is closed or deactivated. This notifies the guest that the associated 
> guest
> display is not active anymore.
> 
> Third patch (0003-ui-gtk-reset-visible...) adds a callback for GTK 
> window-state-
> event. The flag, 'visible' is updated based on the minization status of the 
> window.
> 
> Dongwon Kim (3):
>   ui/gtk: skip drawing guest scanout when associated VC is invisible
>   ui/gtk: set the ui size to 0 when invisible
>   ui/gtk: reset visible flag when window is minimized
> 
>  include/ui/gtk.h |  1 +
>  ui/gtk-egl.c |  8 +++
>  ui/gtk-gl-area.c |  8 +++
>  ui/gtk.c | 62 ++--
>  4 files changed, 77 insertions(+), 2 deletions(-)
> 
> --
> 2.34.1
> 

RE: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated VC is invisible

[Kim, Dongwon]

Hi Marc-André,

Thanks for your feedback. Yes, you are right, rendering doesn't stop on Ubuntu 
system
as it has preview even after the window is minimized. But this is not always 
the case.
Some simple windows managers don't have this preview (thumbnail)
feature and this visible flag is not toggled. And the rendering stops right 
away there when
the window is minimized.

Detaching then closing the window which makes it go back to tabs does not
make the flag reset, too. To us, having this extra flag for VC is one simple 
and intuitive
way to handle such situations (including upcoming guest display hot plug in 
patches)

Thanks!

> Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated
> VC is invisible
> 
> Hi
> 
> On Wed, Jan 31, 2024 at 10:56 PM Kim, Dongwon 
> wrote:
> >
> > Hi Marc-André,
> >
> > > https://docs.gtk.org/gtk3/method.Widget.is_visible.html
> >
> > This is what we had tried first but it didn't seem to work for the case of
> window minimization.
> > I see the visible flag for the GTK widget didn't seem to be toggled
> > for some reason. And when
> 
> Right, because minimize != visible. You can still get window preview with alt-
> tab and other compositor drawings.
> 
> Iow, it should keep rendering even when minimized.
> 
> > closing window, vc->window widget is destroyed so it is not possible
> > to check the flag using this GTK function. Having extra flag bound to
> > VC was most intuitive for the logic I wanted to implement.
> >
> > Thanks!!
> > DW
> >
> > > Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when
> > > associated VC is invisible
> > >
> > > Hi Dongwon
> > >
> > > On Wed, Jan 31, 2024 at 3:50 AM  wrote:
> > > >
> > > > From: Dongwon Kim 
> > > >
> > > > A new flag "visible" is added to show visibility status of the gfx 
> > > > console.
> > > > The flag is set to 'true' when the VC is visible but set to 'false'
> > > > when it is hidden or closed. When the VC is invisible, drawing
> > > > guest frames should be skipped as it will never be completed and
> > > > it would potentially lock up the guest display especially when blob
> scanout is used.
> > >
> > > Can't it skip drawing when the widget is not visible instead?
> > > https://docs.gtk.org/gtk3/method.Widget.is_visible.html
> > >
> > > >
> > > > Cc: Marc-André Lureau 
> > > > Cc: Gerd Hoffmann 
> > > > Cc: Vivek Kasireddy 
> > > >
> > > > Signed-off-by: Dongwon Kim 
> > > > ---
> > > >  include/ui/gtk.h |  1 +
> > > >  ui/gtk-egl.c |  8 
> > > >  ui/gtk-gl-area.c |  8 
> > > >  ui/gtk.c | 10 +-
> > > >  4 files changed, 26 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/ui/gtk.h b/include/ui/gtk.h index
> > > > aa3d637029..2de38e5724 100644
> > > > --- a/include/ui/gtk.h
> > > > +++ b/include/ui/gtk.h
> > > > @@ -57,6 +57,7 @@ typedef struct VirtualGfxConsole {
> > > >  bool y0_top;
> > > >  bool scanout_mode;
> > > >  bool has_dmabuf;
> > > > +bool visible;
> > > >  #endif
> > > >  } VirtualGfxConsole;
> > > >
> > > > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index
> > > > 3af5ac5bcf..993c283191
> > > > 100644
> > > > --- a/ui/gtk-egl.c
> > > > +++ b/ui/gtk-egl.c
> > > > @@ -265,6 +265,10 @@ void
> > > gd_egl_scanout_dmabuf(DisplayChangeListener
> > > > *dcl,  #ifdef CONFIG_GBM
> > > >  VirtualConsole *vc = container_of(dcl, VirtualConsole,
> > > > gfx.dcl);
> > > >
> > > > +if (!vc->gfx.visible) {
> > > > +return;
> > > > +}
> > > > +
> > > >  eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> > > > vc->gfx.esurface, vc->gfx.ectx);
> > > >
> > > > @@ -363,6 +367,10 @@ void gd_egl_flush(DisplayChangeListener *dcl,
> > > >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> > > >  GtkWidget *area = vc->gfx.drawing_area;
> > > >
> > > > +if (!vc->gfx.visible) {
> > > > +return;
> > > > +}
> > > > +
> > > >  if (vc->gfx.guest_fb.dmabuf && !v

RE: [PATCH 2/3] ui/gtk: set the ui size to 0 when invisible

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Tuesday, January 30, 2024 11:13 PM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org
> Subject: Re: [PATCH 2/3] ui/gtk: set the ui size to 0 when invisible
> 
> Hi
> 
> On Wed, Jan 31, 2024 at 3:50 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > UI size is set to 0 when the VC is invisible, which will prevent the
> > further scanout update by notifying the guest that the display is not
> > in active state. Then it is restored to the original size whenever the
> > VC becomes visible again.
> 
> This can have unwanted results on multi monitor setups, such as moving
> windows or icons around on different monitors.

[Kim, Dongwon]  You are right. This is just a choice we made.
> 
> Switching tabs or minimizing the display window shouldn't cause a guest
> display reconfiguration.
> 
> What is the benefit of disabling the monitor here? Is it for performance 
> reasons?

[Kim, Dongwon] Not sure if you recognized it but this patch series was a part of
our VM display hot-plug feature we submitted a few months ago. There, we added 
a new
param called connectors to have a way to fix individual VM displays (in multi 
display env)
on different physical displays there and made the VM display disconnected when
associated physical one is disconnected. We just wanted to make tab switching 
and
window minimization do the similar to make all of this logic consistent. 

However, if it makes more sense to have those displays all connected even when
those are not shown except for the case of hot-plug in, we could change the 
logic.
But as you mentioned, there will be some waste of bandwidth and perf since the
guest will keep sending out scan-out frames that would be just dumped.

This might be a minor thing but another concern is about tab-switching. 
Initially, the guest
will detect only one display even if the max-output is set to N (other than 1). 
Multi displays
will be detected once you detach or switch to another tab. Then if you move to 
the original
tab or close the detached window, the guest won't go back to single display 
setup.
All multi-displays will exist in its setup and this doesn’t look consistent to 
me.

> 
> >
> > Cc: Marc-André Lureau 
> > Cc: Gerd Hoffmann 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/gtk.c | 15 ++-
> >  1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 02eb667d8a..651ed3492f 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1314,10 +1314,12 @@ static void gd_menu_switch_vc(GtkMenuItem
> *item, void *opaque)
> >  GtkDisplayState *s = opaque;
> >  VirtualConsole *vc;
> >  GtkNotebook *nb = GTK_NOTEBOOK(s->notebook);
> > +GdkWindow *window;
> >  gint page;
> >
> >  vc = gd_vc_find_current(s);
> >  vc->gfx.visible = false;
> > +gd_set_ui_size(vc, 0, 0);
> >
> >  vc = gd_vc_find_by_menu(s);
> >  gtk_release_modifiers(s);
> > @@ -1325,6 +1327,9 @@ static void gd_menu_switch_vc(GtkMenuItem
> *item, void *opaque)
> >  page = gtk_notebook_page_num(nb, vc->tab_item);
> >  gtk_notebook_set_current_page(nb, page);
> >  gtk_widget_grab_focus(vc->focus);
> > +window = gtk_widget_get_window(vc->gfx.drawing_area);
> > +gd_set_ui_size(vc, gdk_window_get_width(window),
> > +   gdk_window_get_height(window));
> >  vc->gfx.visible = true;
> >  }
> >  }
> > @@ -1356,6 +1361,7 @@ static gboolean gd_tab_window_close(GtkWidget
> *widget, GdkEvent *event,
> >  GtkDisplayState *s = vc->s;
> >
> >  vc->gfx.visible = false;
> > +gd_set_ui_size(vc, 0, 0);
> >  gtk_widget_set_sensitive(vc->menu_item, true);
> >  gd_widget_reparent(vc->window, s->notebook, vc->tab_item);
> >  gtk_notebook_set_tab_label_text(GTK_NOTEBOOK(s->notebook),
> > @@ -1391,6 +1397,7 @@ static gboolean gd_win_grab(void *opaque)
> > static void gd_menu_untabify(GtkMenuItem *item, void *opaque)  {
> >  GtkDisplayState *s = opaque;
> > +GdkWindow *window;
> >  VirtualConsole *vc = gd_vc_find_current(s);
> >
> >  if (vc->type == GD_VC_GFX &&
> > @@ -1429,6 +1436,10 @@ static void gd_menu_untabify(GtkMenuItem
> *item, void *opaque)
> >  gd_update_geometry_hints(vc);
> >  gd_update_caption(s);
> >  }
> > +
> > +window = gtk_widget_get_window(vc->gfx.drawing_area);
> > +gd_set_ui_size(vc, gdk_window_get_width(window),
> > +   gdk_window_get_height(window));
> >  vc->gfx.visible = true;
> >  }
> >
> > @@ -1753,7 +1764,9 @@ static gboolean gd_configure(GtkWidget *widget,
> > {
> >  VirtualConsole *vc = opaque;
> >
> > -gd_set_ui_size(vc, cfg->width, cfg->height);
> > +if (vc->gfx.visible) {
> > +gd_set_ui_size(vc, cfg->width, cfg->height);
> > +}
> >  return FALSE;
> >  }
> >
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau

RE: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated VC is invisible

[Kim, Dongwon]

Hi Marc-André,

> https://docs.gtk.org/gtk3/method.Widget.is_visible.html

This is what we had tried first but it didn't seem to work for the case of 
window minimization.
I see the visible flag for the GTK widget didn't seem to be toggled for some 
reason. And when
closing window, vc->window widget is destroyed so it is not possible to check 
the flag using
this GTK function. Having extra flag bound to VC was most intuitive for the 
logic I wanted to
implement.

Thanks!!
DW

> Subject: Re: [PATCH 1/3] ui/gtk: skip drawing guest scanout when associated
> VC is invisible
> 
> Hi Dongwon
> 
> On Wed, Jan 31, 2024 at 3:50 AM  wrote:
> >
> > From: Dongwon Kim 
> >
> > A new flag "visible" is added to show visibility status of the gfx console.
> > The flag is set to 'true' when the VC is visible but set to 'false'
> > when it is hidden or closed. When the VC is invisible, drawing guest
> > frames should be skipped as it will never be completed and it would
> > potentially lock up the guest display especially when blob scanout is used.
> 
> Can't it skip drawing when the widget is not visible instead?
> https://docs.gtk.org/gtk3/method.Widget.is_visible.html
> 
> >
> > Cc: Marc-André Lureau 
> > Cc: Gerd Hoffmann 
> > Cc: Vivek Kasireddy 
> >
> > Signed-off-by: Dongwon Kim 
> > ---
> >  include/ui/gtk.h |  1 +
> >  ui/gtk-egl.c |  8 
> >  ui/gtk-gl-area.c |  8 
> >  ui/gtk.c | 10 +-
> >  4 files changed, 26 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/ui/gtk.h b/include/ui/gtk.h index
> > aa3d637029..2de38e5724 100644
> > --- a/include/ui/gtk.h
> > +++ b/include/ui/gtk.h
> > @@ -57,6 +57,7 @@ typedef struct VirtualGfxConsole {
> >  bool y0_top;
> >  bool scanout_mode;
> >  bool has_dmabuf;
> > +bool visible;
> >  #endif
> >  } VirtualGfxConsole;
> >
> > diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c index 3af5ac5bcf..993c283191
> > 100644
> > --- a/ui/gtk-egl.c
> > +++ b/ui/gtk-egl.c
> > @@ -265,6 +265,10 @@ void
> gd_egl_scanout_dmabuf(DisplayChangeListener
> > *dcl,  #ifdef CONFIG_GBM
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
> > vc->gfx.esurface, vc->gfx.ectx);
> >
> > @@ -363,6 +367,10 @@ void gd_egl_flush(DisplayChangeListener *dcl,
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >  GtkWidget *area = vc->gfx.drawing_area;
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  if (vc->gfx.guest_fb.dmabuf && !vc->gfx.guest_fb.dmabuf-
> >draw_submitted) {
> >  graphic_hw_gl_block(vc->gfx.dcl.con, true);
> >  vc->gfx.guest_fb.dmabuf->draw_submitted = true; diff --git
> > a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c index 52dcac161e..04e07bd7ee
> > 100644
> > --- a/ui/gtk-gl-area.c
> > +++ b/ui/gtk-gl-area.c
> > @@ -285,6 +285,10 @@ void
> > gd_gl_area_scanout_flush(DisplayChangeListener *dcl,  {
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  if (vc->gfx.guest_fb.dmabuf && !vc->gfx.guest_fb.dmabuf-
> >draw_submitted) {
> >  graphic_hw_gl_block(vc->gfx.dcl.con, true);
> >  vc->gfx.guest_fb.dmabuf->draw_submitted = true; @@ -299,6
> > +303,10 @@ void gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,
> > #ifdef CONFIG_GBM
> >  VirtualConsole *vc = container_of(dcl, VirtualConsole, gfx.dcl);
> >
> > +if (!vc->gfx.visible) {
> > +return;
> > +}
> > +
> >  gtk_gl_area_make_current(GTK_GL_AREA(vc->gfx.drawing_area));
> >  egl_dmabuf_import_texture(dmabuf);
> >  if (!dmabuf->texture) {
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..02eb667d8a 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1312,15 +1312,20 @@ static void gd_menu_quit(GtkMenuItem *item,
> > void *opaque)  static void gd_menu_switch_vc(GtkMenuItem *item, void
> > *opaque)  {
> >  GtkDisplayState *s = opaque;
> > -VirtualConsole *vc = gd_vc_find_by_menu(s);
> > +VirtualConsole *vc;
> >  GtkNotebook *nb = GTK_NOTEBOOK(s->notebook);
> >  gint page;
> >
> > +vc = gd_vc_find_current(s);
> > +vc->gfx.visible = false;
> > +
> > +vc = gd_vc_find_by_menu(s);
> >  gtk_release_modifiers(s);
> >  if (vc) {
> >  page = gtk_notebook_page_num(nb, vc->tab_item);
> >  gtk_notebook_set_current_page(nb, page);
> >  gtk_widget_grab_focus(vc->focus);
> > +vc->gfx.visible = true;
> >  }
> >  }
> >
> > @@ -1350,6 +1355,7 @@ static gboolean gd_tab_window_close(GtkWidget
> *widget, GdkEvent *event,
> >  VirtualConsole *vc = opaque;
> >  GtkDisplayState *s = vc->s;
> >
> > +vc->gfx.visible = false;
> >  gtk_widget_set_sensitive(vc->menu_item, true);
> >  gd_widget_reparent(vc->window, s->notebook, 

RE: [PATCH 1/3] ui/gtk: flush display pipeline before saving vmstate when blob=true

[Kim, Dongwon]

Hi,

> Subject: RE: [PATCH 1/3] ui/gtk: flush display pipeline before saving vmstate
> when blob=true
> 
> Hi,
> 
> >
> > On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim 
> > wrote:
> > >
> > > If the guest state is paused before it gets a response for the
> > > current scanout frame submission (resource-flush), it won't flush
> > > new frames after being restored as it still waits for the old
> > > response, which is accepted as a scanout render done signal. So it's
> > > needed to unblock the current scanout render pipeline before the run
> > > state is changed to make sure the guest receives the response for
> > > the current frame submission.
> > >
> > > v2: Giving some time for the fence to be signaled before flushing
> > > the pipeline
> > >
> > > Cc: Marc-André Lureau 
> > > Cc: Vivek Kasireddy 
> > > Signed-off-by: Dongwon Kim 
> > > ---
> > >  ui/gtk.c | 19 +++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/ui/gtk.c b/ui/gtk.c
> > > index 810d7fc796..ea8d07833e 100644
> > > --- a/ui/gtk.c
> > > +++ b/ui/gtk.c
> > > @@ -678,6 +678,25 @@ static const DisplayGLCtxOps egl_ctx_ops = {
> > > static void gd_change_runstate(void *opaque, bool running, RunState
> > state)
> > >  {
> > >  GtkDisplayState *s = opaque;
> > > +int i;
> > > +
> > > +if (state == RUN_STATE_SAVE_VM) {
> > > +for (i = 0; i < s->nb_vcs; i++) {
> > > +VirtualConsole *vc = >vc[i];
> > > +
> > > +if (vc->gfx.guest_fb.dmabuf &&
> > > +vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
> > > +eglClientWaitSync(qemu_egl_display,
> > > +  vc->gfx.guest_fb.dmabuf->sync,
> > > +  EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
> > > +  1);
> >
> > This won't work. dmabuf->sync is NULL after egl_dmabuf_create_sync.
> Right, we destroy the sync object after we create the fence from it. If you 
> want
> to use eglClientWaitSync() here, you either need to recreate the sync object
> using fence_fd or don't destroy it in egl_dmabuf_create_fence().
> Either way should be ok but make sure you destroy it when the fence_fd is
> closed.

I guess it makes sense to destroy sync object inside gd_hw_gl_flushed if that 
is the case.
Another thing is I mentioned that "dmabuf == NULL or fence_fd < 0" doesn't seem 
to be checked
in gd_hw_flushed in my previous reply but now I am thinking it is needed 
because gd_hw_gl_flushed
can be called twice with given code change - once when rendering is done and 
the fence is signaled during
eglClientWaitSync and second after eglClientWaitSync. I will come up with a new 
version of patches.
 
> 
> Thanks,
> Vivek
> 
> >
> > I will let Vivek, who wrote the sync code, comment.
> >
> > thanks
> >
> >
> >
> > --
> > Marc-André Lureau

RE: [PATCH 2/3] ui/gtk: unblock pipeline only if fence hasn't been signaled yet

[Kim, Dongwon]

Hi Marc-André,

I reviewed and realized these conditions won't be met in normal situations in 
given upstream code. But we've initially added those conditions in our internal 
code base for dev because we often had to call gd_hw_gl_flushed to forcefully 
unblock from HPD code (i.e. 'connectors' param. Not Upstreamed yet) when VM 
display is disconnected. In such cases, it is needed to make sure there is a 
frame in the pipeline already. Anyway, I think we can check dmabuf==NULL and 
fence_fd < 0 before calling gd_hw_flushed in HPD code just as in 
gd_change_runstate ([PATCH 1/3] ui/gtk: flush display pipeline before saving 
vmstate when blob=true). 

> Subject: Re: [PATCH 2/3] ui/gtk: unblock pipeline only if fence hasn't been
> signaled yet
> 
> Hi
> 
> On Thu, Dec 14, 2023 at 8:26 AM Dongwon Kim 
> wrote:
> >
> > It is needed to unblock the pipeline only if there is an active dmabuf
> > to be rendered and the fence for it is not yet signaled.
> >
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/gtk.c | 14 ++
> >  1 file changed, 10 insertions(+), 4 deletions(-)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index ea8d07833e..073c9eadb8 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -597,10 +597,16 @@ void gd_hw_gl_flushed(void *vcon)
> >  VirtualConsole *vc = vcon;
> >  QemuDmaBuf *dmabuf = vc->gfx.guest_fb.dmabuf;
> >
> > -qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > -close(dmabuf->fence_fd);
> > -dmabuf->fence_fd = -1;
> > -graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +if (!dmabuf) {
> > +return;
> > +}
> 
> When is this function called with dmabuf == NULL or fence_fd < 0?
> 
> > +
> > +if (dmabuf->fence_fd > 0) {
> 
> this should be >= 0
> 
> > +qemu_set_fd_handler(dmabuf->fence_fd, NULL, NULL, NULL);
> > +close(dmabuf->fence_fd);
> > +dmabuf->fence_fd = -1;
> > +graphic_hw_gl_block(vc->gfx.dcl.con, false);
> > +}
> >  }
> >
> >  /** DisplayState Callbacks (opengl version) **/
> > --
> > 2.34.1
> >
> >
> 
> 
> --
> Marc-André Lureau

RE: [PATCH] ui/gtk: flush display pipeline before saving vmstate when blob=true

[Kim, Dongwon]

Hi Marc-André,

> -Original Message-
> From: Marc-André Lureau 
> Sent: Monday, December 4, 2023 11:15 PM
> To: Kim, Dongwon 
> Cc: qemu-devel@nongnu.org; Kasireddy, Vivek 
> Subject: Re: [PATCH] ui/gtk: flush display pipeline before saving vmstate when
> blob=true
> 
> Hi
> 
> On Tue, Dec 5, 2023 at 6:40 AM Dongwon Kim 
> wrote:
> >
> > If the guest state is paused before it gets a response for the current
> > scanout frame submission (resource-flush), it won't start submitting
> > new frames after being restored as it still waits for the old
> > response, which is accepted as a scanout render done signal. So it's
> > needed to unblock the current scanout render pipeline before the run
> > state is changed to make sure the guest receives the response for the
> > current frame submission.
> >
> > Cc: Marc-André Lureau 
> > Cc: Vivek Kasireddy 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/gtk.c | 12 
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 810d7fc796..0f6237dd2f 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -678,6 +678,18 @@ static const DisplayGLCtxOps egl_ctx_ops = {
> > static void gd_change_runstate(void *opaque, bool running, RunState
> > state)  {
> >  GtkDisplayState *s = opaque;
> > +int i;
> > +
> > +if (state == RUN_STATE_SAVE_VM) {
> > +for (i = 0; i < s->nb_vcs; i++) {
> > +VirtualConsole *vc = >vc[i];
> > +
> > +if (vc->gfx.guest_fb.dmabuf) {
> 
> && ..dmabuf->fence_fd >= 0
> 
> > +/* force flushing current scanout blob rendering process */
> > +gd_hw_gl_flushed(vc);
> 
> This defeats the purpose of the fence, maybe we should wait for it to be
> signaled first. At worse, I suppose the client can have some glitches. 
> Although
> since the guest is stopped, this is unlikely.
[Kim, Dongwon] 
So this is the flow you are referring to?

if (vc->gfx.guest_fb.dmabuf &&
vc->gfx.guest_fb.dmabuf->fence_fd >= 0) {
EGLint ret = eglClientWaitSync(qemu_egl_display,
   vc->gfx.guest_fb.dmabuf->sync,
   EGL_SYNC_FLUSH_COMMANDS_BIT_KHR,
   1); /* timeout of 100ms 
*/

if (ret != EGL_CONDITION_SATISFIED_KHR) {
/* force flushing current scanout blob rendering process */
gd_hw_gl_flushed(vc);
}
}

If yes, I will test this then create v2 based on this flow.
Thanks 

RE: [PATCH] ui/gtk: full-screening all detached windows

[Kim, Dongwon]

Hi Marc-André,

> Hi
> 
> On Fri, Oct 13, 2023 at 2:51 AM Dongwon Kim 
> wrote:
> >
> > When turning on or off full-screen menu, all detached windows should
> > be full-screened or un-full-screened altogether.
> 
> I am not convinced this is desirable. Not only having multiple fullscreen 
> windows
> on the same screen is usually a bit harder to deal with. You typically want 
> one
> imho.
> 
> But the most annoying thing is probably that detached windows/consoles do not
> have the same shortcuts as the main window, and you can't unfullscreen them
> then...
> 
> Wouldn't you prefer to have a working fullscreen keyboard shortcut for
> detached tabs instead? This way, each window can be toggled full/unfull
> individually.

[DW] That is right. Two detached windows on the same display would be 
overlapped, which will be ugly. I also thought about that as well as other 
undesirable situations but my initial thought was the full-screen
menu is global and all QEMU windows should be controlled by it. Anyhow, I like 
your idea about individual control.. so we would probably need to add more 
full-screen menus, like 'fullscreen1, fullscreen2, fullscreen3..."
to the menu then also assign a hotkey to each one of them? 

> 
> thanks
> 
> >
> > Cc: Marc-André Lureau 
> > Signed-off-by: Dongwon Kim 
> > ---
> >  ui/gtk.c | 44 ++--
> >  1 file changed, 34 insertions(+), 10 deletions(-)
> >
> > diff --git a/ui/gtk.c b/ui/gtk.c
> > index 935de1209b..3a380f8d59 100644
> > --- a/ui/gtk.c
> > +++ b/ui/gtk.c
> > @@ -1452,29 +1452,53 @@ static void gd_accel_show_menubar(void
> > *opaque)  static void gd_menu_full_screen(GtkMenuItem *item, void
> > *opaque)  {
> >  GtkDisplayState *s = opaque;
> > -VirtualConsole *vc = gd_vc_find_current(s);
> > +VirtualConsole *vc;
> > +int i;
> >
> >  if (!s->full_screen) {
> >  gtk_notebook_set_show_tabs(GTK_NOTEBOOK(s->notebook), FALSE);
> >  gtk_widget_hide(s->menu_bar);
> > -if (vc->type == GD_VC_GFX) {
> > -gtk_widget_set_size_request(vc->gfx.drawing_area, -1, -1);
> > -}
> > -gtk_window_fullscreen(GTK_WINDOW(s->window));
> >  s->full_screen = TRUE;
> > +gtk_window_fullscreen(GTK_WINDOW(s->window));
> > +
> > +for (i = 0; i < s->nb_vcs; i++) {
> > +vc = >vc[i];
> > +if (!vc->window) {
> > +continue;
> > +}
> > +if (vc->type == GD_VC_GFX) {
> > +gtk_widget_set_size_request(vc->gfx.drawing_area, -1, -1);
> > +}
> > +gtk_window_fullscreen(GTK_WINDOW(vc->window));
> > +}
> >  } else {
> >  gtk_window_unfullscreen(GTK_WINDOW(s->window));
> > +
> > +for (i = 0; i < s->nb_vcs; i++) {
> > +vc = >vc[i];
> > +if (!vc->window) {
> > +continue;
> > +}
> > +gtk_window_unfullscreen(GTK_WINDOW(vc->window));
> > +
> > +if (vc->type == GD_VC_GFX) {
> > +vc->gfx.scale_x = 1.0;
> > +vc->gfx.scale_y = 1.0;
> > +gd_update_windowsize(vc);
> > +}
> > +}
> > +
> >  gd_menu_show_tabs(GTK_MENU_ITEM(s->show_tabs_item), s);
> >  if (gtk_check_menu_item_get_active(
> >  GTK_CHECK_MENU_ITEM(s->show_menubar_item))) {
> >  gtk_widget_show(s->menu_bar);
> >  }
> >  s->full_screen = FALSE;
> > -if (vc->type == GD_VC_GFX) {
> > -vc->gfx.scale_x = 1.0;
> > -vc->gfx.scale_y = 1.0;
> > -gd_update_windowsize(vc);
> > -}
> > +}
> > +
> > +vc = gd_vc_find_current(s);
> > +if (!vc) {
> > +return;
> >  }
> >
> >  gd_update_cursor(vc);
> > --
> > 2.20.1
> >

Re: [PATCH for-8.1] vfio/display: Fix missing update to set backing fields

[Kim, Dongwon]

Ok, this regression happened not just because of renaming. Originally 
width and height were representing the size of whole surface that guest 
shares while scanout width and height are for the each scanout. We 
realized backing_width/height are more commonly used to specify the size 
of the whole guest surface so put them in the place of width/height then 
replaced scanout_width/height as well with normal width/height.


On 8/16/2023 3:31 PM, Philippe Mathieu-Daudé wrote:

On 16/8/23 23:55, Alex Williamson wrote:

The below referenced commit renames scanout_width/height to
backing_width/height, but also promotes these fields in various portions
of the egl interface.  Meanwhile vfio dmabuf support has never used the
previous scanout fields and is therefore missed in the update. This
results in a black screen when transitioning from ramfb to dmabuf 
display

when using Intel vGPU with these features.


Referenced commit isn't trivial. Maybe because it is too late here.
I'd have tried to split it. Anyhow, too late (again).

Is vhost-user-gpu also affected? (see VHOST_USER_GPU_DMABUF_SCANOUT
in vhost_user_gpu_handle_display()).


Yeah, backing_width/height should be programmed with plane.width/height 
as well in vhost_user_gpu_handle_display().


Link: https://lists.gnu.org/archive/html/qemu-devel/2023-08/msg02726.html
Fixes: 9ac06df8b684 ("virtio-gpu-udmabuf: correct naming of 
QemuDmaBuf size properties")

Signed-off-by: Alex Williamson 
---

This fixes a regression in dmabuf/EGL support for Intel GVT-g and
potentially the mbochs mdev driver as well.  Once validated by those
that understand dmabuf/EGL integration, I'd welcome QEMU maintainers to
take this directly for v8.1 or queue it as soon as possible for v8.1.1.

  hw/vfio/display.c | 2 ++
  1 file changed, 2 insertions(+)

diff --git a/hw/vfio/display.c b/hw/vfio/display.c
index bec864f482f4..837d9e6a309e 100644
--- a/hw/vfio/display.c
+++ b/hw/vfio/display.c
@@ -243,6 +243,8 @@ static VFIODMABuf 
*vfio_display_get_dmabuf(VFIOPCIDevice *vdev,

  dmabuf->dmabuf_id  = plane.dmabuf_id;
  dmabuf->buf.width  = plane.width;
  dmabuf->buf.height = plane.height;


One thing to note here is the normal width and height in the QemuDmaBuf 
are of a scanout, which could be just a partial area of the guest plane 
here. So we should not use those as normal width and height of the 
QemuDmaBuf unless it is guaranteed the given guest surface (plane in 
this case) is always of single display's.


https://lists.gnu.org/archive/html/qemu-devel/2021-09/msg04737.html


+    dmabuf->buf.backing_width = plane.width;
+    dmabuf->buf.backing_height = plane.height;
  dmabuf->buf.stride = plane.stride;
  dmabuf->buf.fourcc = plane.drm_format;
  dmabuf->buf.modifier = plane.drm_format_mod;

Re: [PATCH 2/2] virtio-gpu: reset gfx resources in main thread

[Kim, Dongwon]

Looking good. By the way, what does 'BH' stand for?

Acked-by: Dongwon Kim 

From: Marc-André Lureau 

Calling OpenGL from different threads can have bad consequences if not
carefully reviewed. It's not generally supported. In my case, I was
debugging a crash in glDeleteTextures from OPENGL32.DLL, where I asked
qemu for gl=es, and thus ANGLE implementation was expected. libepoxy did
resolution of the global pointer for glGenTexture to the GLES version
from the main thread. But it resolved glDeleteTextures to the GL
version, because it was done from a different thread without correct
context. Oops.

Let's stick to the main thread for GL calls by using a BH.

Note: I didn't use atomics for reset_finished check, assuming the BQL
will provide enough of sync, but I might be wrong.

Signed-off-by: Marc-André Lureau 
---
 include/hw/virtio/virtio-gpu.h |  3 +++
 hw/display/virtio-gpu.c| 38 +++---
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h
index 05bee09e1a..390c4642b8 100644
--- a/include/hw/virtio/virtio-gpu.h
+++ b/include/hw/virtio/virtio-gpu.h
@@ -169,6 +169,9 @@ struct VirtIOGPU {
 
 QEMUBH *ctrl_bh;

 QEMUBH *cursor_bh;
+QEMUBH *reset_bh;
+QemuCond reset_cond;
+bool reset_finished;
 
 QTAILQ_HEAD(, virtio_gpu_simple_resource) reslist;

 QTAILQ_HEAD(, virtio_gpu_ctrl_command) cmdq;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index b1f5d392bb..bbd5c6561a 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -14,6 +14,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qemu/iov.h"
+#include "sysemu/cpus.h"
 #include "ui/console.h"
 #include "trace.h"
 #include "sysemu/dma.h"
@@ -41,6 +42,7 @@ virtio_gpu_find_check_resource(VirtIOGPU *g, uint32_t 
resource_id,
 
 static void virtio_gpu_cleanup_mapping(VirtIOGPU *g,

struct virtio_gpu_simple_resource *res);
+static void virtio_gpu_reset_bh(void *opaque);
 
 void virtio_gpu_update_cursor_data(VirtIOGPU *g,

struct virtio_gpu_scanout *s,
@@ -1387,6 +1389,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error 
**errp)
  >mem_reentrancy_guard);
 g->cursor_bh = qemu_bh_new_guarded(virtio_gpu_cursor_bh, g,
>mem_reentrancy_guard);
+g->reset_bh = qemu_bh_new(virtio_gpu_reset_bh, g);
+qemu_cond_init(>reset_cond);
 QTAILQ_INIT(>reslist);
 QTAILQ_INIT(>cmdq);
 QTAILQ_INIT(>fenceq);
@@ -1398,20 +1402,44 @@ static void virtio_gpu_device_unrealize(DeviceState 
*qdev)
 
 g_clear_pointer(>ctrl_bh, qemu_bh_delete);

 g_clear_pointer(>cursor_bh, qemu_bh_delete);
+g_clear_pointer(>reset_bh, qemu_bh_delete);
+qemu_cond_destroy(>reset_cond);
 virtio_gpu_base_device_unrealize(qdev);
 }
 
-void virtio_gpu_reset(VirtIODevice *vdev)

+static void virtio_gpu_reset_bh(void *opaque)
 {
-VirtIOGPU *g = VIRTIO_GPU(vdev);
+VirtIOGPU *g = VIRTIO_GPU(opaque);
 struct virtio_gpu_simple_resource *res, *tmp;
-struct virtio_gpu_ctrl_command *cmd;
 int i = 0;
 
 QTAILQ_FOREACH_SAFE(res, >reslist, next, tmp) {

 virtio_gpu_resource_destroy(g, res);
 }
 
+for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {

+dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
+}
+
+g->reset_finished = true;
+qemu_cond_signal(>reset_cond);
+}
+
+void virtio_gpu_reset(VirtIODevice *vdev)
+{
+VirtIOGPU *g = VIRTIO_GPU(vdev);
+struct virtio_gpu_ctrl_command *cmd;
+
+if (qemu_in_vcpu_thread()) {
+g->reset_finished = false;
+qemu_bh_schedule(g->reset_bh);
+while (!g->reset_finished) {
+qemu_cond_wait_iothread(>reset_cond);
+}
+} else {
+virtio_gpu_reset_bh(g);
+}
+
 while (!QTAILQ_EMPTY(>cmdq)) {
 cmd = QTAILQ_FIRST(>cmdq);
 QTAILQ_REMOVE(>cmdq, cmd, next);
@@ -1425,10 +1453,6 @@ void virtio_gpu_reset(VirtIODevice *vdev)
 g_free(cmd);
 }
 
-for (i = 0; i < g->parent_obj.conf.max_outputs; i++) {

-dpy_gfx_replace_surface(g->parent_obj.scanout[i].con, NULL);
-}
-
 virtio_gpu_base_reset(VIRTIO_GPU_BASE(vdev));
 }
 
--

2.41.0

Re: [PATCH 1/2] virtio-gpu: free BHs, by implementing unrealize

[Kim, Dongwon]

Acked-by: Dongwon Kim  From: Marc-André Lureau 
 Signed-off-by: Marc-André Lureau 
 ---  include/hw/virtio/virtio-gpu.h |  1 + 
 hw/display/virtio-gpu-base.c   |  2 +-  hw/display/virtio-gpu.c    
| 10 ++  3 files changed, 12 insertions(+), 1 deletion(-) diff 
--git a/include/hw/virtio/virtio-gpu.h b/include/hw/virtio/virtio-gpu.h 
index 7ea8ae2bee..05bee09e1a 100644 --- a/include/hw/virtio/virtio-gpu.h 
+++ b/include/hw/virtio/virtio-gpu.h @@ -238,6 +238,7 @@ bool 
virtio_gpu_base_device_realize(DeviceState *qdev, 
 VirtIOHandleOutput ctrl_cb, 
 VirtIOHandleOutput cursor_cb, 
 Error **errp); +void 
virtio_gpu_base_device_unrealize(DeviceState *qdev);  void 
virtio_gpu_base_reset(VirtIOGPUBase *g);  void 
virtio_gpu_base_fill_display_info(VirtIOGPUBase *g, 
 struct virtio_gpu_resp_display_info 
*dpy_info); diff --git a/hw/display/virtio-gpu-base.c 
b/hw/display/virtio-gpu-base.c index 7ab7d08d0a..ca1fb7b16f 100644 --- 
a/hw/display/virtio-gpu-base.c +++ b/hw/display/virtio-gpu-base.c @@ 
-244,7 +244,7 @@ virtio_gpu_base_set_features(VirtIODevice *vdev, 
uint64_t features)  trace_virtio_gpu_features(((features & virgl) == 
virgl));  } -static void +void 
 virtio_gpu_base_device_unrealize(DeviceState *qdev)  {  
VirtIOGPUBase *g = VIRTIO_GPU_BASE(qdev); diff --git 
a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c index 
e8603d78ca..b1f5d392bb 100644 --- a/hw/display/virtio-gpu.c +++ 
b/hw/display/virtio-gpu.c @@ -1392,6 +1392,15 @@ void 
virtio_gpu_device_realize(DeviceState *qdev, Error **errp)  
QTAILQ_INIT(>fenceq);  } +static void 
virtio_gpu_device_unrealize(DeviceState *qdev) +{ +    VirtIOGPU *g = 
VIRTIO_GPU(qdev); + +    g_clear_pointer(>ctrl_bh, qemu_bh_delete); 
+    g_clear_pointer(>cursor_bh, qemu_bh_delete); +    
virtio_gpu_base_device_unrealize(qdev); +} +  void 
virtio_gpu_reset(VirtIODevice *vdev)  {  VirtIOGPU *g = 
VIRTIO_GPU(vdev); @@ -1492,6 +1501,7 @@ static void 
virtio_gpu_class_init(ObjectClass *klass, void *data)  
vgbc->gl_flushed = virtio_gpu_handle_gl_flushed;  vdc->realize = 
virtio_gpu_device_realize; +    vdc->unrealize = 
virtio_gpu_device_unrealize;  vdc->reset = virtio_gpu_reset;  
vdc->get_config = virtio_gpu_get_config;  vdc->set_config = 
virtio_gpu_set_config; -- 2.41.0

Re: [PULL 06/19] ui/gtk: set scanout-mode right before scheduling draw

[Kim, Dongwon]

Hi there,

I guess removing this line would have been causing the problem. Can you 
add this line back and test it?


diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index eee821d73a..98b3a116bf 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -242,7 +242,6 @@ void gd_egl_scanout_texture(DisplayChangeListener *dcl,
  eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
 vc->gfx.esurface, vc->gfx.ectx);
  -    gtk_egl_set_scanout_mode(vc, true);
  egl_fb_setup_for_tex(>gfx.guest_fb, backing_width, 
backing_height,

   backing_id, false);
  }

Thanks!

On 7/20/2023 11:53 PM, Volker Rümelin wrote:

Am 17.07.23 um 14:45 schrieb marcandre.lur...@redhat.com:

From: Dongwon Kim

Setting scanout mode is better to be done very last minute
right because the mode can be reset anytime after it is set in
dpy_gl_scanout_texture by any asynchronouse dpy_refresh call,
which eventually cancels drawing of the guest scanout texture.


Hi Dongwon,

this patch breaks the QEMU guest display on my system. QEMU was 
started with ./qemu-system-x86_64 -machine q35 -device 
virtio-vga-gl,xres=1280,yres=768 -display gtk,zoom-to-fit=off,gl=on. I 
can see the OVMF boot screen and then GRUB. After Linux was started, 
plymouth normally shows the OVMF boot logo and a rotating spinner. 
With your patch the guest screen stays black and I see a text cursor 
in the upper left corner. It seems the guest works without issues. I 
can use ssh to log in and I can't find any obvious errors in the guest 
log files. I tested on a host GNOME desktop under X11 and again under 
Wayland. In both cases the result is a black guest screen.


With best regards,
Volker


Cc: Gerd Hoffmann
Cc: Marc-André Lureau
Cc: Vivek Kasireddy
Signed-off-by: Dongwon Kim
Acked-by: Marc-André Lureau
Message-ID:<20230706183355.29361-1-dongwon@intel.com>
---
  ui/gtk-egl.c | 2 +-
  ui/gtk-gl-area.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index eee821d73a..98b3a116bf 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -242,7 +242,6 @@ void gd_egl_scanout_texture(DisplayChangeListener 
*dcl,

  eglMakeCurrent(qemu_egl_display, vc->gfx.esurface,
 vc->gfx.esurface, vc->gfx.ectx);
  -    gtk_egl_set_scanout_mode(vc, true);
  egl_fb_setup_for_tex(>gfx.guest_fb, backing_width, 
backing_height,

   backing_id, false);
  }
@@ -353,6 +352,7 @@ void gd_egl_flush(DisplayChangeListener *dcl,
  if (vc->gfx.guest_fb.dmabuf && 
!vc->gfx.guest_fb.dmabuf->draw_submitted) {

  graphic_hw_gl_block(vc->gfx.dcl.con, true);
  vc->gfx.guest_fb.dmabuf->draw_submitted = true;
+    gtk_egl_set_scanout_mode(vc, true);
  gtk_widget_queue_draw_area(area, x, y, w, h);
  return;
  }
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 4513d3d059..28d9e49888 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -264,7 +264,6 @@ void 
gd_gl_area_scanout_texture(DisplayChangeListener *dcl,

  return;
  }
  -    gtk_gl_area_set_scanout_mode(vc, true);
  egl_fb_setup_for_tex(>gfx.guest_fb, backing_width, 
backing_height,

   backing_id, false);
  }
@@ -284,6 +283,7 @@ void 
gd_gl_area_scanout_flush(DisplayChangeListener *dcl,
  if (vc->gfx.guest_fb.dmabuf && 
!vc->gfx.guest_fb.dmabuf->draw_submitted) {

  graphic_hw_gl_block(vc->gfx.dcl.con, true);
  vc->gfx.guest_fb.dmabuf->draw_submitted = true;
+    gtk_gl_area_set_scanout_mode(vc, true);
  }
gtk_gl_area_queue_render(GTK_GL_AREA(vc->gfx.drawing_area));
  }

Re: [RFC PATCH 6/9] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kim, Dongwon]

On 7/11/2023 10:52 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/10/2023 11:36 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/9/2023 11:05 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/7/2023 7:07 AM, Markus Armbruster wrote:

[...]


Old question not yet answered: Using a list for the mapping means the
mapping must be dense, e.g. I can't map #0 and #2 but not #1.  Is this
what we want?

No, it doesn't have to be dense. In your example, you can just leave the place 
for VC1 blank. For example, you could do connectors.0=DP-1,connectors.2=HDMI-1. 
But in this case, VC1 won't be activated and stay as disconnected from guest's 
perspective. I think this info is also needed in v2.

Have you tried this?  I believe it'll fail with something like
"Parameter 'connectors.1' missing".

Just tested it. Yeah you are correct. I think I had a bad assumption. Let me 
take a look to see if I can make it work as I assumed.

If sparse mappings make sense, we should provide for them, I think.

An array like '*connectors': ['str'] maps from integers 0, 1, ...  It
can't do sparse (you can't omit integers in the middle).

Yeah, I understand this now. Despite of my initial intention was different, I 
am wondering if we don't allow the sparse mapping in this implementation. Any 
thought on that?

Repeating myself: if sparse mappings make sense, we should provide for
them, I think.
So, do they make sense?  Or asked differently, could a user conceivably
want to *not* place a VC?


It should be very rare. I can't think of any valid use case other than 
test cases for validating this feature. If VC is not mapped to anything 
from the beginning, there is no way to get it displayed. So there is no 
value to do so. So I assume provisioning a full list as a requirement 
would make sense here.



The V2 patch is with that change in the description. Another thing I think is 
to change QAPI design little bit to make it fill the element with null (0) if 
it's not given. Would this be a feasible option?

A 'str' cannot be NULL.  In fact, no QAPI type can be null, except for
'null' (which is always NULL), alternates with a 'null' branch, and
pointer-valued optionals (which are null when absent).


Instead of omitting them, we could map them to null: '*connectors':
['StrOrNull'].  JSON input looks like [null, "HDMI-A-0"].  Since dotted
key syntax does not support null at this time, you'd have to use JSON.

Only an object can do sparse.  However, the QAPI schema language can't
express "object where the keys are integers and the values are strings".
We'd have to use 'any', and check everything manually.

Hmm.  Thoughts?


[...]

Re: [PATCH] virtio-gpu-udmabuf: replacing scanout_width/height with backing_width/height

[Kim, Dongwon]

On 7/10/2023 4:57 AM, Marc-André Lureau wrote:

Hi

On Thu, Jul 6, 2023 at 3:10 AM Dongwon Kim  wrote:

'backing_width' and 'backing_height' are commonly used to indicate
the size
of the whole backing region so it makes sense to use those terms for
VGAUDMABuf as well in place of 'scanout_width' and 'scanout_height'.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 
---
 hw/display/virtio-gpu-udmabuf.c | 8 
 include/ui/console.h            | 4 ++--
 ui/dbus-listener.c              | 4 ++--
 ui/egl-helpers.c                | 4 ++--
 ui/gtk-egl.c                    | 4 ++--
 ui/gtk-gl-area.c                | 4 ++--
 6 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/display/virtio-gpu-udmabuf.c
b/hw/display/virtio-gpu-udmabuf.c
index ef1a740de5..920d457d4a 100644
--- a/hw/display/virtio-gpu-udmabuf.c
+++ b/hw/display/virtio-gpu-udmabuf.c
@@ -186,8 +186,8 @@ static VGPUDMABuf
     dmabuf->buf.stride = fb->stride;
     dmabuf->buf.x = r->x;
     dmabuf->buf.y = r->y;
-    dmabuf->buf.scanout_width = r->width;
-    dmabuf->buf.scanout_height = r->height;
+    dmabuf->buf.backing_width = r->width;
+    dmabuf->buf.backing_height = r->height;
     dmabuf->buf.fourcc = qemu_pixman_to_drm_format(fb->format);
     dmabuf->buf.fd = res->dmabuf_fd;
     dmabuf->buf.allow_fences = true;
@@ -218,8 +218,8 @@ int virtio_gpu_update_dmabuf(VirtIOGPU *g,

     g->dmabuf.primary[scanout_id] = new_primary;
     qemu_console_resize(scanout->con,
-                        new_primary->buf.scanout_width,
- new_primary->buf.scanout_height);
+                        new_primary->buf.backing_width,
+ new_primary->buf.backing_height);
     dpy_gl_scanout_dmabuf(scanout->con, _primary->buf);

     if (old_primary) {
diff --git a/include/ui/console.h b/include/ui/console.h
index f27b2aad4f..3e8b22d6c6 100644
--- a/include/ui/console.h
+++ b/include/ui/console.h
@@ -201,8 +201,8 @@ typedef struct QemuDmaBuf {
     uint32_t  texture;
     uint32_t  x;
     uint32_t  y;
-    uint32_t  scanout_width;
-    uint32_t  scanout_height;
+    uint32_t  backing_width;
+    uint32_t  backing_height;
     bool      y0_top;
     void      *sync;
     int       fence_fd;
diff --git a/ui/dbus-listener.c b/ui/dbus-listener.c
index 0240c39510..7d73681cbc 100644
--- a/ui/dbus-listener.c
+++ b/ui/dbus-listener.c
@@ -420,8 +420,8 @@ static void
dbus_scanout_texture(DisplayChangeListener *dcl,
         .y0_top = backing_y_0_top,
         .x = x,
         .y = y,
-        .scanout_width = w,
-        .scanout_height = h,
+        .backing_width = w,
+        .backing_height = h,


This is not consistent with the function arguments. I think it should 
be after:


.width = w, .height = h, .backing_width = backing_wdth, 
.backing_height = backing_height


Hopefully this inconsistency is not repeated elsewhere.

Yes, you are right. Backing_* is for the whole surface. And normal 
width/height or w/h specifies the sub region as you mentioned earlier in 
all other places. Inconsistency was caused in QemuDmabuf where 
width/height was used as backing_width/height. We should have corrected 
it first. I will send another version of patch to correct this.



thanks

     };

     assert(tex_id);
diff --git a/ui/egl-helpers.c b/ui/egl-helpers.c
index 8f9fbf583e..6b7be5753d 100644
--- a/ui/egl-helpers.c
+++ b/ui/egl-helpers.c
@@ -148,8 +148,8 @@ void egl_fb_blit(egl_fb *dst, egl_fb *src,
bool flip)
     if (src->dmabuf) {
         x1 = src->dmabuf->x;
         y1 = src->dmabuf->y;
-        w = src->dmabuf->scanout_width;
-        h = src->dmabuf->scanout_height;
+        w = src->dmabuf->backing_width;
+        h = src->dmabuf->backing_height;
     }

     w = (x1 + w) > src->width ? src->width - x1 : w;
diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index d59b8cd7d7..7604696d4a 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -259,8 +259,8 @@ void
gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,

     gd_egl_scanout_texture(dcl, dmabuf->texture,
                            dmabuf->y0_top, dmabuf->width,
dmabuf->height,
-                           dmabuf->x, dmabuf->y,
dmabuf->scanout_width,
-                           dmabuf->scanout_height, NULL);
+                           dmabuf->x, dmabuf->y,
dmabuf->backing_width,
+                           dmabuf->backing_height, NULL);

     if (dmabuf->allow_fences) {
         vc->gfx.guest_fb.dmabuf = dmabuf;
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index 7367dfd793..3337a4baa3 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ 

Re: [RFC PATCH 6/9] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kim, Dongwon]

On 7/10/2023 11:36 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/9/2023 11:05 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/7/2023 7:07 AM, Markus Armbruster wrote:

[...]


Ignorant questions:

1. How would I plug / unplug display cables?

I am not sure if I understood your question correctly but 1 or more guest 
displays (GTK windows) are bound to a certain physical displays like HDMI or DP 
monitors. So plug/unplug means we disconnect those physical HDMI or DP cables 
manually. Or this manual hot plug in can be emulated by you write something to 
sysfs depending on what display driver you use.

Let's see whether I understand.

A VC is placed on a *physical* monitor, i.e. a window appears on that
monitor.  That monitor's plug / unplug state is passed through to the
guest, i.e. if I physically unplug / plug the monitor, the guest sees an
unplug / plug of its virtual monitor.  Correct?

This is correct. When a display is disconnected, "monitor-changed" GTK event will be 
triggered then it will call gd_ui_size(0,0) which makes the guest display connection status to 
"disconnected".

Thanks!


Permit me another ignorant question...  Say I have a single monitor.  I
configured my X windows manager to show four virtual desktops.  Can I
use your feature to direct on which virtual desktop each VC is placed?

Would those virtual desktops will be their own connector names like HDMI or DP? 
We use the connector name for the actual physical display you see when running 
xrandr.

Output of xrandr is identical on different virtual desktops for me.


I don't know how virtual desktops are created and managed but if they don't 
have their own connector names that GTK API can read, it won't work within our 
current implementation.

After searching around a bit...  Virtual desktops, a.k.a. workspaces,
are a window manager thing.  Completely different from X displays and
monitors.  Programs can mess with placement (wmctrl does).  No idea
whether GDK provides an interface for it.  No need to discuss this
further at this time.

[...]


Old question not yet answered: Using a list for the mapping means the
mapping must be dense, e.g. I can't map #0 and #2 but not #1.  Is this
what we want?

No, it doesn't have to be dense. In your example, you can just leave the place 
for VC1 blank. For example, you could do connectors.0=DP-1,connectors.2=HDMI-1. 
But in this case, VC1 won't be activated and stay as disconnected from guest's 
perspective. I think this info is also needed in v2.

Have you tried this?  I believe it'll fail with something like
"Parameter 'connectors.1' missing".

Just tested it. Yeah you are correct. I think I had a bad assumption. Let me 
take a look to see if I can make it work as I assumed.

If sparse mappings make sense, we should provide for them, I think.

An array like '*connectors': ['str'] maps from integers 0, 1, ...  It
can't do sparse (you can't omit integers in the middle).


Yeah, I understand this now. Despite of my initial intention was 
different, I am wondering if we don't allow the sparse mapping in this 
implementation. Any thought on that? The V2 patch is with that change in 
the description. Another thing I think is to change QAPI design little 
bit to make it fill the element with null (0) if it's not given. Would 
this be a feasible option?




Instead of omitting them, we could map them to null: '*connectors':
['StrOrNull'].  JSON input looks like [null, "HDMI-A-0"].  Since dotted
key syntax does not support null at this time, you'd have to use JSON.

Only an object can do sparse.  However, the QAPI schema language can't
express "object where the keys are integers and the values are strings".
We'd have to use 'any', and check everything manually.

Hmm.  Thoughts?


[...]

Re: [RFC PATCH 6/9] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kim, Dongwon]

On 7/9/2023 11:05 PM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


On 7/7/2023 7:07 AM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


Hi Markus,

So I've worked on the description of this param. Can you check if this new 
version looks ok?

# @connectors:  List of physical monitor/connector names where the GTK
#           windows containing the respective graphics virtual consoles (VCs)
#   are to be placed. Index of the connector name in the array directly
#   indicates the id of the VC.
#   For example, with "-device gtk,connectors.0=DP-1, 
connectors.1=DP-2",
#   a physical display connected to DP-1 port will be the target monitor
#   for VC0 and the one on DP-2 will be the target for VC1. If there is
#   no connector associated with a VC, then that VC won't be placed 
anywhere
#   before the QEMU is relaunched with a proper connector name set for 
it.
#   If a connector name exists for a VC but the display cable is not 
plugged
#   in when guest is launched, the VC will be just hidden but will show 
up
#   as soon as the cable is plugged in. If a display is connected in 
the beginning
#           but later disconnected, VC will immediately be hidden and guest 
will detect
#   it as a disconnected display. This option does not force 1 to 1 
mapping
#   between the connector and the VC, which means multiple VCs can be 
placed
#   on the same display but vice versa is not possible (a single VC 
duplicated
#   on a multiple displays)
#   (Since 8.1)

Better!

Suggest to replace "that VC won't be placed anywhere" by "that VC won't
be displayed".

yeah, I will update it in v2 and send the new patch shortly.


Ignorant questions:

1. How would I plug / unplug display cables?

I am not sure if I understood your question correctly but 1 or more guest 
displays (GTK windows) are bound to a certain physical displays like HDMI or DP 
monitors. So plug/unplug means we disconnect those physical HDMI or DP cables 
manually. Or this manual hot plug in can be emulated by you write something to 
sysfs depending on what display driver you use.

Let's see whether I understand.

A VC is placed on a *physical* monitor, i.e. a window appears on that
monitor.  That monitor's plug / unplug state is passed through to the
guest, i.e. if I physically unplug / plug the monitor, the guest sees an
unplug / plug of its virtual monitor.  Correct?


This is correct. When a display is disconnected, "monitor-changed" GTK 
event will be triggered then it will call gd_ui_size(0,0) which makes 
the guest display connection status to "disconnected".




Permit me another ignorant question...  Say I have a single monitor.  I
configured my X windows manager to show four virtual desktops.  Can I
use your feature to direct on which virtual desktop each VC is placed?


Would those virtual desktops will be their own connector names like HDMI 
or DP? We use the connector name for the actual physical display you see 
when running xrandr. I don't know how virtual desktops are created and 
managed but if they don't have their own connector names that GTK API 
can read, it won't work within our current implementation.





2. If I connect multiple VCs to the same display, what will I see?  Are
they multiplexed somehow?

Yeah multiple VCs will be shown on that display. But those could be overlapped 
since those are all placed at (0, 0) of display in many cases.. but this all 
depends on how the windows manager determines the starting locations.

Got it, thanks!


Old question not yet answered: Using a list for the mapping means the
mapping must be dense, e.g. I can't map #0 and #2 but not #1.  Is this
what we want?

No, it doesn't have to be dense. In your example, you can just leave the place 
for VC1 blank. For example, you could do connectors.0=DP-1,connectors.2=HDMI-1. 
But in this case, VC1 won't be activated and stay as disconnected from guest's 
perspective. I think this info is also needed in v2.

Have you tried this?  I believe it'll fail with something like
"Parameter 'connectors.1' missing".


Just tested it. Yeah you are correct. I think I had a bad assumption. 
Let me take a look to see if I can make it work as I assumed.





[...]

Re: [RFC PATCH 6/9] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kim, Dongwon]

On 7/7/2023 7:07 AM, Markus Armbruster wrote:

"Kim, Dongwon"  writes:


Hi Markus,

So I've worked on the description of this param. Can you check if this new 
version looks ok?

# @connectors:  List of physical monitor/connector names where the GTK
#           windows containing the respective graphics virtual consoles (VCs)
#   are to be placed. Index of the connector name in the array directly
#   indicates the id of the VC.
#   For example, with "-device gtk,connectors.0=DP-1, 
connectors.1=DP-2",
#   a physical display connected to DP-1 port will be the target monitor
#   for VC0 and the one on DP-2 will be the target for VC1. If there is
#   no connector associated with a VC, then that VC won't be placed 
anywhere
#   before the QEMU is relaunched with a proper connector name set for 
it.
#   If a connector name exists for a VC but the display cable is not 
plugged
#   in when guest is launched, the VC will be just hidden but will show 
up
#   as soon as the cable is plugged in. If a display is connected in 
the beginning
#           but later disconnected, VC will immediately be hidden and guest 
will detect
#   it as a disconnected display. This option does not force 1 to 1 
mapping
#   between the connector and the VC, which means multiple VCs can be 
placed
#   on the same display but vice versa is not possible (a single VC 
duplicated
#   on a multiple displays)
#   (Since 8.1)

Better!

Suggest to replace "that VC won't be placed anywhere" by "that VC won't
be displayed".


yeah, I will update it in v2 and send the new patch shortly.



Ignorant questions:

1. How would I plug / unplug display cables?


I am not sure if I understood your question correctly but 1 or more 
guest displays (GTK windows) are bound to a certain physical displays 
like HDMI or DP monitors. So plug/unplug means we disconnect those 
physical HDMI or DP cables manually. Or this manual hot plug in can be 
emulated by you write something to sysfs depending on what display 
driver you use.




2. If I connect multiple VCs to the same display, what will I see?  Are
they multiplexed somehow?


Yeah multiple VCs will be shown on that display. But those could be 
overlapped since those are all placed at (0, 0) of display in many 
cases.. but this all depends on how the windows manager determines the 
starting locations.




Old question not yet answered: Using a list for the mapping means the
mapping must be dense, e.g. I can't map #0 and #2 but not #1.  Is this
what we want?


No, it doesn't have to be dense. In your example, you can just leave the 
place for VC1 blank. For example, you could do 
connectors.0=DP-1,connectors.2=HDMI-1. But in this case, VC1 won't be 
activated and stay as disconnected from guest's perspective. I think 
this info is also needed in v2.




[...]

Re: [PATCH] virtio-gpu: do not replace surface when scanout is disabled

[Kim, Dongwon]

On 7/4/2023 8:12 AM, Marc-André Lureau wrote:

Hi

On Wed, Jun 28, 2023 at 12:32 AM Dongwon Kim  
wrote:


Surface is replaced with a place holder whenever the surface res
is unreferenced by the guest message. With this logic, there is
very frequent switching between guest display and the place holder
image, which is looking like a flickering display if the guest driver
is designed to unref the current scanout resource before sending out
a new scanout resource. So it is better to leave the current scanout
image until there is a new one flushed by the guest.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 


Why is the driver not setting a different scanout before destroying 
the resource?


I think it's wrong to not replace the surface, as the associated 
scanout resource may be destroyed or explicitly disabled for various 
purposes, and we don't want to display garbage either.


Yeah..I got your point. This is to address very specific to our use-case 
with windows guest that runs virtio-gpu like driver that does unref 
before the next framebuffer is set. And I agree that this sequence 
doesn't look right. Let me check if we could change the sequence in the 
guest driver.




---
 hw/display/virtio-gpu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 66cddd94d9..9d3e922c8f 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -387,7 +387,6 @@ static void
virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
         res->scanout_bitmask &= ~(1 << scanout_id);
     }

-    dpy_gfx_replace_surface(scanout->con, NULL);
     scanout->resource_id = 0;
     scanout->ds = NULL;
     scanout->width = 0;
-- 
2.34.1





--
Marc-André Lureau

Re: [PATCH] ui/gtk: set the area of the scanout texture correctly

[Kim, Dongwon]

On 7/4/2023 9:07 AM, Marc-André Lureau wrote:

Hi

On Mon, Jun 26, 2023 at 9:49 PM Kim, Dongwon  
wrote:


Hi Marc-André Lureau,

On 6/26/2023 4:56 AM, Marc-André Lureau wrote:
> Hi
>
> On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim

> wrote:
>
>     x and y offsets and width and height of the scanout texture
>     is not correctly configured in case guest scanout frame is
>     dmabuf.
>
>     Cc: Gerd Hoffmann 
>     Cc: Marc-André Lureau 
>     Cc: Vivek Kasireddy 
>     Signed-off-by: Dongwon Kim 
>
>
> I find this a bit confusing, and I don't know how to actually
test it.
>
> The only place where scanout_{width, height} are set is
> virtio_gpu_create_dmabuf() and there, they have the same values as
> width and height. it's too easy to get confused with the values
imho.

Yes, scanout_width/height are same as width/height as far as there is
only one guest display exist. But they will be different in case
there
multiple displays on the guest side, configured in extended mode
(when
the guest is running Xorg).

In this case, blob for the guest display is same for scanout 1 and
2 but
each scanout will have different offset and
scanout_width/scanout_height
to reference a sub region in the same blob(dmabuf).

I added x/y/scanout_width/scanout_height with a previous commit:

commit e86a93f55463c088aa0b5260e915ffbf9f86c62b
Author: Dongwon Kim 
Date:   Wed Nov 3 23:51:52 2021 -0700

 virtio-gpu: splitting one extended mode guest fb into n-scanouts

> I find the terminology we use for ScanoutTexture much clearer.
It uses
> backing_{width, height} instead, which indicates quite clearly that
> the usual x/y/w/h are for the sub-region to be shown.
yeah agreed. Then dmabuf->width/height should be changed to
dmabuf->backing_width/height and dmabuf->width/height will be
replacing
dmabuf->scanout_width/scanout_height. I guess this is what you
meant, right?


right, can you send a new patch?
thanks


https://lists.gnu.org/archive/html/qemu-devel/2023-07/msg01081.html

Thanks!



> I think we should have a preliminary commit that renames
> scanout_{width, height}.
>
> Please give some help/hints on how to actually test this code too.

So this patch is just to make things look consistent in the code
level.
Having offset (0,0) in this function call for all different scanouts
didn't look right to me. This code change won't make anything done
differently though. So no test is applicable.

>
> Thanks!
>
>
>     ---
>      ui/gtk-egl.c     | 3 ++-
>      ui/gtk-gl-area.c | 3 ++-
>      2 files changed, 4 insertions(+), 2 deletions(-)
>
>     diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
>     index 19130041bc..e99e3b0d8c 100644
>     --- a/ui/gtk-egl.c
>     +++ b/ui/gtk-egl.c
>     @@ -257,7 +257,8 @@ void
>     gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,
>
>          gd_egl_scanout_texture(dcl, dmabuf->texture,
>                                 dmabuf->y0_top, dmabuf->width,
>     dmabuf->height,
>     -                           0, 0, dmabuf->width,
dmabuf->height);
>     +                           dmabuf->x, dmabuf->y,
>     dmabuf->scanout_width,
>     +  dmabuf->scanout_height);
>
>          if (dmabuf->allow_fences) {
>              vc->gfx.guest_fb.dmabuf = dmabuf;
>     diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
>     index c384a1516b..1605818bd1 100644
>     --- a/ui/gtk-gl-area.c
>     +++ b/ui/gtk-gl-area.c
>     @@ -299,7 +299,8 @@ void
>     gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,
>
>          gd_gl_area_scanout_texture(dcl, dmabuf->texture,
>                                     dmabuf->y0_top, dmabuf->width,
>     dmabuf->height,
>     -                               0, 0, dmabuf->width,
dmabuf->height);
>     +                               dmabuf->x, dmabuf->y,
>     dmabuf->scanout_width,
>     +  dmabuf->scanout_height);
>
>          if (dmabuf->allow_fences) {
>              vc->gfx.guest_fb.dmabuf = dmabuf;
>     --
>     2.34.1
>
>
>
>
> --
> Marc-André Lureau



--
Marc-André Lureau

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Kim, Dongwon]

On 6/21/2023 4:14 AM, Robert Beckett wrote:


On 21/06/2023 09:39, Gerd Hoffmann wrote:

On Tue, Jun 20, 2023 at 01:26:15PM +0100, Robert Beckett wrote:

On 20/06/2023 10:41, Gerd Hoffmann wrote:

    Hi,


The guest driver should be able to restore resources after resume.

Thank you for your suggestion!
As far as I know, resources are created on host side and guest has 
no backup, if resources are destroyed, guest can't restore them.
Or do you mean guest driver need to send commands to re-create 
resources after resume?

The later.  The guest driver knows which resources it has created,
it can restore them after suspend.

Are you sure that this is viable?

How would you propose that a guest kernel could reproduce a resource,
including pixel data upload during a resume?

The kernel would not have any of the pixel data to transfer to host.

Depends on the of resource type.  For resources which are created by
uploading pixel data (using VIRTIO_GPU_CMD_TRANSFER_TO_HOST_*) a guest
mirror exists which can be used for re-upload.


unfortunately this is not always the case.

https://gitlab.freedesktop.org/mesa/mesa/-/blob/main/src/gallium/drivers/virgl/virgl_resource.c#L668 



Often mesa will decide that it won't need to access a resource again 
after initial upload (textures etc). In this case, if it is able to 
copy back from host if needed, it will not maintain the guest shadow 
copy. Instead it will create a single page proxy object. The transfer 
to host will then over fill it to the correct size.


I think this was a fairly huge optimization for them.

I have been only focused on scanout blob so didn't think too much about 
all virgl objects but aren't all the virtio-gpu-object will be 
maintained until they are removed by the driver regardless of the type 
of data they contain? Does Mesa (virgl) remove those objects after they 
are uploaded to the host?




For resources filled by gl rendering ops this is indeed not the case.

Could you explain how you anticipate the guest being able to 
reproduce the

resources please?

Same you do on physical hardware?  Suspend can poweroff your PCI
devices, so there must be some standard way to handle that situation
for resources stored in gpu device memory, which is very similar to
the problem we have here.


In traditional PCI gfx card setups, TTM is used as the memory manager 
in the kernel. This is used to migrate the buffers back from VRAM to 
system pages during a suspend.


This would be suitable for use to track host blob buffers that get 
mapped to guest via the PCI BAR, though would be a significant 
re-architecting of virtio gpu driver.


It would not help with the previously mentioned proxied resources. 
Though in theory the driver could read the resources back from host to 
guest pages during suspend, this would then be potentially complicated 
by suspend time alloc failures etc.



As virtio drivers are by design paravirt drivers ,I think it is 
reasonable to accept some knowledge with and cooperation with the host 
to manage suspend/resume.


It seems to me like a lot of effort and long term maintenance to add 
support for transparent suspend/resume that would otherwise be unneeded.


Perhaps others have alternative designs for this?



take care,
   Gerd

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Kim, Dongwon]

This method - letting QEMU not remove resources would work on S3 case 
but with S4, the QEMU would lose all the resources anyway as the process 
will be terminated. So objects restoring was only option for us as


in [RFC PATCH 2/2] drm/virtio: restore virtio_gpu_objects upon suspend 
and resume (lists.freedesktop.org) 



But I only considered and tested cases with scanout blob resources, so 
this may not cover other resource types...


On 6/7/2023 7:56 PM, Jiqian Chen wrote:

After suspending and resuming guest VM, you will get
a black screen, and the display can't come back.

This is because when guest did suspending, it called
into qemu to call virtio_gpu_gl_reset. In function
virtio_gpu_gl_reset, it destroyed resources and reset
renderer, which were used for display. As a result,
guest's screen can't come back to the time when it was
suspended and only showed black.

So, this patch adds a new ctrl message
VIRTIO_GPU_CMD_STATUS_FREEZING to get notification from
guest. If guest is during suspending, it sets freezing
status of virtgpu to true, this will prevent destroying
resources and resetting renderer when guest calls into
virtio_gpu_gl_reset. If guest is during resuming, it sets
freezing to false, and then virtio_gpu_gl_reset will keep
its origin actions and has no other impaction.

Signed-off-by: Jiqian Chen 
---
  hw/display/virtio-gpu-gl.c  |  9 ++-
  hw/display/virtio-gpu-virgl.c   |  3 +++
  hw/display/virtio-gpu.c | 26 +++--
  include/hw/virtio/virtio-gpu.h  |  3 +++
  include/standard-headers/linux/virtio_gpu.h |  9 +++
  5 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/hw/display/virtio-gpu-gl.c b/hw/display/virtio-gpu-gl.c
index e06be60dfb..e11ad233eb 100644
--- a/hw/display/virtio-gpu-gl.c
+++ b/hw/display/virtio-gpu-gl.c
@@ -100,7 +100,14 @@ static void virtio_gpu_gl_reset(VirtIODevice *vdev)
   */
  if (gl->renderer_inited && !gl->renderer_reset) {
  virtio_gpu_virgl_reset_scanout(g);
-gl->renderer_reset = true;
+/*
+ * If guest is suspending, we shouldn't reset renderer,
+ * otherwise, the display can't come back to the time when
+ * it was suspended after guest resumed.
+ */
+if (!g->freezing) {
+gl->renderer_reset = true;
+}
  }
  }
  
diff --git a/hw/display/virtio-gpu-virgl.c b/hw/display/virtio-gpu-virgl.c

index 73cb92c8d5..183ec92d53 100644
--- a/hw/display/virtio-gpu-virgl.c
+++ b/hw/display/virtio-gpu-virgl.c
@@ -464,6 +464,9 @@ void virtio_gpu_virgl_process_cmd(VirtIOGPU *g,
  case VIRTIO_GPU_CMD_GET_EDID:
  virtio_gpu_get_edid(g, cmd);
  break;
+case VIRTIO_GPU_CMD_STATUS_FREEZING:
+virtio_gpu_cmd_status_freezing(g, cmd);
+break;
  default:
  cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
  break;
diff --git a/hw/display/virtio-gpu.c b/hw/display/virtio-gpu.c
index 5e15c79b94..8f235d7848 100644
--- a/hw/display/virtio-gpu.c
+++ b/hw/display/virtio-gpu.c
@@ -373,6 +373,16 @@ static void virtio_gpu_resource_create_blob(VirtIOGPU *g,
  QTAILQ_INSERT_HEAD(>reslist, res, next);
  }
  
+void virtio_gpu_cmd_status_freezing(VirtIOGPU *g,

+ struct virtio_gpu_ctrl_command *cmd)
+{
+struct virtio_gpu_status_freezing sf;
+
+VIRTIO_GPU_FILL_CMD(sf);
+virtio_gpu_bswap_32(, sizeof(sf));
+g->freezing = sf.freezing;
+}
+
  static void virtio_gpu_disable_scanout(VirtIOGPU *g, int scanout_id)
  {
  struct virtio_gpu_scanout *scanout = >parent_obj.scanout[scanout_id];
@@ -986,6 +996,9 @@ void virtio_gpu_simple_process_cmd(VirtIOGPU *g,
  case VIRTIO_GPU_CMD_RESOURCE_DETACH_BACKING:
  virtio_gpu_resource_detach_backing(g, cmd);
  break;
+case VIRTIO_GPU_CMD_STATUS_FREEZING:
+virtio_gpu_cmd_status_freezing(g, cmd);
+break;
  default:
  cmd->error = VIRTIO_GPU_RESP_ERR_UNSPEC;
  break;
@@ -1344,6 +1357,8 @@ void virtio_gpu_device_realize(DeviceState *qdev, Error 
**errp)
  QTAILQ_INIT(>reslist);
  QTAILQ_INIT(>cmdq);
  QTAILQ_INIT(>fenceq);
+
+g->freezing = false;
  }
  
  void virtio_gpu_reset(VirtIODevice *vdev)

@@ -1352,8 +1367,15 @@ void virtio_gpu_reset(VirtIODevice *vdev)
  struct virtio_gpu_simple_resource *res, *tmp;
  struct virtio_gpu_ctrl_command *cmd;
  
-QTAILQ_FOREACH_SAFE(res, >reslist, next, tmp) {

-virtio_gpu_resource_destroy(g, res);
+/*
+ * If guest is suspending, we shouldn't destroy resources,
+ * otherwise, the display can't come back to the time when
+ * it was suspended after guest resumed.
+ */
+if (!g->freezing) {
+QTAILQ_FOREACH_SAFE(res, >reslist, next, tmp) {
+virtio_gpu_resource_destroy(g, res);
+}
  }
  
  while (!QTAILQ_EMPTY(>cmdq)) {

diff 

Re: [RFC PATCH 6/9] ui/gtk: Add a new parameter to assign connectors/monitors to GFX VCs

[Kim, Dongwon]

Hi Markus,

So I've worked on the description of this param. Can you check if this new 
version looks ok?

# @connectors:  List of physical monitor/connector names where the GTK
#           windows containing the respective graphics virtual consoles (VCs)
#   are to be placed. Index of the connector name in the array directly
#   indicates the id of the VC.
#   For example, with "-device gtk,connectors.0=DP-1, 
connectors.1=DP-2",
#   a physical display connected to DP-1 port will be the target monitor
#   for VC0 and the one on DP-2 will be the target for VC1. If there is
#   no connector associated with a VC, then that VC won't be placed 
anywhere
#   before the QEMU is relaunched with a proper connector name set for 
it.
#   If a connector name exists for a VC but the display cable is not 
plugged
#   in when guest is launched, the VC will be just hidden but will show 
up
#   as soon as the cable is plugged in. If a display is connected in 
the beginning
#           but later disconnected, VC will immediately be hidden and guest 
will detect
#   it as a disconnected display. This option does not force 1 to 1 
mapping
#   between the connector and the VC, which means multiple VCs can be 
placed
#   on the same display but vice versa is not possible (a single VC 
duplicated
#   on a multiple displays)
#   (Since 8.1)

Thanks,
DW

On 6/20/2023 10:51 PM, Markus Armbruster wrote:


Dongwon Kim  writes:


From: Vivek Kasireddy 

The new parameter named "connector" can be used to assign physical
monitors/connectors to individual GFX VCs such that when the monitor
is connected or hotplugged, the associated GTK window would be
moved to it. If the monitor is disconnected or unplugged, the
associated GTK window would be hidden and a relevant disconnect
event would be sent to the Guest.

Usage: -device virtio-gpu-pci,max_outputs=2,blob=true,...
-display gtk,gl=on,connectors.0=eDP-1,connectors.1=DP-1.

Cc: Gerd Hoffmann 
Cc: Daniel P. Berrangé 
Cc: Markus Armbruster 
Cc: Philippe Mathieu-Daudé 
Cc: Marc-André Lureau 
Signed-off-by: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 

[...]


--- a/qapi/ui.json
+++ b/qapi/ui.json
@@ -1315,13 +1315,22 @@
  # @show-menubar: Display the main window menubar.  Defaults to "on".
  # (Since 8.0)
  #
+# @connectors:  List of physical monitor/connector names where the GTK
+#   windows containing the respective graphics virtual consoles
+#   (VCs) are to be placed. If a mapping exists for a VC, it
+#   will be moved to that specific monitor or else it would
+#   not be displayed anywhere and would appear disconnected
+#   to the guest.
+#   (Since 8.1)

Please format like

# @connectors: List of physical monitor/connector names where the GTK
# windows containing the respective graphics virtual consoles
# (VCs) are to be placed.  If a mapping exists for a VC, it will
# be moved to that specific monitor or else it would not be
# displayed anywhere and would appear disconnected to the guest.
# (Since 8.1)

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

The meaning of @connectors is less than clear.  The phrase "If a mapping
exists for a VC" suggests it is a mapping of sorts.  "List of physical
monitor/connector names" indicates it maps to physical monitor /
connector name.  What does it map from?  VC number?  How are VCs
numbered?  Is it the same number we use in QOM /backend/console[NUM]?

Using a list for the mapping means the mapping must be dense, e.g. I
can't map #0 and #2 but not #1.  Is this what we want?

The sentence "If a mapping exists" confusing has a dangling else
ambiguity of sorts.  I can interpret it as

 If a mapping exists for a VC:
 the window will be moved to that specific monitor
 or else it would not be displayed anywhere and would appear ...

or as

 If a mapping exists for a VC:
 the window will be moved to that specific monitor
 or else it would not be displayed anywhere and would appear ...

I think we have three cases:

0. No mapping provided

1. Mapping provided, and the named monitor / connector exists

2. Mapping provided, and the named monitor / connector does not exist

We can go from case 1 to 2 (disconnect) and vice versa (connect) at any
time.

Please spell out behavior for each case, and for the transitions between
case 1 and 2.


+#
  # Since: 2.12
  ##
  { 'struct'  : 'DisplayGTK',
'data': { '*grab-on-hover' : 'bool',
  '*zoom-to-fit'   : 'bool',
  '*show-tabs' : 'bool',
-'*show-menubar'  : 'bool'  } }
+'*show-menubar'  : 'bool',
+'*connectors': ['str'] } }
  
  ##

  # @DisplayEGLHeadless:

[...]

Re: [PATCH] ui/gtk: set the area of the scanout texture correctly

[Kim, Dongwon]

Hi Marc-André Lureau,

On 6/26/2023 4:56 AM, Marc-André Lureau wrote:

Hi

On Wed, Jun 21, 2023 at 11:53 PM Dongwon Kim  
wrote:


x and y offsets and width and height of the scanout texture
is not correctly configured in case guest scanout frame is
dmabuf.

Cc: Gerd Hoffmann 
Cc: Marc-André Lureau 
Cc: Vivek Kasireddy 
Signed-off-by: Dongwon Kim 


I find this a bit confusing, and I don't know how to actually test it.

The only place where scanout_{width, height} are set is 
virtio_gpu_create_dmabuf() and there, they have the same values as 
width and height. it's too easy to get confused with the values imho.


Yes, scanout_width/height are same as width/height as far as there is 
only one guest display exist. But they will be different in case there 
multiple displays on the guest side, configured in extended mode (when 
the guest is running Xorg).


In this case, blob for the guest display is same for scanout 1 and 2 but 
each scanout will have different offset and scanout_width/scanout_height 
to reference a sub region in the same blob(dmabuf).


I added x/y/scanout_width/scanout_height with a previous commit:

commit e86a93f55463c088aa0b5260e915ffbf9f86c62b
Author: Dongwon Kim 
Date:   Wed Nov 3 23:51:52 2021 -0700

    virtio-gpu: splitting one extended mode guest fb into n-scanouts

I find the terminology we use for ScanoutTexture much clearer. It uses 
backing_{width, height} instead, which indicates quite clearly that 
the usual x/y/w/h are for the sub-region to be shown.
yeah agreed. Then dmabuf->width/height should be changed to 
dmabuf->backing_width/height and dmabuf->width/height will be replacing 
dmabuf->scanout_width/scanout_height. I guess this is what you meant, right?
I think we should have a preliminary commit that renames 
scanout_{width, height}.


Please give some help/hints on how to actually test this code too.


So this patch is just to make things look consistent in the code level. 
Having offset (0,0) in this function call for all different scanouts 
didn't look right to me. This code change won't make anything done 
differently though. So no test is applicable.




Thanks!


---
 ui/gtk-egl.c     | 3 ++-
 ui/gtk-gl-area.c | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/ui/gtk-egl.c b/ui/gtk-egl.c
index 19130041bc..e99e3b0d8c 100644
--- a/ui/gtk-egl.c
+++ b/ui/gtk-egl.c
@@ -257,7 +257,8 @@ void
gd_egl_scanout_dmabuf(DisplayChangeListener *dcl,

     gd_egl_scanout_texture(dcl, dmabuf->texture,
                            dmabuf->y0_top, dmabuf->width,
dmabuf->height,
-                           0, 0, dmabuf->width, dmabuf->height);
+                           dmabuf->x, dmabuf->y,
dmabuf->scanout_width,
+                           dmabuf->scanout_height);

     if (dmabuf->allow_fences) {
         vc->gfx.guest_fb.dmabuf = dmabuf;
diff --git a/ui/gtk-gl-area.c b/ui/gtk-gl-area.c
index c384a1516b..1605818bd1 100644
--- a/ui/gtk-gl-area.c
+++ b/ui/gtk-gl-area.c
@@ -299,7 +299,8 @@ void
gd_gl_area_scanout_dmabuf(DisplayChangeListener *dcl,

     gd_gl_area_scanout_texture(dcl, dmabuf->texture,
                                dmabuf->y0_top, dmabuf->width,
dmabuf->height,
-                               0, 0, dmabuf->width, dmabuf->height);
+                               dmabuf->x, dmabuf->y,
dmabuf->scanout_width,
+                               dmabuf->scanout_height);

     if (dmabuf->allow_fences) {
         vc->gfx.guest_fb.dmabuf = dmabuf;
-- 
2.34.1





--
Marc-André Lureau

Re: [QEMU PATCH 1/1] virtgpu: do not destroy resources when guest suspend

[Kim, Dongwon]

Hello,

I just came across this discussion regarding s3/s4 support in virtio-gpu 
driver and QEMU.


We saw similar problem a while ago (QEMU deletes all objects upon 
suspension) and


came up with an experimental solution that is basically making 
virtio-gpu driver to do object creation


for all existing resources once VM is resumed so that he QEMU recreate them.

This method has worked pretty well on our case. I submitted patches for 
this to dri-devel a while ago.


[RFC PATCH 0/2] drm/virtio:virtio-gpu driver freeze-and-restore 
implementation (lists.freedesktop.org) 



This is kernel driver only solution. Nothing has to be changed in QEMU.

Jiqian and other reviewers, can you check this old solution we suggested 
as well?


On 6/20/2023 5:26 AM, Robert Beckett wrote:



On 20/06/2023 10:41, Gerd Hoffmann wrote:

   Hi,


The guest driver should be able to restore resources after resume.

Thank you for your suggestion!
As far as I know, resources are created on host side and guest has 
no backup, if resources are destroyed, guest can't restore them.
Or do you mean guest driver need to send commands to re-create 
resources after resume?

The later.  The guest driver knows which resources it has created,
it can restore them after suspend.



Are you sure that this is viable?

How would you propose that a guest kernel could reproduce a resource, 
including pixel data upload during a resume?


The kernel would not have any of the pixel data to transfer to host. 
This is normally achieved by guest apps calling GL calls and mesa 
asking the kernel to create the textures with the given data (often 
read from a file).
If your suggestion is to get the userland application to do it, that 
would entirely break how suspend/resume is meant to happen. It should 
be transparent to userland applications for the most part.


Could you explain how you anticipate the guest being able to reproduce 
the resources please?






If so, I have some questions. Can guest re-create resources by using
object(virtio_vpu_object) or others? Can the new resources replace the
destroyed resources to continue the suspended display tasks after
resume?

Any display scanout information will be gone too, the guest driver needs
re-create this too (after re-creating the resources).

take care,
   Gerd

Re: [PATCH v1 0/3] ui/gtk: Add a new parameter to assign connectors/monitors to Guests' windows

[Kim, Dongwon]

Hi Markus,

Vivek and I have discussed this already. I am fine with replacing my old 
series with this.


Thanks,

DW

On 9/20/2022 11:06 PM, Markus Armbruster wrote:

"Kasireddy, Vivek"  writes:


Hi Markus,


Any overlap with Dongwon Kim's "[PATCH v5 0/2] handling guest multiple
displays"?

[Kasireddy, Vivek] Yes, there is some overlap but as I mentioned in the cover 
letter,
this series is intended to replace Dongwon's series dealing with multiple 
displays.

I have no idea how I missed that part of your cover letter %-}

Dongwon Kim, would this series work for you?


Message-Id: <20220718233009.18780-1-dongwon@intel.com>
https://lists.nongnu.org/archive/html/qemu-devel/2022-07/msg03212.html

[Kasireddy, Vivek] We felt that using monitor numbers for display/VC assignment
would be cumbersome for users. And, given that his series does not take into 
account
monitor unplug/hotplug events, it's effectiveness would be limited compared to
this one.

Thanks!