[Intel-gfx] [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap

[Chris Wilson]

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

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