Hi Thomas, > Subject: Re: [PATCH] drm/virtgpu: Use vblank timer > > Hi > > Am 09.10.25 um 05:23 schrieb Kasireddy, Vivek: > > Hi Thomas, > > > >> Subject: [PATCH] drm/virtgpu: Use vblank timer > >> > >> Use a vblank timer to simulate the vblank interrupt. The DRM vblank > >> helpers provide an implementation on top of Linux' hrtimer. Virtgpu > >> enables and disables the timer as part of the CRTC. The atomic_flush > >> callback sets up the event. Like vblank interrupts, the vblank timer > >> fires at the rate of the display refresh. > >> > >> Most userspace limits its page flip rate according to the DRM vblank > >> event. Virtgpu's virtual hardware does not provide vblank interrupts, so > >> DRM sends each event ASAP. With the fast access times of virtual display > >> memory, the event rate is much higher than the display mode's refresh > >> rate; creating the next page flip almost immediately. This leads to > >> excessive CPU overhead from even small display updates, such as moving > >> the mouse pointer. > >> > >> This problem affects virtgpu and all other virtual displays. See [1] for > >> a discussion in the context of hypervdrm. > > When running Virtio-gpu with some of the UIs (gtk, spice, etc) in Qemu, > the > > We still need a solution for the other UIs, including the kernel's fbcon > output. > > > Guest display updates are synchronized with a timer (or repaint callback > in > > the case of Gtk) the UI layer provides particularly when blob=true. In other > > words, the Guest's atomic commit does not complete until the Host > signals > > (via a fence) that the update (or flush) is done. > > I know little about virtgpu's hypervisor interfaces. Let's see if I get > the full picture. > > It's the fences list that's being processed at [1], right? This fence > signals the completion of the display update. The plane update sends out > a command to the host and then waits for the ack at [2]. The ack happend > at [3], which calls into [1]. At [4] the guest are available again to be > released. How long the display update takes is up to the host and the > guest's scheduler. When the Guest submits a display update, it is made to wait (but only when blob=true) here: https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/virtio/virtgpu_plane.c#L222
So, in this case, the time it takes for the Guest's update to finish is mostly dependent on the Host's UI update submission. > > That's definitely something different than the vblank, as vblanks happen > at fixed intervals. The Qemu UI timer also runs at regular fixed intervals, so the Guest updates would run at 30 or 60 FPS based on the timer frequency. And, in the case of Gtk, the repaint callback is indirectly tied to the Host's Vblank, and it also occurs at regular intervals when there are pending updates. > > [1] > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/virtio/virtg > pu_fence.c#L120 > [2] > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/virtio/virtg > pu_vq.c#L397 > [3] > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/virtio/virtg > pu_vq.c#L56 > [4] > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/virtio/virtg > pu_vq.c#L265 > > > > > > This is because when blob=true, the Guest's FB is accessed by the Host > without > > making any copies. So, to avoid tearing, the Guest needs to be prevented > from > > accessing its FB until the Host is done. Therefore, I think for virtio-gpu, > > the > virtual > > vblank timer can only be helpful when blob=false as this is the only case > where > > Guest's display updates are unbounded (and Host makes a copy of the > Guest's FB). > > Because virtgpu did not initialize vblanking, DRM always sent out a > vblank event after the completed commit. [5] It's not synchronized to > the display output. This means that virtgpu has always been subject to > tearing and the guest always required to hold buffers until the host > completed its display update. IOW adding vblank timers into the mix > should not change the behavior of virtgpu. It just limits the output As I mentioned, the output or display update frequency is already limited when blob=true, so adding a vblank timer would be redundant in this case. Note that blob=true is a performance optimization where we create a dmabuf (on the Host) backed by Guest FB's memory that is shared with the UI layer. And, AFAIK, the only case where virtio-gpu updates are not limited is when blob=false. Even in this case, there would be no tearing issues seen because the Guest is made to wait until the Host makes a copy of its FB. > frequency. If GNOME's pageflip is synchronized to the vblank, it should > immediately benefit. > > The GTK repaint callback is an interface for applications. How does it > affect, or is affected by, any of this? So, virtio-gpu is almost always coupled with a UI (on the Host) in order to display the Guest's desktop content (fbcon and compositor's FB data) on the Host locally (Gtk, SDL, etc) or streamed to a remote system (Spice, Vnc, etc). And, the rate at which the UI updates are submitted (to the Host compositor for local UIs) is controlled by the UI timer. However, in the case of Qemu Gtk UI, the UI timer is used as a backup as we mostly rely on the repaint callback to figure out when to submit updates. This is because the repaint callback is a more reliable mechanism to determine when it is appropriate to submit an update to the Host compositor. And, until the UI's update is not submitted (and executed by the Host GPU and signaled via an EGL fence), the Guest's update fence is not signaled. We can reliably achieve 60 FPS this way (assuming the Host's refresh rate is 60) for Guest's display updates. Thanks, Vivek > > [5] > https://elixir.bootlin.com/linux/v6.17.1/source/drivers/gpu/drm/drm_atomi > c_helper.c#L2606 > > Best regards > Thomas > > > > > I am not sure how this would affect virgl use-cases but Dmitry can help > explain if > > the vblank timer would be useful there or not. > > > > Thanks, > > Vivek > > > >> Signed-off-by: Thomas Zimmermann <[email protected]> > >> Link: https://lore.kernel.org/dri- > >> > devel/[email protected] > >> amprd02.prod.outlook.com/ # [1] > >> --- > >> drivers/gpu/drm/virtio/virtgpu_display.c | 29 > ++++++++++++++++++++++-- > >> 1 file changed, 27 insertions(+), 2 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c > >> b/drivers/gpu/drm/virtio/virtgpu_display.c > >> index c3315935d8bc..ee134e46eeba 100644 > >> --- a/drivers/gpu/drm/virtio/virtgpu_display.c > >> +++ b/drivers/gpu/drm/virtio/virtgpu_display.c > >> @@ -32,6 +32,8 @@ > >> #include <drm/drm_gem_framebuffer_helper.h> > >> #include <drm/drm_probe_helper.h> > >> #include <drm/drm_simple_kms_helper.h> > >> +#include <drm/drm_vblank.h> > >> +#include <drm/drm_vblank_helper.h> > >> > >> #include "virtgpu_drv.h" > >> > >> @@ -55,6 +57,7 @@ static const struct drm_crtc_funcs > virtio_gpu_crtc_funcs > >> = { > >> .reset = drm_atomic_helper_crtc_reset, > >> .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, > >> .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state, > >> + DRM_CRTC_VBLANK_TIMER_FUNCS, > >> }; > >> > >> static const struct drm_framebuffer_funcs virtio_gpu_fb_funcs = { > >> @@ -99,6 +102,7 @@ static void virtio_gpu_crtc_mode_set_nofb(struct > >> drm_crtc *crtc) > >> static void virtio_gpu_crtc_atomic_enable(struct drm_crtc *crtc, > >> struct drm_atomic_state *state) > >> { > >> + drm_crtc_vblank_on(crtc); > >> } > >> > >> static void virtio_gpu_crtc_atomic_disable(struct drm_crtc *crtc, > >> @@ -108,6 +112,8 @@ static void virtio_gpu_crtc_atomic_disable(struct > >> drm_crtc *crtc, > >> struct virtio_gpu_device *vgdev = dev->dev_private; > >> struct virtio_gpu_output *output = > >> drm_crtc_to_virtio_gpu_output(crtc); > >> > >> + drm_crtc_vblank_off(crtc); > >> + > >> virtio_gpu_cmd_set_scanout(vgdev, output->index, 0, 0, 0, 0, 0); > >> virtio_gpu_notify(vgdev); > >> } > >> @@ -121,9 +127,10 @@ static int virtio_gpu_crtc_atomic_check(struct > >> drm_crtc *crtc, > >> static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc, > >> struct drm_atomic_state *state) > >> { > >> - struct drm_crtc_state *crtc_state = > >> drm_atomic_get_new_crtc_state(state, > >> - crtc); > >> + struct drm_device *dev = crtc->dev; > >> + struct drm_crtc_state *crtc_state = > >> drm_atomic_get_new_crtc_state(state, crtc); > >> struct virtio_gpu_output *output = > >> drm_crtc_to_virtio_gpu_output(crtc); > >> + struct drm_pending_vblank_event *event; > >> > >> /* > >> * virtio-gpu can't do modeset and plane update operations > >> @@ -133,6 +140,20 @@ static void virtio_gpu_crtc_atomic_flush(struct > >> drm_crtc *crtc, > >> */ > >> if (drm_atomic_crtc_needs_modeset(crtc_state)) > >> output->needs_modeset = true; > >> + > >> + spin_lock_irq(&dev->event_lock); > >> + > >> + event = crtc_state->event; > >> + crtc_state->event = NULL; > >> + > >> + if (event) { > >> + if (drm_crtc_vblank_get(crtc) == 0) > >> + drm_crtc_arm_vblank_event(crtc, event); > >> + else > >> + drm_crtc_send_vblank_event(crtc, event); > >> + } > >> + > >> + spin_unlock_irq(&dev->event_lock); > >> } > >> > >> static const struct drm_crtc_helper_funcs virtio_gpu_crtc_helper_funcs > = { > >> @@ -356,6 +377,10 @@ int virtio_gpu_modeset_init(struct > virtio_gpu_device > >> *vgdev) > >> for (i = 0 ; i < vgdev->num_scanouts; ++i) > >> vgdev_output_init(vgdev, i); > >> > >> + ret = drm_vblank_init(vgdev->ddev, vgdev->num_scanouts); > >> + if (ret) > >> + return ret; > >> + > >> drm_mode_config_reset(vgdev->ddev); > >> return 0; > >> } > >> -- > >> 2.51.0 > >> > > -- > -- > Thomas Zimmermann > Graphics Driver Developer > SUSE Software Solutions Germany GmbH > Frankenstrasse 146, 90461 Nuernberg, Germany > GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman > HRB 36809 (AG Nuernberg) >
