diff mbox

[03/17] drm/i915: Pin pages whilst mapping the dma-buf

Message ID 1377557469-4078-4-git-send-email-rodrigo.vivi@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Aug. 26, 2013, 10:50 p.m. UTC 
From: Chris Wilson <chris@chris-wilson.co.uk>

As we attempt to kmalloc after calling get_pages, there is a possibility
that the shrinker may reap the pages we just acquired. To prevent this
we need to increment the pages_pin_count early, so rearrange the code
and error paths to make it so.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_dmabuf.c | 41 ++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 19 deletions(-)

Comments

Lespiau, Damien Aug. 30, 2013, 2:27 p.m. UTC | #1 
On Mon, Aug 26, 2013 at 07:50:55PM -0300, Rodrigo Vivi wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> As we attempt to kmalloc after calling get_pages, there is a possibility
> that the shrinker may reap the pages we just acquired. To prevent this
> we need to increment the pages_pin_count early, so rearrange the code
> and error paths to make it so.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>
Daniel Vetter Sept. 2, 2013, 6:04 a.m. UTC | #2 
On Fri, Aug 30, 2013 at 03:27:47PM +0100, Damien Lespiau wrote:
> On Mon, Aug 26, 2013 at 07:50:55PM -0300, Rodrigo Vivi wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > As we attempt to kmalloc after calling get_pages, there is a possibility
> > that the shrinker may reap the pages we just acquired. To prevent this
> > we need to increment the pages_pin_count early, so rearrange the code
> > and error paths to make it so.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

I've merged the three reviewed patches to dinq, thanks.
-Daniel
> 
> -- 
> Damien
> 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_dmabuf.c | 41 ++++++++++++++++++----------------
> >  1 file changed, 22 insertions(+), 19 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > index e918b05..7d5752f 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> > @@ -42,27 +42,24 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
> >  
> >  	ret = i915_mutex_lock_interruptible(obj->base.dev);
> >  	if (ret)
> > -		return ERR_PTR(ret);
> > +		goto err;
> >  
> >  	ret = i915_gem_object_get_pages(obj);
> > -	if (ret) {
> > -		st = ERR_PTR(ret);
> > -		goto out;
> > -	}
> > +	if (ret)
> > +		goto err_unlock;
> > +
> > +	i915_gem_object_pin_pages(obj);
> >  
> >  	/* Copy sg so that we make an independent mapping */
> >  	st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
> >  	if (st == NULL) {
> > -		st = ERR_PTR(-ENOMEM);
> > -		goto out;
> > +		ret = -ENOMEM;
> > +		goto err_unpin;
> >  	}
> >  
> >  	ret = sg_alloc_table(st, obj->pages->nents, GFP_KERNEL);
> > -	if (ret) {
> > -		kfree(st);
> > -		st = ERR_PTR(ret);
> > -		goto out;
> > -	}
> > +	if (ret)
> > +		goto err_free;
> >  
> >  	src = obj->pages->sgl;
> >  	dst = st->sgl;
> > @@ -73,17 +70,23 @@ static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
> >  	}
> >  
> >  	if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {
> > -		sg_free_table(st);
> > -		kfree(st);
> > -		st = ERR_PTR(-ENOMEM);
> > -		goto out;
> > +		ret =-ENOMEM;
> > +		goto err_free_sg;
> >  	}
> >  
> > -	i915_gem_object_pin_pages(obj);
> > -
> > -out:
> >  	mutex_unlock(&obj->base.dev->struct_mutex);
> >  	return st;
> > +
> > +err_free_sg:
> > +	sg_free_table(st);
> > +err_free:
> > +	kfree(st);
> > +err_unpin:
> > +	i915_gem_object_unpin_pages(obj);
> > +err_unlock:
> > +	mutex_unlock(&obj->base.dev->struct_mutex);
> > +err:
> > +	return ERR_PTR(ret);
> >  }
> >  
> >  static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
> > -- 
> > 1.8.1.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index e918b05..7d5752f 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -42,27 +42,24 @@  static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
 
 	ret = i915_mutex_lock_interruptible(obj->base.dev);
 	if (ret)
-		return ERR_PTR(ret);
+		goto err;
 
 	ret = i915_gem_object_get_pages(obj);
-	if (ret) {
-		st = ERR_PTR(ret);
-		goto out;
-	}
+	if (ret)
+		goto err_unlock;
+
+	i915_gem_object_pin_pages(obj);
 
 	/* Copy sg so that we make an independent mapping */
 	st = kmalloc(sizeof(struct sg_table), GFP_KERNEL);
 	if (st == NULL) {
-		st = ERR_PTR(-ENOMEM);
-		goto out;
+		ret = -ENOMEM;
+		goto err_unpin;
 	}
 
 	ret = sg_alloc_table(st, obj->pages->nents, GFP_KERNEL);
-	if (ret) {
-		kfree(st);
-		st = ERR_PTR(ret);
-		goto out;
-	}
+	if (ret)
+		goto err_free;
 
 	src = obj->pages->sgl;
 	dst = st->sgl;
@@ -73,17 +70,23 @@  static struct sg_table *i915_gem_map_dma_buf(struct dma_buf_attachment *attachme
 	}
 
 	if (!dma_map_sg(attachment->dev, st->sgl, st->nents, dir)) {
-		sg_free_table(st);
-		kfree(st);
-		st = ERR_PTR(-ENOMEM);
-		goto out;
+		ret =-ENOMEM;
+		goto err_free_sg;
 	}
 
-	i915_gem_object_pin_pages(obj);
-
-out:
 	mutex_unlock(&obj->base.dev->struct_mutex);
 	return st;
+
+err_free_sg:
+	sg_free_table(st);
+err_free:
+	kfree(st);
+err_unpin:
+	i915_gem_object_unpin_pages(obj);
+err_unlock:
+	mutex_unlock(&obj->base.dev->struct_mutex);
+err:
+	return ERR_PTR(ret);
 }
 
 static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,