Re: [PATCH] drm/i915: Remove temporary allocation of dma addresses when rotating

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

On pe, 2017-02-17 at 15:10 +0000, 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.
> 
> 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>

<SNIP>

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

i915_gem_object_get_sg has variable names obj, n, *offset, so I'd be
little concerned of sidetracking reader. Rename n into offset?

> +			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;

I'm not sure why moving this line, might as well hoist all these to the
for() line.

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

This is only used once, just inline it.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Could use an A-b from Tvrtko.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx