On 22/02/2017 08:44, Chris Wilson wrote:
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@xxxxxxxxxxxxxxxxxx>
Cc: Matthew Auld <matthew.william.auld@xxxxxxxxx>
Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
<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?
Or use i915_gem_object_get_dma_address in the sg_dma_adress_assignment
directly.
+ 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@xxxxxxxxxxxxxxx>
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.
Well I wasn't concerned about the cache per se, but about whether it is
completely appropriate (best choice) to use it in this particular case.
Because as I said before, for 1920x1080x32 we are talking about a 16KiB
extremely short lived temporary allocation, vs the similar size for the
sg radix cache. But radix cache sticks around the the lifetime of
obj->mm.pages and it wouldn't otherwise be there since AFAICS in
practice no one really touches frame buffers in a way to trigger its
creation.
Those amounts of memory are not a concern, but again, is the
simplification of the code worth the conceptual downsides mentioned
above? Even if we considered 4K frame buffers, when both allocations go
to ~64KiB, would that change anything? I am not sure, probably not for me.
So I am still unsure that we should go with this change.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
