On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > 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@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. >> >> 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. hmm, maybe somebody switched my coffee with decaf, but why isn't lastclose a natural point? ofc if that really doesn't work, the weak-ref thing seems like it would solve it nicely. But if there were a simple solution that didn't involve making fb refcnting more complex, I guess I would prefer that BR, -R > -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@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); >> > + >> > 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 > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
