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@xxxxxxxxx>
> > > 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@xxxxxxxxx> (fix & commit msg)
> > > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> (provider of lipstick)
> > > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > > Cc: Rob Clark <robdclark@xxxxxxxxx>
> > > Cc: Dave Airlie <airlied@xxxxxxxxxx>
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > ---
> > > 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@xxxxxxxx>
> 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@xxxxxxxxx>
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
--
To unsubscribe from this list: send the line "unsubscribe stable" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html