[PATCH] drm: Don't grab an fb reference for the idr
David Herrmann
dh.herrmann at gmail.com
Fri Aug 8 03:35:07 PDT 2014
Hi
On Wed, Aug 6, 2014 at 9:10 AM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> The current refcounting scheme is that the fb lookup idr also holds a
> reference. This works out nicely bacause thus far we've always
> explicitly cleaned up idr entries for framebuffers:
> - Userspace fbs get removed in the rmfb ioctl or when the drm file
> gets closed.
> - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
> at module unload time.
>
> But now i915 also reconstructs the bios fbs for a smooth transition.
> And that fb is purely transitional and should get removed immmediately
> once all crtcs stop using it. Of course if the i915 fbdev code decides
> to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
> in that case the fbdev code will grab it's own reference.
>
> The problem is now that we also want to register that takeover fb in
> the idr, so that userspace can do a smooth transition (animated maybe
> even!) itself. But currently we have no one who will clean up the idr
> reference once that fb isn't useful any more, and so essentially leak
> it.
>
> Fix this by no longer holding a full fb reference for the idr, but
> instead just have a weak reference using kref_get_unless_zero. But
> that requires us to synchronize and clean up with the idr and fb_lock
> in drm_framebuffer_free, so add that. It's a bit ugly that we have to
> unconditionally grab the fb_lock, but without that someone might creep
> through a race.
>
> This leak was caught by the fb leak check in drm_mode_config_cleanup.
> Originally the leak was introduced in
>
> commit 46f297fb83d4f9a6f6891964beb184664341a28b
> Author: Jesse Barnes <jbarnes at virtuousgeek.org>
> Date: Fri Mar 7 08:57:48 2014 -0800
>
> drm/i915: add plane_config fetching infrastructure v2
>
> Cc: Jesse Barnes <jbarnes at virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77511
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> ---
> drivers/gpu/drm/drm_crtc.c | 46 ++++++++++++++++++++++++++++------------------
> 1 file changed, 28 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index fa2be249999c..33ff631c8d23 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -515,9 +515,6 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
> if (ret)
> goto out;
>
> - /* Grab the idr reference. */
> - drm_framebuffer_reference(fb);
> -
> dev->mode_config.num_fb++;
> list_add(&fb->head, &dev->mode_config.fb_list);
> out:
> @@ -527,10 +524,34 @@ out:
> }
> EXPORT_SYMBOL(drm_framebuffer_init);
>
> +/* dev->mode_config.fb_lock must be held! */
> +static void __drm_framebuffer_unregister(struct drm_device *dev,
> + struct drm_framebuffer *fb)
> +{
> + mutex_lock(&dev->mode_config.idr_mutex);
> + idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> + mutex_unlock(&dev->mode_config.idr_mutex);
> +
> + fb->base.id = 0;
> +}
> +
> static void drm_framebuffer_free(struct kref *kref)
> {
> struct drm_framebuffer *fb =
> container_of(kref, struct drm_framebuffer, refcount);
> + struct drm_device *dev = fb->dev;
> +
> + /*
> + * The lookup idr holds a weak reference, which has not necessarily been
> + * removed at this point. Check for that.
> + */
> + mutex_lock(&dev->mode_config.fb_lock);
> + if (fb->base.id) {
> + /* Mark fb as reaped and drop idr ref. */
> + __drm_framebuffer_unregister(dev, fb);
> + }
> + mutex_unlock(&dev->mode_config.fb_lock);
Ewww, this is ugly. You now make the unregistration dynamic and it's
no longer under driver control. The typical device-control flow
assumes there's an authority that controls at which point objects are
registered and unregistered. You now bind it to ref-counts. To be
fair, I think lastclose is equally hackish (and doesn't really work
like you argued already).
I think the real problem is that you want two ref-counts: One
ref-count controls the object availability, the other ref-count
controls the visibility to user-space. This is also what gem does: you
have a kernel-internal ref-count for each gem-object, but you also
have user-space handles. A handle implies a kernel-internal ref-count
but not vice versa. And the object is only accessible from user-space,
as long as you own a handle. I think we want something similar for
FBs. This way you could unregister the bios-FB once no handle exists
(assuming a CRTC owns a handle as long as the FB is used as scan-out).
But I guess no-one wants to implement this, so I still think this
patch is the nicest way to make it work. So I'm fine with it.
Thanks
David
> +
> fb->funcs->destroy(fb);
> }
>
> @@ -567,8 +588,10 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct drm_device *dev,
>
> mutex_lock(&dev->mode_config.fb_lock);
> fb = __drm_framebuffer_lookup(dev, id);
> - if (fb)
> - drm_framebuffer_reference(fb);
> + if (fb) {
> + if (!kref_get_unless_zero(&fb->refcount))
> + fb = NULL;
> + }
> mutex_unlock(&dev->mode_config.fb_lock);
>
> return fb;
> @@ -612,19 +635,6 @@ static void __drm_framebuffer_unreference(struct drm_framebuffer *fb)
> kref_put(&fb->refcount, drm_framebuffer_free_bug);
> }
>
> -/* dev->mode_config.fb_lock must be held! */
> -static void __drm_framebuffer_unregister(struct drm_device *dev,
> - struct drm_framebuffer *fb)
> -{
> - mutex_lock(&dev->mode_config.idr_mutex);
> - idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> - mutex_unlock(&dev->mode_config.idr_mutex);
> -
> - fb->base.id = 0;
> -
> - __drm_framebuffer_unreference(fb);
> -}
> -
> /**
> * drm_framebuffer_unregister_private - unregister a private fb from the lookup idr
> * @fb: fb to unregister
> --
> 1.9.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list