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@xxxxxxxxxxx> >> Signed-off-by: Kyungmin.park <kyungmin.park@xxxxxxxxxxx> >> Cc: Inki Dae <inki.dae@xxxxxxxxxxx> >> Cc: Daniel Vetter <daniel.vetter@xxxxxxxx> >> Cc: Rob Clark <rob.clark@xxxxxxxxxx> >> Cc: Alex Deucher <alexander.deucher@xxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
