[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

The introduction of primary planes has apparently caused a bit of fb
refcounting fun for people. That makes it a good time to clean up the
arcane rules and slight differences between ->update_plane and
->set_config. The new rules are:

- The core holds a reference for both the new and the old fb (if
  they're non-NULL of course) while calling into the driver through
  either ->update_plane or ->set_config.

- Drivers may not clobber plane->fb if their callback fails. If they
  do that, they need to store a pointer to the old fb in it again.
  When calling into the driver plane->fb still points at the current
  (old) framebuffer.

- The core will update the plane->fb pointer on success. Drivers can
  do that themselves too, but aren't required to any more for the
  primary plane.

- The core will update fb refcounts for the plane->fb pointer,
  presuming the drivers hold up their end of the bargain.

v2: Remove now unused tmpfb (Thierry)

v3: Drop broken changes from drm_mode_setplane (Ville). Also polish
the commit message a bit.

v4: Also fix up the handling of ->disable_plane in
drm_plane_force_disable. The issue was that we didn't save plane->fb
over the ->disable_plane call. Just paranoia, nothing relies on this.

v5: Keep still useful comments about directly calling ->set_config,
which I should have done for v4 already. Requested by Matt.

Cc: Thierry Reding 
Cc: Ville Syrj?l? 
Cc: Matt Roper 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c | 13 +++--
 drivers/gpu/drm/drm_plane_helper.c | 10 +-
 2 files changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8b7099abece..f6633cb927bc 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -1145,16 +1145,17 @@ EXPORT_SYMBOL(drm_plane_cleanup);
  */
 void drm_plane_force_disable(struct drm_plane *plane)
 {
+   struct drm_framebuffer *old_fb = plane->fb;
int ret;

-   if (!plane->fb)
+   if (!old_fb)
return;

ret = plane->funcs->disable_plane(plane);
if (ret)
DRM_ERROR("failed to disable plane with busy fb\n");
/* disconnect the plane from the fb and crtc: */
-   __drm_framebuffer_unreference(plane->fb);
+   __drm_framebuffer_unreference(old_fb);
plane->fb = NULL;
plane->crtc = NULL;
 }
@@ -2187,16 +2188,18 @@ int drm_mode_setplane(struct drm_device *dev, void 
*data,
}

drm_modeset_lock_all(dev);
+   old_fb = plane->fb;
ret = plane->funcs->update_plane(plane, crtc, fb,
 plane_req->crtc_x, plane_req->crtc_y,
 plane_req->crtc_w, plane_req->crtc_h,
 plane_req->src_x, plane_req->src_y,
 plane_req->src_w, plane_req->src_h);
if (!ret) {
-   old_fb = plane->fb;
plane->crtc = crtc;
plane->fb = fb;
fb = NULL;
+   } else {
+   old_fb = NULL;
}
drm_modeset_unlock_all(dev);

@@ -2239,9 +2242,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
ret = crtc->funcs->set_config(set);
if (ret == 0) {
crtc->primary->crtc = crtc;
-
-   /* crtc->fb must be updated by ->set_config, enforces this. */
-   WARN_ON(fb != crtc->primary->fb);
+   crtc->primary->fb = fb;
}

list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
diff --git a/drivers/gpu/drm/drm_plane_helper.c 
b/drivers/gpu/drm/drm_plane_helper.c
index 9540ff9f97fe..d966afa7ecae 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -124,7 +124,6 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,
.y2 = crtc->mode.vdisplay,
};
struct drm_connector **connector_list;
-   struct drm_framebuffer *tmpfb;
int num_connectors, ret;

if (!crtc->enabled) {
@@ -178,21 +177,14 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,
set.num_connectors = num_connectors;

/*
-* set_config() adjusts crtc->primary->fb; however the DRM setplane
-* code that called us expects to handle the framebuffer update and
-* reference counting; save and restore the current fb before
-* calling it.
-*
-* N.B., we call set_config() directly here rather than using
+* We call set_config() directly here rather than using
 * drm_mode_set_config_internal.  We're reprogramming the same
 * connectors that were already in use, so we shouldn't need the extra
 * cross-CRTC fb refcounting to accomodate stealing connectors.
 * drm_mode_setplane() already handles the basic refcounting for the
 * 

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Matt Roper]

On Wed, Apr 23, 2014 at 05:37:30PM +0200, Daniel Vetter wrote:
> The introduction of primary planes has apparently caused a bit of fb
> refcounting fun for people. That makes it a good time to clean up the
> arcane rules and slight differences between ->update_plane and
> ->set_config. The new rules are:
> 
> - The core holds a reference for both the new and the old fb (if
>   they're non-NULL of course) while calling into the driver through
>   either ->update_plane or ->set_config.
> 
> - Drivers may not clobber plane->fb if their callback fails. If they
>   do that, they need to store a pointer to the old fb in it again.
>   When calling into the driver plane->fb still points at the current
>   (old) framebuffer.
> 
> - The core will update the plane->fb pointer on success. Drivers can
>   do that themselves too, but aren't required to any more for the
>   primary plane.
> 
> - The core will update fb refcounts for the plane->fb pointer,
>   presuming the drivers hold up their end of the bargain.
> 
> v2: Remove now unused tmpfb (Thierry)
> 
> v3: Drop broken changes from drm_mode_setplane (Ville). Also polish
> the commit message a bit.
> 
> v4: Also fix up the handling of ->disable_plane in
> drm_plane_force_disable. The issue was that we didn't save plane->fb
> over the ->disable_plane call. Just paranoia, nothing relies on this.
> 
> v5: Keep still useful comments about directly calling ->set_config,
> which I should have done for v4 already. Requested by Matt.
> 
> Cc: Thierry Reding 
> Cc: Ville Syrj?l? 
> Cc: Matt Roper 
> Signed-off-by: Daniel Vetter 

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/drm_crtc.c | 13 +++--
>  drivers/gpu/drm/drm_plane_helper.c | 10 +-
>  2 files changed, 8 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d8b7099abece..f6633cb927bc 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1145,16 +1145,17 @@ EXPORT_SYMBOL(drm_plane_cleanup);
>   */
>  void drm_plane_force_disable(struct drm_plane *plane)
>  {
> + struct drm_framebuffer *old_fb = plane->fb;
>   int ret;
>  
> - if (!plane->fb)
> + if (!old_fb)
>   return;
>  
>   ret = plane->funcs->disable_plane(plane);
>   if (ret)
>   DRM_ERROR("failed to disable plane with busy fb\n");
>   /* disconnect the plane from the fb and crtc: */
> - __drm_framebuffer_unreference(plane->fb);
> + __drm_framebuffer_unreference(old_fb);
>   plane->fb = NULL;
>   plane->crtc = NULL;
>  }
> @@ -2187,16 +2188,18 @@ int drm_mode_setplane(struct drm_device *dev, void 
> *data,
>   }
>  
>   drm_modeset_lock_all(dev);
> + old_fb = plane->fb;
>   ret = plane->funcs->update_plane(plane, crtc, fb,
>plane_req->crtc_x, plane_req->crtc_y,
>plane_req->crtc_w, plane_req->crtc_h,
>plane_req->src_x, plane_req->src_y,
>plane_req->src_w, plane_req->src_h);
>   if (!ret) {
> - old_fb = plane->fb;
>   plane->crtc = crtc;
>   plane->fb = fb;
>   fb = NULL;
> + } else {
> + old_fb = NULL;
>   }
>   drm_modeset_unlock_all(dev);
>  
> @@ -2239,9 +2242,7 @@ int drm_mode_set_config_internal(struct drm_mode_set 
> *set)
>   ret = crtc->funcs->set_config(set);
>   if (ret == 0) {
>   crtc->primary->crtc = crtc;
> -
> - /* crtc->fb must be updated by ->set_config, enforces this. */
> - WARN_ON(fb != crtc->primary->fb);
> + crtc->primary->fb = fb;
>   }
>  
>   list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
> diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> b/drivers/gpu/drm/drm_plane_helper.c
> index 9540ff9f97fe..d966afa7ecae 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -124,7 +124,6 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   .y2 = crtc->mode.vdisplay,
>   };
>   struct drm_connector **connector_list;
> - struct drm_framebuffer *tmpfb;
>   int num_connectors, ret;
>  
>   if (!crtc->enabled) {
> @@ -178,21 +177,14 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   set.num_connectors = num_connectors;
>  
>   /*
> -  * set_config() adjusts crtc->primary->fb; however the DRM setplane
> -  * code that called us expects to handle the framebuffer update and
> -  * reference counting; save and restore the current fb before
> -  * calling it.
> -  *
> -  * N.B., we call set_config() directly here rather than using
> +  * We call set_config() directly here rather than using
>* drm_mode_set_config_internal.  We're reprogramming the same
>

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

On Wed, Apr 23, 2014 at 8:36 AM, Daniel Vetter  
wrote:
> Hm, that's indeed a bit fishy. Otoh I don't understand how this blows
> up with existing userspace since we should only hit this if userspace
> does an explicit disable call on the primary plane through the ioctl.
> It's a bug for sure though.

Actually even before

commit b6ccd7b9873dc46becd11838c885d5c783784156
Author: Daniel Vetter 
Date:   Tue Apr 15 10:02:43 2014 +0200

drm/plane-helper: Don't fake-implement primary plane disabling

this was broken since if any other plane was enabled it would return
-EBUSY. So this might actually explain the refcounting woes since the
above patch isn't yet merged into any of the upstream drm trees.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

On Wed, Apr 23, 2014 at 3:08 AM, Matt Roper  
wrote:
> On Tue, Apr 22, 2014 at 04:19:45PM +0200, Daniel Vetter wrote:
>> The introduction of primary planes has apparently caused a bit of fb
>> refcounting fun for people. That makes it a good time to clean up the
>> arcane rules and slight differences between ->update_plane and
>> ->set_config. The new rules are:
>>
>> - The core holds a reference for both the new and the old fb (if
>>   they're non-NULL of course) while calling into the driver through
>>   either ->update_plane or ->set_config.
>>
>> - Drivers may not clobber plane->fb if their callback fails. If they
>>   do that, they need to store a pointer to the old fb in it again.
>>   When calling into the driver plane->fb still points at the current
>>   (old) framebuffer.
>>
>> - The core will update the plane->fb pointer on success. Drivers can
>>   do that themselves too, but aren't required to any more for the
>>   primary plane.
>>
>> - The core will update fb refcounts for the plane->fb pointer,
>>   presuming the drivers hold up their end of the bargain.
>>
>> v2: Remove now unused tmpfb (Thierry)
>>
>> v3: Drop broken changes from drm_mode_setplane (Ville). Also polish
>> the commit message a bit.
>
> It looks like we might have some problems around setplane with fbid=0.
> It looks like we're assuming that disabling a plane always succeeds
> (which is no longer true for helper-based primary planes --- they just
> return -EINVAL on disable now), so we wind up setting old_fb to the
> currently scanned out framebuffer and then unref it at the end of the
> function if I'm reading things correctly.  We also clobber the
> plane->crtc and plane->fb pointers too when this happens.
>
> I think the real problem here was introduced on b6ccd7b9 and I gave you
> an r-b tag on that email, so that's my bad for not catching it before.
> :-(

Hm, that's indeed a bit fishy. Otoh I don't understand how this blows
up with existing userspace since we should only hit this if userspace
does an explicit disable call on the primary plane through the ioctl.
It's a bug for sure though.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Matt Roper]

On Tue, Apr 22, 2014 at 04:19:45PM +0200, Daniel Vetter wrote:
> The introduction of primary planes has apparently caused a bit of fb
> refcounting fun for people. That makes it a good time to clean up the
> arcane rules and slight differences between ->update_plane and
> ->set_config. The new rules are:
> 
> - The core holds a reference for both the new and the old fb (if
>   they're non-NULL of course) while calling into the driver through
>   either ->update_plane or ->set_config.
> 
> - Drivers may not clobber plane->fb if their callback fails. If they
>   do that, they need to store a pointer to the old fb in it again.
>   When calling into the driver plane->fb still points at the current
>   (old) framebuffer.
> 
> - The core will update the plane->fb pointer on success. Drivers can
>   do that themselves too, but aren't required to any more for the
>   primary plane.
> 
> - The core will update fb refcounts for the plane->fb pointer,
>   presuming the drivers hold up their end of the bargain.
> 
> v2: Remove now unused tmpfb (Thierry)
> 
> v3: Drop broken changes from drm_mode_setplane (Ville). Also polish
> the commit message a bit.

It looks like we might have some problems around setplane with fbid=0.
It looks like we're assuming that disabling a plane always succeeds
(which is no longer true for helper-based primary planes --- they just
return -EINVAL on disable now), so we wind up setting old_fb to the
currently scanned out framebuffer and then unref it at the end of the
function if I'm reading things correctly.  We also clobber the
plane->crtc and plane->fb pointers too when this happens.

I think the real problem here was introduced on b6ccd7b9 and I gave you
an r-b tag on that email, so that's my bad for not catching it before.
:-(


Matt

> 
> Cc: Thierry Reding 
> Cc: Ville Syrj?l? 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_crtc.c |  8 
>  drivers/gpu/drm/drm_plane_helper.c | 16 
>  2 files changed, 4 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d8b7099abece..a76000ee2a43 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2187,16 +2187,18 @@ int drm_mode_setplane(struct drm_device *dev, void 
> *data,
>   }
>  
>   drm_modeset_lock_all(dev);
> + old_fb = plane->fb;
>   ret = plane->funcs->update_plane(plane, crtc, fb,
>plane_req->crtc_x, plane_req->crtc_y,
>plane_req->crtc_w, plane_req->crtc_h,
>plane_req->src_x, plane_req->src_y,
>plane_req->src_w, plane_req->src_h);
>   if (!ret) {
> - old_fb = plane->fb;
>   plane->crtc = crtc;
>   plane->fb = fb;
>   fb = NULL;
> + } else {
> + old_fb = NULL;
>   }
>   drm_modeset_unlock_all(dev);
>  
> @@ -2239,9 +2241,7 @@ int drm_mode_set_config_internal(struct drm_mode_set 
> *set)
>   ret = crtc->funcs->set_config(set);
>   if (ret == 0) {
>   crtc->primary->crtc = crtc;
> -
> - /* crtc->fb must be updated by ->set_config, enforces this. */
> - WARN_ON(fb != crtc->primary->fb);
> + crtc->primary->fb = fb;
>   }
>  
>   list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
> diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> b/drivers/gpu/drm/drm_plane_helper.c
> index 9540ff9f97fe..b72736d5541d 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -124,7 +124,6 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   .y2 = crtc->mode.vdisplay,
>   };
>   struct drm_connector **connector_list;
> - struct drm_framebuffer *tmpfb;
>   int num_connectors, ret;
>  
>   if (!crtc->enabled) {
> @@ -177,22 +176,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   set.connectors = connector_list;
>   set.num_connectors = num_connectors;
>  
> - /*
> -  * set_config() adjusts crtc->primary->fb; however the DRM setplane
> -  * code that called us expects to handle the framebuffer update and
> -  * reference counting; save and restore the current fb before
> -  * calling it.
> -  *
> -  * N.B., we call set_config() directly here rather than using
> -  * drm_mode_set_config_internal.  We're reprogramming the same
> -  * connectors that were already in use, so we shouldn't need the extra
> -  * cross-CRTC fb refcounting to accomodate stealing connectors.
> -  * drm_mode_setplane() already handles the basic refcounting for the
> -  * framebuffers involved in this operation.
> -  */

I think there's still some value in the part of this comment that
explains why we're choosing not to call 

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Ville Syrjälä]

On Tue, Apr 22, 2014 at 04:09:09PM +0200, Daniel Vetter wrote:
> On Tue, Apr 22, 2014 at 01:06:22PM +0300, Ville Syrj?l? wrote:
> > On Tue, Apr 22, 2014 at 11:07:13AM +0200, Daniel Vetter wrote:
> > > The introduction of primary planes has apparently caused a bit of fb
> > > refcounting fun for people. That makes it a good time to clean up the
> > > arcane rules and slight differences between ->update_plane and
> > > ->set_config. The new rules are:
> > > 
> > > - The core holds a reference for both the new and the old fb (if their
> > >   non-NULL of course) while calling into the driver through either
> > >   ->update_plane or ->set_config.
> > > 
> > > - Drivers may not clobber plane->fb if their callback fails. If they
> > >   do that, they need to store a pointer to the old fb in it again.
> > >   When calling into the driver plane->fb still points at the current
> > >   (old) framebuffer.
> > > 
> > > - The core will update the plane->fb pointer on success. Drivers can
> > >   do that themselves too.
> > > 
> > > - The core will update fb refcounts for the plane->fb pointer,
> > >   presuming the drivers hold up their end of the bargain.
> > > 
> > > Signed-off-by: Daniel Vetter 
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 12 +---
> > >  drivers/gpu/drm/drm_plane_helper.c | 15 ---
> > >  2 files changed, 5 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index d8b7099abece..966b480ed543 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -2187,24 +2187,24 @@ int drm_mode_setplane(struct drm_device *dev, 
> > > void *data,
> > >   }
> > >  
> > >   drm_modeset_lock_all(dev);
> > > + old_fb = plane->fb;
> > >   ret = plane->funcs->update_plane(plane, crtc, fb,
> > >plane_req->crtc_x, plane_req->crtc_y,
> > >plane_req->crtc_w, plane_req->crtc_h,
> > >plane_req->src_x, plane_req->src_y,
> > >plane_req->src_w, plane_req->src_h);
> > >   if (!ret) {
> > > - old_fb = plane->fb;
> > >   plane->crtc = crtc;
> > >   plane->fb = fb;
> > > - fb = NULL;
> > >   }
> > >   drm_modeset_unlock_all(dev);
> > >  
> > >  out:
> > > - if (fb)
> > > - drm_framebuffer_unreference(fb);
> > > + if (plane->fb)
> > > + drm_framebuffer_reference(old_fb);
> >   ^^
> > 
> > That doesn't look right.
> > 
> > Also you're now dereferencing plane->fb after you drop the locks.
> 
> Oops, will fix.
> 
> > Also i915 fb destruction seems buggy as we do
> > intel_fb->obj->framebuffer_references-- w/o holding struct_mutex.
> 
> Hm, this indeed looks fishy. And the offending patch was actually r-b:
> you.

