[PATCH] drm: Don't assign fbs for universal cursor support to files

[PATCH] drm: Don't assign fbs for universal cursor support to files

[Chris Wilson]

The internal framebuffers we create to remap legacy cursor ioctls to
plane operations for the universal plane support shouldn't be linke to
the file like normal userspace framebuffers. This bug goes back to the
original universal cursor plane support introduced in

commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Tue Jun 10 08:28:10 2014 -0700

    drm: Support legacy cursor ioctls via universal planes when possible (v4)

The isn't too disastrous since fbs are small, we only create one when the
cursor bo gets changed and ultimately they'll be reaped when the window
server restarts.

Conceptually we'd want to just pass NULL for file_priv when creating it,
but the driver needs the file to lookup the underlying buffer object for
cursor id. Instead let's move the file_priv linking out of
add_framebuffer_internal() into the addfb ioctl implementation, which is
the only place it is needed. And also rename the function for a more
accurate since it only creates the fb, but doesn't add it anywhere.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (fix & commit msg)
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (provider of lipstick)
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Rob Clark <robdclark@gmail.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: stable@vger.kernel.org
---
 drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++----------------
 1 file changed, 19 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 927f3445ff38..4c78d12c5418 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -43,9 +43,10 @@
 #include "drm_crtc_internal.h"
 #include "drm_internal.h"
 
-static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
-							struct drm_mode_fb_cmd2 *r,
-							struct drm_file *file_priv);
+static struct drm_framebuffer *
+internal_framebuffer_create(struct drm_device *dev,
+			    struct drm_mode_fb_cmd2 *r,
+			    struct drm_file *file_priv);
 
 /* Avoid boilerplate.  I'm tired of typing. */
 #define DRM_ENUM_NAME_FN(fnname, list)				\
@@ -2919,13 +2920,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
 	 */
 	if (req->flags & DRM_MODE_CURSOR_BO) {
 		if (req->handle) {
-			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
+			fb = internal_framebuffer_create(dev, &fbreq, file_priv);
 			if (IS_ERR(fb)) {
 				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
 				return PTR_ERR(fb);
 			}
-
-			drm_framebuffer_reference(fb);
 		} else {
 			fb = NULL;
 		}
@@ -3284,9 +3283,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
 	return 0;
 }
 
-static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
-							struct drm_mode_fb_cmd2 *r,
-							struct drm_file *file_priv)
+static struct drm_framebuffer *
+internal_framebuffer_create(struct drm_device *dev,
+			    struct drm_mode_fb_cmd2 *r,
+			    struct drm_file *file_priv)
 {
 	struct drm_mode_config *config = &dev->mode_config;
 	struct drm_framebuffer *fb;
@@ -3324,12 +3324,6 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
 		return fb;
 	}
 
-	mutex_lock(&file_priv->fbs_lock);
-	r->fb_id = fb->base.id;
-	list_add(&fb->filp_head, &file_priv->fbs);
-	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
-	mutex_unlock(&file_priv->fbs_lock);
-
 	return fb;
 }
 
@@ -3351,15 +3345,24 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
 int drm_mode_addfb2(struct drm_device *dev,
 		    void *data, struct drm_file *file_priv)
 {
+	struct drm_mode_fb_cmd2 *r = data;
 	struct drm_framebuffer *fb;
 
 	if (!drm_core_check_feature(dev, DRIVER_MODESET))
 		return -EINVAL;
 
-	fb = add_framebuffer_internal(dev, data, file_priv);
+	fb = internal_framebuffer_create(dev, r, file_priv);
 	if (IS_ERR(fb))
 		return PTR_ERR(fb);
 
+	/* Transfer ownership to the filp for reaping on close */
+
+	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
+	mutex_lock(&file_priv->fbs_lock);
+	r->fb_id = fb->base.id;
+	list_add(&fb->filp_head, &file_priv->fbs);
+	mutex_unlock(&file_priv->fbs_lock);
+
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH] drm: Don't assign fbs for universal cursor support to files

[Matt Roper]

On Wed, Feb 25, 2015 at 01:45:26PM +0000, Chris Wilson wrote:
> The internal framebuffers we create to remap legacy cursor ioctls to
> plane operations for the universal plane support shouldn't be linke to
> the file like normal userspace framebuffers. This bug goes back to the
> original universal cursor plane support introduced in
> 
> commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Tue Jun 10 08:28:10 2014 -0700
> 
>     drm: Support legacy cursor ioctls via universal planes when possible (v4)
> 
> The isn't too disastrous since fbs are small, we only create one when the
> cursor bo gets changed and ultimately they'll be reaped when the window
> server restarts.
> 
> Conceptually we'd want to just pass NULL for file_priv when creating it,
> but the driver needs the file to lookup the underlying buffer object for
> cursor id. Instead let's move the file_priv linking out of
> add_framebuffer_internal() into the addfb ioctl implementation, which is
> the only place it is needed. And also rename the function for a more
> accurate since it only creates the fb, but doesn't add it anywhere.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (fix & commit msg)
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (provider of lipstick)
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Rob Clark <robdclark@gmail.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: stable@vger.kernel.org
> ---
>  drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++----------------
>  1 file changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 927f3445ff38..4c78d12c5418 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -43,9 +43,10 @@
>  #include "drm_crtc_internal.h"
>  #include "drm_internal.h"
>  
> -static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> -							struct drm_mode_fb_cmd2 *r,
> -							struct drm_file *file_priv);
> +static struct drm_framebuffer *
> +internal_framebuffer_create(struct drm_device *dev,
> +			    struct drm_mode_fb_cmd2 *r,
> +			    struct drm_file *file_priv);
>  
>  /* Avoid boilerplate.  I'm tired of typing. */
>  #define DRM_ENUM_NAME_FN(fnname, list)				\
> @@ -2919,13 +2920,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
>  	 */
>  	if (req->flags & DRM_MODE_CURSOR_BO) {
>  		if (req->handle) {
> -			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
> +			fb = internal_framebuffer_create(dev, &fbreq, file_priv);
>  			if (IS_ERR(fb)) {
>  				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
>  				return PTR_ERR(fb);
>  			}
> -
> -			drm_framebuffer_reference(fb);

Sorry for the delay reviewing this.  I'll provide an i-g-t test that
checks for these memory leaks shortly.

If I'm not mistaken, this patch will work properly for normal operation,
but I think we might run into problems if your display server gets
killed while a wrapped cursor is onscreen and we need to restore the
fbdev mode.  

From what I can see, we'll wind up in drm_plane_force_disable() which
does:

        plane->old_fb = plane->fb;
        ret = plane->funcs->disable_plane(plane);
        if (ret) {
                DRM_ERROR("failed to disable plane with busy fb\n");
                plane->old_fb = NULL;
                return;
        }
        /* disconnect the plane from the fb and crtc: */
        __drm_framebuffer_unreference(plane->old_fb);

Note the internal __drm_framebuffer_unreference() here rather than a
traditional drm_framebuffer_unreference().  The internal version is only
supposed to be used when we know we're not releasing the last reference
and BUG()'s out if we actually take the reference count down to zero
(which is exactly what we do in this case).

I guess we need to just do away with __drm_framebuffer_unreference() now since
its only call-site is no longer guaranteed to be working on framebuffers that
still have a remaining reference.


Matt

>  		} else {
>  			fb = NULL;
>  		}
> @@ -3284,9 +3283,10 @@ static int framebuffer_check(const struct drm_mode_fb_cmd2 *r)
>  	return 0;
>  }
>  
> -static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> -							struct drm_mode_fb_cmd2 *r,
> -							struct drm_file *file_priv)
> +static struct drm_framebuffer *
> +internal_framebuffer_create(struct drm_device *dev,
> +			    struct drm_mode_fb_cmd2 *r,
> +			    struct drm_file *file_priv)
>  {
>  	struct drm_mode_config *config = &dev->mode_config;
>  	struct drm_framebuffer *fb;
> @@ -3324,12 +3324,6 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
>  		return fb;
>  	}
>  
> -	mutex_lock(&file_priv->fbs_lock);
> -	r->fb_id = fb->base.id;
> -	list_add(&fb->filp_head, &file_priv->fbs);
> -	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
> -	mutex_unlock(&file_priv->fbs_lock);
> -
>  	return fb;
>  }
>  
> @@ -3351,15 +3345,24 @@ static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
>  int drm_mode_addfb2(struct drm_device *dev,
>  		    void *data, struct drm_file *file_priv)
>  {
> +	struct drm_mode_fb_cmd2 *r = data;
>  	struct drm_framebuffer *fb;
>  
>  	if (!drm_core_check_feature(dev, DRIVER_MODESET))
>  		return -EINVAL;
>  
> -	fb = add_framebuffer_internal(dev, data, file_priv);
> +	fb = internal_framebuffer_create(dev, r, file_priv);
>  	if (IS_ERR(fb))
>  		return PTR_ERR(fb);
>  
> +	/* Transfer ownership to the filp for reaping on close */
> +
> +	DRM_DEBUG_KMS("[FB:%d]\n", fb->base.id);
> +	mutex_lock(&file_priv->fbs_lock);
> +	r->fb_id = fb->base.id;
> +	list_add(&fb->filp_head, &file_priv->fbs);
> +	mutex_unlock(&file_priv->fbs_lock);
> +
>  	return 0;
>  }
>  
> -- 
> 2.1.4
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795

Re: [PATCH] drm: Don't assign fbs for universal cursor support to files

[Ville Syrjälä]

On Thu, Feb 26, 2015 at 06:50:19PM -0800, Matt Roper wrote:
> On Wed, Feb 25, 2015 at 01:45:26PM +0000, Chris Wilson wrote:
> > The internal framebuffers we create to remap legacy cursor ioctls to
> > plane operations for the universal plane support shouldn't be linke to
> > the file like normal userspace framebuffers. This bug goes back to the
> > original universal cursor plane support introduced in
> > 
> > commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
> > Author: Matt Roper <matthew.d.roper@intel.com>
> > Date:   Tue Jun 10 08:28:10 2014 -0700
> > 
> >     drm: Support legacy cursor ioctls via universal planes when possible (v4)
> > 
> > The isn't too disastrous since fbs are small, we only create one when the
> > cursor bo gets changed and ultimately they'll be reaped when the window
> > server restarts.
> > 
> > Conceptually we'd want to just pass NULL for file_priv when creating it,
> > but the driver needs the file to lookup the underlying buffer object for
> > cursor id. Instead let's move the file_priv linking out of
> > add_framebuffer_internal() into the addfb ioctl implementation, which is
> > the only place it is needed. And also rename the function for a more
> > accurate since it only creates the fb, but doesn't add it anywhere.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (fix & commit msg)
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (provider of lipstick)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 927f3445ff38..4c78d12c5418 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -43,9 +43,10 @@
> >  #include "drm_crtc_internal.h"
> >  #include "drm_internal.h"
> >  
> > -static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> > -							struct drm_mode_fb_cmd2 *r,
> > -							struct drm_file *file_priv);
> > +static struct drm_framebuffer *
> > +internal_framebuffer_create(struct drm_device *dev,
> > +			    struct drm_mode_fb_cmd2 *r,
> > +			    struct drm_file *file_priv);
> >  
> >  /* Avoid boilerplate.  I'm tired of typing. */
> >  #define DRM_ENUM_NAME_FN(fnname, list)				\
> > @@ -2919,13 +2920,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> >  	 */
> >  	if (req->flags & DRM_MODE_CURSOR_BO) {
> >  		if (req->handle) {
> > -			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
> > +			fb = internal_framebuffer_create(dev, &fbreq, file_priv);
> >  			if (IS_ERR(fb)) {
> >  				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
> >  				return PTR_ERR(fb);
> >  			}
> > -
> > -			drm_framebuffer_reference(fb);
> 
> Sorry for the delay reviewing this.  I'll provide an i-g-t test that
> checks for these memory leaks shortly.
> 
> If I'm not mistaken, this patch will work properly for normal operation,
> but I think we might run into problems if your display server gets
> killed while a wrapped cursor is onscreen and we need to restore the
> fbdev mode.  
> 
> >From what I can see, we'll wind up in drm_plane_force_disable() which
> does:
> 
>         plane->old_fb = plane->fb;
>         ret = plane->funcs->disable_plane(plane);
>         if (ret) {
>                 DRM_ERROR("failed to disable plane with busy fb\n");
>                 plane->old_fb = NULL;
>                 return;
>         }
>         /* disconnect the plane from the fb and crtc: */
>         __drm_framebuffer_unreference(plane->old_fb);
> 
> Note the internal __drm_framebuffer_unreference() here rather than a
> traditional drm_framebuffer_unreference().  The internal version is only
> supposed to be used when we know we're not releasing the last reference
> and BUG()'s out if we actually take the reference count down to zero
> (which is exactly what we do in this case).
> 
> I guess we need to just do away with __drm_framebuffer_unreference() now since
> its only call-site is no longer guaranteed to be working on framebuffers that
> still have a remaining reference.

Since the fb is no longer on the file_priv->fbs list drm_plane_force_disable()
shouldn't actually get called. But that does mean that when a master is
killed (or just closes the fd without turning off the cursor first) it
leaves the internal cursor fb behind, and the next guy to come along
can then see it. That won't happen to any other fb since everything else
is on the fbs list.

I was arguing on irc to Daniel that we should track these internal fbs
separately then for each fd to make sure they get cleaned up like everything
else. Daniel had the notion that we should just remove the force
disable on release for everything. But that may have some security
implications as a master can't really control when it gets killed and so it
might end up leaking something a bit sensitive in whatever buffers were
getting scanned out at the time. Daniel had the idea that there should
be some kind of nanny logind doing such cleanup on behalf of everyone
that gets killed, but that seems overly complicated to me. Also force
disabling everything has been there since forever so simply changing it
seems a bit questionable. Anyway, my feeling is we shouldn't really
expose anything to outside parties unless the process has explicitly
indicated that it's OK.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm: Don't assign fbs for universal cursor support to files

[Daniel Vetter]

On Thu, Feb 26, 2015 at 06:50:19PM -0800, Matt Roper wrote:
> On Wed, Feb 25, 2015 at 01:45:26PM +0000, Chris Wilson wrote:
> > The internal framebuffers we create to remap legacy cursor ioctls to
> > plane operations for the universal plane support shouldn't be linke to
> > the file like normal userspace framebuffers. This bug goes back to the
> > original universal cursor plane support introduced in
> > 
> > commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
> > Author: Matt Roper <matthew.d.roper@intel.com>
> > Date:   Tue Jun 10 08:28:10 2014 -0700
> > 
> >     drm: Support legacy cursor ioctls via universal planes when possible (v4)
> > 
> > The isn't too disastrous since fbs are small, we only create one when the
> > cursor bo gets changed and ultimately they'll be reaped when the window
> > server restarts.
> > 
> > Conceptually we'd want to just pass NULL for file_priv when creating it,
> > but the driver needs the file to lookup the underlying buffer object for
> > cursor id. Instead let's move the file_priv linking out of
> > add_framebuffer_internal() into the addfb ioctl implementation, which is
> > the only place it is needed. And also rename the function for a more
> > accurate since it only creates the fb, but doesn't add it anywhere.
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (fix & commit msg)
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (provider of lipstick)
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Rob Clark <robdclark@gmail.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: stable@vger.kernel.org
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++----------------
> >  1 file changed, 19 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 927f3445ff38..4c78d12c5418 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -43,9 +43,10 @@
> >  #include "drm_crtc_internal.h"
> >  #include "drm_internal.h"
> >  
> > -static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> > -							struct drm_mode_fb_cmd2 *r,
> > -							struct drm_file *file_priv);
> > +static struct drm_framebuffer *
> > +internal_framebuffer_create(struct drm_device *dev,
> > +			    struct drm_mode_fb_cmd2 *r,
> > +			    struct drm_file *file_priv);
> >  
> >  /* Avoid boilerplate.  I'm tired of typing. */
> >  #define DRM_ENUM_NAME_FN(fnname, list)				\
> > @@ -2919,13 +2920,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> >  	 */
> >  	if (req->flags & DRM_MODE_CURSOR_BO) {
> >  		if (req->handle) {
> > -			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
> > +			fb = internal_framebuffer_create(dev, &fbreq, file_priv);
> >  			if (IS_ERR(fb)) {
> >  				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
> >  				return PTR_ERR(fb);
> >  			}
> > -
> > -			drm_framebuffer_reference(fb);
> 
> Sorry for the delay reviewing this.  I'll provide an i-g-t test that
> checks for these memory leaks shortly.
> 
> If I'm not mistaken, this patch will work properly for normal operation,
> but I think we might run into problems if your display server gets
> killed while a wrapped cursor is onscreen and we need to restore the
> fbdev mode.  
> 
> From what I can see, we'll wind up in drm_plane_force_disable() which
> does:
> 
>         plane->old_fb = plane->fb;
>         ret = plane->funcs->disable_plane(plane);
>         if (ret) {
>                 DRM_ERROR("failed to disable plane with busy fb\n");
>                 plane->old_fb = NULL;
>                 return;
>         }
>         /* disconnect the plane from the fb and crtc: */
>         __drm_framebuffer_unreference(plane->old_fb);
> 
> Note the internal __drm_framebuffer_unreference() here rather than a
> traditional drm_framebuffer_unreference().  The internal version is only
> supposed to be used when we know we're not releasing the last reference
> and BUG()'s out if we actually take the reference count down to zero
> (which is exactly what we do in this case).
> 
> I guess we need to just do away with __drm_framebuffer_unreference() now since
> its only call-site is no longer guaranteed to be working on framebuffers that
> still have a remaining reference.

Nice issue you spotted, but this actually goes back to making the idr
reference a weak one in

commit 83f45fc360c8e16a330474860ebda872d1384c8c
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Aug 6 09:10:18 2014 +0200

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

The reference which was guaranteed to be around was from the idr. The race
is really small though since we still remove fbs right away once they go
away from the idr.

I'll prep a patch for this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm: Don't assign fbs for universal cursor support to files

[Matt Roper]

On Fri, Feb 27, 2015 at 12:15:16PM +0100, Daniel Vetter wrote:
> On Thu, Feb 26, 2015 at 06:50:19PM -0800, Matt Roper wrote:
> > On Wed, Feb 25, 2015 at 01:45:26PM +0000, Chris Wilson wrote:
> > > The internal framebuffers we create to remap legacy cursor ioctls to
> > > plane operations for the universal plane support shouldn't be linke to
> > > the file like normal userspace framebuffers. This bug goes back to the
> > > original universal cursor plane support introduced in
> > > 
> > > commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662
> > > Author: Matt Roper <matthew.d.roper@intel.com>
> > > Date:   Tue Jun 10 08:28:10 2014 -0700
> > > 
> > >     drm: Support legacy cursor ioctls via universal planes when possible (v4)
> > > 
> > > The isn't too disastrous since fbs are small, we only create one when the
> > > cursor bo gets changed and ultimately they'll be reaped when the window
> > > server restarts.
> > > 
> > > Conceptually we'd want to just pass NULL for file_priv when creating it,
> > > but the driver needs the file to lookup the underlying buffer object for
> > > cursor id. Instead let's move the file_priv linking out of
> > > add_framebuffer_internal() into the addfb ioctl implementation, which is
> > > the only place it is needed. And also rename the function for a more
> > > accurate since it only creates the fb, but doesn't add it anywhere.
> > > 
> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> (fix & commit msg)
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> (provider of lipstick)
> > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Rob Clark <robdclark@gmail.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: stable@vger.kernel.org
> > > ---
> > >  drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++----------------
> > >  1 file changed, 19 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > > index 927f3445ff38..4c78d12c5418 100644
> > > --- a/drivers/gpu/drm/drm_crtc.c
> > > +++ b/drivers/gpu/drm/drm_crtc.c
> > > @@ -43,9 +43,10 @@
> > >  #include "drm_crtc_internal.h"
> > >  #include "drm_internal.h"
> > >  
> > > -static struct drm_framebuffer *add_framebuffer_internal(struct drm_device *dev,
> > > -							struct drm_mode_fb_cmd2 *r,
> > > -							struct drm_file *file_priv);
> > > +static struct drm_framebuffer *
> > > +internal_framebuffer_create(struct drm_device *dev,
> > > +			    struct drm_mode_fb_cmd2 *r,
> > > +			    struct drm_file *file_priv);
> > >  
> > >  /* Avoid boilerplate.  I'm tired of typing. */
> > >  #define DRM_ENUM_NAME_FN(fnname, list)				\
> > > @@ -2919,13 +2920,11 @@ static int drm_mode_cursor_universal(struct drm_crtc *crtc,
> > >  	 */
> > >  	if (req->flags & DRM_MODE_CURSOR_BO) {
> > >  		if (req->handle) {
> > > -			fb = add_framebuffer_internal(dev, &fbreq, file_priv);
> > > +			fb = internal_framebuffer_create(dev, &fbreq, file_priv);
> > >  			if (IS_ERR(fb)) {
> > >  				DRM_DEBUG_KMS("failed to wrap cursor buffer in drm framebuffer\n");
> > >  				return PTR_ERR(fb);
> > >  			}
> > > -
> > > -			drm_framebuffer_reference(fb);
> > 
> > Sorry for the delay reviewing this.  I'll provide an i-g-t test that
> > checks for these memory leaks shortly.
> > 
> > If I'm not mistaken, this patch will work properly for normal operation,
> > but I think we might run into problems if your display server gets
> > killed while a wrapped cursor is onscreen and we need to restore the
> > fbdev mode.  
> > 
> > From what I can see, we'll wind up in drm_plane_force_disable() which
> > does:
> > 
> >         plane->old_fb = plane->fb;
> >         ret = plane->funcs->disable_plane(plane);
> >         if (ret) {
> >                 DRM_ERROR("failed to disable plane with busy fb\n");
> >                 plane->old_fb = NULL;
> >                 return;
> >         }
> >         /* disconnect the plane from the fb and crtc: */
> >         __drm_framebuffer_unreference(plane->old_fb);
> > 
> > Note the internal __drm_framebuffer_unreference() here rather than a
> > traditional drm_framebuffer_unreference().  The internal version is only
> > supposed to be used when we know we're not releasing the last reference
> > and BUG()'s out if we actually take the reference count down to zero
> > (which is exactly what we do in this case).
> > 
> > I guess we need to just do away with __drm_framebuffer_unreference() now since
> > its only call-site is no longer guaranteed to be working on framebuffers that
> > still have a remaining reference.
> 
> Nice issue you spotted, but this actually goes back to making the idr
> reference a weak one in
> 
> commit 83f45fc360c8e16a330474860ebda872d1384c8c
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Wed Aug 6 09:10:18 2014 +0200
> 
>     drm: Don't grab an fb reference for the idr
> 
> The reference which was guaranteed to be around was from the idr. The race
> is really small though since we still remove fbs right away once they go
> away from the idr.
> 
> I'll prep a patch for this.
> -Daniel

Okay, sounds good.

Reviewed-by: Matt Roper <matthew.d.roper@intel.com>

on this now that your extra patch takes care of my concern.


Matt

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

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel