[PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
On Wed, Aug 07, 2013 at 08:50:20PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote:
> > This fixes a WARN in i915_gem_free_object when the
> > obj->pages_pin_count isn't 0.
> >
> > v2: Add locking to unmap, noticed by Chris Wilson. Note that even
> > though we call unmap with our own dev->struct_mutex held that won't
> > result in an immediate deadlock since we never go through the dma_buf
> > interfaces for our own, reimported buffers. But it's still easy to
> > blow up and anger lockdep, but that's already the case with our ->map
> > implementation. Fixing this for real will involve per dma-buf ww mutex
> > locking by the callers. And lots of fun. So go with the duct-tape
> > approach for now.
> >
> > Cc: Chris Wilson
> > Reported-by: Maarten Lankhorst
> > Cc: Maarten Lankhorst
> > Tested-by: Armin K. (v1)
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > index 63ee1a9..f7e1682 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct
> > dma_buf_attachment *attachment,
> >struct sg_table *sg,
> >enum dma_data_direction dir)
> > {
> > + struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> > +
> > + mutex_lock(&obj->base.dev->struct_mutex);
> > +
> > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
> > sg_free_table(sg);
> > kfree(sg);
> > +
> > + i915_gem_object_unpin_pages(obj);
>
> I am curious - would it logic of first unpinning, and then doing the
> dma_unmap_sg
> make more sense? As in, in the map path we do:
>
> dma_map
> pin
Actually this is the wrong way around, and is a potential issue.
Hindsight is a powerful tool.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
[PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote:
> This fixes a WARN in i915_gem_free_object when the
> obj->pages_pin_count isn't 0.
>
> v2: Add locking to unmap, noticed by Chris Wilson. Note that even
> though we call unmap with our own dev->struct_mutex held that won't
> result in an immediate deadlock since we never go through the dma_buf
> interfaces for our own, reimported buffers. But it's still easy to
> blow up and anger lockdep, but that's already the case with our ->map
> implementation. Fixing this for real will involve per dma-buf ww mutex
> locking by the callers. And lots of fun. So go with the duct-tape
> approach for now.
>
> Cc: Chris Wilson
> Reported-by: Maarten Lankhorst
> Cc: Maarten Lankhorst
> Tested-by: Armin K. (v1)
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 63ee1a9..f7e1682 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct
> dma_buf_attachment *attachment,
> struct sg_table *sg,
> enum dma_data_direction dir)
> {
> + struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> +
> + mutex_lock(&obj->base.dev->struct_mutex);
> +
> dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
> sg_free_table(sg);
> kfree(sg);
> +
> + i915_gem_object_unpin_pages(obj);
I am curious - would it logic of first unpinning, and then doing the
dma_unmap_sg
make more sense? As in, in the map path we do:
dma_map
pin
And in here you do the same:
dma_unmap
unpin
But I would have thought that on a unroll you would do it in reverse
order, so:
unpin
dma_unmap
> +
> + mutex_unlock(&obj->base.dev->struct_mutex);
> }
>
> static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> --
> 1.8.3.2
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
On Wed, Aug 07, 2013 at 08:50:20PM -0400, Konrad Rzeszutek Wilk wrote:
> On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote:
> > This fixes a WARN in i915_gem_free_object when the
> > obj->pages_pin_count isn't 0.
> >
> > v2: Add locking to unmap, noticed by Chris Wilson. Note that even
> > though we call unmap with our own dev->struct_mutex held that won't
> > result in an immediate deadlock since we never go through the dma_buf
> > interfaces for our own, reimported buffers. But it's still easy to
> > blow up and anger lockdep, but that's already the case with our ->map
> > implementation. Fixing this for real will involve per dma-buf ww mutex
> > locking by the callers. And lots of fun. So go with the duct-tape
> > approach for now.
> >
> > Cc: Chris Wilson
> > Reported-by: Maarten Lankhorst
> > Cc: Maarten Lankhorst
> > Tested-by: Armin K. (v1)
> > Signed-off-by: Daniel Vetter
> > ---
> > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > index 63ee1a9..f7e1682 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct
> > dma_buf_attachment *attachment,
> >struct sg_table *sg,
> >enum dma_data_direction dir)
> > {
> > + struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> > +
> > + mutex_lock(&obj->base.dev->struct_mutex);
> > +
> > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
> > sg_free_table(sg);
> > kfree(sg);
> > +
> > + i915_gem_object_unpin_pages(obj);
>
> I am curious - would it logic of first unpinning, and then doing the
> dma_unmap_sg
> make more sense? As in, in the map path we do:
>
> dma_map
> pin
Actually this is the wrong way around, and is a potential issue.
Hindsight is a powerful tool.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
On Wed, Aug 07, 2013 at 12:09:32PM +0200, Daniel Vetter wrote:
> This fixes a WARN in i915_gem_free_object when the
> obj->pages_pin_count isn't 0.
>
> v2: Add locking to unmap, noticed by Chris Wilson. Note that even
> though we call unmap with our own dev->struct_mutex held that won't
> result in an immediate deadlock since we never go through the dma_buf
> interfaces for our own, reimported buffers. But it's still easy to
> blow up and anger lockdep, but that's already the case with our ->map
> implementation. Fixing this for real will involve per dma-buf ww mutex
> locking by the callers. And lots of fun. So go with the duct-tape
> approach for now.
>
> Cc: Chris Wilson
> Reported-by: Maarten Lankhorst
> Cc: Maarten Lankhorst
> Tested-by: Armin K. (v1)
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
> 1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 63ee1a9..f7e1682 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct
> dma_buf_attachment *attachment,
> struct sg_table *sg,
> enum dma_data_direction dir)
> {
> + struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
> +
> + mutex_lock(&obj->base.dev->struct_mutex);
> +
> dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
> sg_free_table(sg);
> kfree(sg);
> +
> + i915_gem_object_unpin_pages(obj);
I am curious - would it logic of first unpinning, and then doing the
dma_unmap_sg
make more sense? As in, in the map path we do:
dma_map
pin
And in here you do the same:
dma_unmap
unpin
But I would have thought that on a unroll you would do it in reverse
order, so:
unpin
dma_unmap
> +
> + mutex_unlock(&obj->base.dev->struct_mutex);
> }
>
> static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
> --
> 1.8.3.2
>
> ___
> 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
[PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
Op 07-08-13 12:09, Daniel Vetter schreef: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > v2: Add locking to unmap, noticed by Chris Wilson. Note that even > though we call unmap with our own dev->struct_mutex held that won't > result in an immediate deadlock since we never go through the dma_buf > interfaces for our own, reimported buffers. But it's still easy to > blow up and anger lockdep, but that's already the case with our ->map > implementation. Fixing this for real will involve per dma-buf ww mutex > locking by the callers. And lots of fun. So go with the duct-tape > approach for now. > > Cc: Chris Wilson > Reported-by: Maarten Lankhorst > Cc: Maarten Lankhorst > Tested-by: Armin K. (v1) > Signed-off-by: Daniel Vetter > Acked, this was my original patch to solve the issue. I want to note that locking struct_mutex here will break lockdep, but it's a problem in drm, not this patch. ~Maarten
[PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
This fixes a WARN in i915_gem_free_object when the
obj->pages_pin_count isn't 0.
v2: Add locking to unmap, noticed by Chris Wilson. Note that even
though we call unmap with our own dev->struct_mutex held that won't
result in an immediate deadlock since we never go through the dma_buf
interfaces for our own, reimported buffers. But it's still easy to
blow up and anger lockdep, but that's already the case with our ->map
implementation. Fixing this for real will involve per dma-buf ww mutex
locking by the callers. And lots of fun. So go with the duct-tape
approach for now.
Cc: Chris Wilson
Reported-by: Maarten Lankhorst
Cc: Maarten Lankhorst
Tested-by: Armin K. (v1)
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 63ee1a9..f7e1682 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment
*attachment,
struct sg_table *sg,
enum dma_data_direction dir)
{
+ struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
+
+ mutex_lock(&obj->base.dev->struct_mutex);
+
dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
sg_free_table(sg);
kfree(sg);
+
+ i915_gem_object_unpin_pages(obj);
+
+ mutex_unlock(&obj->base.dev->struct_mutex);
}
static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
--
1.8.3.2
Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
Op 07-08-13 12:09, Daniel Vetter schreef: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > v2: Add locking to unmap, noticed by Chris Wilson. Note that even > though we call unmap with our own dev->struct_mutex held that won't > result in an immediate deadlock since we never go through the dma_buf > interfaces for our own, reimported buffers. But it's still easy to > blow up and anger lockdep, but that's already the case with our ->map > implementation. Fixing this for real will involve per dma-buf ww mutex > locking by the callers. And lots of fun. So go with the duct-tape > approach for now. > > Cc: Chris Wilson > Reported-by: Maarten Lankhorst > Cc: Maarten Lankhorst > Tested-by: Armin K. (v1) > Signed-off-by: Daniel Vetter > Acked, this was my original patch to solve the issue. I want to note that locking struct_mutex here will break lockdep, but it's a problem in drm, not this patch. ~Maarten ___ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap
This fixes a WARN in i915_gem_free_object when the
obj->pages_pin_count isn't 0.
v2: Add locking to unmap, noticed by Chris Wilson. Note that even
though we call unmap with our own dev->struct_mutex held that won't
result in an immediate deadlock since we never go through the dma_buf
interfaces for our own, reimported buffers. But it's still easy to
blow up and anger lockdep, but that's already the case with our ->map
implementation. Fixing this for real will involve per dma-buf ww mutex
locking by the callers. And lots of fun. So go with the duct-tape
approach for now.
Cc: Chris Wilson
Reported-by: Maarten Lankhorst
Cc: Maarten Lankhorst
Tested-by: Armin K. (v1)
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_gem_dmabuf.c | 8
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 63ee1a9..f7e1682 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -85,9 +85,17 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment
*attachment,
struct sg_table *sg,
enum dma_data_direction dir)
{
+ struct drm_i915_gem_object *obj = attachment->dmabuf->priv;
+
+ mutex_lock(&obj->base.dev->struct_mutex);
+
dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
sg_free_table(sg);
kfree(sg);
+
+ i915_gem_object_unpin_pages(obj);
+
+ mutex_unlock(&obj->base.dev->struct_mutex);
}
static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
--
1.8.3.2
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel
