* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 6:30 [PATCH] drm/prime: drop reference on imported dma-buf come from gem Seung-Woo Kim
@ 2012-09-27 6:52 ` Rob Clark
2012-09-27 7:14 ` 김승우
2012-09-27 13:43 ` Jani Nikula
` (2 subsequent siblings)
3 siblings, 1 reply; 15+ messages in thread
From: Rob Clark @ 2012-09-27 6:52 UTC (permalink / raw)
To: Seung-Woo Kim; +Cc: daniel.vetter, kyungmin.park, alexander.deucher, dri-devel
fwiw, I had a similar patch:
https://patchwork.kernel.org/patch/1229161/
although it was on top of some locking fixes from Daniel (which I
think are also needed):
https://patchwork.kernel.org/patch/1227251/
although that seemed to cause/trigger some explosions which I think
still need to be debugged..
BR,
-R
On Thu, Sep 27, 2012 at 8:30 AM, 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.
>
> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 6:52 ` Rob Clark
@ 2012-09-27 7:14 ` 김승우
2012-09-27 7:38 ` Rob Clark
0 siblings, 1 reply; 15+ messages in thread
From: 김승우 @ 2012-09-27 7:14 UTC (permalink / raw)
To: Rob Clark; +Cc: daniel.vetter, dri-devel, kyungmin.park, alexander.deucher
Hi Rob,
On 2012년 09월 27일 15:52, Rob Clark wrote:
> fwiw, I had a similar patch:
>
> https://patchwork.kernel.org/patch/1229161/
Yes, I already check your patch and even my patch's title is a bit from
your patch.
I thought locking issue blocks your patch, so I sent simple fixes on
current state. How do you think merging bug-fixes at first?
Best Regards,
- Seung-Woo Kim
>
> although it was on top of some locking fixes from Daniel (which I
> think are also needed):
>
> https://patchwork.kernel.org/patch/1227251/
>
> although that seemed to cause/trigger some explosions which I think
> still need to be debugged..
>
> BR,
> -R
>
> On Thu, Sep 27, 2012 at 8:30 AM, 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.
>>
>> 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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 7:14 ` 김승우
@ 2012-09-27 7:38 ` Rob Clark
0 siblings, 0 replies; 15+ messages in thread
From: Rob Clark @ 2012-09-27 7:38 UTC (permalink / raw)
To: sw0312.kim; +Cc: alexander.deucher, daniel.vetter, kyungmin.park, dri-devel
On Thu, Sep 27, 2012 at 9:14 AM, 김승우 <sw0312.kim@samsung.com> wrote:
> Hi Rob,
>
> On 2012년 09월 27일 15:52, Rob Clark wrote:
>> fwiw, I had a similar patch:
>>
>> https://patchwork.kernel.org/patch/1229161/
>
> Yes, I already check your patch and even my patch's title is a bit from
> your patch.
>
> I thought locking issue blocks your patch, so I sent simple fixes on
> current state. How do you think merging bug-fixes at first?
well, the lack of locking is a real problem too.. although I didn't
see quite what the locking changes could have to do with the crash
that Dave had seen. But I don't know if anyone has looked at it more,
and I only re-import my own buffers in omapdrm so I don't think I can
trigger the same scenario myself.
I guess if no one else has a chance to take at least a quick look at
the locking crash, maybe we should take this patch. But I would be
interested get the locking patch too. We are needing both patches on
our product kernels to fix some issues.
BR,
-R
> Best Regards,
> - Seung-Woo Kim
>
>>
>> although it was on top of some locking fixes from Daniel (which I
>> think are also needed):
>>
>> https://patchwork.kernel.org/patch/1227251/
>>
>> although that seemed to cause/trigger some explosions which I think
>> still need to be debugged..
>>
>> BR,
>> -R
>>
>> On Thu, Sep 27, 2012 at 8:30 AM, 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.
>>>
>>> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 6:30 [PATCH] drm/prime: drop reference on imported dma-buf come from gem Seung-Woo Kim
2012-09-27 6:52 ` Rob Clark
@ 2012-09-27 13:43 ` Jani Nikula
2012-11-03 7:59 ` 김승우
2012-11-15 3:52 ` [RESEND][PATCH] " Seung-Woo Kim
2012-12-18 6:30 ` [PATCH] " Dave Airlie
3 siblings, 1 reply; 15+ messages in thread
From: Jani Nikula @ 2012-09-27 13:43 UTC (permalink / raw)
To: Seung-Woo Kim, dri-devel, airlied
Cc: alexander.deucher, daniel.vetter, kyungmin.park, rob.clark
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 haven't looked into why this happens at all, so I'm just sharing this
in case you guys find it helpful.
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 13:43 ` Jani Nikula
@ 2012-11-03 7:59 ` 김승우
0 siblings, 0 replies; 15+ messages in thread
From: 김승우 @ 2012-11-03 7:59 UTC (permalink / raw)
To: Jani Nikula
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 6:30 [PATCH] drm/prime: drop reference on imported dma-buf come from gem Seung-Woo Kim
2012-09-27 6:52 ` Rob Clark
2012-09-27 13:43 ` Jani Nikula
@ 2012-11-15 3:52 ` Seung-Woo Kim
2012-11-19 8:40 ` Inki Dae
2012-11-19 10:27 ` Maarten Lankhorst
2012-12-18 6:30 ` [PATCH] " Dave Airlie
3 siblings, 2 replies; 15+ messages in thread
From: Seung-Woo Kim @ 2012-11-15 3:52 UTC (permalink / raw)
To: dri-devel, airlied
Cc: daniel.vetter, sw0312.kim, rob.clark, kyungmin.park,
alexander.deucher
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.
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
^ permalink raw reply related [flat|nested] 15+ messages in thread* Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-11-15 3:52 ` [RESEND][PATCH] " Seung-Woo Kim
@ 2012-11-19 8:40 ` Inki Dae
2012-11-19 10:27 ` Maarten Lankhorst
1 sibling, 0 replies; 15+ messages in thread
From: Inki Dae @ 2012-11-19 8:40 UTC (permalink / raw)
To: Seung-Woo Kim
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
[-- Attachment #1.1: Type: text/plain, Size: 5679 bytes --]
Hi All,
This patch has been tested with only Exynos driver so other maintainers may
need to test this.
Acked-by: Inki Dae <inki.dae@samsung.com>
2012/11/15 Seung-Woo Kim <sw0312.kim@samsung.com>
> 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.
>
> 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
>
[-- Attachment #1.2: Type: text/html, Size: 6907 bytes --]
[-- Attachment #2: Type: text/plain, Size: 159 bytes --]
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-11-15 3:52 ` [RESEND][PATCH] " Seung-Woo Kim
2012-11-19 8:40 ` Inki Dae
@ 2012-11-19 10:27 ` Maarten Lankhorst
2012-11-20 1:03 ` 김승우
1 sibling, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2012-11-19 10:27 UTC (permalink / raw)
To: Seung-Woo Kim
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
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 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.
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.
~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.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-11-19 10:27 ` Maarten Lankhorst
@ 2012-11-20 1:03 ` 김승우
2012-11-20 10:26 ` Maarten Lankhorst
0 siblings, 1 reply; 15+ messages in thread
From: 김승우 @ 2012-11-20 1:03 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: daniel.vetter, sw0312.kim, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-11-20 1:03 ` 김승우
@ 2012-11-20 10:26 ` Maarten Lankhorst
2012-11-21 5:28 ` 김승우
0 siblings, 1 reply; 15+ messages in thread
From: Maarten Lankhorst @ 2012-11-20 10:26 UTC (permalink / raw)
To: sw0312.kim
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
Op 20-11-12 02:03, 김승우 schreef:
> 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?
When the dma_buf is unreffed and refcount drops to 0, work is queued to free it.
Until then export_dma_buf member points to something, but refcount is 0 on it.
Importing to itself, then closing fd then re-export from the new place would probably trigger it.
>> 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.
I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet,
you no longer have a guarantee that the memory is still valid.
And of course after importing into a different process with its own drm namespace, how does
file_priv->prime.lock still protect serialization?
I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put
is called that is used for the reference inside export_dma_buf.
The release function should only release a reference to whatever backing memory is used.
>> 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.
The whole refcounting is a circular mess, sadly with no easy solution. :/
It seems to me that using gem reference counts is solving this problem at the wrong layer.
A secondary type of reference count is needed for the underlying memory backing.
For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one,
so that the gem object can be destroyed and release its reference on the dma_buf.
WIthout a secondary refcount you end up in a position where you can't reliably and race free unref
the gem object and dma_buf at the same time, since they're both independent interfaces with possibly
different lifetimes.
It would really help if there were hard rules on lifetime of the export_dma_buf member,
until then we're just patching one broken thing with another.
~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RESEND][PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-11-20 10:26 ` Maarten Lankhorst
@ 2012-11-21 5:28 ` 김승우
0 siblings, 0 replies; 15+ messages in thread
From: 김승우 @ 2012-11-21 5:28 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
On 2012년 11월 20일 19:26, Maarten Lankhorst wrote:
> Op 20-11-12 02:03, 김승우 schreef:
>> 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?
> When the dma_buf is unreffed and refcount drops to 0, work is queued to free it.
>
> Until then export_dma_buf member points to something, but refcount is 0 on it.
>
> Importing to itself, then closing fd then re-export from the new place would probably trigger it.
>
Ok, I understood about issue from delayed fput also in your below
comment. Anyway, IMO, it is already on current drm prime.
>>> 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.
> I'm fairly sure that when refcount drops to 0, even though the memory may not be freed yet,
> you no longer have a guarantee that the memory is still valid.
I missed that delayed fput is triggered after recount drops to 0, and
now I understood it can cause invalid access.
>
> And of course after importing into a different process with its own drm namespace, how does
> file_priv->prime.lock still protect serialization?
>
> I don't see any hierarchy either. export_dma_buf needs to be set to NULL before the dma_buf_put
> is called that is used for the reference inside export_dma_buf.
>
> The release function should only release a reference to whatever backing memory is used.
>
>>> 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.
> The whole refcounting is a circular mess, sadly with no easy solution. :/
>
> It seems to me that using gem reference counts is solving this problem at the wrong layer.
> A secondary type of reference count is needed for the underlying memory backing.
>
> For those who use ttm, I would recommend that the dma_buf increases refcount on ttm_bo by one,
> so that the gem object can be destroyed and release its reference on the dma_buf.
>
> WIthout a secondary refcount you end up in a position where you can't reliably and race free unref
> the gem object and dma_buf at the same time, since they're both independent interfaces with possibly
> different lifetimes.
If there is no checking whether dma_buf is came from driver own
exporter, gem_object is newly allocated and refcount of it can be a
secondary refcount at least form mermoy leak issue. So I mentioned about
removing check for exporter.
But I prefer processing re-import as gem_open without considering
dma-buf as current implementation.
>
> It would really help if there were hard rules on lifetime of the export_dma_buf member,
> until then we're just patching one broken thing with another.
Issue about memory leak of re-import was also reported on i915 and there
was Rob's patch set, but other locking issue blocked the patch.
I'm not sure all issue can be solved at once and someone is handling
this at the moment.
Best Regards,
- Seung-Woo Kim
>
>
> ~Maarten
>
>
--
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-09-27 6:30 [PATCH] drm/prime: drop reference on imported dma-buf come from gem Seung-Woo Kim
` (2 preceding siblings ...)
2012-11-15 3:52 ` [RESEND][PATCH] " Seung-Woo Kim
@ 2012-12-18 6:30 ` Dave Airlie
2012-12-18 6:59 ` 김승우
3 siblings, 1 reply; 15+ messages in thread
From: Dave Airlie @ 2012-12-18 6:30 UTC (permalink / raw)
To: Seung-Woo Kim
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
On Thu, Sep 27, 2012 at 4:30 PM, 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.
Okay its taken me a while to circle around and get back to this, but
yes I admit this is needed, but I hate implementing it like this
But I think I'll push it and work out a cleaner solution, I should
also go back and look at the older patches.
Dave.
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-12-18 6:30 ` [PATCH] " Dave Airlie
@ 2012-12-18 6:59 ` 김승우
2012-12-20 0:48 ` Dave Airlie
0 siblings, 1 reply; 15+ messages in thread
From: 김승우 @ 2012-12-18 6:59 UTC (permalink / raw)
To: Dave Airlie
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
Hi Dave,
On 2012년 12월 18일 15:30, Dave Airlie wrote:
> On Thu, Sep 27, 2012 at 4:30 PM, 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.
>
> Okay its taken me a while to circle around and get back to this, but
> yes I admit this is needed, but I hate implementing it like this
>
> But I think I'll push it and work out a cleaner solution, I should
> also go back and look at the older patches.
I want to also report some strange thing in dma-buf prime export.
In drm_prime_handle_to_fd_ioctl(), flags is cleared to only support
DRM_CLOEXEC but in gem_prime_export() callbacks of each driver, it uses
0600 as flags for dma_buf_export() like following.
return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, 0600);
Best Regards,
- Seung-Woo Kim
>
> Dave.
>
--
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/prime: drop reference on imported dma-buf come from gem
2012-12-18 6:59 ` 김승우
@ 2012-12-20 0:48 ` Dave Airlie
0 siblings, 0 replies; 15+ messages in thread
From: Dave Airlie @ 2012-12-20 0:48 UTC (permalink / raw)
To: sw0312.kim
Cc: daniel.vetter, dri-devel, rob.clark, kyungmin.park,
alexander.deucher
> In drm_prime_handle_to_fd_ioctl(), flags is cleared to only support
> DRM_CLOEXEC but in gem_prime_export() callbacks of each driver, it uses
> 0600 as flags for dma_buf_export() like following.
>
> return dma_buf_export(obj, &i915_dmabuf_ops, obj->base.size, 0600);
Oops, nice catch, radeon and nouveau did the correct thing here, I'll
send a patch to fix that.
Dave.
^ permalink raw reply [flat|nested] 15+ messages in thread