[PATCH 3/3] Introduce & use new lightweight SGL iterators

[PATCH 3/3] Introduce & use new lightweight SGL iterators

[Dave Gordon]

The existing for_each_sg_page() iterator is somewhat heavyweight, and is
limiting i915 driver performance in a few benchmarks. So here we
introduce somewhat lighter weight iterators, primarily for use with GEM
objects or other case where we need only deal with whole aligned pages.

Unlike the old iterator, the new iterators use an internal state
structure which is not intended to be accessed by the caller; instead
each takes as a parameter an output variable which is set before each
iteration. This makes them particularly simple to use :)

One of the new iterators provides the caller with the DMA address of
each page in turn; the other provides the 'struct page' pointer required
by many memory management operations.

Various uses of for_each_sg_page() are then converted to the new macros.

Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: linux-kernel@vger.kernel.org
---
 drivers/gpu/drm/i915/i915_drv.h         | 62 +++++++++++++++++++++++++--
 drivers/gpu/drm/i915/i915_gem.c         | 20 ++++-----
 drivers/gpu/drm/i915/i915_gem_fence.c   | 14 +++---
 drivers/gpu/drm/i915/i915_gem_gtt.c     | 76 ++++++++++++++++-----------------
 drivers/gpu/drm/i915/i915_gem_userptr.c |  7 ++-
 5 files changed, 116 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 72f0b02..c0fc6aa 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2125,6 +2125,10 @@ struct drm_i915_gem_object_ops {
 #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
 	(0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
 
+void i915_gem_track_fb(struct drm_i915_gem_object *old,
+		       struct drm_i915_gem_object *new,
+		       unsigned frontbuffer_bits);
+
 struct drm_i915_gem_object {
 	struct drm_gem_object base;
 
@@ -2256,9 +2260,61 @@ struct drm_i915_gem_object {
 };
 #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
 
-void i915_gem_track_fb(struct drm_i915_gem_object *old,
-		       struct drm_i915_gem_object *new,
-		       unsigned frontbuffer_bits);
+/*
+ * New optimised SGL iterator for i915 GEM objects
+ */
+struct sgt_iter {
+	struct scatterlist *sgp;
+	union {
+		unsigned long pfn;
+		unsigned long dma;
+	} ix;
+	unsigned int curr;
+	unsigned int max;
+};
+
+/* Constructor for new iterator */
+static inline struct sgt_iter
+__sgt_iter(struct scatterlist *sgl, bool dma)
+{
+	struct sgt_iter s = { .sgp = sgl };
+
+	if (sgl) {
+		s.max = s.curr = sgl->offset;
+		s.max += sgl->length;
+		if (dma)
+			s.ix.dma = sg_dma_address(sgl);
+		else
+			s.ix.pfn = page_to_pfn(sg_page(sgl));
+	}
+
+	return s;
+}
+
+/**
+ * for_each_sgt_dma - iterate over the DMA addresses of the given sg_table
+ * @__dmap:	DMA address (output)
+ * @__iter:	'struct sgt_iter' (iterator state, internal)
+ * @__sgt:	sg_table to iterate over (input)
+ */
+#define for_each_sgt_dma(__dmap, __iter, __sgt)				\
+	for ((__iter) = __sgt_iter((__sgt)->sgl, true);			\
+	     ((__dmap) = (__iter).ix.dma + (__iter).curr);		\
+	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
+		((__iter) = __sgt_iter(sg_next((__iter).sgp), true), 0))
+
+/**
+ * for_each_sgt_page - iterate over the pages of the given sg_table
+ * @__pp:	page pointer (output)
+ * @__iter:	'struct sgt_iter' (iterator state, internal)
+ * @__sgt:	sg_table to iterate over (input)
+ */
+#define for_each_sgt_page(__pp, __iter, __sgt)				\
+	for ((__iter) = __sgt_iter((__sgt)->sgl, false);		\
+	     ((__pp) = (__iter).ix.pfn == 0 ? NULL :			\
+		pfn_to_page((__iter).ix.pfn + ((__iter).curr >> PAGE_SHIFT)));\
+	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
+		((__iter) = __sgt_iter(sg_next((__iter).sgp), false), 0))
 
 /**
  * Request queue structure.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 38e4e1e..0fe7e95 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2165,7 +2165,8 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 static void
 i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 {
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	struct page *page;
 	int ret;
 
 	BUG_ON(obj->madv == __I915_MADV_PURGED);
@@ -2187,9 +2188,7 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 	if (obj->madv == I915_MADV_DONTNEED)
 		obj->dirty = 0;
 
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
-		struct page *page = sg_page_iter_page(&sg_iter);
-
+	for_each_sgt_page(page, sgt_iter, obj->pages) {
 		if (obj->dirty)
 			set_page_dirty(page);
 
@@ -2246,7 +2245,7 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 	struct address_space *mapping;
 	struct sg_table *st;
 	struct scatterlist *sg;
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
 	struct page *page;
 	unsigned long last_pfn = 0;	/* suppress gcc warning */
 	int ret;
@@ -2343,8 +2342,8 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		put_page(sg_page_iter_page(&sg_iter));
+	for_each_sgt_page(page, sgt_iter, st)
+		put_page(page);
 	sg_free_table(st);
 	kfree(st);
 
@@ -2403,7 +2402,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 {
 	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
 	struct sg_table *sgt = obj->pages;
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	struct page *page;
 	struct page *stack_pages[32];
 	struct page **pages = stack_pages;
 	unsigned long i = 0;
@@ -2423,8 +2423,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
 		}
 	}
 
-	for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0)
-		pages[i++] = sg_page_iter_page(&sg_iter);
+	for_each_sgt_page(page, sgt_iter, sgt)
+		pages[i++] = page;
 
 	/* Check that we have the expected number of pages */
 	GEM_BUG_ON(i != n_pages);
diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
index a2b938e..2b6bdc2 100644
--- a/drivers/gpu/drm/i915/i915_gem_fence.c
+++ b/drivers/gpu/drm/i915/i915_gem_fence.c
@@ -745,15 +745,15 @@ void i915_gem_restore_fences(struct drm_device *dev)
 void
 i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj)
 {
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	struct page *page;
 	int i;
 
 	if (obj->bit_17 == NULL)
 		return;
 
 	i = 0;
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
-		struct page *page = sg_page_iter_page(&sg_iter);
+	for_each_sgt_page(page, sgt_iter, obj->pages) {
 		char new_bit_17 = page_to_phys(page) >> 17;
 		if ((new_bit_17 & 0x1) !=
 		    (test_bit(i, obj->bit_17) != 0)) {
@@ -775,7 +775,8 @@ void i915_gem_restore_fences(struct drm_device *dev)
 void
 i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
 {
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	struct page *page;
 	int page_count = obj->base.size >> PAGE_SHIFT;
 	int i;
 
@@ -790,8 +791,9 @@ void i915_gem_restore_fences(struct drm_device *dev)
 	}
 
 	i = 0;
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
-		if (page_to_phys(sg_page_iter_page(&sg_iter)) & (1 << 17))
+
+	for_each_sgt_page(page, sgt_iter, obj->pages) {
+		if (page_to_phys(page) & (1 << 17))
 			__set_bit(i, obj->bit_17);
 		else
 			__clear_bit(i, obj->bit_17);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 7eab619..4668477 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1839,20 +1839,19 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
 				      enum i915_cache_level cache_level, u32 flags)
 {
 	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
-	gen6_pte_t *pt_vaddr;
 	unsigned first_entry = start >> PAGE_SHIFT;
 	unsigned act_pt = first_entry / GEN6_PTES;
 	unsigned act_pte = first_entry % GEN6_PTES;
-	struct sg_page_iter sg_iter;
+	gen6_pte_t *pt_vaddr = NULL;
+	struct sgt_iter sgt_iter;
+	dma_addr_t addr;
 
-	pt_vaddr = NULL;
-	for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
+	for_each_sgt_dma(addr, sgt_iter, pages) {
 		if (pt_vaddr == NULL)
 			pt_vaddr = kmap_px(ppgtt->pd.page_table[act_pt]);
 
 		pt_vaddr[act_pte] =
-			vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
-				       cache_level, true, flags);
+			vm->pte_encode(addr, cache_level, true, flags);
 
 		if (++act_pte == GEN6_PTES) {
 			kunmap_px(ppgtt, pt_vaddr);
@@ -1861,6 +1860,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
 			act_pte = 0;
 		}
 	}
+
 	if (pt_vaddr)
 		kunmap_px(ppgtt, pt_vaddr);
 }
@@ -2362,22 +2362,20 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
-	unsigned first_entry = start >> PAGE_SHIFT;
-	gen8_pte_t __iomem *gtt_entries =
-		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
-	int i = 0;
-	struct sg_page_iter sg_iter;
-	dma_addr_t addr = 0; /* shut up gcc */
+	struct sgt_iter sgt_iter;
+	gen8_pte_t __iomem *gtt_entries;
+	gen8_pte_t gtt_entry;
+	dma_addr_t addr;
 	int rpm_atomic_seq;
+	int i = 0;
 
 	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
-		addr = sg_dma_address(sg_iter.sg) +
-			(sg_iter.sg_pgoffset << PAGE_SHIFT);
-		gen8_set_pte(&gtt_entries[i],
-			     gen8_pte_encode(addr, level, true));
-		i++;
+	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm + (start >> PAGE_SHIFT);
+
+	for_each_sgt_dma(addr, sgt_iter, st) {
+		gtt_entry = gen8_pte_encode(addr, level, true);
+		gen8_set_pte(&gtt_entries[i++], gtt_entry);
 	}
 
 	/*
@@ -2388,8 +2386,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 	 * hardware should work, we must keep this posting read for paranoia.
 	 */
 	if (i != 0)
-		WARN_ON(readq(&gtt_entries[i-1])
-			!= gen8_pte_encode(addr, level, true));
+		WARN_ON(readq(&gtt_entries[i-1]) != gtt_entry);
 
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
@@ -2440,20 +2437,20 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 {
 	struct drm_i915_private *dev_priv = to_i915(vm->dev);
 	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
-	unsigned first_entry = start >> PAGE_SHIFT;
-	gen6_pte_t __iomem *gtt_entries =
-		(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
-	int i = 0;
-	struct sg_page_iter sg_iter;
-	dma_addr_t addr = 0;
+	struct sgt_iter sgt_iter;
+	gen6_pte_t __iomem *gtt_entries;
+	gen6_pte_t gtt_entry;
+	dma_addr_t addr;
 	int rpm_atomic_seq;
+	int i = 0;
 
 	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
 
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
-		addr = sg_page_iter_dma_address(&sg_iter);
-		iowrite32(vm->pte_encode(addr, level, true, flags), &gtt_entries[i]);
-		i++;
+	gtt_entries = (gen6_pte_t __iomem *)ggtt->gsm + (start >> PAGE_SHIFT);
+
+	for_each_sgt_dma(addr, sgt_iter, st) {
+		gtt_entry = vm->pte_encode(addr, level, true, flags);
+		iowrite32(gtt_entry, &gtt_entries[i++]);
 	}
 
 	/* XXX: This serves as a posting read to make sure that the PTE has
@@ -2462,10 +2459,8 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
 	 * of NUMA access patterns. Therefore, even with the way we assume
 	 * hardware should work, we must keep this posting read for paranoia.
 	 */
-	if (i != 0) {
-		unsigned long gtt = readl(&gtt_entries[i-1]);
-		WARN_ON(gtt != vm->pte_encode(addr, level, true, flags));
-	}
+	if (i != 0)
+		WARN_ON(readl(&gtt_entries[i-1]) != gtt_entry);
 
 	/* This next bit makes the above posting read even more important. We
 	 * want to flush the TLBs only after we're certain all the PTE updates
@@ -3399,9 +3394,11 @@ struct i915_vma *
 intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info,
 			  struct drm_i915_gem_object *obj)
 {
+	const size_t n_pages = obj->base.size / PAGE_SIZE;
 	unsigned int size_pages = rot_info->plane[0].width * rot_info->plane[0].height;
 	unsigned int size_pages_uv;
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	dma_addr_t dma_addr;
 	unsigned long i;
 	dma_addr_t *page_addr_list;
 	struct sg_table *st;
@@ -3410,7 +3407,7 @@ struct i915_vma *
 	int ret = -ENOMEM;
 
 	/* Allocate a temporary list of source pages for random access. */
-	page_addr_list = drm_malloc_gfp(obj->base.size / PAGE_SIZE,
+	page_addr_list = drm_malloc_gfp(n_pages,
 					sizeof(dma_addr_t),
 					GFP_TEMPORARY);
 	if (!page_addr_list)
@@ -3433,11 +3430,10 @@ struct i915_vma *
 
 	/* Populate source page list from the object. */
 	i = 0;
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
-		page_addr_list[i] = sg_page_iter_dma_address(&sg_iter);
-		i++;
-	}
+	for_each_sgt_dma(dma_addr, sgt_iter, obj->pages)
+		page_addr_list[i++] = dma_addr;
 
+	GEM_BUG_ON(i != n_pages);
 	st->nents = 0;
 	sg = st->sgl;
 
diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
index 32d9726..d2c1595 100644
--- a/drivers/gpu/drm/i915/i915_gem_userptr.c
+++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
@@ -706,7 +706,8 @@ struct get_pages_work {
 static void
 i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
 {
-	struct sg_page_iter sg_iter;
+	struct sgt_iter sgt_iter;
+	struct page *page;
 
 	BUG_ON(obj->userptr.work != NULL);
 	__i915_gem_userptr_set_active(obj, false);
@@ -716,9 +717,7 @@ struct get_pages_work {
 
 	i915_gem_gtt_finish_object(obj);
 
-	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
-		struct page *page = sg_page_iter_page(&sg_iter);
-
+	for_each_sgt_page(page, sgt_iter, obj->pages) {
 		if (obj->dirty)
 			set_page_dirty(page);
 
-- 
1.9.1

Re: [Intel-gfx] [PATCH 3/3] Introduce & use new lightweight SGL iterators

[Tvrtko Ursulin]

On 16/05/16 16:19, Dave Gordon wrote:
> The existing for_each_sg_page() iterator is somewhat heavyweight, and is
> limiting i915 driver performance in a few benchmarks. So here we
> introduce somewhat lighter weight iterators, primarily for use with GEM
> objects or other case where we need only deal with whole aligned pages.

Interesting idea, if for nothing then for eliminating the dreaded 
st->nents of for_each_sg_page. :)

Which benchmarks it improves and how much do you know?

It generates more code for me but that seems to be by design, yes? 
Because what were previously calls to __sg_page_iter_next is now 
effectively inlined and instead there is only a call to sg_next, which 
__sg_page_iter_next would call anyway.

> Unlike the old iterator, the new iterators use an internal state
> structure which is not intended to be accessed by the caller; instead
> each takes as a parameter an output variable which is set before each
> iteration. This makes them particularly simple to use :)
>
> One of the new iterators provides the caller with the DMA address of
> each page in turn; the other provides the 'struct page' pointer required
> by many memory management operations.
>
> Various uses of for_each_sg_page() are then converted to the new macros.
>
> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: linux-kernel@vger.kernel.org
> ---
>   drivers/gpu/drm/i915/i915_drv.h         | 62 +++++++++++++++++++++++++--
>   drivers/gpu/drm/i915/i915_gem.c         | 20 ++++-----
>   drivers/gpu/drm/i915/i915_gem_fence.c   | 14 +++---
>   drivers/gpu/drm/i915/i915_gem_gtt.c     | 76 ++++++++++++++++-----------------
>   drivers/gpu/drm/i915/i915_gem_userptr.c |  7 ++-
>   5 files changed, 116 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 72f0b02..c0fc6aa 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2125,6 +2125,10 @@ struct drm_i915_gem_object_ops {
>   #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
>   	(0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>
> +void i915_gem_track_fb(struct drm_i915_gem_object *old,
> +		       struct drm_i915_gem_object *new,
> +		       unsigned frontbuffer_bits);
> +
>   struct drm_i915_gem_object {
>   	struct drm_gem_object base;
>
> @@ -2256,9 +2260,61 @@ struct drm_i915_gem_object {
>   };
>   #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object, base)
>
> -void i915_gem_track_fb(struct drm_i915_gem_object *old,
> -		       struct drm_i915_gem_object *new,
> -		       unsigned frontbuffer_bits);
> +/*
> + * New optimised SGL iterator for i915 GEM objects
> + */
> +struct sgt_iter {
> +	struct scatterlist *sgp;
> +	union {
> +		unsigned long pfn;
> +		unsigned long dma;

dma_addr_t

> +	} ix;
> +	unsigned int curr;
> +	unsigned int max;

I think since this counts in bytes it should match obj->base.size which 
is a size_t?

Also, maybe the iterator would be more efficient if it counted in pages? 
Hm, probably makes no difference, but maybe worth exploring. Was just 
thinking about avoiding the need to to convert byte position to page 
position in every iteration.

> +};
> +
> +/* Constructor for new iterator */
> +static inline struct sgt_iter
> +__sgt_iter(struct scatterlist *sgl, bool dma)
> +{
> +	struct sgt_iter s = { .sgp = sgl };
> +
> +	if (sgl) {
> +		s.max = s.curr = sgl->offset;
> +		s.max += sgl->length;
> +		if (dma)
> +			s.ix.dma = sg_dma_address(sgl);
> +		else
> +			s.ix.pfn = page_to_pfn(sg_page(sgl));

I suppose we can rely on the compiler to optimize one branch away rather 
than having to provide two iterators.

> +	}
> +
> +	return s;
> +}
> +
> +/**
> + * for_each_sgt_dma - iterate over the DMA addresses of the given sg_table
> + * @__dmap:	DMA address (output)
> + * @__iter:	'struct sgt_iter' (iterator state, internal)
> + * @__sgt:	sg_table to iterate over (input)
> + */
> +#define for_each_sgt_dma(__dmap, __iter, __sgt)				\
> +	for ((__iter) = __sgt_iter((__sgt)->sgl, true);			\
> +	     ((__dmap) = (__iter).ix.dma + (__iter).curr);		\
> +	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
> +		((__iter) = __sgt_iter(sg_next((__iter).sgp), true), 0))
> +
> +/**
> + * for_each_sgt_page - iterate over the pages of the given sg_table
> + * @__pp:	page pointer (output)
> + * @__iter:	'struct sgt_iter' (iterator state, internal)
> + * @__sgt:	sg_table to iterate over (input)
> + */
> +#define for_each_sgt_page(__pp, __iter, __sgt)				\
> +	for ((__iter) = __sgt_iter((__sgt)->sgl, false);		\
> +	     ((__pp) = (__iter).ix.pfn == 0 ? NULL :			\
> +		pfn_to_page((__iter).ix.pfn + ((__iter).curr >> PAGE_SHIFT)));\

I could figure out what is the pfn == 0 check for? The other loop has no 
such checks so one of them looks suspicious or I am missing something.

Also I think to be worth it you would have to handle the offset as well. 
That way all usages for for_each_sg_page could be converted and without 
it it leaves a mix of new and old.

> +	     (((__iter).curr += PAGE_SIZE) < (__iter).max) ||		\
> +		((__iter) = __sgt_iter(sg_next((__iter).sgp), false), 0))
>
>   /**
>    * Request queue structure.
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 38e4e1e..0fe7e95 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2165,7 +2165,8 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   static void
>   i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>   {
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
>   	int ret;
>
>   	BUG_ON(obj->madv == __I915_MADV_PURGED);
> @@ -2187,9 +2188,7 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	if (obj->madv == I915_MADV_DONTNEED)
>   		obj->dirty = 0;
>
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> -		struct page *page = sg_page_iter_page(&sg_iter);
> -
> +	for_each_sgt_page(page, sgt_iter, obj->pages) {
>   		if (obj->dirty)
>   			set_page_dirty(page);
>
> @@ -2246,7 +2245,7 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>   	struct address_space *mapping;
>   	struct sg_table *st;
>   	struct scatterlist *sg;
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
>   	struct page *page;
>   	unsigned long last_pfn = 0;	/* suppress gcc warning */
>   	int ret;
> @@ -2343,8 +2342,8 @@ static void i915_gem_object_free_mmap_offset(struct drm_i915_gem_object *obj)
>
>   err_pages:
>   	sg_mark_end(sg);
> -	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
> -		put_page(sg_page_iter_page(&sg_iter));
> +	for_each_sgt_page(page, sgt_iter, st)
> +		put_page(page);
>   	sg_free_table(st);
>   	kfree(st);
>
> @@ -2403,7 +2402,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
>   {
>   	unsigned long n_pages = obj->base.size >> PAGE_SHIFT;
>   	struct sg_table *sgt = obj->pages;
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
>   	struct page *stack_pages[32];
>   	struct page **pages = stack_pages;
>   	unsigned long i = 0;
> @@ -2423,8 +2423,8 @@ static void *i915_gem_object_map(const struct drm_i915_gem_object *obj)
>   		}
>   	}
>
> -	for_each_sg_page(sgt->sgl, &sg_iter, sgt->nents, 0)
> -		pages[i++] = sg_page_iter_page(&sg_iter);
> +	for_each_sgt_page(page, sgt_iter, sgt)
> +		pages[i++] = page;
>
>   	/* Check that we have the expected number of pages */
>   	GEM_BUG_ON(i != n_pages);
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence.c b/drivers/gpu/drm/i915/i915_gem_fence.c
> index a2b938e..2b6bdc2 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence.c
> @@ -745,15 +745,15 @@ void i915_gem_restore_fences(struct drm_device *dev)
>   void
>   i915_gem_object_do_bit_17_swizzle(struct drm_i915_gem_object *obj)
>   {
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
>   	int i;
>
>   	if (obj->bit_17 == NULL)
>   		return;
>
>   	i = 0;
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> -		struct page *page = sg_page_iter_page(&sg_iter);
> +	for_each_sgt_page(page, sgt_iter, obj->pages) {
>   		char new_bit_17 = page_to_phys(page) >> 17;
>   		if ((new_bit_17 & 0x1) !=
>   		    (test_bit(i, obj->bit_17) != 0)) {
> @@ -775,7 +775,8 @@ void i915_gem_restore_fences(struct drm_device *dev)
>   void
>   i915_gem_object_save_bit_17_swizzle(struct drm_i915_gem_object *obj)
>   {
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
>   	int page_count = obj->base.size >> PAGE_SHIFT;
>   	int i;
>
> @@ -790,8 +791,9 @@ void i915_gem_restore_fences(struct drm_device *dev)
>   	}
>
>   	i = 0;
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> -		if (page_to_phys(sg_page_iter_page(&sg_iter)) & (1 << 17))
> +
> +	for_each_sgt_page(page, sgt_iter, obj->pages) {
> +		if (page_to_phys(page) & (1 << 17))
>   			__set_bit(i, obj->bit_17);
>   		else
>   			__clear_bit(i, obj->bit_17);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 7eab619..4668477 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -1839,20 +1839,19 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
>   				      enum i915_cache_level cache_level, u32 flags)
>   {
>   	struct i915_hw_ppgtt *ppgtt = i915_vm_to_ppgtt(vm);
> -	gen6_pte_t *pt_vaddr;
>   	unsigned first_entry = start >> PAGE_SHIFT;
>   	unsigned act_pt = first_entry / GEN6_PTES;
>   	unsigned act_pte = first_entry % GEN6_PTES;
> -	struct sg_page_iter sg_iter;
> +	gen6_pte_t *pt_vaddr = NULL;
> +	struct sgt_iter sgt_iter;
> +	dma_addr_t addr;
>
> -	pt_vaddr = NULL;
> -	for_each_sg_page(pages->sgl, &sg_iter, pages->nents, 0) {
> +	for_each_sgt_dma(addr, sgt_iter, pages) {
>   		if (pt_vaddr == NULL)
>   			pt_vaddr = kmap_px(ppgtt->pd.page_table[act_pt]);
>
>   		pt_vaddr[act_pte] =
> -			vm->pte_encode(sg_page_iter_dma_address(&sg_iter),
> -				       cache_level, true, flags);
> +			vm->pte_encode(addr, cache_level, true, flags);
>
>   		if (++act_pte == GEN6_PTES) {
>   			kunmap_px(ppgtt, pt_vaddr);
> @@ -1861,6 +1860,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
>   			act_pte = 0;
>   		}
>   	}
> +
>   	if (pt_vaddr)
>   		kunmap_px(ppgtt, pt_vaddr);
>   }
> @@ -2362,22 +2362,20 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
>   	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> -	unsigned first_entry = start >> PAGE_SHIFT;
> -	gen8_pte_t __iomem *gtt_entries =
> -		(gen8_pte_t __iomem *)ggtt->gsm + first_entry;
> -	int i = 0;
> -	struct sg_page_iter sg_iter;
> -	dma_addr_t addr = 0; /* shut up gcc */
> +	struct sgt_iter sgt_iter;
> +	gen8_pte_t __iomem *gtt_entries;
> +	gen8_pte_t gtt_entry;
> +	dma_addr_t addr;
>   	int rpm_atomic_seq;
> +	int i = 0;
>
>   	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
>
> -	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
> -		addr = sg_dma_address(sg_iter.sg) +
> -			(sg_iter.sg_pgoffset << PAGE_SHIFT);
> -		gen8_set_pte(&gtt_entries[i],
> -			     gen8_pte_encode(addr, level, true));
> -		i++;
> +	gtt_entries = (gen8_pte_t __iomem *)ggtt->gsm + (start >> PAGE_SHIFT);
> +
> +	for_each_sgt_dma(addr, sgt_iter, st) {
> +		gtt_entry = gen8_pte_encode(addr, level, true);
> +		gen8_set_pte(&gtt_entries[i++], gtt_entry);
>   	}
>
>   	/*
> @@ -2388,8 +2386,7 @@ static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
>   	 * hardware should work, we must keep this posting read for paranoia.
>   	 */
>   	if (i != 0)
> -		WARN_ON(readq(&gtt_entries[i-1])
> -			!= gen8_pte_encode(addr, level, true));
> +		WARN_ON(readq(&gtt_entries[i-1]) != gtt_entry);
>
>   	/* This next bit makes the above posting read even more important. We
>   	 * want to flush the TLBs only after we're certain all the PTE updates
> @@ -2440,20 +2437,20 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>   {
>   	struct drm_i915_private *dev_priv = to_i915(vm->dev);
>   	struct i915_ggtt *ggtt = i915_vm_to_ggtt(vm);
> -	unsigned first_entry = start >> PAGE_SHIFT;
> -	gen6_pte_t __iomem *gtt_entries =
> -		(gen6_pte_t __iomem *)ggtt->gsm + first_entry;
> -	int i = 0;
> -	struct sg_page_iter sg_iter;
> -	dma_addr_t addr = 0;
> +	struct sgt_iter sgt_iter;
> +	gen6_pte_t __iomem *gtt_entries;
> +	gen6_pte_t gtt_entry;
> +	dma_addr_t addr;
>   	int rpm_atomic_seq;
> +	int i = 0;
>
>   	rpm_atomic_seq = assert_rpm_atomic_begin(dev_priv);
>
> -	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
> -		addr = sg_page_iter_dma_address(&sg_iter);
> -		iowrite32(vm->pte_encode(addr, level, true, flags), &gtt_entries[i]);
> -		i++;
> +	gtt_entries = (gen6_pte_t __iomem *)ggtt->gsm + (start >> PAGE_SHIFT);
> +
> +	for_each_sgt_dma(addr, sgt_iter, st) {
> +		gtt_entry = vm->pte_encode(addr, level, true, flags);
> +		iowrite32(gtt_entry, &gtt_entries[i++]);
>   	}
>
>   	/* XXX: This serves as a posting read to make sure that the PTE has
> @@ -2462,10 +2459,8 @@ static void gen6_ggtt_insert_entries(struct i915_address_space *vm,
>   	 * of NUMA access patterns. Therefore, even with the way we assume
>   	 * hardware should work, we must keep this posting read for paranoia.
>   	 */
> -	if (i != 0) {
> -		unsigned long gtt = readl(&gtt_entries[i-1]);
> -		WARN_ON(gtt != vm->pte_encode(addr, level, true, flags));
> -	}
> +	if (i != 0)
> +		WARN_ON(readl(&gtt_entries[i-1]) != gtt_entry);
>
>   	/* This next bit makes the above posting read even more important. We
>   	 * want to flush the TLBs only after we're certain all the PTE updates
> @@ -3399,9 +3394,11 @@ struct i915_vma *
>   intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info,
>   			  struct drm_i915_gem_object *obj)
>   {
> +	const size_t n_pages = obj->base.size / PAGE_SIZE;
>   	unsigned int size_pages = rot_info->plane[0].width * rot_info->plane[0].height;
>   	unsigned int size_pages_uv;
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	dma_addr_t dma_addr;
>   	unsigned long i;
>   	dma_addr_t *page_addr_list;
>   	struct sg_table *st;
> @@ -3410,7 +3407,7 @@ struct i915_vma *
>   	int ret = -ENOMEM;
>
>   	/* Allocate a temporary list of source pages for random access. */
> -	page_addr_list = drm_malloc_gfp(obj->base.size / PAGE_SIZE,
> +	page_addr_list = drm_malloc_gfp(n_pages,
>   					sizeof(dma_addr_t),
>   					GFP_TEMPORARY);
>   	if (!page_addr_list)
> @@ -3433,11 +3430,10 @@ struct i915_vma *
>
>   	/* Populate source page list from the object. */
>   	i = 0;
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> -		page_addr_list[i] = sg_page_iter_dma_address(&sg_iter);
> -		i++;
> -	}
> +	for_each_sgt_dma(dma_addr, sgt_iter, obj->pages)
> +		page_addr_list[i++] = dma_addr;
>
> +	GEM_BUG_ON(i != n_pages);
>   	st->nents = 0;
>   	sg = st->sgl;
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c
> index 32d9726..d2c1595 100644
> --- a/drivers/gpu/drm/i915/i915_gem_userptr.c
> +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c
> @@ -706,7 +706,8 @@ struct get_pages_work {
>   static void
>   i915_gem_userptr_put_pages(struct drm_i915_gem_object *obj)
>   {
> -	struct sg_page_iter sg_iter;
> +	struct sgt_iter sgt_iter;
> +	struct page *page;
>
>   	BUG_ON(obj->userptr.work != NULL);
>   	__i915_gem_userptr_set_active(obj, false);
> @@ -716,9 +717,7 @@ struct get_pages_work {
>
>   	i915_gem_gtt_finish_object(obj);
>
> -	for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) {
> -		struct page *page = sg_page_iter_page(&sg_iter);
> -
> +	for_each_sgt_page(page, sgt_iter, obj->pages) {
>   		if (obj->dirty)
>   			set_page_dirty(page);
>
>

Couldn't spot any problems in this block.

Regards,

Tvrtko

Re: [Intel-gfx] [PATCH 3/3] Introduce & use new lightweight SGL iterators

[Dave Gordon]

On 17/05/16 11:34, Tvrtko Ursulin wrote:
>
> On 16/05/16 16:19, Dave Gordon wrote:
>> The existing for_each_sg_page() iterator is somewhat heavyweight, and is
>> limiting i915 driver performance in a few benchmarks. So here we
>> introduce somewhat lighter weight iterators, primarily for use with GEM
>> objects or other case where we need only deal with whole aligned pages.
>
> Interesting idea, if for nothing then for eliminating the dreaded
> st->nents of for_each_sg_page. :)
>
> Which benchmarks it improves and how much do you know?

I know nothing :)

But last time I posted some easy-to-use iterators, Chris Wilson said 
they didn't address his complaint, which was that the existing ones were 
too slow.

https://lkml.org/lkml/2016/5/9/325

> It generates more code for me but that seems to be by design, yes?
> Because what were previously calls to __sg_page_iter_next is now
> effectively inlined and instead there is only a call to sg_next, which
> __sg_page_iter_next would call anyway.

In the light of Chris' comments, I decided to try some versions that 
were mostly focussed on avoiding the excessive number of function calls 
in the old ones. If this still isn't fast enough, we could also try 
inlining sg_next().

>> Unlike the old iterator, the new iterators use an internal state
>> structure which is not intended to be accessed by the caller; instead
>> each takes as a parameter an output variable which is set before each
>> iteration. This makes them particularly simple to use :)
>>
>> One of the new iterators provides the caller with the DMA address of
>> each page in turn; the other provides the 'struct page' pointer required
>> by many memory management operations.
>>
>> Various uses of for_each_sg_page() are then converted to the new macros.
>>
>> Signed-off-by: Dave Gordon <david.s.gordon@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: linux-kernel@vger.kernel.org
>> ---
>>   drivers/gpu/drm/i915/i915_drv.h         | 62
>> +++++++++++++++++++++++++--
>>   drivers/gpu/drm/i915/i915_gem.c         | 20 ++++-----
>>   drivers/gpu/drm/i915/i915_gem_fence.c   | 14 +++---
>>   drivers/gpu/drm/i915/i915_gem_gtt.c     | 76
>> ++++++++++++++++-----------------
>>   drivers/gpu/drm/i915/i915_gem_userptr.c |  7 ++-
>>   5 files changed, 116 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>> b/drivers/gpu/drm/i915/i915_drv.h
>> index 72f0b02..c0fc6aa 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2125,6 +2125,10 @@ struct drm_i915_gem_object_ops {
>>   #define INTEL_FRONTBUFFER_ALL_MASK(pipe) \
>>       (0xff << (INTEL_FRONTBUFFER_BITS_PER_PIPE * (pipe)))
>>
>> +void i915_gem_track_fb(struct drm_i915_gem_object *old,
>> +               struct drm_i915_gem_object *new,
>> +               unsigned frontbuffer_bits);
>> +
>>   struct drm_i915_gem_object {
>>       struct drm_gem_object base;
>>
>> @@ -2256,9 +2260,61 @@ struct drm_i915_gem_object {
>>   };
>>   #define to_intel_bo(x) container_of(x, struct drm_i915_gem_object,
>> base)
>>
>> -void i915_gem_track_fb(struct drm_i915_gem_object *old,
>> -               struct drm_i915_gem_object *new,
>> -               unsigned frontbuffer_bits);
>> +/*
>> + * New optimised SGL iterator for i915 GEM objects
>> + */
>> +struct sgt_iter {
>> +    struct scatterlist *sgp;
>> +    union {
>> +        unsigned long pfn;
>> +        unsigned long dma;
>
> dma_addr_t
>
>> +    } ix;
>> +    unsigned int curr;
>> +    unsigned int max;
>
> I think since this counts in bytes it should match obj->base.size which
> is a size_t?

Could be, although I think no GEM object can be greater than 4G.

> Also, maybe the iterator would be more efficient if it counted in pages?
> Hm, probably makes no difference, but maybe worth exploring. Was just
> thinking about avoiding the need to to convert byte position to page
> position in every iteration.

I tried that too, you still end up with byte/page conversions, just in 
different places. Anyway, it's only a single shift instruction.

>> +};
>> +
>> +/* Constructor for new iterator */
>> +static inline struct sgt_iter
>> +__sgt_iter(struct scatterlist *sgl, bool dma)
>> +{
>> +    struct sgt_iter s = { .sgp = sgl };
>> +
>> +    if (sgl) {
>> +        s.max = s.curr = sgl->offset;
>> +        s.max += sgl->length;
>> +        if (dma)
>> +            s.ix.dma = sg_dma_address(sgl);
>> +        else
>> +            s.ix.pfn = page_to_pfn(sg_page(sgl));
>
> I suppose we can rely on the compiler to optimize one branch away rather
> than having to provide two iterators.

Yes; the bool is always a compile-time const so the compiler only 
generates the relevant side of the branch.

>> +    }
>> +
>> +    return s;
>> +}
>> +
>> +/**
>> + * for_each_sgt_dma - iterate over the DMA addresses of the given
>> sg_table
>> + * @__dmap:    DMA address (output)
>> + * @__iter:    'struct sgt_iter' (iterator state, internal)
>> + * @__sgt:    sg_table to iterate over (input)
>> + */
>> +#define for_each_sgt_dma(__dmap, __iter, __sgt)                \
>> +    for ((__iter) = __sgt_iter((__sgt)->sgl, true);            \
>> +         ((__dmap) = (__iter).ix.dma + (__iter).curr);        \
>> +         (((__iter).curr += PAGE_SIZE) < (__iter).max) ||        \
>> +        ((__iter) = __sgt_iter(sg_next((__iter).sgp), true), 0))
>> +
>> +/**
>> + * for_each_sgt_page - iterate over the pages of the given sg_table
>> + * @__pp:    page pointer (output)
>> + * @__iter:    'struct sgt_iter' (iterator state, internal)
>> + * @__sgt:    sg_table to iterate over (input)
>> + */
>> +#define for_each_sgt_page(__pp, __iter, __sgt)                \
>> +    for ((__iter) = __sgt_iter((__sgt)->sgl, false);        \
>> +         ((__pp) = (__iter).ix.pfn == 0 ? NULL :            \
>> +        pfn_to_page((__iter).ix.pfn + ((__iter).curr >> PAGE_SHIFT)));\
>
> I could figure out what is the pfn == 0 check for? The other loop has no
> such checks so one of them looks suspicious or I am missing something.

The other *does* have a similar check -- this is the loop-break 
condition :) But here we need to avoid passing the invalid pfn 0 to 
pfn_to_page(), whereas in the other one we're just doing an addition, so 
we can terminate on (ix.dma == curr == 0).

> Also I think to be worth it you would have to handle the offset as well.
> That way all usages for for_each_sg_page could be converted and without
> it it leaves a mix of new and old.

Do you mean offset-within-page? That actually does work for DMA 
addresses (you get the same offset within each page), and it's ignored 
for page iteration.

Or do you mean starting-offset? That's really hard :( You can't find the 
n'th page without walking the entire list from the start. So I didn't 
write an iterator with a starting offset, and the few cases which need 
that can't easily be converted.

(I do have a "convenience" iterator with an offset as previously posted, 
but it's not a fast inlined version).

I'll try a couple more tweaks to this ...

.Dave.

Re: [Intel-gfx] [PATCH 3/3] Introduce & use new lightweight SGL iterators

[Chris Wilson]

On Tue, May 17, 2016 at 01:05:48PM +0100, Dave Gordon wrote:
> On 17/05/16 11:34, Tvrtko Ursulin wrote:
> >
> >On 16/05/16 16:19, Dave Gordon wrote:
> >>The existing for_each_sg_page() iterator is somewhat heavyweight, and is
> >>limiting i915 driver performance in a few benchmarks. So here we
> >>introduce somewhat lighter weight iterators, primarily for use with GEM
> >>objects or other case where we need only deal with whole aligned pages.
> >
> >Interesting idea, if for nothing then for eliminating the dreaded
> >st->nents of for_each_sg_page. :)
> >
> >Which benchmarks it improves and how much do you know?
> 
> I know nothing :)
> 
> But last time I posted some easy-to-use iterators, Chris Wilson said
> they didn't address his complaint, which was that the existing ones
> were too slow.

These aren't very good either... Compared to the sg iters I have:

         gem:exec:fault:1MiB:    -4.32%
  gem:exec:fault:1MiB:forked:    -5.66%
        gem:exec:fault:16MiB:   -13.33%
 gem:exec:fault:16MiB:forked:   -12.03%
       gem:exec:fault:256MiB:   -15.28%
gem:exec:fault:256MiB:forked:   -16.98%

(I was really hoping to be able to drop a patch!)

Patch used for reference:

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 03b7c2e..d7c1431 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3787,4 +3787,56 @@ int remap_io_mapping(struct vm_area_struct *vma,
 #define i915_gem_object_for_each_vma(vma, obj) \
        list_for_each_entry_check(vma, &(obj)->vma_list, obj_link, &(obj)->base.dev->struct_mutex)
 
+struct sgt_iter {
+       struct scatterlist *sgp;
+       union {
+               unsigned long pfn;
+               unsigned long dma;
+       } ix;
+       unsigned int curr;
+       unsigned int max;
+};
+
+static inline struct sgt_iter
+__sgt_iter(struct scatterlist *sgl, bool dma)
+{
+       struct sgt_iter s = { .sgp = sgl };
+
+       if (sgl) {
+               s.max = s.curr = sgl->offset;
+               s.max += sgl->length;
+               if (dma)
+                       s.ix.dma = sg_dma_address(sgl);
+               else
+                       s.ix.pfn = page_to_pfn(sg_page(sgl));
+       }
+
+       return s;
+}
+
+/**
+ * for_each_sgt_dma - iterate over the DMA addresses of the given sg_table
+ * @__dmap:    DMA address (output)
+ * @__iter:    'struct sgt_iter' (iterator state, internal)
+ * @__sgt:     sg_table to iterate over (input)
+ */
+#define for_each_sgt_dma(__dmap, __iter, __sgt)                                \
+       for ((__iter) = __sgt_iter((__sgt)->sgl, true);                 \
+            ((__dmap) = (__iter).ix.dma + (__iter).curr);              \
+            (((__iter).curr += PAGE_SIZE) < (__iter).max) ||           \
+            ((__iter) = __sgt_iter(sg_next((__iter).sgp), true), 0))
+
+/**
+ * for_each_sgt_page - iterate over the pages of the given sg_table
+ * @__pp:      page pointer (output)
+ * @__iter:    'struct sgt_iter' (iterator state, internal)
+ * @__sgt:     sg_table to iterate over (input)
+ */
+#define for_each_sgt_page(__pp, __iter, __sgt)                         \
+       for ((__iter) = __sgt_iter((__sgt)->sgl, false);                \
+            ((__pp) = (__iter).ix.pfn == 0 ? NULL :                    \
+             pfn_to_page((__iter).ix.pfn + ((__iter).curr >> PAGE_SHIFT)));\
+            (((__iter).curr += PAGE_SIZE) < (__iter).max) ||           \
+            ((__iter) = __sgt_iter(sg_next((__iter).sgp), false), 0))
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 603895a..3fcb540 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -1571,18 +1571,19 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm,
        unsigned act_pt = first_entry / GEN6_PTES;
        unsigned act_pte = first_entry % GEN6_PTES - 1;
        u32 pte_encode = vm->pte_encode(0, cache_level, true, flags);
-       struct st_iter iter;
+       struct sgt_iter iter;
        gen6_pte_t *pt_vaddr;
+       dma_addr_t addr;
 
        pt_vaddr = kmap_px(ppgtt, &ppgtt->pd.page_table[act_pt]);
-       st_for_each_address(&iter, pages) {
+       for_each_sgt_dma(addr, iter, pages) {
                if (++act_pte == GEN6_PTES) {
                        kunmap_px(pt_vaddr);
                        pt_vaddr = kmap_px(ppgtt,
                                           &ppgtt->pd.page_table[++act_pt]);
                        act_pte = 0;
                }
-               pt_vaddr[act_pte] = pte_encode | GEN6_PTE_ADDR_ENCODE(iter.dma);
+               pt_vaddr[act_pte] = pte_encode | GEN6_PTE_ADDR_ENCODE(addr);
        }
        kunmap_px(pt_vaddr);
 }

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH 3/3] Introduce & use new lightweight SGL iterators

[Dave Gordon]

On 19/05/2016 18:27, Chris Wilson wrote:
> On Tue, May 17, 2016 at 01:05:48PM +0100, Dave Gordon wrote:
>> On 17/05/16 11:34, Tvrtko Ursulin wrote:
>>> On 16/05/16 16:19, Dave Gordon wrote:
>>>> The existing for_each_sg_page() iterator is somewhat heavyweight, and is
>>>> limiting i915 driver performance in a few benchmarks. So here we
>>>> introduce somewhat lighter weight iterators, primarily for use with GEM
>>>> objects or other case where we need only deal with whole aligned pages.
>>> Interesting idea, if for nothing then for eliminating the dreaded
>>> st->nents of for_each_sg_page. :)
>>>
>>> Which benchmarks it improves and how much do you know?
>> I know nothing :)
>>
>> But last time I posted some easy-to-use iterators, Chris Wilson said
>> they didn't address his complaint, which was that the existing ones
>> were too slow.
> These aren't very good either... Compared to the sg iters I have:
>
>           gem:exec:fault:1MiB:    -4.32%
>    gem:exec:fault:1MiB:forked:    -5.66%
>          gem:exec:fault:16MiB:   -13.33%
>   gem:exec:fault:16MiB:forked:   -12.03%
>         gem:exec:fault:256MiB:   -15.28%
> gem:exec:fault:256MiB:forked:   -16.98%
>
> (I was really hoping to be able to drop a patch!)

I think you've inlined sg_next() as well? That was the next thing I was 
going to try with these ...
I'll post the version with inline sg_next, but I'm on holiday now so 
won't be around until the end of the month.

.Dave.