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

[Matt Roper <matthew.d.roper@intel.com>]

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