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