[PATCH] drm: refcnt drm_framebuffer

[PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

From: Rob Clark <rob@ti.com>

This simplifies drm fb lifetime, and if the crtc/plane needs to hold
a ref to the fb when disabling a pipe until the next vblank, this
avoids the need to make disabling an overlay synchronous.  This is a
problem that shows up when userspace is using a drm plane to
implement a hw cursor.. making overlay disable synchronous causes
a performance problem when x11 is rapidly enabling/disabling the
hw cursor.  But not making it synchronous opens up a race condition
for crashing if userspace turns around and immediately deletes the
fb.  Refcnt'ing the fb makes it possible to solve this problem.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/drm_crtc.c                |   38 ++++++++++++++++++++++++++---
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +--
 drivers/gpu/drm/i915/intel_display.c      |    4 +--
 include/drm/drm_crtc.h                    |    4 +++
 4 files changed, 43 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 08a7aa7..2f928a3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
 	int ret;
 
+	kref_init(&fb->refcount);
+
 	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
 	if (ret)
 		return ret;
@@ -307,6 +309,36 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
+static void drm_framebuffer_free(struct kref *kref)
+{
+	struct drm_framebuffer *fb =
+			container_of(kref, struct drm_framebuffer, refcount);
+	fb->funcs->destroy(fb);
+}
+
+/**
+ * drm_framebuffer_unreference - unref a framebuffer
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ */
+void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	kref_put(&fb->refcount, drm_framebuffer_free);
+}
+EXPORT_SYMBOL(drm_framebuffer_unreference);
+
+/**
+ * drm_framebuffer_reference - incr the fb refcnt
+ */
+void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+	kref_get(&fb->refcount);
+}
+EXPORT_SYMBOL(drm_framebuffer_reference);
+
 /**
  * drm_framebuffer_cleanup - remove a framebuffer object
  * @fb: framebuffer to remove
@@ -1031,7 +1063,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		fb->funcs->destroy(fb);
+		drm_framebuffer_unreference(fb);
 	}
 
 	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
@@ -2339,7 +2371,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 	/* TODO unhock the destructor from the buffer object */
 
 	list_del(&fb->filp_head);
-	fb->funcs->destroy(fb);
+	drm_framebuffer_unreference(fb);
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
@@ -2490,7 +2522,7 @@ void drm_fb_release(struct drm_file *priv)
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
 		list_del(&fb->filp_head);
-		fb->funcs->destroy(fb);
+		drm_framebuffer_unreference(fb);
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d5586cc..05695d6 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
 	/* release drm framebuffer and real buffer */
 	if (fb_helper->fb && fb_helper->fb->funcs) {
 		fb = fb_helper->fb;
-		if (fb && fb->funcs->destroy)
-			fb->funcs->destroy(fb);
+		if (fb)
+			drm_framebuffer_unreference(fb);
 	}
 
 	/* release linux framebuffer */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a8538ac..a9d2328 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5467,7 +5467,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_unreference(old->release_fb);
 		crtc->fb = old_fb;
 		return false;
 	}
@@ -5497,7 +5497,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_unreference(old->release_fb);
 
 		return;
 	}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index bac55c2..8a5b16d 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -221,6 +221,7 @@ struct drm_display_info {
 };
 
 struct drm_framebuffer_funcs {
+	/* note: use drm_framebuffer_unreference() */
 	void (*destroy)(struct drm_framebuffer *framebuffer);
 	int (*create_handle)(struct drm_framebuffer *fb,
 			     struct drm_file *file_priv,
@@ -245,6 +246,7 @@ struct drm_framebuffer_funcs {
 
 struct drm_framebuffer {
 	struct drm_device *dev;
+	struct kref refcount;
 	struct list_head head;
 	struct drm_mode_object base;
 	const struct drm_framebuffer_funcs *funcs;
@@ -923,6 +925,8 @@ extern void drm_framebuffer_set_object(struct drm_device *dev,
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
+void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+void drm_framebuffer_reference(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 extern int drmfb_probe(struct drm_device *dev, struct drm_crtc *crtc);
 extern int drmfb_remove(struct drm_device *dev, struct drm_framebuffer *fb);
-- 
1.7.9.5

Re: [PATCH] drm: refcnt drm_framebuffer

[Chris Wilson]

On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
> 
> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
> a ref to the fb when disabling a pipe until the next vblank, this
> avoids the need to make disabling an overlay synchronous.  This is a
> problem that shows up when userspace is using a drm plane to
> implement a hw cursor.. making overlay disable synchronous causes
> a performance problem when x11 is rapidly enabling/disabling the
> hw cursor.  But not making it synchronous opens up a race condition
> for crashing if userspace turns around and immediately deletes the
> fb.  Refcnt'ing the fb makes it possible to solve this problem.

Presumably you have a follow-on patch putting the new refcnt to use so
that we can judge whether you truly need refcnting on the fb itself in
addition to the refcnted object and the various hw bookkeeping that
needs to be performed?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> From: Rob Clark <rob@ti.com>
>>
>> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
>> a ref to the fb when disabling a pipe until the next vblank, this
>> avoids the need to make disabling an overlay synchronous.  This is a
>> problem that shows up when userspace is using a drm plane to
>> implement a hw cursor.. making overlay disable synchronous causes
>> a performance problem when x11 is rapidly enabling/disabling the
>> hw cursor.  But not making it synchronous opens up a race condition
>> for crashing if userspace turns around and immediately deletes the
>> fb.  Refcnt'ing the fb makes it possible to solve this problem.
>
> Presumably you have a follow-on patch putting the new refcnt to use so
> that we can judge whether you truly need refcnting on the fb itself in
> addition to the refcnted object and the various hw bookkeeping that
> needs to be performed?

Yes, I do.. although it is a bit experimental at this point, so not
really ready to be submitted as anything other than an RFC.. it is
part of omapdrm kms re-write to use dispc directly rather than go thru
omapdss.  (With omapdss we don't hit this issue because disabling
overlays is forced to be synchronous.. so instead we have the
performance problem I mentioned.)

I *could* just rely on the GEM refcnt, but that gets messier when you
take into account multi-planar formats.  I suppose I also could have
my own internal refcnt'd object to hold the set of GEM objects
associated w/ the fb, but, well, that seems a bit silly.  And
refcnt'ing the fb had been mentioned previously as a good thing to do
(I think it was danvet?)

BR,
-R

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: refcnt drm_framebuffer

[Chris Wilson]

On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> >> From: Rob Clark <rob@ti.com>
> >>
> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
> >> a ref to the fb when disabling a pipe until the next vblank, this
> >> avoids the need to make disabling an overlay synchronous.  This is a
> >> problem that shows up when userspace is using a drm plane to
> >> implement a hw cursor.. making overlay disable synchronous causes
> >> a performance problem when x11 is rapidly enabling/disabling the
> >> hw cursor.  But not making it synchronous opens up a race condition
> >> for crashing if userspace turns around and immediately deletes the
> >> fb.  Refcnt'ing the fb makes it possible to solve this problem.
> >
> > Presumably you have a follow-on patch putting the new refcnt to use so
> > that we can judge whether you truly need refcnting on the fb itself in
> > addition to the refcnted object and the various hw bookkeeping that
> > needs to be performed?
> 
> Yes, I do.. although it is a bit experimental at this point, so not
> really ready to be submitted as anything other than an RFC.. it is
> part of omapdrm kms re-write to use dispc directly rather than go thru
> omapdss.  (With omapdss we don't hit this issue because disabling
> overlays is forced to be synchronous.. so instead we have the
> performance problem I mentioned.)
> 
> I *could* just rely on the GEM refcnt, but that gets messier when you
> take into account multi-planar formats.  I suppose I also could have
> my own internal refcnt'd object to hold the set of GEM objects
> associated w/ the fb, but, well, that seems a bit silly.  And
> refcnt'ing the fb had been mentioned previously as a good thing to do
> (I think it was danvet?)

Sure, there are a few places in the code that have caused ordering
issues in the past due to lack of refcnting the fb... But since you
haven't fixed up those cases, I'm looking for justification for adding
that extra bit of complexity. Adding a new interface and no users is
just asking for trouble.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> >> From: Rob Clark <rob@ti.com>
>> >>
>> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
>> >> a ref to the fb when disabling a pipe until the next vblank, this
>> >> avoids the need to make disabling an overlay synchronous.  This is a
>> >> problem that shows up when userspace is using a drm plane to
>> >> implement a hw cursor.. making overlay disable synchronous causes
>> >> a performance problem when x11 is rapidly enabling/disabling the
>> >> hw cursor.  But not making it synchronous opens up a race condition
>> >> for crashing if userspace turns around and immediately deletes the
>> >> fb.  Refcnt'ing the fb makes it possible to solve this problem.
>> >
>> > Presumably you have a follow-on patch putting the new refcnt to use so
>> > that we can judge whether you truly need refcnting on the fb itself in
>> > addition to the refcnted object and the various hw bookkeeping that
>> > needs to be performed?
>>
>> Yes, I do.. although it is a bit experimental at this point, so not
>> really ready to be submitted as anything other than an RFC.. it is
>> part of omapdrm kms re-write to use dispc directly rather than go thru
>> omapdss.  (With omapdss we don't hit this issue because disabling
>> overlays is forced to be synchronous.. so instead we have the
>> performance problem I mentioned.)
>>
>> I *could* just rely on the GEM refcnt, but that gets messier when you
>> take into account multi-planar formats.  I suppose I also could have
>> my own internal refcnt'd object to hold the set of GEM objects
>> associated w/ the fb, but, well, that seems a bit silly.  And
>> refcnt'ing the fb had been mentioned previously as a good thing to do
>> (I think it was danvet?)
>
> Sure, there are a few places in the code that have caused ordering
> issues in the past due to lack of refcnting the fb... But since you
> haven't fixed up those cases, I'm looking for justification for adding
> that extra bit of complexity. Adding a new interface and no users is
> just asking for trouble.

fwiw, my line of reasoning was: from userspace perspective, once you
do drm_mode_rmfb(), the fb is gone, so I didn't change where it gets
removed from the list of fb's or anything like that.  Instead I just
left it so that a driver could, if it wants, take an extra ref
temporarily to the fb to keep it around for a bit.  I didn't change
any of the existing drivers, other than update the to call
drm_framebuffer_unreference() instead of fb->funcs->delete() directly,
because I didn't want to break anything in existing drivers, and I
figured the new behavior was better as an opt-in by individual drivers
when they want to take advantage of refcnt'ing.  But if you have
suggestions about related cleanups that should be made, I'm willing to
take that on.

Anyways, like I mentioned, I'm still a little ways from being ready to
submit anything other than RFC patches in omapdrm to use fb
refcnt'ing, so this isn't something that needs to be merged
immediately.  But it seemed like low risk and like it would be
something that other drivers could take advantage of, so I figured it
was worth sending this patch now.

BR,
-R

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> >> From: Rob Clark <rob@ti.com>
>> >>
>> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
>> >> a ref to the fb when disabling a pipe until the next vblank, this
>> >> avoids the need to make disabling an overlay synchronous.  This is a
>> >> problem that shows up when userspace is using a drm plane to
>> >> implement a hw cursor.. making overlay disable synchronous causes
>> >> a performance problem when x11 is rapidly enabling/disabling the
>> >> hw cursor.  But not making it synchronous opens up a race condition
>> >> for crashing if userspace turns around and immediately deletes the
>> >> fb.  Refcnt'ing the fb makes it possible to solve this problem.
>> >
>> > Presumably you have a follow-on patch putting the new refcnt to use so
>> > that we can judge whether you truly need refcnting on the fb itself in
>> > addition to the refcnted object and the various hw bookkeeping that
>> > needs to be performed?
>>
>> Yes, I do.. although it is a bit experimental at this point, so not
>> really ready to be submitted as anything other than an RFC.. it is
>> part of omapdrm kms re-write to use dispc directly rather than go thru
>> omapdss.  (With omapdss we don't hit this issue because disabling
>> overlays is forced to be synchronous.. so instead we have the
>> performance problem I mentioned.)
>>
>> I *could* just rely on the GEM refcnt, but that gets messier when you
>> take into account multi-planar formats.  I suppose I also could have
>> my own internal refcnt'd object to hold the set of GEM objects
>> associated w/ the fb, but, well, that seems a bit silly.  And
>> refcnt'ing the fb had been mentioned previously as a good thing to do
>> (I think it was danvet?)
>
> Sure, there are a few places in the code that have caused ordering
> issues in the past due to lack of refcnting the fb... But since you
> haven't fixed up those cases, I'm looking for justification for adding
> that extra bit of complexity. Adding a new interface and no users is
> just asking for trouble.

hmm, I did realize that drm_plane cleanup only happens as a result of
drm_framebuffer_cleanup().. which doesn't work too well if the driver
is holding a ref to the fb :-/

so I guess at a minimum I need to fix plane cleanup to be part of
drm_fb_helper_restore_fbdev_mode()

BR,
-R

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm: refcnt drm_framebuffer

[Ville Syrjälä]

On Tue, Jul 31, 2012 at 02:28:29PM -0500, Rob Clark wrote:
> On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> >> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
> >> >> From: Rob Clark <rob@ti.com>
> >> >>
> >> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
> >> >> a ref to the fb when disabling a pipe until the next vblank, this
> >> >> avoids the need to make disabling an overlay synchronous.  This is a
> >> >> problem that shows up when userspace is using a drm plane to
> >> >> implement a hw cursor.. making overlay disable synchronous causes
> >> >> a performance problem when x11 is rapidly enabling/disabling the
> >> >> hw cursor.  But not making it synchronous opens up a race condition
> >> >> for crashing if userspace turns around and immediately deletes the
> >> >> fb.  Refcnt'ing the fb makes it possible to solve this problem.
> >> >
> >> > Presumably you have a follow-on patch putting the new refcnt to use so
> >> > that we can judge whether you truly need refcnting on the fb itself in
> >> > addition to the refcnted object and the various hw bookkeeping that
> >> > needs to be performed?
> >>
> >> Yes, I do.. although it is a bit experimental at this point, so not
> >> really ready to be submitted as anything other than an RFC.. it is
> >> part of omapdrm kms re-write to use dispc directly rather than go thru
> >> omapdss.  (With omapdss we don't hit this issue because disabling
> >> overlays is forced to be synchronous.. so instead we have the
> >> performance problem I mentioned.)
> >>
> >> I *could* just rely on the GEM refcnt, but that gets messier when you
> >> take into account multi-planar formats.  I suppose I also could have
> >> my own internal refcnt'd object to hold the set of GEM objects
> >> associated w/ the fb, but, well, that seems a bit silly.  And
> >> refcnt'ing the fb had been mentioned previously as a good thing to do
> >> (I think it was danvet?)
> >
> > Sure, there are a few places in the code that have caused ordering
> > issues in the past due to lack of refcnting the fb... But since you
> > haven't fixed up those cases, I'm looking for justification for adding
> > that extra bit of complexity. Adding a new interface and no users is
> > just asking for trouble.
> 
> hmm, I did realize that drm_plane cleanup only happens as a result of
> drm_framebuffer_cleanup().. which doesn't work too well if the driver
> is holding a ref to the fb :-/
> 
> so I guess at a minimum I need to fix plane cleanup to be part of
> drm_fb_helper_restore_fbdev_mode()

Your patch would still significantly change the behavior of
drm_mode_rmfb(). Currently it disables all planes and crtcs which
currently use the fb, and it removes the fb id from the idr so that
no new users of the fb can appear afterwards.

Not that I really like the current behaviour of drm_mode_rmfb(), but
it's been like that always, so changing it doesn't seem acceptable.

-- 
Ville Syrjälä
Intel OTC
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Wed, Aug 1, 2012 at 6:36 AM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Tue, Jul 31, 2012 at 02:28:29PM -0500, Rob Clark wrote:
>> On Tue, Jul 31, 2012 at 12:47 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > On Tue, 31 Jul 2012 12:41:28 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> >> On Tue, Jul 31, 2012 at 12:00 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> >> > On Tue, 31 Jul 2012 11:20:21 -0500, Rob Clark <rob.clark@linaro.org> wrote:
>> >> >> From: Rob Clark <rob@ti.com>
>> >> >>
>> >> >> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
>> >> >> a ref to the fb when disabling a pipe until the next vblank, this
>> >> >> avoids the need to make disabling an overlay synchronous.  This is a
>> >> >> problem that shows up when userspace is using a drm plane to
>> >> >> implement a hw cursor.. making overlay disable synchronous causes
>> >> >> a performance problem when x11 is rapidly enabling/disabling the
>> >> >> hw cursor.  But not making it synchronous opens up a race condition
>> >> >> for crashing if userspace turns around and immediately deletes the
>> >> >> fb.  Refcnt'ing the fb makes it possible to solve this problem.
>> >> >
>> >> > Presumably you have a follow-on patch putting the new refcnt to use so
>> >> > that we can judge whether you truly need refcnting on the fb itself in
>> >> > addition to the refcnted object and the various hw bookkeeping that
>> >> > needs to be performed?
>> >>
>> >> Yes, I do.. although it is a bit experimental at this point, so not
>> >> really ready to be submitted as anything other than an RFC.. it is
>> >> part of omapdrm kms re-write to use dispc directly rather than go thru
>> >> omapdss.  (With omapdss we don't hit this issue because disabling
>> >> overlays is forced to be synchronous.. so instead we have the
>> >> performance problem I mentioned.)
>> >>
>> >> I *could* just rely on the GEM refcnt, but that gets messier when you
>> >> take into account multi-planar formats.  I suppose I also could have
>> >> my own internal refcnt'd object to hold the set of GEM objects
>> >> associated w/ the fb, but, well, that seems a bit silly.  And
>> >> refcnt'ing the fb had been mentioned previously as a good thing to do
>> >> (I think it was danvet?)
>> >
>> > Sure, there are a few places in the code that have caused ordering
>> > issues in the past due to lack of refcnting the fb... But since you
>> > haven't fixed up those cases, I'm looking for justification for adding
>> > that extra bit of complexity. Adding a new interface and no users is
>> > just asking for trouble.
>>
>> hmm, I did realize that drm_plane cleanup only happens as a result of
>> drm_framebuffer_cleanup().. which doesn't work too well if the driver
>> is holding a ref to the fb :-/
>>
>> so I guess at a minimum I need to fix plane cleanup to be part of
>> drm_fb_helper_restore_fbdev_mode()
>
> Your patch would still significantly change the behavior of
> drm_mode_rmfb(). Currently it disables all planes and crtcs which
> currently use the fb, and it removes the fb id from the idr so that
> no new users of the fb can appear afterwards.
>
> Not that I really like the current behaviour of drm_mode_rmfb(), but
> it's been like that always, so changing it doesn't seem acceptable.

yeah, I'm working on an update that decouples the crtc/plane shutdown
from deleting the fb, which should address these issues

BR,
-R

> --
> Ville Syrjälä
> Intel OTC
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

From: Rob Clark <rob@ti.com>

This simplifies drm fb lifetime, and if the crtc/plane needs to hold
a ref to the fb when disabling a pipe until the next vblank, this
avoids the need to make disabling an overlay synchronous.  This is a
problem that shows up when userspace is using a drm plane to
implement a hw cursor.. making overlay disable synchronous causes
a performance problem when x11 is rapidly enabling/disabling the
hw cursor.  But not making it synchronous opens up a race condition
for crashing if userspace turns around and immediately deletes the
fb.  Refcnt'ing the fb makes it possible to solve this problem.

v1: original
v2: add drm_framebuffer_remove() which is called in all paths where
    fb->funcs->destroy() was directly called before.  This cleans
    up the CRTCs/planes that the fb was attached to.  You should
    only directly use drm_framebuffer_unreference() if you are also
    using drm_framebuffer_reference() to keep a ref to the fb.

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
 drivers/gpu/drm/i915/intel_display.c      |    4 +-
 drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
 include/drm/drm_crtc.h                    |    5 ++
 5 files changed, 78 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 901de9a..96e236f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
 	int ret;
 
+	kref_init(&fb->refcount);
+
 	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
 	if (ret)
 		return ret;
@@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
+static void drm_framebuffer_free(struct kref *kref)
+{
+	struct drm_framebuffer *fb =
+			container_of(kref, struct drm_framebuffer, refcount);
+	fb->funcs->destroy(fb);
+}
+
+/**
+ * drm_framebuffer_unreference - unref a framebuffer
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ */
+void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	kref_put(&fb->refcount, drm_framebuffer_free);
+}
+EXPORT_SYMBOL(drm_framebuffer_unreference);
+
+/**
+ * drm_framebuffer_reference - incr the fb refcnt
+ */
+void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	kref_get(&fb->refcount);
+}
+EXPORT_SYMBOL(drm_framebuffer_reference);
+
 /**
  * drm_framebuffer_cleanup - remove a framebuffer object
  * @fb: framebuffer to remove
@@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
+	/*
+	 * This could be moved to drm_framebuffer_remove(), but for
+	 * debugging is nice to keep around the list of fb's that are
+	 * no longer associated w/ a drm_file but are not unreferenced
+	 * yet.  (i915 and omapdrm have debugfs files which will show
+	 * this.)
+	 */
+	drm_mode_object_put(dev, &fb->base);
+	list_del(&fb->head);
+	dev->mode_config.num_fb--;
+}
+EXPORT_SYMBOL(drm_framebuffer_cleanup);
+
+/**
+ * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * @fb: framebuffer to remove
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ *
+ * Scans all the CRTCs and planes in @dev's mode_config.  If they're
+ * using @fb, removes it, setting it to NULL.
+ */
+void drm_framebuffer_remove(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	struct drm_mode_set set;
@@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 		}
 	}
 
-	drm_mode_object_put(dev, &fb->base);
-	list_del(&fb->head);
-	dev->mode_config.num_fb--;
+	list_del(&fb->filp_head);
+
+	drm_framebuffer_unreference(fb);
 }
-EXPORT_SYMBOL(drm_framebuffer_cleanup);
+EXPORT_SYMBOL(drm_framebuffer_remove);
 
 /**
  * drm_crtc_init - Initialise a new CRTC object
@@ -1032,7 +1092,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 
 	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
@@ -2343,11 +2403,8 @@ int drm_mode_rmfb(struct drm_device *dev,
 		goto out;
 	}
 
-	/* TODO release all crtc connected to the framebuffer */
-	/* TODO unhock the destructor from the buffer object */
-
 	list_del(&fb->filp_head);
-	fb->funcs->destroy(fb);
+	drm_framebuffer_remove(fb);
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
@@ -2497,8 +2554,7 @@ void drm_fb_release(struct drm_file *priv)
 
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-		list_del(&fb->filp_head);
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d5586cc..f4ac433 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
 	/* release drm framebuffer and real buffer */
 	if (fb_helper->fb && fb_helper->fb->funcs) {
 		fb = fb_helper->fb;
-		if (fb && fb->funcs->destroy)
-			fb->funcs->destroy(fb);
+		if (fb)
+			drm_framebuffer_remove(fb);
 	}
 
 	/* release linux framebuffer */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a69a3d0..2109c9f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 		crtc->fb = old_fb;
 		return false;
 	}
@@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 
 		return;
 	}
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 8c6ed3b..8a027bb 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -276,7 +276,7 @@ fail:
 		if (fbi)
 			framebuffer_release(fbi);
 		if (fb)
-			fb->funcs->destroy(fb);
+			drm_framebuffer_remove(fb);
 	}
 
 	return ret;
@@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
 
 	/* this will free the backing object */
 	if (fbdev->fb)
-		fbdev->fb->funcs->destroy(fbdev->fb);
+		drm_framebuffer_remove(fbdev->fb);
 
 	kfree(fbdev);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a82e0a2..2442d01 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -219,6 +219,7 @@ struct drm_display_info {
 };
 
 struct drm_framebuffer_funcs {
+	/* note: use drm_framebuffer_remove() */
 	void (*destroy)(struct drm_framebuffer *framebuffer);
 	int (*create_handle)(struct drm_framebuffer *fb,
 			     struct drm_file *file_priv,
@@ -243,6 +244,7 @@ struct drm_framebuffer_funcs {
 
 struct drm_framebuffer {
 	struct drm_device *dev;
+	struct kref refcount;
 	struct list_head head;
 	struct drm_mode_object base;
 	const struct drm_framebuffer_funcs *funcs;
@@ -921,6 +923,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
+void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+void drm_framebuffer_reference(struct drm_framebuffer *fb);
+void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 
 extern void drm_connector_attach_property(struct drm_connector *connector,
-- 
1.7.9.5

Re: [PATCH] drm: refcnt drm_framebuffer

[Daniel Vetter]

On Tue, Sep 04, 2012 at 06:10:04PM -0500, Rob Clark wrote:
> From: Rob Clark <rob@ti.com>
> 
> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
> a ref to the fb when disabling a pipe until the next vblank, this
> avoids the need to make disabling an overlay synchronous.  This is a
> problem that shows up when userspace is using a drm plane to
> implement a hw cursor.. making overlay disable synchronous causes
> a performance problem when x11 is rapidly enabling/disabling the
> hw cursor.  But not making it synchronous opens up a race condition
> for crashing if userspace turns around and immediately deletes the
> fb.  Refcnt'ing the fb makes it possible to solve this problem.
> 
> v1: original
> v2: add drm_framebuffer_remove() which is called in all paths where
>     fb->funcs->destroy() was directly called before.  This cleans
>     up the CRTCs/planes that the fb was attached to.  You should
>     only directly use drm_framebuffer_unreference() if you are also
>     using drm_framebuffer_reference() to keep a ref to the fb.
> 
> Signed-off-by: Rob Clark <rob@ti.com>

Yeah, this could also be rather useful to move a few things to saner
places in drm/i915. The fb->refcount is a bit strange though since only
driver-internals can be reference counted, all the userspace interface
stuff in the drm core has to abide to the old api which mandates that we
remove all uses of a given fb at rmfb time. I guess we'll just have to
live with that.

Still, can you add a small comment in drm_crtc.h to the new refcount
explaining this? And maybe extend the note about the ->destroy callback to
say "call _remove or _unrefence instead"? Otherwise

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> ---
>  drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
>  drivers/gpu/drm/i915/intel_display.c      |    4 +-
>  drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
>  include/drm/drm_crtc.h                    |    5 ++
>  5 files changed, 78 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 901de9a..96e236f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  {
>  	int ret;
>  
> +	kref_init(&fb->refcount);
> +
>  	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
>  	if (ret)
>  		return ret;
> @@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
>  
> +static void drm_framebuffer_free(struct kref *kref)
> +{
> +	struct drm_framebuffer *fb =
> +			container_of(kref, struct drm_framebuffer, refcount);
> +	fb->funcs->destroy(fb);
> +}
> +
> +/**
> + * drm_framebuffer_unreference - unref a framebuffer
> + *
> + * LOCKING:
> + * Caller must hold mode config lock.
> + */
> +void drm_framebuffer_unreference(struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = fb->dev;
> +	DRM_DEBUG("FB ID: %d\n", fb->base.id);
> +	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +	kref_put(&fb->refcount, drm_framebuffer_free);
> +}
> +EXPORT_SYMBOL(drm_framebuffer_unreference);
> +
> +/**
> + * drm_framebuffer_reference - incr the fb refcnt
> + */
> +void drm_framebuffer_reference(struct drm_framebuffer *fb)
> +{
> +	DRM_DEBUG("FB ID: %d\n", fb->base.id);
> +	kref_get(&fb->refcount);
> +}
> +EXPORT_SYMBOL(drm_framebuffer_reference);
> +
>  /**
>   * drm_framebuffer_cleanup - remove a framebuffer object
>   * @fb: framebuffer to remove
> @@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
>  void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  {
>  	struct drm_device *dev = fb->dev;
> +	/*
> +	 * This could be moved to drm_framebuffer_remove(), but for
> +	 * debugging is nice to keep around the list of fb's that are
> +	 * no longer associated w/ a drm_file but are not unreferenced
> +	 * yet.  (i915 and omapdrm have debugfs files which will show
> +	 * this.)
> +	 */
> +	drm_mode_object_put(dev, &fb->base);
> +	list_del(&fb->head);
> +	dev->mode_config.num_fb--;
> +}
> +EXPORT_SYMBOL(drm_framebuffer_cleanup);
> +
> +/**
> + * drm_framebuffer_remove - remove and unreference a framebuffer object
> + * @fb: framebuffer to remove
> + *
> + * LOCKING:
> + * Caller must hold mode config lock.
> + *
> + * Scans all the CRTCs and planes in @dev's mode_config.  If they're
> + * using @fb, removes it, setting it to NULL.
> + */
> +void drm_framebuffer_remove(struct drm_framebuffer *fb)
> +{
> +	struct drm_device *dev = fb->dev;
>  	struct drm_crtc *crtc;
>  	struct drm_plane *plane;
>  	struct drm_mode_set set;
> @@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  		}
>  	}
>  
> -	drm_mode_object_put(dev, &fb->base);
> -	list_del(&fb->head);
> -	dev->mode_config.num_fb--;
> +	list_del(&fb->filp_head);
> +
> +	drm_framebuffer_unreference(fb);
>  }
> -EXPORT_SYMBOL(drm_framebuffer_cleanup);
> +EXPORT_SYMBOL(drm_framebuffer_remove);
>  
>  /**
>   * drm_crtc_init - Initialise a new CRTC object
> @@ -1032,7 +1092,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	}
>  
>  	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
> -		fb->funcs->destroy(fb);
> +		drm_framebuffer_remove(fb);
>  	}
>  
>  	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> @@ -2343,11 +2403,8 @@ int drm_mode_rmfb(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	/* TODO release all crtc connected to the framebuffer */
> -	/* TODO unhock the destructor from the buffer object */
> -
>  	list_del(&fb->filp_head);
> -	fb->funcs->destroy(fb);
> +	drm_framebuffer_remove(fb);
>  
>  out:
>  	mutex_unlock(&dev->mode_config.mutex);
> @@ -2497,8 +2554,7 @@ void drm_fb_release(struct drm_file *priv)
>  
>  	mutex_lock(&dev->mode_config.mutex);
>  	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> -		list_del(&fb->filp_head);
> -		fb->funcs->destroy(fb);
> +		drm_framebuffer_remove(fb);
>  	}
>  	mutex_unlock(&dev->mode_config.mutex);
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index d5586cc..f4ac433 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
>  	/* release drm framebuffer and real buffer */
>  	if (fb_helper->fb && fb_helper->fb->funcs) {
>  		fb = fb_helper->fb;
> -		if (fb && fb->funcs->destroy)
> -			fb->funcs->destroy(fb);
> +		if (fb)
> +			drm_framebuffer_remove(fb);
>  	}
>  
>  	/* release linux framebuffer */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a69a3d0..2109c9f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
>  	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
>  		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>  		if (old->release_fb)
> -			old->release_fb->funcs->destroy(old->release_fb);
> +			drm_framebuffer_remove(old->release_fb);
>  		crtc->fb = old_fb;
>  		return false;
>  	}
> @@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
>  		drm_helper_disable_unused_functions(dev);
>  
>  		if (old->release_fb)
> -			old->release_fb->funcs->destroy(old->release_fb);
> +			drm_framebuffer_remove(old->release_fb);
>  
>  		return;
>  	}
> diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
> index 8c6ed3b..8a027bb 100644
> --- a/drivers/staging/omapdrm/omap_fbdev.c
> +++ b/drivers/staging/omapdrm/omap_fbdev.c
> @@ -276,7 +276,7 @@ fail:
>  		if (fbi)
>  			framebuffer_release(fbi);
>  		if (fb)
> -			fb->funcs->destroy(fb);
> +			drm_framebuffer_remove(fb);
>  	}
>  
>  	return ret;
> @@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
>  
>  	/* this will free the backing object */
>  	if (fbdev->fb)
> -		fbdev->fb->funcs->destroy(fbdev->fb);
> +		drm_framebuffer_remove(fbdev->fb);
>  
>  	kfree(fbdev);
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a82e0a2..2442d01 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -219,6 +219,7 @@ struct drm_display_info {
>  };
>  
>  struct drm_framebuffer_funcs {
> +	/* note: use drm_framebuffer_remove() */
>  	void (*destroy)(struct drm_framebuffer *framebuffer);
>  	int (*create_handle)(struct drm_framebuffer *fb,
>  			     struct drm_file *file_priv,
> @@ -243,6 +244,7 @@ struct drm_framebuffer_funcs {
>  
>  struct drm_framebuffer {
>  	struct drm_device *dev;
> +	struct kref refcount;
>  	struct list_head head;
>  	struct drm_mode_object base;
>  	const struct drm_framebuffer_funcs *funcs;
> @@ -921,6 +923,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
>  extern int drm_framebuffer_init(struct drm_device *dev,
>  				struct drm_framebuffer *fb,
>  				const struct drm_framebuffer_funcs *funcs);
> +void drm_framebuffer_unreference(struct drm_framebuffer *fb);
> +void drm_framebuffer_reference(struct drm_framebuffer *fb);
> +void drm_framebuffer_remove(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>  
>  extern void drm_connector_attach_property(struct drm_connector *connector,
> -- 
> 1.7.9.5
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

[PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

From: Rob Clark <rob@ti.com>

This simplifies drm fb lifetime, and if the crtc/plane needs to hold
a ref to the fb when disabling a pipe until the next vblank, this
avoids the need to make disabling an overlay synchronous.  This is a
problem that shows up when userspace is using a drm plane to
implement a hw cursor.. making overlay disable synchronous causes
a performance problem when x11 is rapidly enabling/disabling the
hw cursor.  But not making it synchronous opens up a race condition
for crashing if userspace turns around and immediately deletes the
fb.  Refcnt'ing the fb makes it possible to solve this problem.

v1: original
v2: add drm_framebuffer_remove() which is called in all paths where
    fb->funcs->destroy() was directly called before.  This cleans
    up the CRTCs/planes that the fb was attached to.  You should
    only directly use drm_framebuffer_unreference() if you are also
    using drm_framebuffer_reference() to keep a ref to the fb.
v3: add comment explaining the fb refcount

Signed-off-by: Rob Clark <rob@ti.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

---
 drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
 drivers/gpu/drm/i915/intel_display.c      |    4 +-
 drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
 include/drm/drm_crtc.h                    |   14 ++++++
 5 files changed, 87 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 901de9a..96e236f 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
 	int ret;
 
+	kref_init(&fb->refcount);
+
 	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
 	if (ret)
 		return ret;
@@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
+static void drm_framebuffer_free(struct kref *kref)
+{
+	struct drm_framebuffer *fb =
+			container_of(kref, struct drm_framebuffer, refcount);
+	fb->funcs->destroy(fb);
+}
+
+/**
+ * drm_framebuffer_unreference - unref a framebuffer
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ */
+void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	kref_put(&fb->refcount, drm_framebuffer_free);
+}
+EXPORT_SYMBOL(drm_framebuffer_unreference);
+
+/**
+ * drm_framebuffer_reference - incr the fb refcnt
+ */
+void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	kref_get(&fb->refcount);
+}
+EXPORT_SYMBOL(drm_framebuffer_reference);
+
 /**
  * drm_framebuffer_cleanup - remove a framebuffer object
  * @fb: framebuffer to remove
@@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
+	/*
+	 * This could be moved to drm_framebuffer_remove(), but for
+	 * debugging is nice to keep around the list of fb's that are
+	 * no longer associated w/ a drm_file but are not unreferenced
+	 * yet.  (i915 and omapdrm have debugfs files which will show
+	 * this.)
+	 */
+	drm_mode_object_put(dev, &fb->base);
+	list_del(&fb->head);
+	dev->mode_config.num_fb--;
+}
+EXPORT_SYMBOL(drm_framebuffer_cleanup);
+
+/**
+ * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * @fb: framebuffer to remove
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ *
+ * Scans all the CRTCs and planes in @dev's mode_config.  If they're
+ * using @fb, removes it, setting it to NULL.
+ */
+void drm_framebuffer_remove(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	struct drm_mode_set set;
@@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 		}
 	}
 
-	drm_mode_object_put(dev, &fb->base);
-	list_del(&fb->head);
-	dev->mode_config.num_fb--;
+	list_del(&fb->filp_head);
+
+	drm_framebuffer_unreference(fb);
 }
-EXPORT_SYMBOL(drm_framebuffer_cleanup);
+EXPORT_SYMBOL(drm_framebuffer_remove);
 
 /**
  * drm_crtc_init - Initialise a new CRTC object
@@ -1032,7 +1092,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 
 	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
@@ -2343,11 +2403,8 @@ int drm_mode_rmfb(struct drm_device *dev,
 		goto out;
 	}
 
-	/* TODO release all crtc connected to the framebuffer */
-	/* TODO unhock the destructor from the buffer object */
-
 	list_del(&fb->filp_head);
-	fb->funcs->destroy(fb);
+	drm_framebuffer_remove(fb);
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
@@ -2497,8 +2554,7 @@ void drm_fb_release(struct drm_file *priv)
 
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-		list_del(&fb->filp_head);
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d5586cc..f4ac433 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
 	/* release drm framebuffer and real buffer */
 	if (fb_helper->fb && fb_helper->fb->funcs) {
 		fb = fb_helper->fb;
-		if (fb && fb->funcs->destroy)
-			fb->funcs->destroy(fb);
+		if (fb)
+			drm_framebuffer_remove(fb);
 	}
 
 	/* release linux framebuffer */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a69a3d0..2109c9f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 		crtc->fb = old_fb;
 		return false;
 	}
@@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 
 		return;
 	}
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 8c6ed3b..8a027bb 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -276,7 +276,7 @@ fail:
 		if (fbi)
 			framebuffer_release(fbi);
 		if (fb)
