Changeset 74172 in vbox for trunk/src/VBox/Additions

Timestamp:

Sep 10, 2018 8:17:28 AM (7 years ago)

Author:

vboxsync

svn:sync-xref-src-repo-rev:

124944

Message:

Additions/drm: do not unpin an old frame buffer before the new one is in place.
bugref:9240: Additions/linux/vboxvideo: GPU memory management bug
We have seen sporadic cases of [unpin bad] messages in guest dmesg, and were
able to connect some of them to unpinning an old frame buffer when pinning a
new one before the new pin had succeeded. If the new frame buffer was too
big to be pinned in the available video memory the pin operation failed, but
the old frame buffer was already unpinned. As this did not get tracked in
the error path a subsequent unpin operation on it failed with the message
above.
This change fixes that by only unpinning the old frame buffer after the new
one has been successfully pinned. While this potentially increases memory
pressure further, it avoids the case where we unpin the old one, fail to pin
the new one and fail to re-pin the old one, as well as the case of the old one
getting re-pinned at a different address.
Thank you Hans de Goede for review and comments/suggestions.

File:

• trunk/src/VBox/Additions/linux/drm/vbox_mode.c (modified) (6 diffs)

trunk/src/VBox/Additions/linux/drm/vbox_mode.c

@@ -242 +242 @@
 }
 
-static int vbox_crtc_do_set_base(struct drm_crtc *crtc,
+static int vbox_crtc_set_base(struct drm_crtc *crtc,
                                  struct drm_framebuffer *old_fb, int x, int y)
 {
@@ -253 +253 @@
         u64 gpu_addr;
 
-        /* Unpin the previous fb. */
+        vbox_fb = to_vbox_framebuffer(CRTC_FB(crtc));
+        obj = vbox_fb->obj;
+        bo = gem_to_vbox_bo(obj);
+
+        ret = vbox_bo_reserve(bo, false);
+        if (ret)
+                return ret;
+
+        ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr);
+        vbox_bo_unreserve(bo);
+        if (ret)
+                return ret;
+
+        /* Unpin the previous fb.  Do this after the new one has been pinned rather
+         * than before and re-pinning it on failure in case that fails too. */
         if (old_fb) {
                 vbox_fb = to_vbox_framebuffer(old_fb);
@@ -259 +273 @@
                 bo = gem_to_vbox_bo(obj);
                 ret = vbox_bo_reserve(bo, false);
-                if (ret)
-                        return ret;
-
-                vbox_bo_unpin(bo);
-                vbox_bo_unreserve(bo);
-        }
-
-        vbox_fb = to_vbox_framebuffer(CRTC_FB(crtc));
-        obj = vbox_fb->obj;
-        bo = gem_to_vbox_bo(obj);
-
-        ret = vbox_bo_reserve(bo, false);
-        if (ret)
-                return ret;
-
-        ret = vbox_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr);
-        vbox_bo_unreserve(bo);
-        if (ret)
-                return ret;
+                /* This should never fail, as no one else should be accessing it and we
+                 * should be running under the modeset locks. */
+                if (!ret) {
+                        vbox_bo_unpin(bo);
+                        vbox_bo_unreserve(bo);
+                }
+        }
 
         vbox_crtc->fb_offset = gpu_addr;
@@ -293 +295 @@
 }
 
-static int vbox_crtc_mode_set_base(struct drm_crtc *crtc, int x, int y,
-                                   struct drm_framebuffer *old_fb)
-{
-        return vbox_crtc_do_set_base(crtc, old_fb, x, y);
-}
-
 static int vbox_crtc_mode_set(struct drm_crtc *crtc,
                               struct drm_display_mode *mode,
@@ -305 +301 @@
 {
         struct vbox_private *vbox = crtc->dev->dev_private;
-        int rc = 0;
-
-        vbox_crtc_mode_set_base(crtc, x, y, old_fb);
-
+        int rc = vbox_crtc_set_base(crtc, old_fb, x, y);
+        if (rc)
+                return rc;
         mutex_lock(&vbox->hw_mutex);
         rc = vbox_set_view(crtc);
@@ -337 +332 @@
         .mode_fixup = vbox_crtc_mode_fixup,
         .mode_set = vbox_crtc_mode_set,
-        /* .mode_set_base = vbox_crtc_mode_set_base, */
         .disable = vbox_crtc_disable,
         .prepare = vbox_crtc_prepare,