Hi Thomas, Dmitry,
> Subject: Re: [PATCH v4] drm/virtgpu: Make vblank event dependent on the
> external sync
>
> On 10/27/25 12:52, Thomas Zimmermann wrote:
> > For each plane, store the buffer object's sync status in the state
> > of the plane's respective CRTC. During the CRTC's atomic flush,
> > ignore the vblank timer if any of the CRTC's plane's buffer object
> > is synchronized to an external source. Instead send the vblank event
> > immediately. Avoids corner cases where a vblank event happens too
> > late for the next frame to be page flipped in time.
> >
> > Exporters of GEM objects sometimes interfere with the vblank timer;
> > resulting in framerate drops. Hence detect this case and handle it
> > as outlined above.
> >
> > The vblank timer is still active and fires in regular intervals
> > according to the guest display refresh. This rate limits clients
> > that only wait for the next vblank to occur, such as fbcon. These
> > clients would otherwise produce a very high number of frames per
> > second.
> >
> > v4:
> > - only handle imported GEM buffer objects (Dmitri, Vivek)
> > - fix test for plane visibility (Vivek)
> > - always enable vblank timer to make waiting clients happy
> > v3:
> > - disable vblank unconditionally
> > - compute status on each commit
> > v2:
> > - enable/disable vblank timer according to buffer setup
> >
> > Signed-off-by: Thomas Zimmermann <[email protected]>
> > ---
> > This patch was previously called "drm/virtgpu: Make vblank event
> > dependent on the host resource". Earlier versions where meant for
> > testing, rather than being merged. See [1] for a discussion of when
> > the fixed problem happens.
> >
> > [1] https://lore.kernel.org/dri-devel/20251016145230.79270-1-
> > [email protected]/
> > ---
> > drivers/gpu/drm/virtio/virtgpu_display.c | 67 ++++++++++++++++++++++--
> > drivers/gpu/drm/virtio/virtgpu_drv.h | 15 ++++++
> > drivers/gpu/drm/virtio/virtgpu_plane.c | 28 +++++++++-
> > 3 files changed, 104 insertions(+), 6 deletions(-)
>
> No problems spotted. I'd only extended comment in the code explicitly
> stating that drm_gem_is_imported() is the workaround, might edit it on
> applying. Will wait for Vivek's feedback. Thanks.
I applied this patch and tested 3 different scenarios:
1) blob=true + imported buffers
root@localhost:weston_upstream/weston# ./build/clients/weston-simple-egl -o &
Using config: r8g8b8a0
has EGL_EXT_buffer_age and EGL_EXT_swap_buffers_with_damage
has EGL_EXT_surface_compression
245 frames in 5 seconds: 49.000000 fps
236 frames in 5 seconds: 47.200001 fps
279 frames in 5 seconds: 55.799999 fps
240 frames in 5 seconds: 48.000000 fps
244 frames in 5 seconds: 48.799999 fps
283 frames in 5 seconds: 56.599998 fps
274 frames in 5 seconds: 54.799999 fps
2) blob=true
root@localhost:/weston_upstream/weston# ./build/clients/weston-simple-egl -o &
Using config: r8g8b8a0
has EGL_EXT_buffer_age and EGL_EXT_swap_buffers_with_damage
has EGL_EXT_surface_compression
167 frames in 5 seconds: 33.400002 fps
208 frames in 5 seconds: 41.599998 fps
203 frames in 5 seconds: 40.599998 fps
252 frames in 5 seconds: 50.400002 fps
180 frames in 5 seconds: 36.000000 fps
219 frames in 5 seconds: 43.799999 fps
176 frames in 5 seconds: 35.200001 fps
220 frames in 5 seconds: 44.000000 fps
3) blob=false
root@localhost:/weston_upstream/weston# ./build/clients/weston-simple-egl -o &
[1] 1137
Using config: r8g8b8a0
has EGL_EXT_buffer_age and EGL_EXT_swap_buffers_with_damage
has EGL_EXT_surface_compression
285 frames in 5 seconds: 57.000000 fps
291 frames in 5 seconds: 58.200001 fps
290 frames in 5 seconds: 58.000000 fps
297 frames in 5 seconds: 59.400002 fps
291 frames in 5 seconds: 58.200001 fps
292 frames in 5 seconds: 58.400002 fps
The problem in the first 2 cases above is that as long as blob=true, the vblank
timer
is not helpful as we would be over-synchronizing. Note that even in the second
case, if blob=true, then even dumb BOs would satisfy bo->guest_blob condition.
So, I added the following changes to Thomas' patch and can now achieve FPS >= 58
in all 3 cases above:
diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c
b/drivers/gpu/drm/virtio/virtgpu_display.c
index a4fa948023da..10c4a16a3908 100644
--- a/drivers/gpu/drm/virtio/virtgpu_display.c
+++ b/drivers/gpu/drm/virtio/virtgpu_display.c
@@ -159,7 +159,10 @@ 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);
+ struct virtio_gpu_crtc_state *vgcrtc_state =
to_virtio_gpu_crtc_state(crtc->state);
+
+ if (!virtio_gpu_crtc_state_send_event_on_flush(vgcrtc_state))
+ drm_crtc_vblank_on(crtc);
}
static void virtio_gpu_crtc_atomic_disable(struct drm_crtc *crtc,
diff --git a/drivers/gpu/drm/virtio/virtgpu_plane.c
b/drivers/gpu/drm/virtio/virtgpu_plane.c
index 634e1a930783..8381892e6d4e 100644
--- a/drivers/gpu/drm/virtio/virtgpu_plane.c
+++ b/drivers/gpu/drm/virtio/virtgpu_plane.c
@@ -99,6 +99,8 @@ static const struct drm_plane_funcs virtio_gpu_plane_funcs = {
static int virtio_gpu_plane_atomic_check(struct drm_plane *plane,
struct drm_atomic_state *state)
{
+ struct drm_device *dev = plane->dev;
+ struct virtio_gpu_device *vgdev = dev->dev_private;
struct drm_plane_state *new_plane_state =
drm_atomic_get_new_plane_state(state,
plane);
struct drm_plane_state *old_plane_state =
drm_atomic_get_old_plane_state(state,
@@ -138,13 +140,14 @@ static int virtio_gpu_plane_atomic_check(struct drm_plane
*plane,
for (i = 0; i < new_plane_state->fb->format->num_planes; ++i) {
struct drm_gem_object *obj =
drm_gem_fb_get_obj(new_plane_state->fb, i);
+ struct virtio_gpu_object *bo = gem_to_virtio_gpu_obj(obj);
/*
* Exporters of GEM buffer objects sometimes interfere with the
* vblank timer. Mark the plane as externally synchronized if we
* find an imported GEM buffer object.
*/
- if (drm_gem_is_imported(obj)) {
+ if (bo->guest_blob && !vgdev->has_virgl_3d) {
vgcrtc_state->plane_synced_to_ext |=
drm_plane_mask(plane);
break; /* only need to find one */
For testing the 3 cases, I ran Gnome Wayland like this:
XDG_SESSION_TYPE=wayland dbus-run-session -- ./bin/gnome-shell --wayland
--no-x11 &
and here are the relevant options for Qemu cmd line I tested with:
./qemu-system-x86_64 -m 4096m
-device virtio-gpu,max_outputs=1,xres=1920,yres=1080,blob=true
-display gtk,gl=on
-object memory-backend-memfd,id=mem1,size=4096M
-machine q35,accel=kvm,memory-backend=mem1 -usb -device usb-tablet -serial stdio
Thanks,
Vivek
>
> Reviewed-by: Dmitry Osipenko <[email protected]>
> Tested-by: Dmitry Osipenko <[email protected]>
>
> --
> Best regards,
> Dmitry