-			fb->funcs->destroy(fb);
+			drm_framebuffer_remove(fb);
 	}
 
 	return ret;
@@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
 
 	/* this will free the backing object */
 	if (fbdev->fb)
-		fbdev->fb->funcs->destroy(fbdev->fb);
+		drm_framebuffer_remove(fbdev->fb);
 
 	kfree(fbdev);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a82e0a2..1422b36 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -219,6 +219,7 @@ struct drm_display_info {
 };
 
 struct drm_framebuffer_funcs {
+	/* note: use drm_framebuffer_remove() */
 	void (*destroy)(struct drm_framebuffer *framebuffer);
 	int (*create_handle)(struct drm_framebuffer *fb,
 			     struct drm_file *file_priv,
@@ -243,6 +244,16 @@ struct drm_framebuffer_funcs {
 
 struct drm_framebuffer {
 	struct drm_device *dev;
+	/*
+	 * Note that the fb is refcounted for the benefit of driver internals,
+	 * for example some hw, disabling a CRTC/plane is asynchronous, and
+	 * scanout does not actually complete until the next vblank.  So some
+	 * cleanup (like releasing the reference(s) on the backing GEM bo(s))
+	 * should be deferred.  In cases like this, the driver would like to
+	 * hold a ref to the fb even though it has already been removed from
+	 * userspace perspective.
+	 */
+	struct kref refcount;
 	struct list_head head;
 	struct drm_mode_object base;
 	const struct drm_framebuffer_funcs *funcs;
@@ -921,6 +932,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
+void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+void drm_framebuffer_reference(struct drm_framebuffer *fb);
+void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 
 extern void drm_connector_attach_property(struct drm_connector *connector,
-- 
1.7.9.5

Re: [PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

On Wed, Sep 5, 2012 at 1:17 PM, Rob Clark <rob.clark@linaro.org> wrote:
> From: Rob Clark <rob@ti.com>
>
> This simplifies drm fb lifetime, and if the crtc/plane needs to hold
> a ref to the fb when disabling a pipe until the next vblank, this
> avoids the need to make disabling an overlay synchronous.  This is a
> problem that shows up when userspace is using a drm plane to
> implement a hw cursor.. making overlay disable synchronous causes
> a performance problem when x11 is rapidly enabling/disabling the
> hw cursor.  But not making it synchronous opens up a race condition
> for crashing if userspace turns around and immediately deletes the
> fb.  Refcnt'ing the fb makes it possible to solve this problem.
>
> v1: original
> v2: add drm_framebuffer_remove() which is called in all paths where
>     fb->funcs->destroy() was directly called before.  This cleans
>     up the CRTCs/planes that the fb was attached to.  You should
>     only directly use drm_framebuffer_unreference() if you are also
>     using drm_framebuffer_reference() to keep a ref to the fb.
> v3: add comment explaining the fb refcount
>
> Signed-off-by: Rob Clark <rob@ti.com>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> ---
>  drivers/gpu/drm/drm_crtc.c                |   78 +++++++++++++++++++++++++----
>  drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
>  drivers/gpu/drm/i915/intel_display.c      |    4 +-
>  drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
>  include/drm/drm_crtc.h                    |   14 ++++++
>  5 files changed, 87 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 901de9a..96e236f 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  {
>         int ret;
>
> +       kref_init(&fb->refcount);
> +
>         ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
>         if (ret)
>                 return ret;
> @@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
>  }
>  EXPORT_SYMBOL(drm_framebuffer_init);
>
> +static void drm_framebuffer_free(struct kref *kref)
> +{
> +       struct drm_framebuffer *fb =
> +                       container_of(kref, struct drm_framebuffer, refcount);
> +       fb->funcs->destroy(fb);
> +}
> +
> +/**
> + * drm_framebuffer_unreference - unref a framebuffer
> + *
> + * LOCKING:
> + * Caller must hold mode config lock.
> + */
> +void drm_framebuffer_unreference(struct drm_framebuffer *fb)
> +{
> +       struct drm_device *dev = fb->dev;
> +       DRM_DEBUG("FB ID: %d\n", fb->base.id);
> +       WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
> +       kref_put(&fb->refcount, drm_framebuffer_free);
> +}
> +EXPORT_SYMBOL(drm_framebuffer_unreference);
> +
> +/**
> + * drm_framebuffer_reference - incr the fb refcnt
> + */
> +void drm_framebuffer_reference(struct drm_framebuffer *fb)
> +{
> +       DRM_DEBUG("FB ID: %d\n", fb->base.id);
> +       kref_get(&fb->refcount);
> +}
> +EXPORT_SYMBOL(drm_framebuffer_reference);
> +
>  /**
>   * drm_framebuffer_cleanup - remove a framebuffer object
>   * @fb: framebuffer to remove
> @@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
>  void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>  {
>         struct drm_device *dev = fb->dev;
> +       /*
> +        * This could be moved to drm_framebuffer_remove(), but for
> +        * debugging is nice to keep around the list of fb's that are
> +        * no longer associated w/ a drm_file but are not unreferenced
> +        * yet.  (i915 and omapdrm have debugfs files which will show
> +        * this.)
> +        */
> +       drm_mode_object_put(dev, &fb->base);
> +       list_del(&fb->head);
> +       dev->mode_config.num_fb--;
> +}
> +EXPORT_SYMBOL(drm_framebuffer_cleanup);
> +
> +/**
> + * drm_framebuffer_remove - remove and unreference a framebuffer object
> + * @fb: framebuffer to remove
> + *
> + * LOCKING:
> + * Caller must hold mode config lock.
> + *
> + * Scans all the CRTCs and planes in @dev's mode_config.  If they're
> + * using @fb, removes it, setting it to NULL.
> + */
> +void drm_framebuffer_remove(struct drm_framebuffer *fb)
> +{
> +       struct drm_device *dev = fb->dev;
>         struct drm_crtc *crtc;
>         struct drm_plane *plane;
>         struct drm_mode_set set;
> @@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
>                 }
>         }
>
> -       drm_mode_object_put(dev, &fb->base);
> -       list_del(&fb->head);
> -       dev->mode_config.num_fb--;
> +       list_del(&fb->filp_head);
> +
> +       drm_framebuffer_unreference(fb);
>  }
> -EXPORT_SYMBOL(drm_framebuffer_cleanup);
> +EXPORT_SYMBOL(drm_framebuffer_remove);
>
>  /**
>   * drm_crtc_init - Initialise a new CRTC object
> @@ -1032,7 +1092,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>         }
>
>         list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
> -               fb->funcs->destroy(fb);
> +               drm_framebuffer_remove(fb);
>         }
>
>         list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
> @@ -2343,11 +2403,8 @@ int drm_mode_rmfb(struct drm_device *dev,
>                 goto out;
>         }
>
> -       /* TODO release all crtc connected to the framebuffer */
> -       /* TODO unhock the destructor from the buffer object */
> -
>         list_del(&fb->filp_head);

errrg,  that list_del() was supposed to be removed as well..  so let's
try this one more time

BR,
-R

> -       fb->funcs->destroy(fb);
> +       drm_framebuffer_remove(fb);
>
>  out:
>         mutex_unlock(&dev->mode_config.mutex);
> @@ -2497,8 +2554,7 @@ void drm_fb_release(struct drm_file *priv)
>
>         mutex_lock(&dev->mode_config.mutex);
>         list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
> -               list_del(&fb->filp_head);
> -               fb->funcs->destroy(fb);
> +               drm_framebuffer_remove(fb);
>         }
>         mutex_unlock(&dev->mode_config.mutex);
>  }
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> index d5586cc..f4ac433 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
> @@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
>         /* release drm framebuffer and real buffer */
>         if (fb_helper->fb && fb_helper->fb->funcs) {
>                 fb = fb_helper->fb;
> -               if (fb && fb->funcs->destroy)
> -                       fb->funcs->destroy(fb);
> +               if (fb)
> +                       drm_framebuffer_remove(fb);
>         }
>
>         /* release linux framebuffer */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a69a3d0..2109c9f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
>         if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
>                 DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
>                 if (old->release_fb)
> -                       old->release_fb->funcs->destroy(old->release_fb);
> +                       drm_framebuffer_remove(old->release_fb);
>                 crtc->fb = old_fb;
>                 return false;
>         }
> @@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
>                 drm_helper_disable_unused_functions(dev);
>
>                 if (old->release_fb)
> -                       old->release_fb->funcs->destroy(old->release_fb);
> +                       drm_framebuffer_remove(old->release_fb);
>
>                 return;
>         }
> diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
> index 8c6ed3b..8a027bb 100644
> --- a/drivers/staging/omapdrm/omap_fbdev.c
> +++ b/drivers/staging/omapdrm/omap_fbdev.c
> @@ -276,7 +276,7 @@ fail:
>                 if (fbi)
>                         framebuffer_release(fbi);
>                 if (fb)
> -                       fb->funcs->destroy(fb);
> +                       drm_framebuffer_remove(fb);
>         }
>
>         return ret;
> @@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
>
>         /* this will free the backing object */
>         if (fbdev->fb)
> -               fbdev->fb->funcs->destroy(fbdev->fb);
> +               drm_framebuffer_remove(fbdev->fb);
>
>         kfree(fbdev);
>
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index a82e0a2..1422b36 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -219,6 +219,7 @@ struct drm_display_info {
>  };
>
>  struct drm_framebuffer_funcs {
> +       /* note: use drm_framebuffer_remove() */
>         void (*destroy)(struct drm_framebuffer *framebuffer);
>         int (*create_handle)(struct drm_framebuffer *fb,
>                              struct drm_file *file_priv,
> @@ -243,6 +244,16 @@ struct drm_framebuffer_funcs {
>
>  struct drm_framebuffer {
>         struct drm_device *dev;
> +       /*
> +        * Note that the fb is refcounted for the benefit of driver internals,
> +        * for example some hw, disabling a CRTC/plane is asynchronous, and
> +        * scanout does not actually complete until the next vblank.  So some
> +        * cleanup (like releasing the reference(s) on the backing GEM bo(s))
> +        * should be deferred.  In cases like this, the driver would like to
> +        * hold a ref to the fb even though it has already been removed from
> +        * userspace perspective.
> +        */
> +       struct kref refcount;
>         struct list_head head;
>         struct drm_mode_object base;
>         const struct drm_framebuffer_funcs *funcs;
> @@ -921,6 +932,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
>  extern int drm_framebuffer_init(struct drm_device *dev,
>                                 struct drm_framebuffer *fb,
>                                 const struct drm_framebuffer_funcs *funcs);
> +void drm_framebuffer_unreference(struct drm_framebuffer *fb);
> +void drm_framebuffer_reference(struct drm_framebuffer *fb);
> +void drm_framebuffer_remove(struct drm_framebuffer *fb);
>  extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
>
>  extern void drm_connector_attach_property(struct drm_connector *connector,
> --
> 1.7.9.5
>

[PATCH] drm: refcnt drm_framebuffer

[Rob Clark]

From: Rob Clark <rob@ti.com>

This simplifies drm fb lifetime, and if the crtc/plane needs to hold
a ref to the fb when disabling a pipe until the next vblank, this
avoids the need to make disabling an overlay synchronous.  This is a
problem that shows up when userspace is using a drm plane to
implement a hw cursor.. making overlay disable synchronous causes
a performance problem when x11 is rapidly enabling/disabling the
hw cursor.  But not making it synchronous opens up a race condition
for crashing if userspace turns around and immediately deletes the
fb.  Refcnt'ing the fb makes it possible to solve this problem.

v1: original
v2: add drm_framebuffer_remove() which is called in all paths where
    fb->funcs->destroy() was directly called before.  This cleans
    up the CRTCs/planes that the fb was attached to.  You should
    only directly use drm_framebuffer_unreference() if you are also
    using drm_framebuffer_reference() to keep a ref to the fb.
v3: add comment explaining the fb refcount
v4: remove duplicate 'list_del(&fb->filp_head)'

Signed-off-by: Rob Clark <rob@ti.com>
---
 drivers/gpu/drm/drm_crtc.c                |   79 ++++++++++++++++++++++++-----
 drivers/gpu/drm/exynos/exynos_drm_fbdev.c |    4 +-
 drivers/gpu/drm/i915/intel_display.c      |    4 +-
 drivers/staging/omapdrm/omap_fbdev.c      |    4 +-
 include/drm/drm_crtc.h                    |   14 +++++
 5 files changed, 87 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 901de9a..7552675 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -294,6 +294,8 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 {
 	int ret;
 
+	kref_init(&fb->refcount);
+
 	ret = drm_mode_object_get(dev, &fb->base, DRM_MODE_OBJECT_FB);
 	if (ret)
 		return ret;
@@ -307,6 +309,38 @@ int drm_framebuffer_init(struct drm_device *dev, struct drm_framebuffer *fb,
 }
 EXPORT_SYMBOL(drm_framebuffer_init);
 
+static void drm_framebuffer_free(struct kref *kref)
+{
+	struct drm_framebuffer *fb =
+			container_of(kref, struct drm_framebuffer, refcount);
+	fb->funcs->destroy(fb);
+}
+
+/**
+ * drm_framebuffer_unreference - unref a framebuffer
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ */
+void drm_framebuffer_unreference(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
+	kref_put(&fb->refcount, drm_framebuffer_free);
+}
+EXPORT_SYMBOL(drm_framebuffer_unreference);
+
+/**
+ * drm_framebuffer_reference - incr the fb refcnt
+ */
+void drm_framebuffer_reference(struct drm_framebuffer *fb)
+{
+	DRM_DEBUG("FB ID: %d\n", fb->base.id);
+	kref_get(&fb->refcount);
+}
+EXPORT_SYMBOL(drm_framebuffer_reference);
+
 /**
  * drm_framebuffer_cleanup - remove a framebuffer object
  * @fb: framebuffer to remove
@@ -320,6 +354,32 @@ EXPORT_SYMBOL(drm_framebuffer_init);
 void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 {
 	struct drm_device *dev = fb->dev;
+	/*
+	 * This could be moved to drm_framebuffer_remove(), but for
+	 * debugging is nice to keep around the list of fb's that are
+	 * no longer associated w/ a drm_file but are not unreferenced
+	 * yet.  (i915 and omapdrm have debugfs files which will show
+	 * this.)
+	 */
+	drm_mode_object_put(dev, &fb->base);
+	list_del(&fb->head);
+	dev->mode_config.num_fb--;
+}
+EXPORT_SYMBOL(drm_framebuffer_cleanup);
+
+/**
+ * drm_framebuffer_remove - remove and unreference a framebuffer object
+ * @fb: framebuffer to remove
+ *
+ * LOCKING:
+ * Caller must hold mode config lock.
+ *
+ * Scans all the CRTCs and planes in @dev's mode_config.  If they're
+ * using @fb, removes it, setting it to NULL.
+ */
+void drm_framebuffer_remove(struct drm_framebuffer *fb)
+{
+	struct drm_device *dev = fb->dev;
 	struct drm_crtc *crtc;
 	struct drm_plane *plane;
 	struct drm_mode_set set;
@@ -350,11 +410,11 @@ void drm_framebuffer_cleanup(struct drm_framebuffer *fb)
 		}
 	}
 
-	drm_mode_object_put(dev, &fb->base);
-	list_del(&fb->head);
-	dev->mode_config.num_fb--;
+	list_del(&fb->filp_head);
+
+	drm_framebuffer_unreference(fb);
 }
-EXPORT_SYMBOL(drm_framebuffer_cleanup);
+EXPORT_SYMBOL(drm_framebuffer_remove);
 
 /**
  * drm_crtc_init - Initialise a new CRTC object
@@ -1032,7 +1092,7 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	}
 
 	list_for_each_entry_safe(fb, fbt, &dev->mode_config.fb_list, head) {
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 
 	list_for_each_entry_safe(crtc, ct, &dev->mode_config.crtc_list, head) {
@@ -2343,11 +2403,7 @@ int drm_mode_rmfb(struct drm_device *dev,
 		goto out;
 	}
 
-	/* TODO release all crtc connected to the framebuffer */
-	/* TODO unhock the destructor from the buffer object */
-
-	list_del(&fb->filp_head);
-	fb->funcs->destroy(fb);
+	drm_framebuffer_remove(fb);
 
 out:
 	mutex_unlock(&dev->mode_config.mutex);
@@ -2497,8 +2553,7 @@ void drm_fb_release(struct drm_file *priv)
 
 	mutex_lock(&dev->mode_config.mutex);
 	list_for_each_entry_safe(fb, tfb, &priv->fbs, filp_head) {
-		list_del(&fb->filp_head);
-		fb->funcs->destroy(fb);
+		drm_framebuffer_remove(fb);
 	}
 	mutex_unlock(&dev->mode_config.mutex);
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
index d5586cc..f4ac433 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fbdev.c
@@ -266,8 +266,8 @@ static void exynos_drm_fbdev_destroy(struct drm_device *dev,
 	/* release drm framebuffer and real buffer */
 	if (fb_helper->fb && fb_helper->fb->funcs) {
 		fb = fb_helper->fb;
-		if (fb && fb->funcs->destroy)
-			fb->funcs->destroy(fb);
+		if (fb)
+			drm_framebuffer_remove(fb);
 	}
 
 	/* release linux framebuffer */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a69a3d0..2109c9f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5687,7 +5687,7 @@ bool intel_get_load_detect_pipe(struct intel_encoder *intel_encoder,
 	if (!drm_crtc_helper_set_mode(crtc, mode, 0, 0, old_fb)) {
 		DRM_DEBUG_KMS("failed to set mode on load-detect pipe\n");
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 		crtc->fb = old_fb;
 		return false;
 	}
@@ -5717,7 +5717,7 @@ void intel_release_load_detect_pipe(struct intel_encoder *intel_encoder,
 		drm_helper_disable_unused_functions(dev);
 
 		if (old->release_fb)
-			old->release_fb->funcs->destroy(old->release_fb);
+			drm_framebuffer_remove(old->release_fb);
 
 		return;
 	}
diff --git a/drivers/staging/omapdrm/omap_fbdev.c b/drivers/staging/omapdrm/omap_fbdev.c
index 8c6ed3b..8a027bb 100644
--- a/drivers/staging/omapdrm/omap_fbdev.c
+++ b/drivers/staging/omapdrm/omap_fbdev.c
@@ -276,7 +276,7 @@ fail:
 		if (fbi)
 			framebuffer_release(fbi);
 		if (fb)
-			fb->funcs->destroy(fb);
+			drm_framebuffer_remove(fb);
 	}
 
 	return ret;
@@ -401,7 +401,7 @@ void omap_fbdev_free(struct drm_device *dev)
 
 	/* this will free the backing object */
 	if (fbdev->fb)
-		fbdev->fb->funcs->destroy(fbdev->fb);
+		drm_framebuffer_remove(fbdev->fb);
 
 	kfree(fbdev);
 
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index a82e0a2..1422b36 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -219,6 +219,7 @@ struct drm_display_info {
 };
 
 struct drm_framebuffer_funcs {
+	/* note: use drm_framebuffer_remove() */
 	void (*destroy)(struct drm_framebuffer *framebuffer);
 	int (*create_handle)(struct drm_framebuffer *fb,
 			     struct drm_file *file_priv,
@@ -243,6 +244,16 @@ struct drm_framebuffer_funcs {
 
 struct drm_framebuffer {
 	struct drm_device *dev;
+	/*
+	 * Note that the fb is refcounted for the benefit of driver internals,
+	 * for example some hw, disabling a CRTC/plane is asynchronous, and
+	 * scanout does not actually complete until the next vblank.  So some
+	 * cleanup (like releasing the reference(s) on the backing GEM bo(s))
+	 * should be deferred.  In cases like this, the driver would like to
+	 * hold a ref to the fb even though it has already been removed from
+	 * userspace perspective.
+	 */
+	struct kref refcount;
 	struct list_head head;
 	struct drm_mode_object base;
 	const struct drm_framebuffer_funcs *funcs;
@@ -921,6 +932,9 @@ extern int drm_object_property_get_value(struct drm_mode_object *obj,
 extern int drm_framebuffer_init(struct drm_device *dev,
 				struct drm_framebuffer *fb,
 				const struct drm_framebuffer_funcs *funcs);
+void drm_framebuffer_unreference(struct drm_framebuffer *fb);
+void drm_framebuffer_reference(struct drm_framebuffer *fb);
+void drm_framebuffer_remove(struct drm_framebuffer *fb);
 extern void drm_framebuffer_cleanup(struct drm_framebuffer *fb);
 
 extern void drm_connector_attach_property(struct drm_connector *connector,
-- 
1.7.9.5