Re: [PATCH] drm/etnaviv: fix memory leak when mapping prime imported buffers
Hi Lucas, > Am Mittwoch, den 20.05.2020, 12:10 +0200 schrieb Martin Fuzzey: > What's the use-case where you did hit this issue? mmap'ing of imported > buffers through the etnaviv DRM device is currently not well defined > and I was pondering the idea of forbidding it completely by not > returning a mmap offset for those objects. > I hit this on Android 8 (on i.MX6 using mesa 20.0.6 with gbm gralloc and drm hwcomposer) and had a memory leak every time an activity was started. I'm not sure exactly why but Android does a gralloc.lock() and gralloc.unlock() on every activity startup. Those map and unmap the buffer. Under Android (at least in 8+) the actual graphics buffer allocations are done by a dedicated process (android.hardware.graphics.allocator@2.0-service) because it uses a "binderized HAL" for the allocation [https://source.android.com/devices/architecture/hal-types] This means that buffers are *always* imported (though they are usually only mmaped for SW rendering or screen shots). Regards, Martin
Re: [PATCH] drm/etnaviv: fix memory leak when mapping prime imported buffers
Hi Martin, Am Mittwoch, den 20.05.2020, 12:10 +0200 schrieb Martin Fuzzey: > When using mmap() on a prime imported buffer allocated by a > different driver (such as imx-drm) the later munmap() does > not correctly decrement the refcount of the original enaviv_gem_object, > leading to a leak. > > Signed-off-by: Martin Fuzzey > Cc: sta...@vger.kernel.org What's the use-case where you did hit this issue? mmap'ing of imported buffers through the etnaviv DRM device is currently not well defined and I was pondering the idea of forbidding it completely by not returning a mmap offset for those objects. Regards, Lucas > --- > drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 20 +++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > index f24dd21..28a01b8 100644 > --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c > @@ -93,7 +93,25 @@ static void *etnaviv_gem_prime_vmap_impl(struct > etnaviv_gem_object *etnaviv_obj) > static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, > struct vm_area_struct *vma) > { > - return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); > + int ret; > + > + ret = dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); > + > + /* drm_gem_mmap_obj() has already been called before this function > + * and has incremented our refcount, expecting it to be decremented > + * on unmap() via drm_gem_vm_close(). > + * However dma_buf_mmap() invokes drm_gem_cma_prime_mmap() > + * that ends up updating the vma->vma_private_data to point to the > + * dma_buf's gem object. > + * Hence our gem object here will not have its refcount decremented > + * when userspace does unmap(). > + * So decrement the refcount here to avoid a memory leak if the dma > + * buf mapping was successful. > + */ > + if (!ret) > + drm_gem_object_put_unlocked(_obj->base); > + > + return ret; > } > > static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
[PATCH] drm/etnaviv: fix memory leak when mapping prime imported buffers
When using mmap() on a prime imported buffer allocated by a different driver (such as imx-drm) the later munmap() does not correctly decrement the refcount of the original enaviv_gem_object, leading to a leak. Signed-off-by: Martin Fuzzey Cc: sta...@vger.kernel.org --- drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c | 20 +++- 1 file changed, 19 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c index f24dd21..28a01b8 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c @@ -93,7 +93,25 @@ static void *etnaviv_gem_prime_vmap_impl(struct etnaviv_gem_object *etnaviv_obj) static int etnaviv_gem_prime_mmap_obj(struct etnaviv_gem_object *etnaviv_obj, struct vm_area_struct *vma) { - return dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); + int ret; + + ret = dma_buf_mmap(etnaviv_obj->base.dma_buf, vma, 0); + + /* drm_gem_mmap_obj() has already been called before this function +* and has incremented our refcount, expecting it to be decremented +* on unmap() via drm_gem_vm_close(). +* However dma_buf_mmap() invokes drm_gem_cma_prime_mmap() +* that ends up updating the vma->vma_private_data to point to the +* dma_buf's gem object. +* Hence our gem object here will not have its refcount decremented +* when userspace does unmap(). +* So decrement the refcount here to avoid a memory leak if the dma +* buf mapping was successful. +*/ + if (!ret) + drm_gem_object_put_unlocked(_obj->base); + + return ret; } static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = { -- 1.9.1