drm: refcnt drm_framebuffer (v4.1)

This simplifies drm fb lifetime, and if the crtc/plane needs to hold
a ref to the fb when disabling a pipe until the next vblank, this
avoids the need to make disabling an overlay synchronous.  This is a
problem that shows up when userspace is using a drm plane to
implement a hw cursor.. making overlay disable synchronous causes
a performance problem when x11 is rapidly enabling/disabling the
hw cursor.  But not making it synchronous opens up a race condition
for crashing if userspace turns around and immediately deletes the
fb.  Refcnt'ing the fb makes it possible to solve this problem.

v1: original
v2: add drm_framebuffer_remove() which is called in all paths where
    fb->funcs->destroy() was directly called before.  This cleans
    up the CRTCs/planes that the fb was attached to.  You should
    only directly use drm_framebuffer_unreference() if you are also
    using drm_framebuffer_reference() to keep a ref to the fb.
v3: add comment explaining the fb refcount
v4: remove duplicate 'list_del(&fb->filp_head)'

[airlied: v4.1: fix local rejection]

Signed-off-by: Rob Clark <rob@ti.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>

4 files changed, 85 insertions, 16 deletions

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index c418c772a7e5..39afe13a5fa2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
         int ret;
 
+        kref_init(&fb->refcount);
+
         ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
         if (ret)
                 return ret;
@@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
+static void drm_framebuffer_free(struct kref *kref)
+{
+        struct drm_framebuffer *fb =
+                        container_of(kref, struct drm_framebuffer, refcount);
+        fb->funcs->destroy(fb);
+}
+
+/**
+ * drm_framebuffer_unreference - unref a framebuffer
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ */
+void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+        struct drm_device *dev = fb->dev;
+        DRM_DEBUG("FB ID: %d\n", fb->base.id);
+        WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+        kref_put(&fb->refcount, drm_framebuffer_free);
+}
+EXPORT_SYMBOL(drm_framebuffer_unreference);
+
+/**
+ * drm_framebuffer_reference - incr the fb refcnt
+ */
+void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+        DRM_DEBUG("FB ID: %d\n", fb->base.id);
+        kref_get(&fb->refcount);
+}
+EXPORT_SYMBOL(drm_framebuffer_reference);
+
 /**
  * drm_framebuffer_cleanup - remove a framebuffer object
  * @fb: framebuffer to remove
@@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
         struct drm_device *dev = fb->dev;
+        /*
+         * This could be moved to drm_framebuffer_remove(), but for
+         * debugging is nice to keep around the list of fb's that are
+         * no longer associated w/ a drm_file but are not unreferenced
+         * yet.  (i915 and omapdrm have debugfs files which will show
+         * this.)
+         */
+        drm_mode_object_put(dev, &fb->base);
+        list_del(&fb->head);
+        dev->mode_config.num_fb--;
+}
+EXPORT_SYMBOL(drm_framebuffer_cleanup);
+
+/**
+ * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * @fb: framebuffer to remove
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ *
+ * Scans all the CRTCs and planes in @dev's mode_config.  If they're
+ * using @fb, removes it, setting it to NULL.
+ */
+void drm_framebuffer_remove(struct drm_framebuffer *fb)
+{
+        struct drm_device *dev = fb->dev;
         struct drm_crtc *crtc;
         struct drm_plane *plane;
         struct drm_mode_set set;
@@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
                 }
         }
 
-        drm_mode_object_put(dev, &fb->base);
-        list_del(&fb->head);
-        dev->mode_config.num_fb--;
+        list_del(&fb->filp_head);
+
+        drm_framebuffer_unreference(fb);
 }
-EXPORT_SYMBOL(drm_framebuffer_cleanup);
+EXPORT_SYMBOL(drm_framebuffer_remove);
 
 /**
  * drm_crtc_init - Initialise a new CRTC object
@@ -1031,7 +1091,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
         }
 
         list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-                fb->funcs->destroy(fb);
+                drm_framebuffer_remove(fb);
         }
 
         list_for_each_entry_safe(plane, plt, &dev->mode_config.plane_list,
@@ -2337,11 +2397,7 @@ int drm_mode_rmfb(struct drm_device *dev,
                 goto out;
         }
 
-        /* TODO release all crtc connected to the framebuffer */
-        /* TODO unhock the destructor from the buffer object */
-
-        list_del(&fb->filp_head);
-        fb->funcs->destroy(fb);
+        drm_framebuffer_remove(fb);
 
 out:
         mutex_unlock(&dev->mode_config.mutex);
@@ -2491,8 +2547,7 @@ void drm_fb_release(struct drm_file *priv)
 
         mutex_lock(&dev->mode_config.mutex);
         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-                list_del(&fb->filp_head);
-                fb->funcs->destroy(fb);
+                drm_framebuffer_remove(fb);
         }
         mutex_unlock(&dev->mode_config.mutex);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d5586cc75163..f4ac43356583 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
         /* release drm framebuffer and real buffer */
         if (fb_helper->fb && fb_helper->fb->funcs) {
                 fb = fb_helper->fb;
-                if (fb && fb->funcs->destroy)
-                        fb->funcs->destroy(fb);
+                if (fb)
+                        drm_framebuffer_remove(fb);
         }
 
         /* release linux framebuffer */
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 8c6ed3b0c6f6..8a027bb77d97 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -276,7 +276,7 @@ fail:
                 if (fbi)
                         framebuffer_release(fbi);
                 if (fb)
-                        fb->funcs->destroy(fb);
+                        drm_framebuffer_remove(fb);
         }
 
         return ret;
@@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
 
         /* this will free the backing object */
         if (fbdev->fb)
-                fbdev->fb->funcs->destroy(fbdev->fb);
+                drm_framebuffer_remove(fbdev->fb);
 
         kfree(fbdev);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index eb91d520ce0b..68fdb299e39f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -218,6 +218,7 @@ struct drm_display_info {
 };
 
 struct drm_framebuffer_funcs {
+        /* note: use drm_framebuffer_remove() */
         void (*destroy)(struct drm_framebuffer *framebuffer);
         int (*create_handle)(struct drm_framebuffer *fb,
                              struct drm_file *file_priv,
@@ -242,6 +243,16 @@ struct drm_framebuffer_funcs {
 
 struct drm_framebuffer {
         struct drm_device *dev;
+        /*
+         * Note that the fb is refcounted for the benefit of driver internals,
+         * for example some hw, disabling a CRTC/plane is asynchronous, and
+         * scanout does not actually complete until the next vblank.  So some
+         * cleanup (like releasing the reference(s) on the backing GEM bo(s))
+         * should be deferred.  In cases like this, the driver would like to
+         * hold a ref to the fb even though it has already been removed from
+         * userspace perspective.
+         */
+        struct kref refcount;
         struct list_head head;
         struct drm_mode_object base;
         const struct drm_framebuffer_funcs *funcs;
@@ -919,6 +930,9 @@ extern void drm_framebuffer_set_object(struct drm_device *dev,
 extern int drm_framebuffer_init(struct drm_device *dev,
                                 struct drm_framebuffer *fb,
                                 const struct drm_framebuffer_funcs *funcs);
+extern void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+extern void drm_framebuffer_reference(struct drm_framebuffer *fb);
+extern void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 extern int drmfb_probe(struct drm_device *dev, struct drm_crtc *crtc);
 extern int drmfb_remove(struct drm_device *dev, struct drm_framebuffer *fb);