Dang. I suck.

> Not sure what to do here really.

Just s/drm_gem_object_unreference_unlocked/drm_gem_object_unreference/
and grab struct_mutex by hand around both operations?

> -Daniel
> 
> > 
> > >   if (old_fb)
> > >   drm_framebuffer_unreference(old_fb);
> > > + drm_framebuffer_unreference(fb);
> > >  
> > >   return ret;
> > >  }
> > > @@ -2239,9 +2239,7 @@ int drm_mode_set_config_internal(struct 
> > > drm_mode_set *set)
> > >   ret = crtc->funcs->set_config(set);
> > >   if (ret == 0) {
> > >   crtc->primary->crtc = crtc;
> > > -
> > > - /* crtc->fb must be updated by ->set_config, enforces this. */
> > > - WARN_ON(fb != crtc->primary->fb);
> > > + crtc->primary->fb = fb;
> > >   }
> > >  
> > >   list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
> > > diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> > > b/drivers/gpu/drm/drm_plane_helper.c
> > > index e768d35ff22e..8db129c44fea 100644
> > > --- a/drivers/gpu/drm/drm_plane_helper.c
> > > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > > @@ -175,22 +175,7 @@ int drm_primary_helper_update(struct drm_plane 
> > > *plane, struct drm_crtc *crtc,
> > >   set.connectors = connector_list;
> > >   set.num_connectors = num_connectors;
> > >  
> > > - /*
> > > -  * set_config() adjusts crtc->primary->fb; however the DRM setplane
> > > -  * code that called us expects to handle the framebuffer update and
> > > -  * reference counting; save and restore the current fb before
> > > -  * calling it.
> > > -  *
> > > -  * N.B., we call set_config() directly here rather than using
> > > -  * drm_mode_set_config_internal.  We're reprogramming the same
> > > -  * connectors that were already in use, so we shouldn't need the extra
> > > -  * cross-CRTC fb refcounting to accomodate stealing connectors.
> > > -  * drm_mode_setplane() already handles the basic refcounting for the
> > > -  * framebuffers involved in this operation.
> > > -  */
> > > - tmpfb = plane->fb;
> > >   ret = crtc->funcs->set_config();
> > > - plane->fb = tmpfb;
> > >  
> > >   kfree(connector_list);
> > >   

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

On Tue, Apr 22, 2014 at 4:28 PM, Ville Syrj?l?
 wrote:
>> Not sure what to do here really.
>
> Just s/drm_gem_object_unreference_unlocked/drm_gem_object_unreference/
> and grab struct_mutex by hand around both operations?

I was kinda hoping for someone to go ahead and fix the entire fb
tracking locking madness while at it ;-) I'll do the fixup.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

The introduction of primary planes has apparently caused a bit of fb
refcounting fun for people. That makes it a good time to clean up the
arcane rules and slight differences between ->update_plane and
->set_config. The new rules are:

- The core holds a reference for both the new and the old fb (if
  they're non-NULL of course) while calling into the driver through
  either ->update_plane or ->set_config.

- Drivers may not clobber plane->fb if their callback fails. If they
  do that, they need to store a pointer to the old fb in it again.
  When calling into the driver plane->fb still points at the current
  (old) framebuffer.

- The core will update the plane->fb pointer on success. Drivers can
  do that themselves too, but aren't required to any more for the
  primary plane.

- The core will update fb refcounts for the plane->fb pointer,
  presuming the drivers hold up their end of the bargain.

v2: Remove now unused tmpfb (Thierry)

v3: Drop broken changes from drm_mode_setplane (Ville). Also polish
the commit message a bit.

Cc: Thierry Reding 
Cc: Ville Syrj?l? 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c |  8 
 drivers/gpu/drm/drm_plane_helper.c | 16 
 2 files changed, 4 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8b7099abece..a76000ee2a43 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2187,16 +2187,18 @@ int drm_mode_setplane(struct drm_device *dev, void 
*data,
}

drm_modeset_lock_all(dev);
+   old_fb = plane->fb;
ret = plane->funcs->update_plane(plane, crtc, fb,
 plane_req->crtc_x, plane_req->crtc_y,
 plane_req->crtc_w, plane_req->crtc_h,
 plane_req->src_x, plane_req->src_y,
 plane_req->src_w, plane_req->src_h);
if (!ret) {
-   old_fb = plane->fb;
plane->crtc = crtc;
plane->fb = fb;
fb = NULL;
+   } else {
+   old_fb = NULL;
}
drm_modeset_unlock_all(dev);

@@ -2239,9 +2241,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
ret = crtc->funcs->set_config(set);
if (ret == 0) {
crtc->primary->crtc = crtc;
-
-   /* crtc->fb must be updated by ->set_config, enforces this. */
-   WARN_ON(fb != crtc->primary->fb);
+   crtc->primary->fb = fb;
}

list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
diff --git a/drivers/gpu/drm/drm_plane_helper.c 
b/drivers/gpu/drm/drm_plane_helper.c
index 9540ff9f97fe..b72736d5541d 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -124,7 +124,6 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,
.y2 = crtc->mode.vdisplay,
};
struct drm_connector **connector_list;
-   struct drm_framebuffer *tmpfb;
int num_connectors, ret;

