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

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

[Daniel Vetter]

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@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

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

[Rob Clark]

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?

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

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

[Daniel Vetter]

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

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

[Rob Clark]

On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel@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@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@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

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

[Daniel Vetter]

On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel@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@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?

There is no lastclose for the bios ;-)

Let me elaborate on what happens:

1. BIOS sets up an initial config with a framebuffer in stolen.

2. i915 takes over and reconstructs all the state, so now we have all the
crtcs enabled using a framebuffer for all of them which wraps the bios
allocation.

2b. (optional) reuse that framebuffer for fbdev.

-> That special bios fb has the following references:
- 1 reference for each crtc that's using it
- 1 optional reference if it's reused as the fbdev fb
- 1 reference for the idr

3. Userspace takes over, potentially doing a getfb on the current
(bios-inherited) fb for smooth transition, but then does a modeset to its
own fb.

-> After this all the we've dropped the crtc references and we also want
to drop the idr reference (since no one will ever use this framebuffer
again). But there's simply no good place to do that. Lastclose might only
happen before we shut down the system again, which is a bit too late.

Note that the getfb call creates a gem handle for the fb object, so the
backing storage might survive for a lot longer than the fb.

> 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

Well I didn't come up with anything else really. Plan b would be to add
hooks after any plane updates and manually check whether that special fb
has lost all but its idr reference, and if so clean it up. That seems to
be a lot more fragile and convoluted than converting the idr to a weak
reference.

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

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

[Rob Clark]

On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
>> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel@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@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?
>
> There is no lastclose for the bios ;-)
>
> Let me elaborate on what happens:
>
> 1. BIOS sets up an initial config with a framebuffer in stolen.
>
> 2. i915 takes over and reconstructs all the state, so now we have all the
> crtcs enabled using a framebuffer for all of them which wraps the bios
> allocation.
>
> 2b. (optional) reuse that framebuffer for fbdev.
>
> -> That special bios fb has the following references:
> - 1 reference for each crtc that's using it
> - 1 optional reference if it's reused as the fbdev fb
> - 1 reference for the idr
>
> 3. Userspace takes over, potentially doing a getfb on the current
> (bios-inherited) fb for smooth transition, but then does a modeset to its
> own fb.
>
> -> After this all the we've dropped the crtc references and we also want
> to drop the idr reference (since no one will ever use this framebuffer
> again). But there's simply no good place to do that. Lastclose might only
> happen before we shut down the system again, which is a bit too late.

Well, you could still cleanup your idr reference on current
userspace's lastclose.. that doesn't do much good, I suppose, if
current userspace never exits.  But if first userspace is plymouth or
something like that, you would get cleaned up on the
splash->{x11/wayland} transition..

if that isn't sufficient, then yeah I guess the more fancy weak-ref
stuff needed..

BR,
-R

> Note that the getfb call creates a gem handle for the fb object, so the
> backing storage might survive for a lot longer than the fb.
>
>> 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
>
> Well I didn't come up with anything else really. Plan b would be to add
> hooks after any plane updates and manually check whether that special fb
> has lost all but its idr reference, and if so clean it up. That seems to
> be a lot more fragile and convoluted than converting the idr to a weak
> reference.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:59:29PM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
> >> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter <daniel@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@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?
> >
> > There is no lastclose for the bios ;-)
> >
> > Let me elaborate on what happens:
> >
> > 1. BIOS sets up an initial config with a framebuffer in stolen.
> >
> > 2. i915 takes over and reconstructs all the state, so now we have all the
> > crtcs enabled using a framebuffer for all of them which wraps the bios
> > allocation.
> >
> > 2b. (optional) reuse that framebuffer for fbdev.
> >
> > -> That special bios fb has the following references:
> > - 1 reference for each crtc that's using it
> > - 1 optional reference if it's reused as the fbdev fb
> > - 1 reference for the idr
> >
> > 3. Userspace takes over, potentially doing a getfb on the current
> > (bios-inherited) fb for smooth transition, but then does a modeset to its
> > own fb.
> >
> > -> After this all the we've dropped the crtc references and we also want
> > to drop the idr reference (since no one will ever use this framebuffer
> > again). But there's simply no good place to do that. Lastclose might only
> > happen before we shut down the system again, which is a bit too late.
> 
> Well, you could still cleanup your idr reference on current
> userspace's lastclose.. that doesn't do much good, I suppose, if
> current userspace never exits.  But if first userspace is plymouth or
> something like that, you would get cleaned up on the
> splash->{x11/wayland} transition..

So plymouth starts, but it doesn't exit until X starts (since otherwise
the fb gets removed and the crtc shut off and we switch to fbcon). So
again no lastclose to link into. So I don't think any check in a close
callback will cut it.

> if that isn't sufficient, then yeah I guess the more fancy weak-ref
> stuff needed..

Trust me I don't like it either ;-) But for this we really have no natural
point to reap the lookup reference like with gem objects, or normal
userspace framebuffers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[David Herrmann]

Hi

On Wed, Aug 6, 2014 at 9: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.
>
> 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);

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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Daniel Vetter]

On Fri, Aug 8, 2014 at 12:35 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> 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).

Nope, it don't bind the unregister to the refcount, I only make the
registerstration weak so that you can do such games. For all the
normal framebuffers and anything controlled by drivers we can still
unregister at any point before the final unref.

> 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.

This doesn't actually solve anything here - the problem is exactly
that this fb just exists, with no one having an access handle (beyond
the internal usage refcount) on it. Not the fbdev emulation, not
anything from userspace. But we want userspace to get at it with a
gettfb call for smooth transitions, so simply not registering it also
won't work.

The thing just hangs of a few crtcs and should survive as long as
that's the case, but as soon as it's no longer used it should get
reaped. So the right place to hook the unregistration into is really
when the last refcount goes away.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch