Re: [PATCH 25/34] drm/i915: Track active timelines

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

On 21/01/2019 22:21, Chris Wilson wrote:
> Now that we pin timelines around use, we have a clearly defined lifetime
> and convenient points at which we can track only the active timelines.
> This allows us to reduce the list iteration to only consider those
> active timelines and not all.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_drv.h      |  2 +-
>   drivers/gpu/drm/i915/i915_gem.c      |  4 +--
>   drivers/gpu/drm/i915/i915_reset.c    |  2 +-
>   drivers/gpu/drm/i915/i915_timeline.c | 39 ++++++++++++++++++----------
>   4 files changed, 29 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index c00eaf2889fb..5577e0e1034f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1977,7 +1977,7 @@ struct drm_i915_private {
>   
>   		struct i915_gt_timelines {
>   			struct mutex mutex; /* protects list, tainted by GPU */
> -			struct list_head list;
> +			struct list_head active_list;
>   
>   			/* Pack multiple timelines' seqnos into the same page */
>   			spinlock_t hwsp_lock;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 4e0de22f0166..9c499edb4c13 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3246,7 +3246,7 @@ wait_for_timelines(struct drm_i915_private *i915,
>   		return timeout;
>   
>   	mutex_lock(&gt->mutex);
> -	list_for_each_entry(tl, &gt->list, link) {
> +	list_for_each_entry(tl, &gt->active_list, link) {
>   		struct i915_request *rq;
>   
>   		rq = i915_gem_active_get_unlocked(&tl->last_request);
> @@ -3274,7 +3274,7 @@ wait_for_timelines(struct drm_i915_private *i915,
>   
>   		/* restart after reacquiring the lock */
>   		mutex_lock(&gt->mutex);
> -		tl = list_entry(&gt->list, typeof(*tl), link);
> +		tl = list_entry(&gt->active_list, typeof(*tl), link);
>   	}
>   	mutex_unlock(&gt->mutex);
>   
> diff --git a/drivers/gpu/drm/i915/i915_reset.c b/drivers/gpu/drm/i915/i915_reset.c
> index 09edf488f711..9b9169508139 100644
> --- a/drivers/gpu/drm/i915/i915_reset.c
> +++ b/drivers/gpu/drm/i915/i915_reset.c
> @@ -852,7 +852,7 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
>   	 * No more can be submitted until we reset the wedged bit.
>   	 */
>   	mutex_lock(&i915->gt.timelines.mutex);
> -	list_for_each_entry(tl, &i915->gt.timelines.list, link) {
> +	list_for_each_entry(tl, &i915->gt.timelines.active_list, link) {
>   		struct i915_request *rq;
>   		long timeout;
>   
> diff --git a/drivers/gpu/drm/i915/i915_timeline.c b/drivers/gpu/drm/i915/i915_timeline.c
> index 69ee33dfa340..007348b1b469 100644
> --- a/drivers/gpu/drm/i915/i915_timeline.c
> +++ b/drivers/gpu/drm/i915/i915_timeline.c
> @@ -117,7 +117,6 @@ int i915_timeline_init(struct drm_i915_private *i915,
>   		       const char *name,
>   		       struct i915_vma *hwsp)
>   {
> -	struct i915_gt_timelines *gt = &i915->gt.timelines;
>   	void *vaddr;
>   
>   	/*
> @@ -161,10 +160,6 @@ int i915_timeline_init(struct drm_i915_private *i915,
>   
>   	i915_syncmap_init(&timeline->sync);
>   
> -	mutex_lock(&gt->mutex);
> -	list_add(&timeline->link, &gt->list);
> -	mutex_unlock(&gt->mutex);
> -
>   	return 0;
>   }
>   
> @@ -173,7 +168,7 @@ void i915_timelines_init(struct drm_i915_private *i915)
>   	struct i915_gt_timelines *gt = &i915->gt.timelines;
>   
>   	mutex_init(&gt->mutex);
> -	INIT_LIST_HEAD(&gt->list);
> +	INIT_LIST_HEAD(&gt->active_list);
>   
>   	spin_lock_init(&gt->hwsp_lock);
>   	INIT_LIST_HEAD(&gt->hwsp_free_list);
> @@ -182,6 +177,24 @@ void i915_timelines_init(struct drm_i915_private *i915)
>   	i915_gem_shrinker_taints_mutex(i915, &gt->mutex);
>   }
>   
> +static void timeline_active(struct i915_timeline *tl)
> +{
> +	struct i915_gt_timelines *gt = &tl->i915->gt.timelines;
> +
> +	mutex_lock(&gt->mutex);
> +	list_add(&tl->link, &gt->active_list);
> +	mutex_unlock(&gt->mutex);
> +}
> +
> +static void timeline_inactive(struct i915_timeline *tl)
> +{
> +	struct i915_gt_timelines *gt = &tl->i915->gt.timelines;
> +
> +	mutex_lock(&gt->mutex);
> +	list_del(&tl->link);
> +	mutex_unlock(&gt->mutex);
> +}

Bike shedding comments only:
Would it be better to use a verb suffix? Even though timeline_activate 
also wouldn't sound perfect. Since it is file local - activate_timeline? 
Or even just inline to pin/unpin. Unless more gets put into them later..

> +
>   /**
>    * i915_timelines_park - called when the driver idles
>    * @i915: the drm_i915_private device
> @@ -198,7 +211,7 @@ void i915_timelines_park(struct drm_i915_private *i915)
>   	struct i915_timeline *timeline;
>   
>   	mutex_lock(&gt->mutex);
> -	list_for_each_entry(timeline, &gt->list, link) {
> +	list_for_each_entry(timeline, &gt->active_list, link) {
>   		/*
>   		 * All known fences are completed so we can scrap
>   		 * the current sync point tracking and start afresh,
> @@ -212,15 +225,9 @@ void i915_timelines_park(struct drm_i915_private *i915)
>   
>   void i915_timeline_fini(struct i915_timeline *timeline)
>   {
> -	struct i915_gt_timelines *gt = &timeline->i915->gt.timelines;
> -
>   	GEM_BUG_ON(timeline->pin_count);
>   	GEM_BUG_ON(!list_empty(&timeline->requests));
>   
> -	mutex_lock(&gt->mutex);
> -	list_del(&timeline->link);
> -	mutex_unlock(&gt->mutex);
> -
>   	i915_syncmap_free(&timeline->sync);
>   	hwsp_free(timeline);
>   
> @@ -263,6 +270,8 @@ int i915_timeline_pin(struct i915_timeline *tl)
>   	if (err)
>   		goto unpin;
>   
> +	timeline_active(tl);
> +
>   	return 0;
>   
>   unpin:
> @@ -276,6 +285,8 @@ void i915_timeline_unpin(struct i915_timeline *tl)
>   	if (--tl->pin_count)
>   		return;
>   
> +	timeline_inactive(tl);
> +
>   	/*
>   	 * Since this timeline is idle, all bariers upon which we were waiting
>   	 * must also be complete and so we can discard the last used barriers
> @@ -299,7 +310,7 @@ void i915_timelines_fini(struct drm_i915_private *i915)
>   {
>   	struct i915_gt_timelines *gt = &i915->gt.timelines;
>   
> -	GEM_BUG_ON(!list_empty(&gt->list));
> +	GEM_BUG_ON(!list_empty(&gt->active_list));
>   	GEM_BUG_ON(!list_empty(&gt->hwsp_free_list));
>   
>   	mutex_destroy(&gt->mutex);
> 

Never mind the bikeshedding comments:

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