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 <ch...@chris-wilson.co.uk>
> >>Cc: Matthew Auld <matthew.william.a...@gmail.com>
> >>Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> >>Cc: Tvrtko Ursulin <tvrtko.ursu...@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.lahti...@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
