[PATCH] drm/prime: keep a reference from the handle to exported dma-buf (v4)

[Dave Airlie]

On Thu, Apr 18, 2013 at 2:16 AM, Imre Deak <imre.deak at intel.com> wrote:
> On Wed, 2013-04-17 at 10:21 +0200, Daniel Vetter wrote:
>> On Wed, Apr 17, 2013 at 02:38:02PM +1000, Dave Airlie wrote:
>> > Currently we have a problem with this:
>> > 1. i915: create gem object
>> > 2. i915: export gem object to prime
>> > 3. radeon: import gem object
>> > 4. close prime fd
>> > 5. radeon: unref object
>> > 6. i915: unref object
>> >
>> > i915 has an imported object reference in its file priv, that isn't
>> > cleaned up properly until fd close. The reference gets added at step 2,
>> > but at step 6 we don't have enough info to clean it up.
>> >
>> > The solution is to take a reference on the dma-buf when we export it,
>> > and drop the reference when the gem handle goes away.
>> >
>> > So when we export a dma_buf from a gem object, we keep track of it
>> > with the handle, we take a reference to the dma_buf. When we close
>> > the handle (i.e. userspace is finished with the buffer), we drop
>> > the reference to the dma_buf, and it gets collected.
>> >
>> > This patch isn't meant to fix any other problem or bikesheds, and it doesn't
>> > fix any races with other scenarios.
>> >
>> > v1.1: move export symbol line back up.
>> >
>> > v2: okay I had to do a bit more, as the first patch showed a leak
>> > on one of my tests, that I found using the dma-buf debugfs support,
>> > the problem case is exporting a buffer twice with the same handle,
>> > we'd add another export handle for it unnecessarily, however
>> > we now fail if we try to export the same object with a different gem handle,
>> > however I'm not sure if that is a case I want to support, and I've
>> > gotten the code to WARN_ON if we hit something like that.
>> >
>> > v2.1: rebase this patch, write better commit msg.
>> > v3: cleanup error handling, track import vs export in linked list,
>> > these two patches were separate previously, but seem to work better
>> > like this.
>> > v4: danvet is correct, this code is no longer useful, since the buffer
>> > better exist, so remove it.
>> >
>> > Signed-off-by: Dave Airlie <airlied at redhat.com>
>>
>> Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
>>
>> On bikeshed review level I wonder whether we shouldn't just grab a
>> reference unconditonally when inserting it into the handle_to_fd lookup
>> lists. But I need to think through a few other cases and apparently the
>> self-import test is still a bit broken. So this is material for follow-up
>> patches.
>
> Yes, this is needed, otherwise closing the prime fd will leave stale
> pointers to the dma buf in the importer's lookup table. I've sent an
> update to igt/prime_self_test to intel-gfx for showing this.
>

Okay I've spent time on this today and yes I agree, so I'll post v5 of my patch
with that in it.

I'd like to also merge your other patch, it seems fine.

Dave.