Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

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 <chris@chris-wilson.co.uk>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com>
> Tested-by: Armin K. <krejzi@email.com> (v1)
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  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