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

[Lukas Wunner <lukas@wunner.de>]

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).

Best regards,

Lukas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx