[Intel-gfx] [PATCH v3 2/2] drm/i915: On fb alloc failure, unref gem object where it gets refed

[Chris Wilson]

On Sun, Jul 05, 2015 at 06:49:02PM +0200, Lukas Wunner wrote:
> Hi Chris,
> 
> thank you for the quick response (on a weekend no less).
> 
> 
> On Sat, Jul 04, 2015 at 01:31:48PM +0100, Chris Wilson wrote:
> > > -	return intel_framebuffer_create(dev, &mode_cmd, obj);
> > > +	fb = intel_framebuffer_create(dev, &mode_cmd, obj);
> > > +	if (IS_ERR(fb))
> > > +		drm_gem_object_unreference(&obj->base);
> > 
> > This needs to be drm_gem_object_unreference_unlocked().
> 
> You're absolutely right, thanks for pointing this out.
> I'm posting a rectified v4 right now.
> 
> 
> > It's much simpler if you just document this as consuming the
> > obj reference.
> 
> Yes but I believe that is what Ville took exception to.
> 
> If you guys all agree that documenting this is sufficient then
> you can just merge Tvrtko's v2. The rationale of the v3 + v4
> I've submitted is to offer an alternative in the hope of pushing
> this forward.
> 
> 
> > If you want to fix it, you have to move the struct_mutex into
> > the caller i.e. eliminate intel_framebuffer_create() and call
> > __intel_framebuffer_create().
> 
> Hm, I don't understand. The (locking) intel_framebuffer_create
> is used by intel_framebuffer_create_for_mode as well as
> intel_user_framebuffer_create.
> 
> The (non-locking) __intel_framebuffer_create is used by
> intelfb_alloc. So it seems both are needed. Daniel added
> __intel_framebuffer_create with a8bb6818270c ("drm/i915:
> Fix error path leak in fbdev fb allocation"). Incidentally
> this is also the commit that introduced the double unref. :-)
> 
> We could eliminate the (non-locking) __intel_framebuffer_create
> however by briefly unlocking struct_mutex in intelfb_alloc after
> i915_gem_alloc_object and then relocking before
> intel_pin_and_fence_fb_obj (this is on top of Tvrtko's patch
> which moves the locking from intelfb_create to intelfb_alloc).

I mean, there will only be the non-locking variant, the mutex is moved
to the caller.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre