Re: [PATCH 2/3] drm/i915: Release the active tracker tree upon idling

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 30/01/2019 20:50, Chris Wilson wrote:
> As soon as we detect that the active tracker is idle and we prepare to
> call the retire callback, release the storage for our tree of
> per-timeline nodes. We expect these to be infrequently usage and quick

s/usage/used/

> to allocate, so there is little benefit in keeping the tree cached and
> we would prefer to return the pages back to the system in a timely
> fashion.
> 
> This also means that when we finalize the struct as a whole, we know as
> the activity tracker must be idle, the tree has already been released.
> Indeed we can reduce i915_active_fini() just the assertions that there
> is nothing to do.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_active.c | 33 +++++++++++++++++++++---------
>   drivers/gpu/drm/i915/i915_active.h |  4 ++++
>   2 files changed, 27 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_active.c b/drivers/gpu/drm/i915/i915_active.c
> index 91950d778cab..b1fefe98f9a6 100644
> --- a/drivers/gpu/drm/i915/i915_active.c
> +++ b/drivers/gpu/drm/i915/i915_active.c
> @@ -16,12 +16,29 @@ struct active_node {
>   	u64 timeline;
>   };
>   
> +static void
> +__active_park(struct i915_active *ref)
> +{
> +	struct active_node *it, *n;
> +
> +	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
> +		GEM_BUG_ON(i915_gem_active_isset(&it->base));
> +		kfree(it);
> +	}
> +	ref->tree = RB_ROOT;
> +}
> +
>   static void
>   __active_retire(struct i915_active *ref)
>   {
>   	GEM_BUG_ON(!ref->count);
> -	if (!--ref->count)
> -		ref->retire(ref);
> +	if (--ref->count)
> +		return;
> +
> +	/* return the unused nodes to our slabcache */
> +	__active_park(ref);
> +
> +	ref->retire(ref);
>   }
>   
>   static void
> @@ -210,18 +227,14 @@ int i915_request_await_active(struct i915_request *rq, struct i915_active *ref)
>   	return 0;
>   }
>   
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>   void i915_active_fini(struct i915_active *ref)
>   {
> -	struct active_node *it, *n;
> -
>   	GEM_BUG_ON(i915_gem_active_isset(&ref->last));
> -
> -	rbtree_postorder_for_each_entry_safe(it, n, &ref->tree, node) {
> -		GEM_BUG_ON(i915_gem_active_isset(&it->base));
> -		kfree(it);
> -	}
> -	ref->tree = RB_ROOT;
> +	GEM_BUG_ON(!RB_EMPTY_ROOT(&ref->tree));
> +	GEM_BUG_ON(ref->count);
>   }
> +#endif
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
>   #include "selftests/i915_active.c"
> diff --git a/drivers/gpu/drm/i915/i915_active.h b/drivers/gpu/drm/i915/i915_active.h
> index 8bd7cf757d1a..6130c6770d10 100644
> --- a/drivers/gpu/drm/i915/i915_active.h
> +++ b/drivers/gpu/drm/i915/i915_active.h
> @@ -57,6 +57,10 @@ i915_active_is_idle(const struct i915_active *ref)
>   	return !ref->count;
>   }
>   
> +#if IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)
>   void i915_active_fini(struct i915_active *ref);
> +#else
> +static inline void i915_active_fini(struct i915_active *ref) { }
> +#endif
>   
>   #endif /* _I915_ACTIVE_H_ */
> 

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx