vmap consolidation

vmap consolidation

[Chris Wilson]

We have a couple of pieces of code that wish to take further advantange
of prolonged vmappings: execlists (ringbuffers), the cmdparser and the GuC
(workqueues). This series refactors the whole-object vmapping code and
caches it on the drm_i915_gem_object until it is released along with the
object's pages (i.e. under memory pressure or at the end of the object's
lifetime). This was stalled until we had a notifier in place from the
mm/vmalloc for when we ran out of vmalloc arena (quite possible on 32bit
machines which only reserve ~128MiB globally).

This should unblock a couple of perf.orientated series, but doesn't offer
any new features for itself.
-Chris

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/6] drm/i915/dmabuf: Tighten struct_mutex for unmap_dma_buf

[Chris Wilson]

We only need the struct_mutex to manipulate the pages_pin_count on the
object, we do not need to hold our BKL when freeing the exported
scatterlist.

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

diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index 0506016e18e0..b7d46800c49f 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -95,14 +95,12 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
 
-	mutex_lock(&obj->base.dev->struct_mutex);
-
 	dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
 	sg_free_table(sg);
 	kfree(sg);
 
+	mutex_lock(&obj->base.dev->struct_mutex);
 	i915_gem_object_unpin_pages(obj);
-
 	mutex_unlock(&obj->base.dev->struct_mutex);
 }
 
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915/dmabuf: Tighten struct_mutex for unmap_dma_buf

[Tvrtko Ursulin]

On 05/04/16 13:57, Chris Wilson wrote:
> We only need the struct_mutex to manipulate the pages_pin_count on the
> object, we do not need to hold our BKL when freeing the exported
> scatterlist.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index 0506016e18e0..b7d46800c49f 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -95,14 +95,12 @@ static void i915_gem_unmap_dma_buf(struct dma_buf_attachment *attachment,
>   {
>   	struct drm_i915_gem_object *obj = dma_buf_to_obj(attachment->dmabuf);
>
> -	mutex_lock(&obj->base.dev->struct_mutex);
> -
>   	dma_unmap_sg(attachment->dev, sg->sgl, sg->nents, dir);
>   	sg_free_table(sg);
>   	kfree(sg);
>
> +	mutex_lock(&obj->base.dev->struct_mutex);
>   	i915_gem_object_unpin_pages(obj);
> -
>   	mutex_unlock(&obj->base.dev->struct_mutex);
>   }
>
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/6] drm/i915: Consolidate common error handling in intel_pin_and_map_ringbuffer_obj

[Chris Wilson]

After we pin the ringbuffer into the GGTT, all error paths need to unpin
it again. Move this common step into one block, and make the unable to
iomap error code consistent (i.e. treat it as out of memory to avoid
confusing it with a invalid argument).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_ringbuffer.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 2e864b7a528b..c6ae92529fdc 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2121,15 +2121,13 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 			return ret;
 
 		ret = i915_gem_object_set_to_cpu_domain(obj, true);
-		if (ret) {
-			i915_gem_object_ggtt_unpin(obj);
-			return ret;
-		}
+		if (ret)
+			goto err_unpin;
 
 		ringbuf->virtual_start = vmap_obj(obj);
 		if (ringbuf->virtual_start == NULL) {
-			i915_gem_object_ggtt_unpin(obj);
-			return -ENOMEM;
+			ret = -ENOMEM;
+			goto err_unpin;
 		}
 	} else {
 		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
@@ -2137,10 +2135,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 			return ret;
 
 		ret = i915_gem_object_set_to_gtt_domain(obj, true);
-		if (ret) {
-			i915_gem_object_ggtt_unpin(obj);
-			return ret;
-		}
+		if (ret)
+			goto err_unpin;
 
 		/* Access through the GTT requires the device to be awake. */
 		assert_rpm_wakelock_held(dev_priv);
@@ -2148,14 +2144,17 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		ringbuf->virtual_start = ioremap_wc(ggtt->mappable_base +
 						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
 		if (ringbuf->virtual_start == NULL) {
-			i915_gem_object_ggtt_unpin(obj);
-			return -EINVAL;
+			ret = -ENOMEM;
+			goto err_unpin;
 		}
 	}
 
 	ringbuf->vma = i915_gem_obj_to_ggtt(obj);
-
 	return 0;
+
+err_unpin:
+	i915_gem_object_ggtt_unpin(obj);
+	return ret;
 }
 
 static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/6] drm/i915: Consolidate common error handling in intel_pin_and_map_ringbuffer_obj

[Tvrtko Ursulin]

On 05/04/16 13:57, Chris Wilson wrote:
> After we pin the ringbuffer into the GGTT, all error paths need to unpin
> it again. Move this common step into one block, and make the unable to
> iomap error code consistent (i.e. treat it as out of memory to avoid
> confusing it with a invalid argument).
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 25 ++++++++++++-------------
>   1 file changed, 12 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 2e864b7a528b..c6ae92529fdc 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2121,15 +2121,13 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   			return ret;
>
>   		ret = i915_gem_object_set_to_cpu_domain(obj, true);
> -		if (ret) {
> -			i915_gem_object_ggtt_unpin(obj);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_unpin;
>
>   		ringbuf->virtual_start = vmap_obj(obj);
>   		if (ringbuf->virtual_start == NULL) {
> -			i915_gem_object_ggtt_unpin(obj);
> -			return -ENOMEM;
> +			ret = -ENOMEM;
> +			goto err_unpin;
>   		}
>   	} else {
>   		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
> @@ -2137,10 +2135,8 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   			return ret;
>
>   		ret = i915_gem_object_set_to_gtt_domain(obj, true);
> -		if (ret) {
> -			i915_gem_object_ggtt_unpin(obj);
> -			return ret;
> -		}
> +		if (ret)
> +			goto err_unpin;
>
>   		/* Access through the GTT requires the device to be awake. */
>   		assert_rpm_wakelock_held(dev_priv);
> @@ -2148,14 +2144,17 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   		ringbuf->virtual_start = ioremap_wc(ggtt->mappable_base +
>   						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
>   		if (ringbuf->virtual_start == NULL) {
> -			i915_gem_object_ggtt_unpin(obj);
> -			return -EINVAL;
> +			ret = -ENOMEM;
> +			goto err_unpin;
>   		}
>   	}
>
>   	ringbuf->vma = i915_gem_obj_to_ggtt(obj);
> -

O_o :D

>   	return 0;
> +
> +err_unpin:
> +	i915_gem_object_ggtt_unpin(obj);
> +	return ret;
>   }
>
>   static void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Refactor duplicate object vmap functions

[Chris Wilson]

We now have two implementations for vmapping a whole object, one for
dma-buf and one for the ringbuffer. If we couple the vmapping into the
obj->pages lifetime, then we can reuse an obj->vmapping for both and at
the same time couple it into the shrinker.

v2: Mark the failable kmalloc() as __GFP_NOWARN (vsyrjala)
v3: Call unpin_vmap from the right dmabuf unmapper

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h         | 12 +++++---
 drivers/gpu/drm/i915/i915_gem.c         | 45 ++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 49 ++++-----------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 26 ++---------------
 4 files changed, 60 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 6443745d4182..5fedb1b7d8d3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2167,10 +2167,7 @@ struct drm_i915_gem_object {
 		struct scatterlist *sg;
 		int last;
 	} get_page;
-
-	/* prime dma-buf support */
-	void *dma_buf_vmapping;
-	int vmapping_count;
+	void *vmapping;
 
 	/** Breadcrumb of last rendering to the buffer.
 	 * There can only be one writer, but we allow for multiple readers.
@@ -2985,12 +2982,19 @@ static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
 	BUG_ON(obj->pages == NULL);
 	obj->pages_pin_count++;
 }
+
 static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
 {
 	BUG_ON(obj->pages_pin_count == 0);
 	obj->pages_pin_count--;
 }
 
+void *__must_check i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj);
+static inline void i915_gem_object_unpin_vmap(struct drm_i915_gem_object *obj)
+{
+	i915_gem_object_unpin_pages(obj);
+}
+
 int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
 int i915_gem_object_sync(struct drm_i915_gem_object *obj,
 			 struct intel_engine_cs *to,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 40f90c7e718a..be4cf13343d5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2232,6 +2232,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	 * lists early. */
 	list_del(&obj->global_list);
 
+	if (obj->vmapping) {
+		vunmap(obj->vmapping);
+		obj->vmapping = NULL;
+	}
+
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
@@ -2400,6 +2405,46 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
 	return 0;
 }
 
+void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
+{
+	int ret;
+
+	ret = i915_gem_object_get_pages(obj);
+	if (ret)
+		return ERR_PTR(ret);
+
+	i915_gem_object_pin_pages(obj);
+
+	if (obj->vmapping == NULL) {
+		struct sg_page_iter sg_iter;
+		struct page **pages;
+		int n;
+
+		n = obj->base.size >> PAGE_SHIFT;
+		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY | __GFP_NOWARN);
+		if (pages == NULL)
+			pages = drm_malloc_ab(n, sizeof(*pages));
+		if (pages != NULL) {
+			n = 0;
+			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
+				pages[n++] = sg_page_iter_page(&sg_iter);
+
+			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
+			if (obj->vmapping == NULL) {
+				i915_gem_shrink_all(to_i915(obj->base.dev));
+				obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
+			}
+			drm_free_large(pages);
+		}
+		if (obj->vmapping == NULL) {
+			i915_gem_object_unpin_pages(obj);
+			return ERR_PTR(-ENOMEM);
+		}
+	}
+
+	return obj->vmapping;
+}
+
 void i915_vma_move_to_active(struct i915_vma *vma,
 			     struct drm_i915_gem_request *req)
 {
diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
index b7d46800c49f..8d8feadee18c 100644
--- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
+++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
@@ -108,51 +108,17 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
 {
 	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
 	struct drm_device *dev = obj->base.dev;
-	struct sg_page_iter sg_iter;
-	struct page **pages;
-	int ret, i;
+	void *addr;
+	int ret;
 
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ERR_PTR(ret);
 
-	if (obj->dma_buf_vmapping) {
-		obj->vmapping_count++;
-		goto out_unlock;
-	}
-
-	ret = i915_gem_object_get_pages(obj);
-	if (ret)
-		goto err;
-
-	i915_gem_object_pin_pages(obj);
-
-	ret = -ENOMEM;
-
-	pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
-	if (pages == NULL)
-		goto err_unpin;
-
-	i = 0;
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
-		pages[i++] = sg_page_iter_page(&sg_iter);
-
-	obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL);
-	drm_free_large(pages);
-
-	if (!obj->dma_buf_vmapping)
-		goto err_unpin;
-
-	obj->vmapping_count = 1;
-out_unlock:
+	addr = i915_gem_object_pin_vmap(obj);
 	mutex_unlock(&dev->struct_mutex);
-	return obj->dma_buf_vmapping;
 
-err_unpin:
-	i915_gem_object_unpin_pages(obj);
-err:
-	mutex_unlock(&dev->struct_mutex);
-	return ERR_PTR(ret);
+	return addr;
 }
 
 static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
@@ -161,12 +127,7 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
 	struct drm_device *dev = obj->base.dev;
 
 	mutex_lock(&dev->struct_mutex);
-	if (--obj->vmapping_count == 0) {
-		vunmap(obj->dma_buf_vmapping);
-		obj->dma_buf_vmapping = NULL;
-
-		i915_gem_object_unpin_pages(obj);
-	}
+	i915_gem_object_unpin_vmap(obj);
 	mutex_unlock(&dev->struct_mutex);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index c6ae92529fdc..42db699cf326 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2078,35 +2078,13 @@ static int init_phys_status_page(struct intel_engine_cs *engine)
 void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 {
 	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
-		vunmap(ringbuf->virtual_start);
+		i915_gem_object_unpin_vmap(ringbuf->obj);
 	else
 		iounmap(ringbuf->virtual_start);
-	ringbuf->virtual_start = NULL;
 	ringbuf->vma = NULL;
 	i915_gem_object_ggtt_unpin(ringbuf->obj);
 }
 
