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

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 3: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.
> 
> 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?

There's no natural point really but when we drop the last reference for
it. Going the weak reference route looked the most natural. And I honestly
expect other drivers to eventually do the same - forcing a modeset on
boot-up is kinda not too pretty, and permanently reserving a big
framebuffer just for the bios doesn't sound good either. This approach
would nicely solve it for everyone.
-Daniel

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch