[PATCH] drm: Don't grab an fb reference for the idr
Rob Clark
robdclark at gmail.com
Wed Aug 6 06:12:42 PDT 2014
On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel at ffwll.ch> 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 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?
>
> 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 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
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
More information about the dri-devel
mailing list