Re: [PATCH] drm: refcnt drm_framebuffer

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