-static u32 *vmap_obj(struct drm_i915_gem_object *obj)
-{
-	struct sg_page_iter sg_iter;
-	struct page **pages;
-	void *addr;
-	int i;
-
-	pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
-	if (pages == NULL)
-		return NULL;
-
-	i = 0;
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
-		pages[i++] = sg_page_iter_page(&sg_iter);
-
-	addr = vmap(pages, i, 0, PAGE_KERNEL);
-	drm_free_large(pages);
-
-	return addr;
-}
-
 int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 				     struct intel_ringbuffer *ringbuf)
 {
@@ -2124,7 +2102,7 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 		if (ret)
 			goto err_unpin;
 
-		ringbuf->virtual_start = vmap_obj(obj);
+		ringbuf->virtual_start = i915_gem_object_pin_vmap(obj);
 		if (ringbuf->virtual_start == NULL) {
 			ret = -ENOMEM;
 			goto err_unpin;
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Refactor duplicate object vmap functions

[Tvrtko Ursulin]

On 05/04/16 13:57, Chris Wilson wrote:
> We now have two implementations for vmapping a whole object, one for
> dma-buf and one for the ringbuffer. If we couple the vmapping into the
> obj->pages lifetime, then we can reuse an obj->vmapping for both and at
> the same time couple it into the shrinker.

I suppose Dave could respin the "vmap_range" helper on top of this 
series to further consolidate cmd parser and i915_gem_object_pin_vmap.

> v2: Mark the failable kmalloc() as __GFP_NOWARN (vsyrjala)
> v3: Call unpin_vmap from the right dmabuf unmapper
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h         | 12 +++++---
>   drivers/gpu/drm/i915/i915_gem.c         | 45 ++++++++++++++++++++++++++++++
>   drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 49 ++++-----------------------------
>   drivers/gpu/drm/i915/intel_ringbuffer.c | 26 ++---------------
>   4 files changed, 60 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 6443745d4182..5fedb1b7d8d3 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2167,10 +2167,7 @@ struct drm_i915_gem_object {
>   		struct scatterlist *sg;
>   		int last;
>   	} get_page;
> -
> -	/* prime dma-buf support */
> -	void *dma_buf_vmapping;
> -	int vmapping_count;
> +	void *vmapping;
>
>   	/** Breadcrumb of last rendering to the buffer.
>   	 * There can only be one writer, but we allow for multiple readers.
> @@ -2985,12 +2982,19 @@ static inline void i915_gem_object_pin_pages(struct drm_i915_gem_object *obj)
>   	BUG_ON(obj->pages == NULL);
>   	obj->pages_pin_count++;
>   }
> +
>   static inline void i915_gem_object_unpin_pages(struct drm_i915_gem_object *obj)
>   {
>   	BUG_ON(obj->pages_pin_count == 0);
>   	obj->pages_pin_count--;
>   }
>
> +void *__must_check i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj);
> +static inline void i915_gem_object_unpin_vmap(struct drm_i915_gem_object *obj)
> +{
> +	i915_gem_object_unpin_pages(obj);
> +}
> +
>   int __must_check i915_mutex_lock_interruptible(struct drm_device *dev);
>   int i915_gem_object_sync(struct drm_i915_gem_object *obj,
>   			 struct intel_engine_cs *to,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 40f90c7e718a..be4cf13343d5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2232,6 +2232,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>   	 * lists early. */
>   	list_del(&obj->global_list);
>
> +	if (obj->vmapping) {
> +		vunmap(obj->vmapping);
> +		obj->vmapping = NULL;
> +	}
> +
>   	ops->put_pages(obj);
>   	obj->pages = NULL;
>
> @@ -2400,6 +2405,46 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
>   	return 0;
>   }
>
> +void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)

Kerneldoc would be cool, if for nothing then for the return value.

> +{
> +	int ret;
> +

lockdep_assert_held maybe?

> +	ret = i915_gem_object_get_pages(obj);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	i915_gem_object_pin_pages(obj);
> +
> +	if (obj->vmapping == NULL) {
> +		struct sg_page_iter sg_iter;
> +		struct page **pages;
> +		int n;
> +
> +		n = obj->base.size >> PAGE_SHIFT;
> +		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY | __GFP_NOWARN);
> +		if (pages == NULL)
> +			pages = drm_malloc_ab(n, sizeof(*pages));
> +		if (pages != NULL) {
> +			n = 0;
> +			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> +				pages[n++] = sg_page_iter_page(&sg_iter);
> +
> +			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
> +			if (obj->vmapping == NULL) {
> +				i915_gem_shrink_all(to_i915(obj->base.dev));

Won't the shrinker already run via the new notifier? Why call it again 
and for all objects this time?

Also, act on the return value before retrying vmap?

> +				obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
> +			}
> +			drm_free_large(pages);
> +		}
> +		if (obj->vmapping == NULL) {
> +			i915_gem_object_unpin_pages(obj);
> +			return ERR_PTR(-ENOMEM);
> +		}
> +	}
> +
> +	return obj->vmapping;
> +}
> +
>   void i915_vma_move_to_active(struct i915_vma *vma,
>   			     struct drm_i915_gem_request *req)
>   {
> diff --git a/drivers/gpu/drm/i915/i915_gem_dmabuf.c b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> index b7d46800c49f..8d8feadee18c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> +++ b/drivers/gpu/drm/i915/i915_gem_dmabuf.c
> @@ -108,51 +108,17 @@ static void *i915_gem_dmabuf_vmap(struct dma_buf *dma_buf)
>   {
>   	struct drm_i915_gem_object *obj = dma_buf_to_obj(dma_buf);
>   	struct drm_device *dev = obj->base.dev;
> -	struct sg_page_iter sg_iter;
> -	struct page **pages;
> -	int ret, i;
> +	void *addr;
> +	int ret;
>
>   	ret = i915_mutex_lock_interruptible(dev);
>   	if (ret)
>   		return ERR_PTR(ret);
>
> -	if (obj->dma_buf_vmapping) {
> -		obj->vmapping_count++;
> -		goto out_unlock;
> -	}
> -
> -	ret = i915_gem_object_get_pages(obj);
> -	if (ret)
> -		goto err;
> -
> -	i915_gem_object_pin_pages(obj);
> -
> -	ret = -ENOMEM;
> -
> -	pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
> -	if (pages == NULL)
> -		goto err_unpin;
> -
> -	i = 0;
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> -		pages[i++] = sg_page_iter_page(&sg_iter);
> -
> -	obj->dma_buf_vmapping = vmap(pages, i, 0, PAGE_KERNEL);
> -	drm_free_large(pages);
> -
> -	if (!obj->dma_buf_vmapping)
> -		goto err_unpin;
> -
> -	obj->vmapping_count = 1;
> -out_unlock:
> +	addr = i915_gem_object_pin_vmap(obj);
>   	mutex_unlock(&dev->struct_mutex);
> -	return obj->dma_buf_vmapping;
>
> -err_unpin:
> -	i915_gem_object_unpin_pages(obj);
> -err:
> -	mutex_unlock(&dev->struct_mutex);
> -	return ERR_PTR(ret);
> +	return addr;
>   }
>
>   static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
> @@ -161,12 +127,7 @@ static void i915_gem_dmabuf_vunmap(struct dma_buf *dma_buf, void *vaddr)
>   	struct drm_device *dev = obj->base.dev;
>
>   	mutex_lock(&dev->struct_mutex);
> -	if (--obj->vmapping_count == 0) {
> -		vunmap(obj->dma_buf_vmapping);
> -		obj->dma_buf_vmapping = NULL;
> -
> -		i915_gem_object_unpin_pages(obj);
> -	}
> +	i915_gem_object_unpin_vmap(obj);
>   	mutex_unlock(&dev->struct_mutex);
>   }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index c6ae92529fdc..42db699cf326 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2078,35 +2078,13 @@ static int init_phys_status_page(struct intel_engine_cs *engine)
>   void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>   {
>   	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
> -		vunmap(ringbuf->virtual_start);
> +		i915_gem_object_unpin_vmap(ringbuf->obj);
>   	else
>   		iounmap(ringbuf->virtual_start);
> -	ringbuf->virtual_start = NULL;
>   	ringbuf->vma = NULL;
>   	i915_gem_object_ggtt_unpin(ringbuf->obj);
>   }
>
> -static u32 *vmap_obj(struct drm_i915_gem_object *obj)
> -{
> -	struct sg_page_iter sg_iter;
> -	struct page **pages;
> -	void *addr;
> -	int i;
> -
> -	pages = drm_malloc_ab(obj->base.size >> PAGE_SHIFT, sizeof(*pages));
> -	if (pages == NULL)
> -		return NULL;
> -
> -	i = 0;
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> -		pages[i++] = sg_page_iter_page(&sg_iter);
> -
> -	addr = vmap(pages, i, 0, PAGE_KERNEL);
> -	drm_free_large(pages);
> -
> -	return addr;
> -}
> -
>   int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   				     struct intel_ringbuffer *ringbuf)
>   {
> @@ -2124,7 +2102,7 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>   		if (ret)
>   			goto err_unpin;
>
> -		ringbuf->virtual_start = vmap_obj(obj);
> +		ringbuf->virtual_start = i915_gem_object_pin_vmap(obj);
>   		if (ringbuf->virtual_start == NULL) {
>   			ret = -ENOMEM;
>   			goto err_unpin;
>

Minus the questions above looks good.

Regards,

Tvrtko

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/6] drm/i915: Refactor duplicate object vmap functions

[Chris Wilson]

On Wed, Apr 06, 2016 at 10:30:15AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/04/16 13:57, Chris Wilson wrote:
> >We now have two implementations for vmapping a whole object, one for
> >dma-buf and one for the ringbuffer. If we couple the vmapping into the
> >obj->pages lifetime, then we can reuse an obj->vmapping for both and at
> >the same time couple it into the shrinker.
> 
> I suppose Dave could respin the "vmap_range" helper on top of this
> series to further consolidate cmd parser and
> i915_gem_object_pin_vmap.

Why though? The plan (see the earlier patches) is to completely replace
the cmdparser's current pessimism with this.
 
> >@@ -2400,6 +2405,46 @@ i915_gem_object_get_pages(struct drm_i915_gem_object *obj)
> >  	return 0;
> >  }
> >
> >+void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
> 
> Kerneldoc would be cool, if for nothing then for the return value.
> 
> >+{
> >+	int ret;
> >+
> 
> lockdep_assert_held maybe?

Urm, see long term plans ;)

Yes, it applies now. I have in mind going through adding the
lockdep_assert_held() precisely because I have some patches to remove
the locking requirements around some core functions.

> >+	ret = i915_gem_object_get_pages(obj);
> >+	if (ret)
> >+		return ERR_PTR(ret);
> >+
> >+	i915_gem_object_pin_pages(obj);
> >+
> >+	if (obj->vmapping == NULL) {
> >+		struct sg_page_iter sg_iter;
> >+		struct page **pages;
> >+		int n;
> >+
> >+		n = obj->base.size >> PAGE_SHIFT;
> >+		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY | __GFP_NOWARN);
> >+		if (pages == NULL)
> >+			pages = drm_malloc_ab(n, sizeof(*pages));
> >+		if (pages != NULL) {
> >+			n = 0;
> >+			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> >+				pages[n++] = sg_page_iter_page(&sg_iter);
> >+
> >+			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
> >+			if (obj->vmapping == NULL) {
> >+				i915_gem_shrink_all(to_i915(obj->base.dev));
> 
> Won't the shrinker already run via the new notifier? Why call it
> again and for all objects this time?

Left over, forgot to update this chunk. The shrinker will be run for
both any kmalloc failures inside vmap as well as the arena notifier. So
we can indeed remove this manual shrinking. If we wanted to we could do
something like get_pages_gtt where we shrink ourselves before we allow the
notifier to run. I'm going to say that is overkill here...

> Also, act on the return value before retrying vmap?

No. The return value doesn't give a good indication as to whether the
next attempt will succeed or not (and we care more about the failure to
allocate than the transistory failure to shrink).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/6] drm/i915/shrinker: Restrict vmap purge to objects with vmaps

[Chris Wilson]

When called because we have run out of vmap address space, we only need
to recover objects that have vmappings and not all.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 10 +++++++++-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5fedb1b7d8d3..99e2cd3bb290 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3311,6 +3311,7 @@ unsigned long i915_gem_shrink(struct drm_i915_private *dev_priv,
 #define I915_SHRINK_UNBOUND 0x2
 #define I915_SHRINK_BOUND 0x4
 #define I915_SHRINK_ACTIVE 0x8
+#define I915_SHRINK_VMAPS 0x10
 unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
 void i915_gem_shrinker_init(struct drm_i915_private *dev_priv);
 void i915_gem_shrinker_cleanup(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index 39943793edcc..56b4ec31d798 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -167,6 +167,10 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			    obj->madv != I915_MADV_DONTNEED)
 				continue;
 
+			if (flags & I915_SHRINK_VMAPS &&
+			    obj->vmapping == NULL)
+				continue;
+
 			if ((flags & I915_SHRINK_ACTIVE) == 0 && obj->active)
 				continue;
 
@@ -388,7 +392,11 @@ i915_gem_shrinker_vmap(struct notifier_block *nb, unsigned long event, void *ptr
 	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 5000))
 		return NOTIFY_DONE;
 
-	freed_pages = i915_gem_shrink_all(dev_priv);
+	freed_pages = i915_gem_shrink(dev_priv, -1UL,
+				      I915_SHRINK_BOUND |
+				      I915_SHRINK_UNBOUND |
+				      I915_SHRINK_ACTIVE |
+				      I915_SHRINK_VMAPS);
 
 	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
 
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/6] drm,i915: Introduce drm_malloc_gfp()

[Chris Wilson]

I have instances where I want to use drm_malloc_ab() but with a custom
gfp mask. And with those, where I want a temporary allocation, I want to
try a high-order kmalloc() before using a vmalloc().

So refactor my usage into drm_malloc_gfp().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: dri-devel@lists.freedesktop.org
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Dave Airlie <airlied@redhat.com>
---
 drivers/gpu/drm/i915/i915_gem.c            |  4 +---
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  8 +++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c        |  5 +++--
 drivers/gpu/drm/i915/i915_gem_userptr.c    | 15 ++++-----------
 include/drm/drm_mem_util.h                 | 19 +++++++++++++++++++
 5 files changed, 30 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index be4cf13343d5..985f067c1f0e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2421,9 +2421,7 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
 		int n;
 
 		n = obj->base.size >> PAGE_SHIFT;
-		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY | __GFP_NOWARN);
-		if (pages == NULL)
-			pages = drm_malloc_ab(n, sizeof(*pages));
+		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
 		if (pages != NULL) {
 			n = 0;
 			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 0ee61fd014df..6ee4f00f620c 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1783,11 +1783,9 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 		return -EINVAL;
 	}
 
