From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH] drm: Don't assign fbs for universal cursor support to files Date: Thu, 26 Feb 2015 18:50:19 -0800 Message-ID: <20150227025019.GA18829@intel.com> References: <1424871926-4640-1-git-send-email-chris@chris-wilson.co.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Content-Disposition: inline In-Reply-To: <1424871926-4640-1-git-send-email-chris@chris-wilson.co.uk> Sender: stable-owner@vger.kernel.org To: Chris Wilson Cc: dri-devel@lists.freedesktop.org, Daniel Vetter , Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Rob Clark , Dave Airlie , stable@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org 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 t= o > the file like normal userspace framebuffers. This bug goes back to th= e > original universal cursor plane support introduced in >=20 > commit 161d0dc1dccb17ff7a38f462c7c0d4ef8bcc5662 > Author: Matt Roper > Date: Tue Jun 10 08:28:10 2014 -0700 >=20 > drm: Support legacy cursor ioctls via universal planes when possi= ble (v4) >=20 > 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 wind= ow > server restarts. >=20 > 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. >=20 > Signed-off-by: Daniel Vetter (fix & commit = msg) > Signed-off-by: Chris Wilson (provider of l= ipstick) > Cc: Ville Syrj=E4l=E4 > Cc: Matt Roper > Cc: Rob Clark > Cc: Dave Airlie > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/drm_crtc.c | 35 +++++++++++++++++++---------------- > 1 file changed, 19 insertions(+), 16 deletions(-) >=20 > 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" > =20 > -static struct drm_framebuffer *add_framebuffer_internal(struct drm_d= evice *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); > =20 > /* 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 d= rm_crtc *crtc, > */ > if (req->flags & DRM_MODE_CURSOR_BO) { > if (req->handle) { > - fb =3D add_framebuffer_internal(dev, &fbreq, file_priv); > + fb =3D 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. =20 =46rom what I can see, we'll wind up in drm_plane_force_disable() which does: plane->old_fb =3D plane->fb; ret =3D plane->funcs->disable_plane(plane); if (ret) { DRM_ERROR("failed to disable plane with busy fb\n"); plane->old_fb =3D 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 onl= y 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() no= w since its only call-site is no longer guaranteed to be working on framebuffer= s that still have a remaining reference. Matt > } else { > fb =3D NULL; > } > @@ -3284,9 +3283,10 @@ static int framebuffer_check(const struct drm_= mode_fb_cmd2 *r) > return 0; > } > =20 > -static struct drm_framebuffer *add_framebuffer_internal(struct drm_d= evice *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 =3D &dev->mode_config; > struct drm_framebuffer *fb; > @@ -3324,12 +3324,6 @@ static struct drm_framebuffer *add_framebuffer= _internal(struct drm_device *dev, > return fb; > } > =20 > - mutex_lock(&file_priv->fbs_lock); > - r->fb_id =3D 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; > } > =20 > @@ -3351,15 +3345,24 @@ static struct drm_framebuffer *add_framebuffe= r_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 =3D data; > struct drm_framebuffer *fb; > =20 > if (!drm_core_check_feature(dev, DRIVER_MODESET)) > return -EINVAL; > =20 > - fb =3D add_framebuffer_internal(dev, data, file_priv); > + fb =3D internal_framebuffer_create(dev, r, file_priv); > if (IS_ERR(fb)) > return PTR_ERR(fb); > =20 > + /* 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 =3D fb->base.id; > + list_add(&fb->filp_head, &file_priv->fbs); > + mutex_unlock(&file_priv->fbs_lock); > + > return 0; > } > =20 > --=20 > 2.1.4 >=20 --=20 Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795