if (!crtc->enabled) {
@@ -177,22 +176,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,
set.connectors = connector_list;
set.num_connectors = num_connectors;

-   /*
-* set_config() adjusts crtc->primary->fb; however the DRM setplane
-* code that called us expects to handle the framebuffer update and
-* reference counting; save and restore the current fb before
-* calling it.
-*
-* N.B., we call set_config() directly here rather than using
-* drm_mode_set_config_internal.  We're reprogramming the same
-* connectors that were already in use, so we shouldn't need the extra
-* cross-CRTC fb refcounting to accomodate stealing connectors.
-* drm_mode_setplane() already handles the basic refcounting for the
-* framebuffers involved in this operation.
-*/
-   tmpfb = plane->fb;
ret = crtc->funcs->set_config();
-   plane->fb = tmpfb;

kfree(connector_list);
return ret;
-- 
1.9.2

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

On Tue, Apr 22, 2014 at 01:06:22PM +0300, Ville Syrj?l? wrote:
> On Tue, Apr 22, 2014 at 11:07:13AM +0200, Daniel Vetter wrote:
> > The introduction of primary planes has apparently caused a bit of fb
> > refcounting fun for people. That makes it a good time to clean up the
> > arcane rules and slight differences between ->update_plane and
> > ->set_config. The new rules are:
> > 
> > - The core holds a reference for both the new and the old fb (if their
> >   non-NULL of course) while calling into the driver through either
> >   ->update_plane or ->set_config.
> > 
> > - Drivers may not clobber plane->fb if their callback fails. If they
> >   do that, they need to store a pointer to the old fb in it again.
> >   When calling into the driver plane->fb still points at the current
> >   (old) framebuffer.
> > 
> > - The core will update the plane->fb pointer on success. Drivers can
> >   do that themselves too.
> > 
> > - The core will update fb refcounts for the plane->fb pointer,
> >   presuming the drivers hold up their end of the bargain.
> > 
> > Signed-off-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 12 +---
> >  drivers/gpu/drm/drm_plane_helper.c | 15 ---
> >  2 files changed, 5 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index d8b7099abece..966b480ed543 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -2187,24 +2187,24 @@ int drm_mode_setplane(struct drm_device *dev, void 
> > *data,
> > }
> >  
> > drm_modeset_lock_all(dev);
> > +   old_fb = plane->fb;
> > ret = plane->funcs->update_plane(plane, crtc, fb,
> >  plane_req->crtc_x, plane_req->crtc_y,
> >  plane_req->crtc_w, plane_req->crtc_h,
> >  plane_req->src_x, plane_req->src_y,
> >  plane_req->src_w, plane_req->src_h);
> > if (!ret) {
> > -   old_fb = plane->fb;
> > plane->crtc = crtc;
> > plane->fb = fb;
> > -   fb = NULL;
> > }
> > drm_modeset_unlock_all(dev);
> >  
> >  out:
> > -   if (fb)
> > -   drm_framebuffer_unreference(fb);
> > +   if (plane->fb)
> > +   drm_framebuffer_reference(old_fb);
>   ^^
> 
> That doesn't look right.
> 
> Also you're now dereferencing plane->fb after you drop the locks.

Oops, will fix.

> Also i915 fb destruction seems buggy as we do
> intel_fb->obj->framebuffer_references-- w/o holding struct_mutex.

Hm, this indeed looks fishy. And the offending patch was actually r-b:
you. Not sure what to do here really.
-Daniel

> 
> > if (old_fb)
> > drm_framebuffer_unreference(old_fb);
> > +   drm_framebuffer_unreference(fb);
> >  
> > return ret;
> >  }
> > @@ -2239,9 +2239,7 @@ int drm_mode_set_config_internal(struct drm_mode_set 
> > *set)
> > ret = crtc->funcs->set_config(set);
> > if (ret == 0) {
> > crtc->primary->crtc = crtc;
> > -
> > -   /* crtc->fb must be updated by ->set_config, enforces this. */
> > -   WARN_ON(fb != crtc->primary->fb);
> > +   crtc->primary->fb = fb;
> > }
> >  
> > list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
> > diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> > b/drivers/gpu/drm/drm_plane_helper.c
> > index e768d35ff22e..8db129c44fea 100644
> > --- a/drivers/gpu/drm/drm_plane_helper.c
> > +++ b/drivers/gpu/drm/drm_plane_helper.c
> > @@ -175,22 +175,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> > struct drm_crtc *crtc,
> > set.connectors = connector_list;
> > set.num_connectors = num_connectors;
> >  
> > -   /*
> > -* set_config() adjusts crtc->primary->fb; however the DRM setplane
> > -* code that called us expects to handle the framebuffer update and
> > -* reference counting; save and restore the current fb before
> > -* calling it.
> > -*
> > -* N.B., we call set_config() directly here rather than using
> > -* drm_mode_set_config_internal.  We're reprogramming the same
> > -* connectors that were already in use, so we shouldn't need the extra
> > -* cross-CRTC fb refcounting to accomodate stealing connectors.
> > -* drm_mode_setplane() already handles the basic refcounting for the
> > -* framebuffers involved in this operation.
> > -*/
> > -   tmpfb = plane->fb;
> > ret = crtc->funcs->set_config();
> > -   plane->fb = tmpfb;
> >  
> > kfree(connector_list);
> > return ret;
> > -- 
> > 1.9.2
> > 
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrj?l?
> Intel OTC

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - 

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Ville Syrjälä]

On Tue, Apr 22, 2014 at 11:07:13AM +0200, Daniel Vetter wrote:
> The introduction of primary planes has apparently caused a bit of fb
> refcounting fun for people. That makes it a good time to clean up the
> arcane rules and slight differences between ->update_plane and
> ->set_config. The new rules are:
> 
> - The core holds a reference for both the new and the old fb (if their
>   non-NULL of course) while calling into the driver through either
>   ->update_plane or ->set_config.
> 
> - Drivers may not clobber plane->fb if their callback fails. If they
>   do that, they need to store a pointer to the old fb in it again.
>   When calling into the driver plane->fb still points at the current
>   (old) framebuffer.
> 
> - The core will update the plane->fb pointer on success. Drivers can
>   do that themselves too.
> 
> - The core will update fb refcounts for the plane->fb pointer,
>   presuming the drivers hold up their end of the bargain.
> 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_crtc.c | 12 +---
>  drivers/gpu/drm/drm_plane_helper.c | 15 ---
>  2 files changed, 5 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d8b7099abece..966b480ed543 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -2187,24 +2187,24 @@ int drm_mode_setplane(struct drm_device *dev, void 
> *data,
>   }
>  
>   drm_modeset_lock_all(dev);
> + old_fb = plane->fb;
>   ret = plane->funcs->update_plane(plane, crtc, fb,
>plane_req->crtc_x, plane_req->crtc_y,
>plane_req->crtc_w, plane_req->crtc_h,
>plane_req->src_x, plane_req->src_y,
>plane_req->src_w, plane_req->src_h);
>   if (!ret) {
> - old_fb = plane->fb;
>   plane->crtc = crtc;
>   plane->fb = fb;
> - fb = NULL;
>   }
>   drm_modeset_unlock_all(dev);
>  
>  out:
> - if (fb)
> - drm_framebuffer_unreference(fb);
> + if (plane->fb)
> + drm_framebuffer_reference(old_fb);
  ^^

That doesn't look right.

Also you're now dereferencing plane->fb after you drop the locks.

Also i915 fb destruction seems buggy as we do
intel_fb->obj->framebuffer_references-- w/o holding struct_mutex.

>   if (old_fb)
>   drm_framebuffer_unreference(old_fb);
> + drm_framebuffer_unreference(fb);
>  
>   return ret;
>  }
> @@ -2239,9 +2239,7 @@ int drm_mode_set_config_internal(struct drm_mode_set 
> *set)
>   ret = crtc->funcs->set_config(set);
>   if (ret == 0) {
>   crtc->primary->crtc = crtc;
> -
> - /* crtc->fb must be updated by ->set_config, enforces this. */
> - WARN_ON(fb != crtc->primary->fb);
> + crtc->primary->fb = fb;
>   }
>  
>   list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
> diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> b/drivers/gpu/drm/drm_plane_helper.c
> index e768d35ff22e..8db129c44fea 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -175,22 +175,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   set.connectors = connector_list;
>   set.num_connectors = num_connectors;
>  
> - /*
> -  * set_config() adjusts crtc->primary->fb; however the DRM setplane
> -  * code that called us expects to handle the framebuffer update and
> -  * reference counting; save and restore the current fb before
> -  * calling it.
> -  *
> -  * N.B., we call set_config() directly here rather than using
> -  * drm_mode_set_config_internal.  We're reprogramming the same
> -  * connectors that were already in use, so we shouldn't need the extra
> -  * cross-CRTC fb refcounting to accomodate stealing connectors.
> -  * drm_mode_setplane() already handles the basic refcounting for the
> -  * framebuffers involved in this operation.
> -  */
> - tmpfb = plane->fb;
>   ret = crtc->funcs->set_config();
> - plane->fb = tmpfb;
>  
>   kfree(connector_list);
>   return ret;
> -- 
> 1.9.2
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Thierry Reding]

On Tue, Apr 22, 2014 at 11:07:13AM +0200, Daniel Vetter wrote:
[...]
> diff --git a/drivers/gpu/drm/drm_plane_helper.c 
> b/drivers/gpu/drm/drm_plane_helper.c
> index e768d35ff22e..8db129c44fea 100644
> --- a/drivers/gpu/drm/drm_plane_helper.c
> +++ b/drivers/gpu/drm/drm_plane_helper.c
> @@ -175,22 +175,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
> struct drm_crtc *crtc,
>   set.connectors = connector_list;
>   set.num_connectors = num_connectors;
>  
> - /*
> -  * set_config() adjusts crtc->primary->fb; however the DRM setplane
> -  * code that called us expects to handle the framebuffer update and
> -  * reference counting; save and restore the current fb before
> -  * calling it.
> -  *
> -  * N.B., we call set_config() directly here rather than using
> -  * drm_mode_set_config_internal.  We're reprogramming the same
> -  * connectors that were already in use, so we shouldn't need the extra
> -  * cross-CRTC fb refcounting to accomodate stealing connectors.
> -  * drm_mode_setplane() already handles the basic refcounting for the
> -  * framebuffers involved in this operation.
> -  */
> - tmpfb = plane->fb;
>   ret = crtc->funcs->set_config();
> - plane->fb = tmpfb;

This makes tmpfb unused. Other than that this looks sane to me and it
doesn't make things worse from what I can tell:

Tested-by: Thierry Reding 
-- next part --
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: 

[PATCH] drm: Simplify fb refcounting rules around ->update_plane

[Daniel Vetter]

The introduction of primary planes has apparently caused a bit of fb
refcounting fun for people. That makes it a good time to clean up the
arcane rules and slight differences between ->update_plane and
->set_config. The new rules are:

- The core holds a reference for both the new and the old fb (if their
  non-NULL of course) while calling into the driver through either
  ->update_plane or ->set_config.

- Drivers may not clobber plane->fb if their callback fails. If they
  do that, they need to store a pointer to the old fb in it again.
  When calling into the driver plane->fb still points at the current
  (old) framebuffer.

- The core will update the plane->fb pointer on success. Drivers can
  do that themselves too.

- The core will update fb refcounts for the plane->fb pointer,
  presuming the drivers hold up their end of the bargain.

Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c | 12 +---
 drivers/gpu/drm/drm_plane_helper.c | 15 ---
 2 files changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d8b7099abece..966b480ed543 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -2187,24 +2187,24 @@ int drm_mode_setplane(struct drm_device *dev, void 
*data,
}

drm_modeset_lock_all(dev);
+   old_fb = plane->fb;
ret = plane->funcs->update_plane(plane, crtc, fb,
 plane_req->crtc_x, plane_req->crtc_y,
 plane_req->crtc_w, plane_req->crtc_h,
 plane_req->src_x, plane_req->src_y,
 plane_req->src_w, plane_req->src_h);
if (!ret) {
-   old_fb = plane->fb;
plane->crtc = crtc;
plane->fb = fb;
-   fb = NULL;
}
drm_modeset_unlock_all(dev);

 out:
-   if (fb)
-   drm_framebuffer_unreference(fb);
+   if (plane->fb)
+   drm_framebuffer_reference(old_fb);
if (old_fb)
drm_framebuffer_unreference(old_fb);
+   drm_framebuffer_unreference(fb);

return ret;
 }
@@ -2239,9 +2239,7 @@ int drm_mode_set_config_internal(struct drm_mode_set *set)
ret = crtc->funcs->set_config(set);
if (ret == 0) {
crtc->primary->crtc = crtc;
-
-   /* crtc->fb must be updated by ->set_config, enforces this. */
-   WARN_ON(fb != crtc->primary->fb);
+   crtc->primary->fb = fb;
}

list_for_each_entry(tmp, >dev->mode_config.crtc_list, head) {
diff --git a/drivers/gpu/drm/drm_plane_helper.c 
b/drivers/gpu/drm/drm_plane_helper.c
index e768d35ff22e..8db129c44fea 100644
--- a/drivers/gpu/drm/drm_plane_helper.c
+++ b/drivers/gpu/drm/drm_plane_helper.c
@@ -175,22 +175,7 @@ int drm_primary_helper_update(struct drm_plane *plane, 
struct drm_crtc *crtc,
set.connectors = connector_list;
set.num_connectors = num_connectors;

-   /*
-* set_config() adjusts crtc->primary->fb; however the DRM setplane
-* code that called us expects to handle the framebuffer update and
-* reference counting; save and restore the current fb before
-* calling it.
-*
-* N.B., we call set_config() directly here rather than using
-* drm_mode_set_config_internal.  We're reprogramming the same
-* connectors that were already in use, so we shouldn't need the extra
-* cross-CRTC fb refcounting to accomodate stealing connectors.
-* drm_mode_setplane() already handles the basic refcounting for the
-* framebuffers involved in this operation.
-*/
-   tmpfb = plane->fb;
ret = crtc->funcs->set_config();
-   plane->fb = tmpfb;

kfree(connector_list);
return ret;
-- 
1.9.2