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

[David Herrmann <dh.herrmann@gmail.com>]

Hi

On Wed, Aug 6, 2014 at 9:10 AM, Daniel Vetter <daniel.vetter@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@virtuousgeek.org>
> Date:   Fri Mar 7 08:57:48 2014 -0800
>
>     drm/i915: add plane_config fetching infrastructure v2
>
> Cc:  Jesse Barnes <jbarnes@virtuousgeek.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77511
> Signed-off-by: Daniel Vetter <daniel.vetter@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel