Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem

[김승우 <sw0312.kim@samsung.com>]

Hi Maarten,

On 2012년 11월 19일 19:27, Maarten Lankhorst wrote:
> Op 15-11-12 04:52, Seung-Woo Kim schreef:
>> Increasing ref counts of both dma-buf and gem for imported dma-buf
>> come from gem makes memory leak. release function of dma-buf cannot
>> be called because f_count of dma-buf increased by importing gem and
>> gem ref count cannot be decrease because of exported dma-buf.
>>
>> So I add dma_buf_put() for imported gem come from its own gem into
>> each drivers having prime_import and prime_export capabilities. With
>> this, only gem ref count is increased if importing gem exported from
>> gem of same driver.
>>
>> Signed-off-by: Seung-Woo Kim <sw0312.kim@samsung.com>
>> Signed-off-by: Kyungmin.park <kyungmin.park@samsung.com>
>> Cc: Inki Dae <inki.dae@samsung.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Rob Clark <rob.clark@linaro.org>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> ---
>> Mmap failiure in i915 was reported by Jani, and I think it was fixed
>> with Dave's commit "drm/prime: add exported buffers to current fprivs
>> imported buffer list (v2)", so I resend this patch.
>>
>> I can send exynos only, but this issue is common in all drm driver
>> having both prime inport and export, IMHO, it's better to send for
>> all drivers.
> I fear that  this won't solve the issue completely and keeps open a small race.

I don't believe my patch can fix all issue related with gem prime
completely. But it seems that it can solve unrecoverable memory leak
caused by re-importing GEM. Anyway, can you give me an example of other
race issue?

> 
> I don't like the current destruction path either. It really looks like export_dma_buf
> should be unset when the exported buffer is closed in the original file. Anything else is racy.
> Especially setting export_dma_buf to NULL won't work with the current delayed fput semantics.

I cannot figure out all aspect of delayed fput, but until delayed work
is called, dma_buf is not released so export_dma_buf is valid.
Considering this, I can't find possible issue of delayed fput.

> 
> So to me it seems instead we should do something like this:
> 
> - dmabuf_release callback is a noop, no ref is held by the dma-buf.
> - attach and detach ops increase reference by 1*.
> 
> - when the buffer is exported, export_dma_buf is set by core code and kept around
>   alive until the gem object is closed.
> 
> - dma_buf_put is not called by import function. This reference removed as part
>   of gem object close instead.

Considering re-import, where drm file priv is different from exporter's
file priv, attach and detach are not called because gem object is reused.

How do you think remove checking whether dma_buf is came from driver own
exporter? Because without this checking, gem object is newly created,
and then re-import issue will disappear.

Thanks and Regards,
- Seung-Woo Kim

> 
> ~Maarten
> 
> *) Lockdep will rightfully hate this as it reopens a locking inversion, some solution for that is needed.
> 
> Maybe a post detach callback for dma-buf with all locks dropped would be best here?
> Other way would be to schedule delayed work with the sole purpose of unreffing in the detach op,
> similar to how atomic_dec_and_mutex_lock works.
> 
> 

-- 
Seung-Woo Kim
Samsung Software R&D Center
--

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel