[PATCH 1/3] drm/i915: Map the ringbuffer using WB on LLC machines

[PATCH 1/3] drm/i915: Map the ringbuffer using WB on LLC machines

[Chris Wilson]

If we have llc coherency, we can write directly into the ringbuffer
using ordinary cached writes rather than forcing WC access.

v2: An important consequence is that we can forgo the mappable request
for WB ringbuffers, allowing for many more simultaneous contexts.

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

diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index d608be46ea6e..f81ec7785fac 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1966,11 +1966,35 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
 
 void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
 {
-	iounmap(ringbuf->virtual_start);
+	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
+		vunmap(ringbuf->virtual_start);
+	else
+		iounmap(ringbuf->virtual_start);
 	ringbuf->virtual_start = 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)
 {
@@ -1978,21 +2002,39 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
 	struct drm_i915_gem_object *obj = ringbuf->obj;
 	int ret;
 
-	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
-	if (ret)
-		return ret;
+	if (HAS_LLC(dev_priv) && !obj->stolen) {
+		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, 0);
+		if (ret)
+			return ret;
 
-	ret = i915_gem_object_set_to_gtt_domain(obj, true);
-	if (ret) {
-		i915_gem_object_ggtt_unpin(obj);
-		return ret;
-	}
+		ret = i915_gem_object_set_to_cpu_domain(obj, true);
+		if (ret) {
+			i915_gem_object_ggtt_unpin(obj);
+			return ret;
+		}
+
+		ringbuf->virtual_start = vmap_obj(obj);
+		if (ringbuf->virtual_start == NULL) {
+			i915_gem_object_ggtt_unpin(obj);
+			return -ENOMEM;
+		}
+	} else {
+		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
+		if (ret)
+			return ret;
 
-	ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
-			i915_gem_obj_ggtt_offset(obj), ringbuf->size);
-	if (ringbuf->virtual_start == NULL) {
-		i915_gem_object_ggtt_unpin(obj);
-		return -EINVAL;
+		ret = i915_gem_object_set_to_gtt_domain(obj, true);
+		if (ret) {
+			i915_gem_object_ggtt_unpin(obj);
+			return ret;
+		}
+
+		ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
+						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
+		if (ringbuf->virtual_start == NULL) {
+			i915_gem_object_ggtt_unpin(obj);
+			return -EINVAL;
+		}
 	}
 
 	return 0;
-- 
2.6.1

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

[PATCH 2/3] 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.

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         | 41 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 55 +++++----------------------------
 drivers/gpu/drm/i915/intel_ringbuffer.c | 53 ++++++++++---------------------
 4 files changed, 72 insertions(+), 89 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4093eedfd664..343a0a723d2c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2089,10 +2089,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.
@@ -2840,12 +2837,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 78640aecf86d..446cf0662a38 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2159,6 +2159,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
+	if (obj->vmapping) {
+		vunmap(obj->vmapping);
+		obj->vmapping = NULL;
+	}
+
 	i915_gem_object_invalidate(obj);
 
 	return 0;
@@ -2325,6 +2330,42 @@ 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);
+		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);
+			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 e9c2bfd85b52..5c4163d3845a 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);
 
-	i915_gem_object_unpin_pages(obj);
-
+	mutex_lock(&obj->base.dev->struct_mutex);
+	i915_gem_object_unpin_vmap(obj);
 	mutex_unlock(&obj->base.dev->struct_mutex);
 }
 
@@ -110,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)
@@ -163,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_pages(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 f81ec7785fac..49aa52440db2 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1967,34 +1967,12 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
 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;
 	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)
 {
@@ -2008,15 +1986,14 @@ 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 unpin;
 
-		ringbuf->virtual_start = vmap_obj(obj);
-		if (ringbuf->virtual_start == NULL) {
-			i915_gem_object_ggtt_unpin(obj);
-			return -ENOMEM;
+		ringbuf->virtual_start = i915_gem_object_pin_vmap(obj);
+		if (IS_ERR(ringbuf->virtual_start)) {
+			ret = PTR_ERR(ringbuf->virtual_start);
+			ringbuf->virtual_start = NULL;
+			goto unpin;
 		}
 	} else {
 		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
@@ -2024,20 +2001,22 @@ 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 unpin;
 
 		ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.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 unpin;
 		}
 	}
 
 	return 0;
+
+unpin:
+	i915_gem_object_ggtt_unpin(obj);
+	return ret;
 }
 
 void intel_destroy_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
-- 
2.6.1

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

[PATCH 3/3] drm/i915: Treat ringbuffer writes as write to normal memory

[Chris Wilson]

Ringbuffers are now being written to either through LLC or WC paths, so
treating them as simply iomem is no longer adequate. However, for the
older !llc hardware, the hardware is documentated as treating the TAIL
register update as serialising, so we can relax the barriers when filling
the rings (but even if it were not, it is still an uncached register write
and so serialising anyway.).

For simplicity, let's ignore the iomem annotation.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_lrc.c        |  7 +------
 drivers/gpu/drm/i915/intel_lrc.h        |  6 +++---
 drivers/gpu/drm/i915/intel_ringbuffer.c |  7 +------
 drivers/gpu/drm/i915/intel_ringbuffer.h | 17 ++++++++++++-----
 4 files changed, 17 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index d38746c5370d..10020505be75 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -713,13 +713,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
 
 static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
 {
-	uint32_t __iomem *virt;
 	int rem = ringbuf->size - ringbuf->tail;
-
-	virt = ringbuf->virtual_start + ringbuf->tail;
-	rem /= 4;
-	while (rem--)
-		iowrite32(MI_NOOP, virt++);
+	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
 
 	ringbuf->tail = 0;
 	intel_ring_update_space(ringbuf);
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 64f89f9982a2..8b56fb934763 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -53,8 +53,9 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
  */
 static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
 {
-	ringbuf->tail &= ringbuf->size - 1;
+	intel_ringbuffer_advance(ringbuf);
 }
+
 /**
  * intel_logical_ring_emit() - write a DWORD to the ringbuffer.
  * @ringbuf: Ringbuffer to write to.
@@ -63,8 +64,7 @@ static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
 static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
 					   u32 data)
 {
-	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
-	ringbuf->tail += 4;
+	intel_ringbuffer_emit(ringbuf, data);
 }
 
 /* Logical Ring Contexts */
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 49aa52440db2..0eaaab92dea0 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -2180,13 +2180,8 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
 
 static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
 {
-	uint32_t __iomem *virt;
 	int rem = ringbuf->size - ringbuf->tail;
-
-	virt = ringbuf->virtual_start + ringbuf->tail;
-	rem /= 4;
-	while (rem--)
-		iowrite32(MI_NOOP, virt++);
+	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
 
 	ringbuf->tail = 0;
 	intel_ring_update_space(ringbuf);
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 2e85fda94963..ff290765e6c9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -427,17 +427,24 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request);
 
 int __must_check intel_ring_begin(struct drm_i915_gem_request *req, int n);
 int __must_check intel_ring_cacheline_align(struct drm_i915_gem_request *req);
+static inline void intel_ringbuffer_emit(struct intel_ringbuffer *rb,
+					 u32 data)
+{
+	*(uint32_t __force *)(rb->virtual_start + rb->tail) = data;
+	rb->tail += 4;
+}
+static inline void intel_ringbuffer_advance(struct intel_ringbuffer *rb)
+{
+	rb->tail &= rb->size - 1;
+}
 static inline void intel_ring_emit(struct intel_engine_cs *ring,
 				   u32 data)
 {
-	struct intel_ringbuffer *ringbuf = ring->buffer;
-	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
-	ringbuf->tail += 4;
+	intel_ringbuffer_emit(ring->buffer, data);
 }
 static inline void intel_ring_advance(struct intel_engine_cs *ring)
 {
-	struct intel_ringbuffer *ringbuf = ring->buffer;
-	ringbuf->tail &= ringbuf->size - 1;
+	intel_ringbuffer_advance(ring->buffer);
 }
 int __intel_ring_space(int head, int tail, int size);
 void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
-- 
2.6.1

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

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

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 01:39:55PM +0100, 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.
> 
> 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         | 41 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_gem_dmabuf.c  | 55 +++++----------------------------
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 53 ++++++++++---------------------
>  4 files changed, 72 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 4093eedfd664..343a0a723d2c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2089,10 +2089,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.
> @@ -2840,12 +2837,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 78640aecf86d..446cf0662a38 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2159,6 +2159,11 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>  	ops->put_pages(obj);
>  	obj->pages = NULL;
>  
> +	if (obj->vmapping) {
> +		vunmap(obj->vmapping);
> +		obj->vmapping = NULL;
> +	}
> +
>  	i915_gem_object_invalidate(obj);
>  
>  	return 0;
> @@ -2325,6 +2330,42 @@ 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);

Random driveby: kmalloc_array()

Also __GFP_NOWARN?

I wonder if the drm_malloc stuff should do the kmalloc attempt
regardless fo the size, so we wouldn't have to do it here?

> +		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);
> +			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)
>  {
<snip>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Thu, Oct 08, 2015 at 03:58:13PM +0300, Ville Syrjälä wrote:
> > +		pages = kmalloc(n*sizeof(*pages), GFP_TEMPORARY);
> 
> Random driveby: kmalloc_array()

That would be scary, imagine having enough pages in the object to
overflow unsigned long :)
 
> Also __GFP_NOWARN?

True.
 
> I wonder if the drm_malloc stuff should do the kmalloc attempt
> regardless fo the size, so we wouldn't have to do it here?

It's a pattern I've reused in a few places, so probably worthy of being
refactored. Something like

-static __inline__ void *drm_malloc_ab(size_t nmemb, size_t size)
+static __inline__ void *drm_malloc_ab(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_KERNEL);
+               return kmalloc(nmemb * size, gfp);
+
+       if (gfp & GFP_TEMPORARY) {
+               void *ptr = kmalloc(nmemb * size, gfp | __GFP_NOWARN);
+               if (ptr)
+                       return ptr;
+       }
 
        return __vmalloc(size * nmemb,
-                        GFP_KERNEL | __GFP_HIGHMEM, PAGE_KERNEL);
+                        gfp | __GFP_HIGHMEM, PAGE_KERNEL);
 }
 

Would need to call it my_drm_malloc_ab() to avoid having to update
everywhere.
-Chris

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

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

[kbuild test robot]

Hi Chris,

[auto build test WARNING on v4.3-rc4 -- if it's inappropriate base, please ignore]

reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> drivers/gpu/drm/i915/intel_ringbuffer.c:1990:40: sparse: incorrect type in assignment (different address spaces)
   drivers/gpu/drm/i915/intel_ringbuffer.c:1990:40:    expected void [noderef] <asn:2>*virtual_start
   drivers/gpu/drm/i915/intel_ringbuffer.c:1990:40:    got void *

vim +1990 drivers/gpu/drm/i915/intel_ringbuffer.c

  1974	int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
  1975					     struct intel_ringbuffer *ringbuf)
  1976	{
  1977		struct drm_i915_private *dev_priv = to_i915(dev);
  1978		struct drm_i915_gem_object *obj = ringbuf->obj;
  1979		int ret;
  1980	
  1981		if (HAS_LLC(dev_priv) && !obj->stolen) {
  1982			ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, 0);
  1983			if (ret)
  1984				return ret;
  1985	
  1986			ret = i915_gem_object_set_to_cpu_domain(obj, true);
  1987			if (ret)
  1988				goto unpin;
  1989	
> 1990			ringbuf->virtual_start = i915_gem_object_pin_vmap(obj);
  1991			if (IS_ERR(ringbuf->virtual_start)) {
  1992				ret = PTR_ERR(ringbuf->virtual_start);
  1993				ringbuf->virtual_start = NULL;
  1994				goto unpin;
  1995			}
  1996		} else {
  1997			ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
  1998			if (ret)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915: Map the ringbuffer using WB on LLC machines

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 01:39:54PM +0100, Chris Wilson wrote:
> If we have llc coherency, we can write directly into the ringbuffer
> using ordinary cached writes rather than forcing WC access.
> 
> v2: An important consequence is that we can forgo the mappable request
> for WB ringbuffers, allowing for many more simultaneous contexts.

Fits in with my understanding on llc coherency.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_ringbuffer.c | 70 ++++++++++++++++++++++++++-------
>  1 file changed, 56 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index d608be46ea6e..f81ec7785fac 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -1966,11 +1966,35 @@ static int init_phys_status_page(struct intel_engine_cs *ring)
>  
>  void intel_unpin_ringbuffer_obj(struct intel_ringbuffer *ringbuf)
>  {
> -	iounmap(ringbuf->virtual_start);
> +	if (HAS_LLC(ringbuf->obj->base.dev) && !ringbuf->obj->stolen)
> +		vunmap(ringbuf->virtual_start);
> +	else
> +		iounmap(ringbuf->virtual_start);
>  	ringbuf->virtual_start = 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)
>  {
> @@ -1978,21 +2002,39 @@ int intel_pin_and_map_ringbuffer_obj(struct drm_device *dev,
>  	struct drm_i915_gem_object *obj = ringbuf->obj;
>  	int ret;
>  
> -	ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
> -	if (ret)
> -		return ret;
> +	if (HAS_LLC(dev_priv) && !obj->stolen) {
> +		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, 0);
> +		if (ret)
> +			return ret;
>  
> -	ret = i915_gem_object_set_to_gtt_domain(obj, true);
> -	if (ret) {
> -		i915_gem_object_ggtt_unpin(obj);
> -		return ret;
> -	}
> +		ret = i915_gem_object_set_to_cpu_domain(obj, true);
> +		if (ret) {
> +			i915_gem_object_ggtt_unpin(obj);
> +			return ret;
> +		}
> +
> +		ringbuf->virtual_start = vmap_obj(obj);
> +		if (ringbuf->virtual_start == NULL) {
> +			i915_gem_object_ggtt_unpin(obj);
> +			return -ENOMEM;
> +		}
> +	} else {
> +		ret = i915_gem_obj_ggtt_pin(obj, PAGE_SIZE, PIN_MAPPABLE);
> +		if (ret)
> +			return ret;
>  
> -	ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
> -			i915_gem_obj_ggtt_offset(obj), ringbuf->size);
> -	if (ringbuf->virtual_start == NULL) {
> -		i915_gem_object_ggtt_unpin(obj);
> -		return -EINVAL;
> +		ret = i915_gem_object_set_to_gtt_domain(obj, true);
> +		if (ret) {
> +			i915_gem_object_ggtt_unpin(obj);
> +			return ret;
> +		}
> +
> +		ringbuf->virtual_start = ioremap_wc(dev_priv->gtt.mappable_base +
> +						    i915_gem_obj_ggtt_offset(obj), ringbuf->size);
> +		if (ringbuf->virtual_start == NULL) {
> +			i915_gem_object_ggtt_unpin(obj);
> +			return -EINVAL;
> +		}
>  	}
>  
>  	return 0;
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Treat ringbuffer writes as write to normal memory

[Ville Syrjälä]

On Thu, Oct 08, 2015 at 01:39:56PM +0100, Chris Wilson wrote:
> Ringbuffers are now being written to either through LLC or WC paths, so
> treating them as simply iomem is no longer adequate. However, for the
> older !llc hardware, the hardware is documentated as treating the TAIL
> register update as serialising, so we can relax the barriers when filling
> the rings (but even if it were not, it is still an uncached register write
> and so serialising anyway.).
> 
> For simplicity, let's ignore the iomem annotation.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c        |  7 +------
>  drivers/gpu/drm/i915/intel_lrc.h        |  6 +++---
>  drivers/gpu/drm/i915/intel_ringbuffer.c |  7 +------
>  drivers/gpu/drm/i915/intel_ringbuffer.h | 17 ++++++++++++-----
>  4 files changed, 17 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index d38746c5370d..10020505be75 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -713,13 +713,8 @@ intel_logical_ring_advance_and_submit(struct drm_i915_gem_request *request)
>  
>  static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>  {
> -	uint32_t __iomem *virt;
>  	int rem = ringbuf->size - ringbuf->tail;
> -
> -	virt = ringbuf->virtual_start + ringbuf->tail;
> -	rem /= 4;
> -	while (rem--)
> -		iowrite32(MI_NOOP, virt++);
> +	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
>  
>  	ringbuf->tail = 0;
>  	intel_ring_update_space(ringbuf);
> diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
> index 64f89f9982a2..8b56fb934763 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.h
> +++ b/drivers/gpu/drm/i915/intel_lrc.h
> @@ -53,8 +53,9 @@ int logical_ring_flush_all_caches(struct drm_i915_gem_request *req);
>   */
>  static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
>  {
> -	ringbuf->tail &= ringbuf->size - 1;
> +	intel_ringbuffer_advance(ringbuf);
>  }
> +
>  /**
>   * intel_logical_ring_emit() - write a DWORD to the ringbuffer.
>   * @ringbuf: Ringbuffer to write to.
> @@ -63,8 +64,7 @@ static inline void intel_logical_ring_advance(struct intel_ringbuffer *ringbuf)
>  static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
>  					   u32 data)
>  {
> -	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
> -	ringbuf->tail += 4;
> +	intel_ringbuffer_emit(ringbuf, data);
>  }
>  
>  /* Logical Ring Contexts */
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 49aa52440db2..0eaaab92dea0 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -2180,13 +2180,8 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
>  
>  static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
>  {
> -	uint32_t __iomem *virt;
>  	int rem = ringbuf->size - ringbuf->tail;
> -
> -	virt = ringbuf->virtual_start + ringbuf->tail;
> -	rem /= 4;
> -	while (rem--)
> -		iowrite32(MI_NOOP, virt++);
> +	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);

Hmm. The lrc copy looks identical to this one. But looks like most of
intel_lrc.c ring handling is copy pasted anyway, so there's even more
that could be thrown out.

This patch seems OK to me
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
>  	ringbuf->tail = 0;
>  	intel_ring_update_space(ringbuf);
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 2e85fda94963..ff290765e6c9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -427,17 +427,24 @@ int intel_ring_alloc_request_extras(struct drm_i915_gem_request *request);
>  
>  int __must_check intel_ring_begin(struct drm_i915_gem_request *req, int n);
>  int __must_check intel_ring_cacheline_align(struct drm_i915_gem_request *req);
> +static inline void intel_ringbuffer_emit(struct intel_ringbuffer *rb,
> +					 u32 data)
> +{
> +	*(uint32_t __force *)(rb->virtual_start + rb->tail) = data;
> +	rb->tail += 4;
> +}
> +static inline void intel_ringbuffer_advance(struct intel_ringbuffer *rb)
> +{
> +	rb->tail &= rb->size - 1;
> +}
>  static inline void intel_ring_emit(struct intel_engine_cs *ring,
>  				   u32 data)
>  {
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	iowrite32(data, ringbuf->virtual_start + ringbuf->tail);
> -	ringbuf->tail += 4;
> +	intel_ringbuffer_emit(ring->buffer, data);
>  }
>  static inline void intel_ring_advance(struct intel_engine_cs *ring)
>  {
> -	struct intel_ringbuffer *ringbuf = ring->buffer;
> -	ringbuf->tail &= ringbuf->size - 1;
> +	intel_ringbuffer_advance(ring->buffer);
>  }
>  int __intel_ring_space(int head, int tail, int size);
>  void intel_ring_update_space(struct intel_ringbuffer *ringbuf);
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Treat ringbuffer writes as write to normal memory

[Chris Wilson]

On Fri, Oct 16, 2015 at 06:06:09PM +0300, Ville Syrjälä wrote:
> On Thu, Oct 08, 2015 at 01:39:56PM +0100, Chris Wilson wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > index 49aa52440db2..0eaaab92dea0 100644
> > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> > @@ -2180,13 +2180,8 @@ static int ring_wait_for_space(struct intel_engine_cs *ring, int n)
> >  
> >  static void __wrap_ring_buffer(struct intel_ringbuffer *ringbuf)
> >  {
> > -	uint32_t __iomem *virt;
> >  	int rem = ringbuf->size - ringbuf->tail;
> > -
> > -	virt = ringbuf->virtual_start + ringbuf->tail;
> > -	rem /= 4;
> > -	while (rem--)
> > -		iowrite32(MI_NOOP, virt++);
> > +	memset(ringbuf->virtual_start + ringbuf->tail, 0, rem);
> 
> Hmm. The lrc copy looks identical to this one. But looks like most of
> intel_lrc.c ring handling is copy pasted anyway, so there's even more
> that could be thrown out.

Yes, only this time I am trying to be sneaky and only delete small
chunks at a time. It may take a few years to do what could be done in a
morning, but whatever.
-Chris

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