From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [PATCH 1/2] drm/i915: unpin backing storage in dmabuf_unmap Date: Wed, 7 Aug 2013 20:50:20 -0400 Message-ID: <20130808005020.GJ2810@phenom.dumpdata.com> References: <1375870173-20269-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1375870173-20269-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development , DRI Development List-Id: dri-devel@lists.freedesktop.org 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