Re: [PATCH 4/4] drm/i915: create compact dma scatter lists for gem objects

[Daniel Vetter <daniel@ffwll.ch>]

On Sat, Feb 09, 2013 at 05:27:36PM +0200, Imre Deak wrote:
> So far we created a sparse dma scatter list for gem objects, where each
> scatter list entry represented only a single page. In the future we'll
> have to handle compact scatter lists too where each entry can consist of
> multiple pages, for example for objects imported through PRIME.
> 
> The previous patches have already fixed up all other places where the
> i915 driver _walked_ these lists. Here we have the corresponding fix to
> _create_ compact lists. It's not a performance or memory footprint
> improvement, but it helps to better exercise the new logic.
> 
> Reference: http://www.spinics.net/lists/dri-devel/msg33917.html
> Signed-off-by: Imre Deak <imre.deak@intel.com>

Just a quick question: Have you checked with printks or so that we indeed
create such coalesced sg entries every once in a while?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_gem.c |   31 ++++++++++++++++++++++---------
>  1 file changed, 22 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4a199e0..1028dd5 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1623,9 +1623,8 @@ i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>  static void
>  i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>  {
> -	int page_count = obj->base.size / PAGE_SIZE;
> -	struct scatterlist *sg;
> -	int ret, i;
> +	struct drm_sg_iter sg_iter;
> +	int ret;
>  
>  	BUG_ON(obj->madv == __I915_MADV_PURGED);
>  
> @@ -1645,8 +1644,8 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
>  	if (obj->madv == I915_MADV_DONTNEED)
>  		obj->dirty = 0;
>  
> -	for_each_sg(obj->pages->sgl, sg, page_count, i) {
> -		struct page *page = sg_page(sg);
> +	drm_for_each_sg_page(&sg_iter, obj->pages->sgl, 0) {
> +		struct page *page = sg_iter.page;
>  
>  		if (obj->dirty)
>  			set_page_dirty(page);
> @@ -1740,7 +1739,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	struct address_space *mapping;
>  	struct sg_table *st;
>  	struct scatterlist *sg;
> +	struct drm_sg_iter sg_iter;
>  	struct page *page;
> +	unsigned long last_pfn = 0;	/* suppress gcc warning */
>  	gfp_t gfp;
>  
>  	/* Assert that the object is not currently in any GPU domain. As it
> @@ -1770,7 +1771,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	gfp = mapping_gfp_mask(mapping);
>  	gfp |= __GFP_NORETRY | __GFP_NOWARN | __GFP_NO_KSWAPD;
>  	gfp &= ~(__GFP_IO | __GFP_WAIT);
> -	for_each_sg(st->sgl, sg, page_count, i) {
> +	sg = st->sgl;
> +	st->nents = 0;
> +	for (i = 0; i < page_count; i++) {
>  		page = shmem_read_mapping_page_gfp(mapping, i, gfp);
>  		if (IS_ERR(page)) {
>  			i915_gem_purge(dev_priv, page_count);
> @@ -1793,9 +1796,18 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  			gfp &= ~(__GFP_IO | __GFP_WAIT);
>  		}
>  
> -		sg_set_page(sg, page, PAGE_SIZE, 0);
> +		if (!i || page_to_pfn(page) != last_pfn + 1) {
> +			if (i)
> +				sg = sg_next(sg);
> +			st->nents++;
> +			sg_set_page(sg, page, PAGE_SIZE, 0);
> +		} else {
> +			sg->length += PAGE_SIZE;
> +		}
> +		last_pfn = page_to_pfn(page);
>  	}
>  
> +	sg_mark_end(sg);
>  	obj->pages = st;
>  
>  	if (i915_gem_object_needs_bit17_swizzle(obj))
> @@ -1804,8 +1816,9 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  	return 0;
>  
>  err_pages:
> -	for_each_sg(st->sgl, sg, i, page_count)
> -		page_cache_release(sg_page(sg));
> +	sg_mark_end(sg);
> +	drm_for_each_sg_page(&sg_iter, st->sgl, 0)
> +		page_cache_release(sg_iter.page);
>  	sg_free_table(st);
>  	kfree(st);
>  	return PTR_ERR(page);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch