Re: [CI 1/6] drm/i915: Remove temporary allocation of dma addresses when rotating

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 17/02/2017 15:12, Chris Wilson wrote:
> The object already stores (computed on the fly) the index to dma address
> so use it instead of reallocating a large temporary array every time we
> bind a rotated framebuffer.

On the other hand how big is the radix tree for a large framebuffer? I 
remember those nodes were quite chunky and will hang around for the 
lifetime of the object. While the above mentioned large temporary array 
needs to be allocated only if rotated VMAs have been discarded due GGTT 
pressure, no?

On the other other hand maybe the radix tree won't be so big in the 
typical case, due sg entry coalescing, but it will hang around for much 
longer.

> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Matthew Auld <matthew.william.auld@gmail.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 77 ++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 47a38272f54c..848dbb926fd1 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3043,27 +3043,32 @@ void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
>  }
>
>  static struct scatterlist *
> -rotate_pages(const dma_addr_t *in, unsigned int offset,
> -	     unsigned int width, unsigned int height,
> -	     unsigned int stride,
> +rotate_pages(struct drm_i915_gem_object *obj,
> +	     const struct intel_rotation_plane_info *p,
>  	     struct sg_table *st, struct scatterlist *sg)
>  {
>  	unsigned int column, row;
> -	unsigned int src_idx;
>
> -	for (column = 0; column < width; column++) {
> -		src_idx = stride * (height - 1) + column;
> -		for (row = 0; row < height; row++) {
> -			st->nents++;
> +	for (column = 0; column < p->width; column++) {
> +		unsigned long src_idx =
> +			p->stride * (p->height - 1) + column + p->offset;
> +		for (row = 0; row < p->height; row++) {
> +			struct scatterlist *src;
> +			unsigned int n;
> +
> +			src = i915_gem_object_get_sg(obj, src_idx, &n);
> +			src_idx -= p->stride;
> +
>  			/* We don't need the pages, but need to initialize
>  			 * the entries so the sg list can be happily traversed.
>  			 * The only thing we need are DMA addresses.
>  			 */
>  			sg_set_page(sg, NULL, PAGE_SIZE, 0);
> -			sg_dma_address(sg) = in[offset + src_idx];
> +			sg_dma_address(sg) = sg_dma_address(src) + n*PAGE_SIZE;
>  			sg_dma_len(sg) = PAGE_SIZE;
> -			sg = sg_next(sg);
> -			src_idx -= stride;
> +			sg = __sg_next(sg);
> +
> +			st->nents++;
>  		}
>  	}
>
> @@ -3074,62 +3079,30 @@ static noinline struct sg_table *
>  intel_rotate_pages(struct intel_rotation_info *rot_info,
>  		   struct drm_i915_gem_object *obj)
>  {
> -	const unsigned long n_pages = obj->base.size / PAGE_SIZE;
> -	unsigned int size = intel_rotation_info_size(rot_info);
> -	struct sgt_iter sgt_iter;
> -	dma_addr_t dma_addr;
> -	unsigned long i;
> -	dma_addr_t *page_addr_list;
> -	struct sg_table *st;
> +	const unsigned int size = intel_rotation_info_size(rot_info);
>  	struct scatterlist *sg;
> +	struct sg_table *st;
> +	unsigned long i;
>  	int ret = -ENOMEM;
>
> -	/* Allocate a temporary list of source pages for random access. */
> -	page_addr_list = drm_malloc_gfp(n_pages,
> -					sizeof(dma_addr_t),
> -					GFP_TEMPORARY);
> -	if (!page_addr_list)
> -		return ERR_PTR(ret);
> -
> -	/* Allocate target SG list. */
>  	st = kmalloc(sizeof(*st), GFP_KERNEL);
>  	if (!st)
> -		goto err_st_alloc;
> +		goto err;
>
>  	ret = sg_alloc_table(st, size, GFP_KERNEL);
>  	if (ret)
> -		goto err_sg_alloc;
> -
> -	/* Populate source page list from the object. */
> -	i = 0;
> -	for_each_sgt_dma(dma_addr, sgt_iter, obj->mm.pages)
> -		page_addr_list[i++] = dma_addr;
> +		goto err;
>
> -	GEM_BUG_ON(i != n_pages);
>  	st->nents = 0;
>  	sg = st->sgl;
> -
> -	for (i = 0 ; i < ARRAY_SIZE(rot_info->plane); i++) {
> -		sg = rotate_pages(page_addr_list, rot_info->plane[i].offset,
> -				  rot_info->plane[i].width, rot_info->plane[i].height,
> -				  rot_info->plane[i].stride, st, sg);
> -	}
> -
> -	DRM_DEBUG_KMS("Created rotated page mapping for object size %zu (%ux%u tiles, %u pages)\n",
> -		      obj->base.size, rot_info->plane[0].width, rot_info->plane[0].height, size);

Hm given how chatty KMS log level is this one wasn't that harmful but 
OK. Use to save me looking in debugfs/i915_gem_framebuffer and eyeball 
the VMA list. Granted that is much more manageable now after you added 
the human readable output there.

> -
> -	drm_free_large(page_addr_list);
> +	for (i = 0 ; i < ARRAY_SIZE(rot_info->plane); i++)
> +		sg = rotate_pages(obj, &rot_info->plane[i], st, sg);
> +	GEM_BUG_ON(st->nents != size);
>
>  	return st;
>
> -err_sg_alloc:
> +err:
>  	kfree(st);
> -err_st_alloc:
> -	drm_free_large(page_addr_list);
> -
> -	DRM_DEBUG_KMS("Failed to create rotated mapping for object size %zu! (%ux%u tiles, %u pages)\n",
> -		      obj->base.size, rot_info->plane[0].width, rot_info->plane[0].height, size);
> -
>  	return ERR_PTR(ret);
>  }
>
>

Code looks fine. But I would like to think about numbers some more.

Regards,

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