| Message ID | 1375866908-5000-3-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67391 > --- > drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > index 63ee1a9..0bf3d51 100644 > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c > @@ -85,9 +85,13 @@ 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; > + > dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); > sg_free_table(sg); > kfree(sg); > + > + i915_gem_object_unpin_pages(obj); > } > > static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf) > -- > 1.8.3.2 >
On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: > This fixes a WARN in i915_gem_free_object when the > obj->pages_pin_count isn't 0. > > Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Papers over the WARN with iffy locking. Not all callers hold struct_mutex, right? Worse some do, some don't... What's the long term plan here? -Chris
On Wed, Aug 7, 2013 at 11:29 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Wed, Aug 07, 2013 at 11:15:07AM +0200, Daniel Vetter wrote: >> This fixes a WARN in i915_gem_free_object when the >> obj->pages_pin_count isn't 0. >> >> Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Papers over the WARN with iffy locking. Not all callers hold > struct_mutex, right? Worse some do, some don't... Oops, that needs mutex locking here. I've checked the code and none of the callers here should ever hold our own dev->struct_mutex (due to the self-import checks we bypass dma-buf for our own objects) so no immediate deadlock. But it's easy to create circles and piss off lockded ofc. > What's the long term plan here? Per-bo locking with ww mutexes. Locking is done by the callers of map/unmap, sonce only those can properly do the ww locking dance on all relevent buffers of a batch upfront. It's going to be fun ;-) Looking closer I've also spotted that our map_buf callback has a call to i915_mutex_lock_interruptible, which means the map can fail with -EINTR. Currently it seems unspec'ed whether map is allowed to do that, but current callers certainly can't cope with this. I'll throw a 2nd patch on top. -Daniel
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c index 63ee1a9..0bf3d51 100644 --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c @@ -85,9 +85,13 @@ 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; + dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir); sg_free_table(sg); kfree(sg); + + i915_gem_object_unpin_pages(obj); } static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
This fixes a WARN in i915_gem_free_object when the obj->pages_pin_count isn't 0. Reported-by: Maarten Lankhorst <maarten.lankhorst@canonical.com> Cc: Maarten Lankhorst <maarten.lankhorst@canonical.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 ++++ 1 file changed, 4 insertions(+)