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

[Chris Wilson]

On Mon, Feb 27, 2017 at 10:14:12AM +0000, Tvrtko Ursulin wrote:
> 
> On 27/02/2017 10:06, Chris Wilson wrote:
> >On Mon, Feb 27, 2017 at 09:55:10AM +0000, Tvrtko Ursulin wrote:
> >>
> >>On 22/02/2017 08:44, Chris Wilson wrote:
> >>>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.
> >
> >Again, the complaint you have here are general concerns about caching
> >the mapping. Avoiding using the cache instead of improving the cache
> >seems the wrong approach.
> 
> Depends what kind of improvments to the cache you have in mind. If
> you are thinking about size then I disagree, I think it is efficient
> enough already. But if you are thinking about the lifetime
> management then it is obvious from all that I have written so far
> that I would agree with that. Since the core of my "complaint" is
> the lifetime mismatch, and not the size.
> 
> For lifetime I am not sure what you could do. Exposing the size of
> it, with maybe some other bits attached the the object, to the
> shrinker I think doesn't make much sense since the sizes are so
> small compared to the backing store sizes.
> 
> Perhaps you could add an explicit reset of the cache after the
> rotation is done with it, but then the only remaining benefit will
> be avoiding greater than zero order allocations. I say the only
> one.. it would still be a good one. Just have no idea if this level
> of cache usage would satisfy you!
> 
> Perhaps you could say what kind of optimisation you have in mind to
> save me guessing? :)

I was thinking you would like an inactivity timer. Or we could have a
separate shrinker, as that's the principal cache management system.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre