[Intel-gfx] [PATCH] drm/i915: Remove temporary allocation of dma addresses when rotating

[Chris Wilson]

On Wed, Feb 22, 2017 at 08:29:06AM +0000, Tvrtko Ursulin wrote:
> 
> On 21/02/2017 15:01, Joonas Lahtinen wrote:
> >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 at chris-wilson.co.uk>
> >>Cc: Matthew Auld <matthew.william.auld at gmail.com>
> >>Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursulin at 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 at linux.intel.com>
> >
> >Could use an A-b from Tvrtko.
> 
> I did not like it in another thread, well, better say I was
> concerned about the increased memory use by the radix tree which
> would then stick around for the obj->pages lifetime (long time for a
> framebuffer I thought). While the temporary array allocations here
> are not that big and very temporary.
> 
> I guess someone needs to bite the bullet and try and figure out how
> exactly big is the radix tree for some mixes of more or less
> coalesced sg entries.

I also think that's an argument for improving the general cache rather
than arguing against using it.
-Chris
> 

-- 
Chris Wilson, Intel Open Source Technology Centre