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

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

Hi Jani,

Sorry for late reply.

On 2012년 09월 27일 22:43, Jani Nikula wrote:
> On Thu, 27 Sep 2012, Seung-Woo Kim <sw0312.kim@samsung.com> wrote:
>> 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.
> 
> There's a reference leak bug [1] related to prime self import found by
> intel-gpu-tools tests. Without much thinking, I just applied this patch
> to see if it makes a difference. Instead, it now fails with:
> 
> prime_self_import: prime_self_import.c:61: check_bo: Assertion `ptr1'
> failed.
> 
> The assert is at [2].

I think it was possible to re-use a handle imported_buf list which is
already deleted, and it was fixed after Dave's commit "drm/prime: add
exported buffers to current fprivs imported buffer list (v2)".

> 
> I haven't looked into why this happens at all, so I'm just sharing this
> in case you guys find it helpful.

If you still have same assert, I think it will be very helpful to let me
know exactly where the assert occurs from 5 check_bo() in test
application in your link.

Thanks and Best Regards,
- Seung-Woo Kim

> 
> BR,
> Jani.
> 
> 
> 
> [1] https://bugs.freedesktop.org/show_bug.cgi?id=54111
> [2] http://cgit.freedesktop.org/xorg/app/intel-gpu-tools/tree/tests/prime_self_import.c#n61
> 
> 
>>
>> 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>
>> ---
>>  drivers/gpu/drm/exynos/exynos_drm_dmabuf.c |    5 +++++
>>  drivers/gpu/drm/i915/i915_gem_dmabuf.c     |    5 +++++
>>  drivers/gpu/drm/nouveau/nouveau_prime.c    |    1 +
>>  drivers/gpu/drm/radeon/radeon_prime.c      |    1 +
>>  drivers/staging/omapdrm/omap_gem_dmabuf.c  |    5 +++++
>>  5 files changed, 17 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
>> index ae13feb..b0897c9 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dmabuf.c
>> @@ -211,7 +211,12 @@ struct drm_gem_object *exynos_dmabuf_prime_import(struct drm_device *drm_dev,
>>  
>>  		/* is it from our device? */
>>  		if (obj->dev == drm_dev) {
>> +			/*
>> +			 * Importing dmabuf exported from out own gem increases
>> +			 * refcount on gem itself instead of f_count of dmabuf.
>> +			 */
>>  			drm_gem_object_reference(obj);
>> +			dma_buf_put(dma_buf);
>>  			return obj;
>>  		}
>>  	}
>> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> index aa308e1..32e6287 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
>> @@ -188,7 +188,12 @@ struct drm_gem_object *i915_gem_prime_import(struct drm_device *dev,
>>  		obj = dma_buf->priv;
>>  		/* is it from our device? */
>>  		if (obj->base.dev == dev) {
>> +			/*
>> +			 * Importing dmabuf exported from out own gem increases
>> +			 * refcount on gem itself instead of f_count of dmabuf.
>> +			 */
>>  			drm_gem_object_reference(&obj->base);
>> +			dma_buf_put(dma_buf);
>>  			return &obj->base;
>>  		}
>>  	}
>> diff --git a/drivers/gpu/drm/nouveau/nouveau_prime.c b/drivers/gpu/drm/nouveau/nouveau_prime.c
>> index a25cf2c..bb653c6 100644
>> --- a/drivers/gpu/drm/nouveau/nouveau_prime.c
>> +++ b/drivers/gpu/drm/nouveau/nouveau_prime.c
>> @@ -199,6 +199,7 @@ struct drm_gem_object *nouveau_gem_prime_import(struct drm_device *dev,
>>  		if (nvbo->gem) {
>>  			if (nvbo->gem->dev == dev) {
>>  				drm_gem_object_reference(nvbo->gem);
>> +				dma_buf_put(dma_buf);
>>  				return nvbo->gem;
>>  			}
>>  		}
>> diff --git a/drivers/gpu/drm/radeon/radeon_prime.c b/drivers/gpu/drm/radeon/radeon_prime.c
>> index 6bef46a..d344a3be 100644
>> --- a/drivers/gpu/drm/radeon/radeon_prime.c
>> +++ b/drivers/gpu/drm/radeon/radeon_prime.c
>> @@ -195,6 +195,7 @@ struct drm_gem_object *radeon_gem_prime_import(struct drm_device *dev,
>>  		bo = dma_buf->priv;
>>  		if (bo->gem_base.dev == dev) {
>>  			drm_gem_object_reference(&bo->gem_base);
>> +			dma_buf_put(dma_buf);
>>  			return &bo->gem_base;
>>  		}
>>  	}
>> diff --git a/drivers/staging/omapdrm/omap_gem_dmabuf.c b/drivers/staging/omapdrm/omap_gem_dmabuf.c
>> index 42728e0..5b50eb6 100644
>> --- a/drivers/staging/omapdrm/omap_gem_dmabuf.c
>> +++ b/drivers/staging/omapdrm/omap_gem_dmabuf.c
>> @@ -207,7 +207,12 @@ struct drm_gem_object * omap_gem_prime_import(struct drm_device *dev,
>>  		obj = buffer->priv;
>>  		/* is it from our device? */
>>  		if (obj->dev == dev) {
>> +			/*
>> +			 * Importing dmabuf exported from out own gem increases
>> +			 * refcount on gem itself instead of f_count of dmabuf.
>> +			 */
>>  			drm_gem_object_reference(obj);
>> +			dma_buf_put(buffer);
>>  			return obj;
>>  		}
>>  	}
>> -- 
>> 1.7.4.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 

-- 
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