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

[Tvrtko Ursulin]

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 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?

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 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.

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