Hi Thomas, > Subject: Re: [PATCH] drm/virtgpu: Use vblank timer > > Hi > > Am 10.10.25 um 06:25 schrieb Kasireddy, Vivek: > [...] > >> 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. > > To summarize my understanding: virtgpu currently generates vblank events > as soon as if finished updating the display. Which can be too fast for > blob=false. For blob=true, the display update is (reasonably assumed to > be) synchronized with the host update and thereby rate limited by the > host's display output. Yeah, that is mostly correct.
> > Adding the vblank timer: For blob=true, if a vblank timeout happens > during an ongoing display update, it does not send an event. If the > display update finishes before the vblank times out, it arms the vblank > event and the next vblank timeout will send it out. The vblank timer > thereby makes events appear at regular intervals instead of ASAP. > > So why add the complexity of handling blob=true separately when it > should integrate well with the existing vblank framework? There are a few reasons for this: - When blob=true, the most important goal is to prevent tearing (i.e, to ensure that Guest VM and Host are not accessing the Guest's FB simultaneously). As explained previously, just ensuring that this goal is met will automatically result in rate limited updates that are in sync with the Host compositor's repaint cycle (if using Gtk UI). So, given this, adding a vblank timer would not help in this case and instead would lead to updates that are no longer in sync with the Host compositor's repaint cycle. - Qemu (and Qemu UI) also works with virtio-gpu driver that is part of Windows VMs where a vblank timer may or may not be available. So, it is easier to support this case by having a UI timer/repaint cb and making the Guest VM just wait until the update (or resource flush) is completed to ensure synchronization and rate limited updates. - Even if a vblank timer is available for a Guest VM (Windows or Linux) its frequency may not match the UI update frequency, so there might either be wasteful updates or too few updates. For example, if the Host's rate is 30 FPS, but the Guest VM's vblank timer is running at 60 FPS, then roughly 30 frames would not be displayed each cycle IIUC. > > Please also note that it's not just about compositors. DRM's fbcon > synchronizes its output to the vblank period. If no vblank is supported, > it'll happily fire out display updates ASAP even for blob=true. IIUC, the fbcon updates would still be rate limited in the blob=true case as the vblank event would only be sent out after the atomic/plane update is finished right? Thanks, Vivek > > Best regards > Thomas > > > 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) > >> > > -- > -- > 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) >
