[PATCH] drm: Don't grab an fb reference for the idr

[Rob Clark]

On Wed, Aug 6, 2014 at 3: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.

ewww..  couldn't you do some scheme on lastclose to check if no more
crtc's are scanning out that fb, and if not then remove the idr?

BR,
-R


> 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);
> +
>         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