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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux