Hi Christian,
On 8/9 星期二 18:18, Christian König wrote:
Hi Jeffy,
Am 09.08.22 um 12:02 schrieb Chen Jeffy:
Hi Christian,
On 8/9 星期二 17:08, Christian König wrote:
Hi Jeffy,
Am 09.08.22 um 09:55 schrieb Christian König:
[SNIP]
So we are allowing GEM object to have multiple handles, and GEM
object could have at most one dma-buf, doesn't that means that
dma-buf could map to multiple handles?
No, at least not for the same GEM file private. That's the reason
why the rb is indexed by the dma_buf object and not the handle.
In other words the rb is so that you have exactly one handle for
each dma_buf in each file private.
I don't think so, because if user get multiple handles for the same
GEM obj and use drm_gem_prime_handle_to_fd() for those handles
Mhm, that works? This is illegal and should have been prevented
somehow.
At least I see the problem now. I'm just not sure how to fix it.
Your v2 patch indeed prevents leakage of the drm_prime_member for the
additional handles, but those shouldn't have been added in the first
place.
The issue is that with this we make it unpredictable which handle is
returned. E.g. if we have handle 2,5,7 it can be that because of
re-balancing the tree sometimes 2 and sometimes 5 is returned.
Maybe cache the latest returned handle in the obj(after
drm_gem_prime_fd_to_handle), and clear it when that handle been
deleted in drm_gem_handle_delete()?
That won't work. The handle is per fpriv, but the same object is used by
multiple fpriv instances. >
What we could maybe do is to prevent adding multiple lockup structures
when there is already one, but that's not something I can easily judge.
So maybe we need to protect that unique lookup structure been deleted
before deleting the last handle, and make the handle unique for GEM obj,
in case of that unique lookup's handle been deleted earlier that others?
How about adding a GEM obj rbtree too, and make drm_prime_member kref-ed?
So the
drm_prime_add_buf_handle/drm_gem_handle_create_tail/drm_gem_handle_delete
would be looking up drm_prime_member by GEM obj, then update dmabuf rb
and inc/dec drm_prime_member's kref,
drm_gem_prime_fd_to_handle/drm_gem_prime_handle_to_fd remain unchanged.
Daniel seems to be either very busy or on vacation otherwise he would
have chimed in by now.
Anyway, your patch seems to at least fix the of hand memory leak, so
feel free to add my rb to the v2 and push it to drm-misc-fixes for now.
Thanks,
Christian.
Something like:
drm_gem_prime_fd_to_handle
handle = drm_prime_lookup_buf_handle(buf)
obj = obj_from_handle(handle)
if !obj->primary_handle
obj->primary_handle = handle
return obj->primary_handle
Or maybe limit GEM obj with a single lifetime handle?
That's not really a good idea and breaks a couple of assumptions as
far as I know.
Ideas?
Thanks,
Christian.