-	exec2_list = kmalloc(sizeof(*exec2_list)*args->buffer_count,
-			     GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (exec2_list == NULL)
-		exec2_list = drm_malloc_ab(sizeof(*exec2_list),
-					   args->buffer_count);
+	exec2_list = drm_malloc_gfp(args->buffer_count,
+				    sizeof(*exec2_list),
+				    GFP_TEMPORARY);
 	if (exec2_list == NULL) {
 		DRM_DEBUG("Failed to allocate exec list for %d buffers\n",
 			  args->buffer_count);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index ae9cb2735767..18f2bd7caad5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3413,8 +3413,9 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info,
 	int ret = -ENOMEM;
 
 	/* Allocate a temporary list of source pages for random access. */
-	page_addr_list = drm_malloc_ab(obj->base.size / PAGE_SIZE,
-				       sizeof(dma_addr_t));
+	page_addr_list = drm_malloc_gfp(obj->base.size / PAGE_SIZE,
+					sizeof(dma_addr_t),
+					GFP_TEMPORARY);
 	if (!page_addr_list)
 		return ERR_PTR(ret);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 291a9393493d..67883ebf9504 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -494,10 +494,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work)
 	ret = -ENOMEM;
 	pinned = 0;
 
-	pvec = kmalloc(npages*sizeof(struct page *),
-		       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
-	if (pvec == NULL)
-		pvec = drm_malloc_ab(npages, sizeof(struct page *));
+	pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY);
 	if (pvec != NULL) {
 		struct mm_struct *mm = obj->userptr.mm->mm;
 
@@ -634,14 +631,10 @@ i915_gem_userptr_get_pages(struct drm_i915_gem_object *obj)
 	pvec = NULL;
 	pinned = 0;
 	if (obj->userptr.mm->mm == current->mm) {
-		pvec = kmalloc(num_pages*sizeof(struct page *),
-			       GFP_TEMPORARY | __GFP_NOWARN | __GFP_NORETRY);
+		pvec = drm_malloc_gfp(num_pages, sizeof(struct page *), GFP_TEMPORARY);
 		if (pvec == NULL) {
-			pvec = drm_malloc_ab(num_pages, sizeof(struct page *));
-			if (pvec == NULL) {
-				__i915_gem_userptr_set_active(obj, false);
-				return -ENOMEM;
-			}
+			__i915_gem_userptr_set_active(obj, false);
+			return -ENOMEM;
 		}
 
 		pinned = __get_user_pages_fast(obj->userptr.ptr, num_pages,
diff --git a/include/drm/drm_mem_util.h b/include/drm/drm_mem_util.h
index e42495ad8136..741ce75a72b4 100644
--- a/include/drm/drm_mem_util.h
+++ b/include/drm/drm_mem_util.h
@@ -54,6 +54,25 @@ static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
 			 GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 
+static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
+{
+	if (size != 0 && nmemb > SIZE_MAX / size)
+		return NULL;
+
+	if (size * nmemb <= PAGE_SIZE)
+	    return kmalloc(nmemb * size, gfp);
+
+	if (gfp & __GFP_RECLAIMABLE) {
+		void *ptr = kmalloc(nmemb * size,
+				    gfp | __GFP_NOWARN | __GFP_NORETRY);
+		if (ptr)
+			return ptr;
+	}
+
+	return __vmalloc(size * nmemb,
+			 gfp | __GFP_HIGHMEM, PAGE_KERNEL);
+}
+
 static __inline void drm_free_large(void *ptr)
 {
 	kvfree(ptr);
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm,i915: Introduce drm_malloc_gfp()

[Chris Wilson]

On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
> I have instances where I want to use drm_malloc_ab() but with a custom
> gfp mask. And with those, where I want a temporary allocation, I want to
> try a high-order kmalloc() before using a vmalloc().
> 
> So refactor my usage into drm_malloc_gfp().
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: dri-devel@lists.freedesktop.org
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Acked-by: Dave Airlie <airlied@redhat.com>

> +static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
> +{
> +	if (size != 0 && nmemb > SIZE_MAX / size)
> +		return NULL;

I know Dave G. has some fancy code to detect when the size parameter is
not constant, but one thing I noticed was that gcc would uninline this
function and we would lose the constant folding. Is there anything we
can do to convince gcc to avoid a div here (other than pure macro)?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/6] drm,i915: Introduce drm_malloc_gfp()

[Tvrtko Ursulin]

On 05/04/16 14:05, Chris Wilson wrote:
> On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
>> I have instances where I want to use drm_malloc_ab() but with a custom
>> gfp mask. And with those, where I want a temporary allocation, I want to
>> try a high-order kmalloc() before using a vmalloc().
>>
>> So refactor my usage into drm_malloc_gfp().
>>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Acked-by: Dave Airlie <airlied@redhat.com>
>
>> +static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
>> +{
>> +	if (size != 0 && nmemb > SIZE_MAX / size)
>> +		return NULL;
>
> I know Dave G. has some fancy code to detect when the size parameter is
> not constant, but one thing I noticed was that gcc would uninline this
> function and we would lose the constant folding. Is there anything we
> can do to convince gcc to avoid a div here (other than pure macro)?

Don't know, apart from maybe _always_inline if it is not considered too big.

But I wanted to ask, why it is interesting to allow size == 0 ? Why not:

	if (size == 0 || nmemb > SIZE_MAX / size)
		return NULL;

?

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm,i915: Introduce drm_malloc_gfp()

[Chris Wilson]

On Wed, Apr 06, 2016 at 10:40:19AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/04/16 14:05, Chris Wilson wrote:
> >On Tue, Apr 05, 2016 at 01:57:36PM +0100, Chris Wilson wrote:
> >>I have instances where I want to use drm_malloc_ab() but with a custom
> >>gfp mask. And with those, where I want a temporary allocation, I want to
> >>try a high-order kmalloc() before using a vmalloc().
> >>
> >>So refactor my usage into drm_malloc_gfp().
> >>
> >>Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >>Cc: dri-devel@lists.freedesktop.org
> >>Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>Acked-by: Dave Airlie <airlied@redhat.com>
> >
> >>+static __inline__ void *drm_malloc_gfp(size_t nmemb, size_t size, gfp_t gfp)
> >>+{
> >>+	if (size != 0 && nmemb > SIZE_MAX / size)
> >>+		return NULL;
> >
> >I know Dave G. has some fancy code to detect when the size parameter is
> >not constant, but one thing I noticed was that gcc would uninline this
> >function and we would lose the constant folding. Is there anything we
> >can do to convince gcc to avoid a div here (other than pure macro)?
> 
> Don't know, apart from maybe _always_inline if it is not considered too big.
> 
> But I wanted to ask, why it is interesting to allow size == 0 ? Why not:
> 
> 	if (size == 0 || nmemb > SIZE_MAX / size)
> 		return NULL;
> 
> ?

Cargo-culting. I guess the only thought was to avoid the div-by-zero and
to fallthrough to returning kmalloc(0) for equivalent behaviour.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

If we want a contiguous mapping of a single page sized object, we can
forgo using vmap() and just use a regular kmap(). Note that this is only
suitable if the desired pgprot_t is comptabitible.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 985f067c1f0e..dc8e1b76c896 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	list_del(&obj->global_list);
 
 	if (obj->vmapping) {
-		vunmap(obj->vmapping);
+		if (obj->base.size == PAGE_SIZE)
+			kunmap(kmap_to_page(obj->vmapping));
+		else
+			vunmap(obj->vmapping);
 		obj->vmapping = NULL;
 	}
 
@@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
 	i915_gem_object_pin_pages(obj);
 
 	if (obj->vmapping == NULL) {
-		struct sg_page_iter sg_iter;
 		struct page **pages;
-		int n;
 
-		n = obj->base.size >> PAGE_SHIFT;
-		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
+		pages = NULL;
+		if (obj->base.size == PAGE_SIZE)
+			obj->vmapping = kmap(sg_page(obj->pages->sgl));
+		else
+			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
+					       sizeof(*pages),
+					       GFP_TEMPORARY);
 		if (pages != NULL) {
+			struct sg_page_iter sg_iter;
+			int n;
+
 			n = 0;
-			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
+			for_each_sg_page(obj->pages->sgl, &sg_iter,
+					 obj->pages->nents, 0)
 				pages[n++] = sg_page_iter_page(&sg_iter);
 
 			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
-- 
2.8.0.rc3

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

On Tue, Apr 05, 2016 at 01:57:37PM +0100, Chris Wilson wrote:
> If we want a contiguous mapping of a single page sized object, we can
> forgo using vmap() and just use a regular kmap(). Note that this is only
> suitable if the desired pgprot_t is comptabitible.

One day, I will enable :set spell. Does anyone know if there is a .vim
rule to detect git? Or something to stuff in git config core.editor ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Matthew Auld]

I use :autocmd FileType gitcommit setlocal spell
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Tvrtko Ursulin]

On 05/04/16 13:57, Chris Wilson wrote:
> If we want a contiguous mapping of a single page sized object, we can
> forgo using vmap() and just use a regular kmap(). Note that this is only
> suitable if the desired pgprot_t is comptabitible.

You noticed the spelling already. :)

>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 985f067c1f0e..dc8e1b76c896 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>   	list_del(&obj->global_list);
>
>   	if (obj->vmapping) {
> -		vunmap(obj->vmapping);
> +		if (obj->base.size == PAGE_SIZE)
> +			kunmap(kmap_to_page(obj->vmapping));
> +		else
> +			vunmap(obj->vmapping);

Can't think of a reason why it would be better but there is also 
is_vmalloc_addr(addr) as used by kvfree.

>   		obj->vmapping = NULL;
>   	}
>
> @@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
>   	i915_gem_object_pin_pages(obj);
>
>   	if (obj->vmapping == NULL) {
> -		struct sg_page_iter sg_iter;
>   		struct page **pages;
> -		int n;
>
> -		n = obj->base.size >> PAGE_SHIFT;
> -		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
> +		pages = NULL;
> +		if (obj->base.size == PAGE_SIZE)
> +			obj->vmapping = kmap(sg_page(obj->pages->sgl));
> +		else
> +			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
> +					       sizeof(*pages),
> +					       GFP_TEMPORARY);
>   		if (pages != NULL) {
> +			struct sg_page_iter sg_iter;
> +			int n;
> +
>   			n = 0;
> -			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> +			for_each_sg_page(obj->pages->sgl, &sg_iter,
> +					 obj->pages->nents, 0)
>   				pages[n++] = sg_page_iter_page(&sg_iter);
>
>   			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
>

Two problems I can spot are:

1. Callers of pin_vmap now don't know which kind of address they are 
getting. Maybe call it pin_kvmap or something? Just mention in kerneldoc 
could be enough.

2. Shrinker will try to kick out kmapped objects because they have 
obj->vmapping set.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Chris Wilson]

On Wed, Apr 06, 2016 at 10:49:36AM +0100, Tvrtko Ursulin wrote:
> >diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >index 985f067c1f0e..dc8e1b76c896 100644
> >--- a/drivers/gpu/drm/i915/i915_gem.c
> >+++ b/drivers/gpu/drm/i915/i915_gem.c
> >@@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> >  	list_del(&obj->global_list);
> >
> >  	if (obj->vmapping) {
> >-		vunmap(obj->vmapping);
> >+		if (obj->base.size == PAGE_SIZE)
> >+			kunmap(kmap_to_page(obj->vmapping));
> >+		else
> >+			vunmap(obj->vmapping);
> 
> Can't think of a reason why it would be better but there is also
> is_vmalloc_addr(addr) as used by kvfree.

For consistency with the shrinker (see below).

> >  		obj->vmapping = NULL;
> >  	}
> >
> >@@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
> >  	i915_gem_object_pin_pages(obj);
> >
> >  	if (obj->vmapping == NULL) {
> >-		struct sg_page_iter sg_iter;
> >  		struct page **pages;
> >-		int n;
> >
> >-		n = obj->base.size >> PAGE_SHIFT;
> >-		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
> >+		pages = NULL;
> >+		if (obj->base.size == PAGE_SIZE)
> >+			obj->vmapping = kmap(sg_page(obj->pages->sgl));
> >+		else
> >+			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
> >+					       sizeof(*pages),
> >+					       GFP_TEMPORARY);
> >  		if (pages != NULL) {
> >+			struct sg_page_iter sg_iter;
> >+			int n;
> >+
> >  			n = 0;
> >-			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
> >+			for_each_sg_page(obj->pages->sgl, &sg_iter,
> >+					 obj->pages->nents, 0)
> >  				pages[n++] = sg_page_iter_page(&sg_iter);
> >
> >  			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
> >
> 
> Two problems I can spot are:
> 
> 1. Callers of pin_vmap now don't know which kind of address they are
> getting. Maybe call it pin_kvmap or something? Just mention in
> kerneldoc could be enough.

I think just mention, and we can rename this to i915_gem_object_pin_map().
Hmm. I liked the pin in the name since it ties to to pin_pages (later
I plan to change that to get_pages and get_vmap/get_map as the pin
becomes implicit).

> 2. Shrinker will try to kick out kmapped objects because they have
> obj->vmapping set.

Not caring that much since the vmap_purge is very heavy weight, but we
can apply is_vmalloc_addr() to the shrinker.

Ok, happy to call this obj->mapping and i915_gem_object_pin_map() ?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Tvrtko Ursulin]

On 06/04/16 11:05, Chris Wilson wrote:
> On Wed, Apr 06, 2016 at 10:49:36AM +0100, Tvrtko Ursulin wrote:
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 985f067c1f0e..dc8e1b76c896 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>>   	list_del(&obj->global_list);
>>>
>>>   	if (obj->vmapping) {
>>> -		vunmap(obj->vmapping);
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			kunmap(kmap_to_page(obj->vmapping));
>>> +		else
>>> +			vunmap(obj->vmapping);
>>
>> Can't think of a reason why it would be better but there is also
>> is_vmalloc_addr(addr) as used by kvfree.
>
> For consistency with the shrinker (see below).
>
>>>   		obj->vmapping = NULL;
>>>   	}
>>>
>>> @@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
>>>   	i915_gem_object_pin_pages(obj);
>>>
>>>   	if (obj->vmapping == NULL) {
>>> -		struct sg_page_iter sg_iter;
>>>   		struct page **pages;
>>> -		int n;
>>>
>>> -		n = obj->base.size >> PAGE_SHIFT;
>>> -		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
>>> +		pages = NULL;
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			obj->vmapping = kmap(sg_page(obj->pages->sgl));
>>> +		else
>>> +			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
>>> +					       sizeof(*pages),
>>> +					       GFP_TEMPORARY);
>>>   		if (pages != NULL) {
>>> +			struct sg_page_iter sg_iter;
>>> +			int n;
>>> +
>>>   			n = 0;
>>> -			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
>>> +			for_each_sg_page(obj->pages->sgl, &sg_iter,
>>> +					 obj->pages->nents, 0)
>>>   				pages[n++] = sg_page_iter_page(&sg_iter);
>>>
>>>   			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
>>>
>>
>> Two problems I can spot are:
>>
>> 1. Callers of pin_vmap now don't know which kind of address they are
>> getting. Maybe call it pin_kvmap or something? Just mention in
>> kerneldoc could be enough.
>
> I think just mention, and we can rename this to i915_gem_object_pin_map().
> Hmm. I liked the pin in the name since it ties to to pin_pages (later
> I plan to change that to get_pages and get_vmap/get_map as the pin
> becomes implicit).
>
>> 2. Shrinker will try to kick out kmapped objects because they have
>> obj->vmapping set.
>
> Not caring that much since the vmap_purge is very heavy weight, but we
> can apply is_vmalloc_addr() to the shrinker.
>
> Ok, happy to call this obj->mapping and i915_gem_object_pin_map() ?

Sounds good. (Including the is_vmalloc_addr() smartening in the shrinker.)

Regards,

Tvrtko


_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: Avoid allocating a vmap arena for a single page

[Dave Gordon]

On 06/04/16 11:05, Chris Wilson wrote:
> On Wed, Apr 06, 2016 at 10:49:36AM +0100, Tvrtko Ursulin wrote:
>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>> index 985f067c1f0e..dc8e1b76c896 100644
>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>> @@ -2233,7 +2233,10 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>>   	list_del(&obj->global_list);
>>>
>>>   	if (obj->vmapping) {
>>> -		vunmap(obj->vmapping);
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			kunmap(kmap_to_page(obj->vmapping));
>>> +		else
>>> +			vunmap(obj->vmapping);
>>
>> Can't think of a reason why it would be better but there is also
>> is_vmalloc_addr(addr) as used by kvfree.
>
> For consistency with the shrinker (see below).

What I don't like here is the repetition (and correlation) of the 
PAGE_SIZE test, which has to be kept in sync with the corresponding one 
at the point where the mapping was set up. If we're going to overload 
the same field to store two different types of mapping, there should be 
an explicit flag to say which we chose. Or failing that, then actually 
test the mapping itself (as in is_vmalloc_addr()).

>>>   		obj->vmapping = NULL;
>>>   	}
>>>
>>> @@ -2416,15 +2419,22 @@ void *i915_gem_object_pin_vmap(struct drm_i915_gem_object *obj)
>>>   	i915_gem_object_pin_pages(obj);
>>>
>>>   	if (obj->vmapping == NULL) {
>>> -		struct sg_page_iter sg_iter;
>>>   		struct page **pages;
>>> -		int n;
>>>
>>> -		n = obj->base.size >> PAGE_SHIFT;
>>> -		pages = drm_malloc_gfp(n, sizeof(*pages), GFP_TEMPORARY);
>>> +		pages = NULL;
>>> +		if (obj->base.size == PAGE_SIZE)
>>> +			obj->vmapping = kmap(sg_page(obj->pages->sgl));
>>> +		else
>>> +			pages = drm_malloc_gfp(obj->base.size >> PAGE_SHIFT,
>>> +					       sizeof(*pages),
>>> +					       GFP_TEMPORARY);
>>>   		if (pages != NULL) {
>>> +			struct sg_page_iter sg_iter;
>>> +			int n;
>>> +
>>>   			n = 0;
>>> -			for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0)
>>> +			for_each_sg_page(obj->pages->sgl, &sg_iter,
>>> +					 obj->pages->nents, 0)
>>>   				pages[n++] = sg_page_iter_page(&sg_iter);
>>>
>>>   			obj->vmapping = vmap(pages, n, 0, PAGE_KERNEL);
>>>
>>
>> Two problems I can spot are:
>>
>> 1. Callers of pin_vmap now don't know which kind of address they are
>> getting. Maybe call it pin_kvmap or something? Just mention in
>> kerneldoc could be enough.
>
> I think just mention, and we can rename this to i915_gem_object_pin_map().
> Hmm. I liked the pin in the name since it ties to to pin_pages (later
> I plan to change that to get_pages and get_vmap/get_map as the pin
> becomes implicit).
>
>> 2. Shrinker will try to kick out kmapped objects because they have
>> obj->vmapping set.
>
> Not caring that much since the vmap_purge is very heavy weight, but we
> can apply is_vmalloc_addr() to the shrinker.
>
> Ok, happy to call this obj->mapping and i915_gem_object_pin_map() ?
> -Chris

Quite happy with the rename, and returning either type (a (virtual) 
address is just an address), but not with the implementation repeating 
the decision code.

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [1/6] drm/i915/dmabuf: Tighten struct_mutex for unmap_dma_buf

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915/dmabuf: Tighten struct_mutex for unmap_dma_buf
URL   : https://patchwork.freedesktop.org/series/5325/
State : failure

== Summary ==

Series 5325v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/5325/revisions/1/mbox/

Test drv_module_reload_basic:
                pass       -> INCOMPLETE (hsw-brixbox)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-wf_vblank:
                fail       -> PASS       (snb-x220t)
Test pm_rpm:
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:159  dwarn:0   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:135  pass:119  dwarn:0   dfail:0   fail:0   skip:15 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:131  dwarn:1   dfail:0   fail:0   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:173  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:196  pass:162  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1805/

48d2a0497cd1d0630fbb59fb7871e4d678458307 drm-intel-nightly: 2016y-04m-05d-12h-30m-42s UTC integration manifest
6f826022acd130aa83d30319e478a3ae104d0184 drm/i915: Avoid allocating a vmap arena for a single page
366ed67bbb44c347ef88481b14ed7fca2b564f5c drm,i915: Introduce drm_malloc_gfp()
bf183e22422866f4cbbd5beb09b33d7b0c9c7ad8 drm/i915/shrinker: Restrict vmap purge to objects with vmaps
53fb89e12fc2c54d3e32898f4b50e69df4dc7984 drm/i915: Refactor duplicate object vmap functions
f0b43f7cf1a1f807b8aa5210b853049b1ff1ec95 drm/i915: Consolidate common error handling in intel_pin_and_map_ringbuffer_obj
d4f6a386b819281e94c18511af9ac439ed943236 drm/i915/dmabuf: Tighten struct_mutex for unmap_dma_buf

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx