drm: refcnt drm_framebuffer

Message ID 1346869072-28599-1-git-send-email-rob.clark@linaro.org
State New
Headers show

Commit Message

Rob Clark Sept. 5, 2012, 6:17 p.m.
From: Rob Clark <rob@ti.com>

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

Signed-off-by: Rob Clark <rob@ti.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

---
 drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
 drivers/gpu/drm/i915/intel_display.c      |    4 +-
 drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
 include/drm/drm_crtc.h                    |   14 ++++++
 5 files changed, 87 insertions(+), 17 deletions(-)

Comments

Rob Clark Sept. 5, 2012, 8:59 p.m. | #1
On Wed, Sep 5, 2012 at 1:17 PM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> 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
>
> Signed-off-by: Rob Clark <rob@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> ---
>  drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
>  drivers/gpu/drm/i915/intel_display.c      |    4 +-
>  drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
>  include/drm/drm_crtc.h                    |   14 ++++++
>  5 files changed, 87 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 901de9a..96e236f 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
> @@ -1032,7 +1092,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(crtc, ct, &dev->mode_config.crtc_list, head) {
> @@ -2343,11 +2403,8 @@ 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);

errrg,  that list_del() was supposed to be removed as well..  so let's
try this one more time

BR,
-R

> -       fb->funcs->destroy(fb);
> +       drm_framebuffer_remove(fb);
>
>  out:
>         mutex_unlock(&dev->mode_config.mutex);
> @@ -2497,8 +2554,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 d5586cc..f4ac433 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/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a69a3d0..2109c9f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
>         if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
>                 DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>                 if (old->release_fb)
> -                       old->release_fb->funcs->destroy(old->release_fb);
> +                       drm_framebuffer_remove(old->release_fb);
>                 crtc->fb = old_fb;
>                 return false;
>         }
> @@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
>                 drm_helper_disable_unused_functions(dev);
>
>                 if (old->release_fb)
> -                       old->release_fb->funcs->destroy(old->release_fb);
> +                       drm_framebuffer_remove(old->release_fb);
>
>                 return;
>         }
> diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
> index 8c6ed3b..8a027bb 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 a82e0a2..1422b36 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -219,6 +219,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,
> @@ -243,6 +244,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;
> @@ -921,6 +932,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
>  extern int drm_framebuffer_init(struct drm_device *dev,
>                                 struct drm_framebuffer *fb,
>                                 const struct drm_framebuffer_funcs *funcs);
> +void drm_framebuffer_unreference(struct drm_framebuffer *fb);
> +void drm_framebuffer_reference(struct drm_framebuffer *fb);
> +void drm_framebuffer_remove(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>
>  extern void drm_connector_attach_property(struct drm_connector *connector,
> --
> 1.7.9.5
>

Patch

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 901de9a..96e236f 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
@@ -1032,7 +1092,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(crtc, ct, &dev->mode_config.crtc_list, head) {
@@ -2343,11 +2403,8 @@  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);
@@ -2497,8 +2554,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 d5586cc..f4ac433 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/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a69a3d0..2109c9f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5687,7 +5687,7 @@  bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 		crtc->fb = old_fb;
 		return false;
 	}
@@ -5717,7 +5717,7 @@  void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 
 		return;
 	}
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 8c6ed3b..8a027bb 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 a82e0a2..1422b36 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -219,6 +219,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,
@@ -243,6 +244,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;
@@ -921,6 +932,9 @@  extern int drm_object_property_get_value(struct drm_mode_object *obj,
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
+void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+void drm_framebuffer_reference(struct drm_framebuffer *fb);
+void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 
 extern void drm_connector_attach_property(struct drm_connector *connector,