| Message ID | 1346869072-28599-1-git-send-email-rob.clark@linaro.org (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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,