On Tue, Sep 04, 2012 at 06:10:04PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@xxxxxx>
>
> 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.
>
> Signed-off-by: Rob Clark <rob@xxxxxx>
Yeah, this could also be rather useful to move a few things to saner
places in drm/i915. The fb->refcount is a bit strange though since only
driver-internals can be reference counted, all the userspace interface
stuff in the drm core has to abide to the old api which mandates that we
remove all uses of a given fb at rmfb time. I guess we'll just have to
live with that.
Still, can you add a small comment in drm_crtc.h to the new refcount
explaining this? And maybe extend the note about the ->destroy callback to
say "call _remove or _unrefence instead"? Otherwise
Reviewed-by: Daniel Vetter <daniel.vetter@xxxxxxxx>
> ---
> 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 | 5 ++
> 5 files changed, 78 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);
> - 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..2442d01 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,7 @@ struct drm_framebuffer_funcs {
>
> struct drm_framebuffer {
> struct drm_device *dev;
> + struct kref refcount;
> struct list_head head;
> struct drm_mode_object base;
> const struct drm_framebuffer_funcs *funcs;
> @@ -921,6 +923,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
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Mail: daniel@xxxxxxxx
Mobile: +41 (0)79 365 57 48
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
