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; } }
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
Hi Rob,
On 2012년 09월 27일 15:52, Rob Clark wrote:
fwiw, I had a similar patch:
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
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:
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
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_i...
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
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_i...
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
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; } }
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 gemincreases
* refcount on gem itself instead of f_count ofdmabuf.
*/ 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 gemincreases
* refcount on gem itself instead of f_count ofdmabuf.
*/ 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 gemincreases
* refcount on gem itself instead of f_count ofdmabuf.
*/ 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
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.
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.
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
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
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.
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.
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.
dri-devel@lists.freedesktop.org
-
Dave Airlie -
Inki Dae -
Jani Nikula -
Maarten Lankhorst -
Rob Clark -
Seung-Woo Kim -
김승우