[Intel-gfx] [PATCH 23/24] drm/i915: Keep a recent cache of freed contexts objects for reuse

[Tvrtko Ursulin]

On 18/05/2017 15:19, Chris Wilson wrote:
> On Thu, May 18, 2017 at 02:52:41PM +0100, Tvrtko Ursulin wrote:
>>
>> On 18/05/2017 10:46, Chris Wilson wrote:
>>> Keep the recently freed context objects for reuse. This allows us to use
>>> the current GGTT bindings and dma bound pages, avoiding any clflushes as
>>> required. We mark the objects as purgeable under memory pressure, and
>>> reap the list of freed objects as soon as the device is idle.
>>
>> Some description on when is this interesting and by how much?
>
> No interesting workload (a few synthetic benchmarks including the GL).
> Best argument would be a minor improvement in application startup.

In this case I would be reluctant to complicate the code for no benefit.

>> Since you drop everything on idle it must be for some workload which
>> likes to go through context while keeping busy?
>
> Yup, it's just the rule of thumb I have (from experience with the
> cmdparser batch caching).
>
>>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>> drivers/gpu/drm/i915/i915_drv.h                  |  2 +
>>> drivers/gpu/drm/i915/i915_gem.c                  |  1 +
>>> drivers/gpu/drm/i915/i915_gem_context.c          | 59 ++++++++++++++++++++++--
>>> drivers/gpu/drm/i915/i915_gem_context.h          |  5 ++
>>> drivers/gpu/drm/i915/intel_lrc.c                 |  2 +-
>>> drivers/gpu/drm/i915/intel_ringbuffer.c          |  2 +-
>>> drivers/gpu/drm/i915/selftests/mock_context.c    |  1 +
>>> drivers/gpu/drm/i915/selftests/mock_gem_device.c |  1 +
>>> 8 files changed, 68 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>>> index 1d55bbde68df..1fa1e7d48f02 100644
>>> --- a/drivers/gpu/drm/i915/i915_drv.h
>>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> @@ -2316,6 +2316,8 @@ struct drm_i915_private {
>>> 		struct llist_head free_list;
>>> 		struct work_struct free_work;
>>>
>>> +		struct list_head freed_objects;
>>
>> free_ctx_objects maybe?
>
> This is i915->contexts.freed_objects (new contexts substruct). There is
> already a precedent for using freed_ (mainly because I think its a
> better description for that phase, free is too similar to avail,
> although you may persuade me that free is better).

My bad, I thought it is drm_i915_private.

>> Knowing you I am surprised this is not bucketed! :)
>
> I didn't bother since we only really have two sizes - also reason why I
> didn't make it per-engine so that we could share the xcs context
> objects. My expectation is that we won't be caught up in slow list
> searches, so went for simplicity for a change.
>
>>> +struct drm_i915_gem_object *
>>> +i915_gem_context_create_object(struct drm_i915_private *i915,
>>> +			       unsigned long size)
>>> +{
>>> +	struct drm_i915_gem_object *obj, *on;
>>> +
>>> +	lockdep_assert_held(&i915->drm.struct_mutex);
>>> +
>>> +	list_for_each_entry_safe(obj, on,
>>> +				 &i915->contexts.freed_objects,
>>> +				 batch_pool_link) {
>>> +		if (obj->mm.madv != I915_MADV_DONTNEED) {
>>> +			/* Purge the heretic! */
>>
>> I don't see this can happen in the current version?
>
> The power of the shrinker. Remember it can and will stab you in the back
> at any time.

Ok I forgot about the purged state, I just was surprised that anyone 
would be able to move the state away from don't need.

Regards,

Tvrtko