Re: [PATCH] drm/i915/gt: Make timeslice duration configurable

[Mika Kuoppala <mika.kuoppala@linux.intel.com>]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> Execlists uses a scheduling quantum (a timeslice) to alternate execution
> between ready-to-run contexts of equal priority. This ensures that all
> users (though only if they of equal importance) have the opportunity to
> run and prevents livelocks where contexts may have implicit ordering due
> to userspace semaphores. However, not all workloads necessarily benefit
> from timeslicing and in the extreme some sysadmin may want to disable or
> reduce the timeslicing granularity.
>
> The timeslicing mechanism can be compiled out with

s/compiled/disabled

>
> 	./scripts/config --set-val DRM_I915_TIMESLICE_DURATION 0
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/Kconfig.profile         | 15 ++++++++
>  drivers/gpu/drm/i915/gt/intel_engine.h       |  9 +++++
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c    |  2 +
>  drivers/gpu/drm/i915/gt/intel_engine_types.h |  1 +
>  drivers/gpu/drm/i915/gt/intel_lrc.c          | 39 ++++++++++++++------
>  drivers/gpu/drm/i915/gt/selftest_lrc.c       | 13 ++++++-
>  6 files changed, 65 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/Kconfig.profile b/drivers/gpu/drm/i915/Kconfig.profile
> index 8ab7af5eb311..1799537a3228 100644
> --- a/drivers/gpu/drm/i915/Kconfig.profile
> +++ b/drivers/gpu/drm/i915/Kconfig.profile
> @@ -59,3 +59,18 @@ config DRM_I915_STOP_TIMEOUT
>  	  damage as the system is reset in order to recover. The corollary is
>  	  that the reset itself may take longer and so be more disruptive to
>  	  interactive or low latency workloads.
> +
> +config DRM_I915_TIMESLICE_DURATION
> +	int "Scheduling quantum for userspace batches (ms, jiffy granularity)"
> +	default 1 # milliseconds
> +	help
> +	  When two user batches of equal priority are executing, we will
> +	  alternate execution of each batch to ensure forward progress of
> +	  all users. This is necessary in some cases where there may be
> +	  an implicit dependency between those batches that requires
> +	  concurrent execution in order for them to proceed, e.g. they
> +	  interact with each other via userspace semaphores. Each context
> +	  is scheduled for execution for the timeslice duration, before
> +	  switching to the next context.
> +
> +	  May be 0 to disable timeslicing.
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h b/drivers/gpu/drm/i915/gt/intel_engine.h
> index c6895938b626..0597b77f5818 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine.h
> @@ -335,4 +335,13 @@ intel_engine_has_preempt_reset(const struct intel_engine_cs *engine)
>  	return intel_engine_has_preemption(engine);
>  }
>  
> +static inline bool
> +intel_engine_has_timeslices(const struct intel_engine_cs *engine)
> +{
> +	if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
> +		return 0;
s/0/false;

> +
> +	return intel_engine_has_semaphores(engine);
> +}
> +
>  #endif /* _INTEL_RINGBUFFER_H_ */
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 9cc1ea6519ec..2afa2ef90482 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -315,6 +315,8 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id)
>  		CONFIG_DRM_I915_PREEMPT_TIMEOUT;
>  	engine->props.stop_timeout_ms =
>  		CONFIG_DRM_I915_STOP_TIMEOUT;
> +	engine->props.timeslice_duration_ms =
> +		CONFIG_DRM_I915_TIMESLICE_DURATION;
>  
>  	/*
>  	 * To be overridden by the backend on setup. However to facilitate
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_types.h b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> index e8ea12b96755..c5d1047a4bc5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_types.h
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_types.h
> @@ -523,6 +523,7 @@ struct intel_engine_cs {
>  		unsigned long heartbeat_interval_ms;
>  		unsigned long preempt_timeout_ms;
>  		unsigned long stop_timeout_ms;
> +		unsigned long timeslice_duration_ms;
>  	} props;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c
> index 2f474c1f5c54..6fb3def5ba16 100644
> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c
> @@ -1467,7 +1467,7 @@ need_timeslice(struct intel_engine_cs *engine, const struct i915_request *rq)
>  {
>  	int hint;
>  
> -	if (!intel_engine_has_semaphores(engine))
> +	if (!intel_engine_has_timeslices(engine))
>  		return false;
>  
>  	if (list_is_last(&rq->sched.link, &engine->active.requests))
> @@ -1488,15 +1488,32 @@ switch_prio(struct intel_engine_cs *engine, const struct i915_request *rq)
>  	return rq_prio(list_next_entry(rq, sched.link));
>  }
>  
> -static bool
> -enable_timeslice(const struct intel_engine_execlists *execlists)
> +static inline unsigned long
> +timeslice(const struct intel_engine_cs *engine)

could be timeslice_duration, but not insisting

> +{
> +	return READ_ONCE(engine->props.timeslice_duration_ms);
> +}
> +
> +static unsigned long
> +active_timeslice(const struct intel_engine_cs *engine)
>  {
> -	const struct i915_request *rq = *execlists->active;
> +	const struct i915_request *rq = *engine->execlists.active;
>  
>  	if (i915_request_completed(rq))
> -		return false;
> +		return 0;
> +
> +	if (engine->execlists.switch_priority_hint < effective_prio(rq))
> +		return 0;
> +
> +	return timeslice(engine);
> +}
> +
> +static void set_timeslice(struct intel_engine_cs *engine)
> +{
> +	if (!intel_engine_has_timeslices(engine))
> +		return;
>  
> -	return execlists->switch_priority_hint >= effective_prio(rq);
> +	set_timer_ms(&engine->execlists.timer, active_timeslice(engine));
>  }
>  
>  static void record_preemption(struct intel_engine_execlists *execlists)
> @@ -1667,8 +1684,9 @@ static void execlists_dequeue(struct intel_engine_cs *engine)
>  				 */
>  				if (!execlists->timer.expires &&
>  				    need_timeslice(engine, last))
> -					mod_timer(&execlists->timer,
> -						  jiffies + 1);
> +					set_timer_ms(&execlists->timer,
> +						     timeslice(engine));


I tripped into the hidden cancellation in here. Not that it
would happen. Still upset I am of this set_timer_ms(timer, 0)

Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>

> +
>  				return;
>  			}
>  
> @@ -2092,10 +2110,7 @@ static void process_csb(struct intel_engine_cs *engine)
>  				       execlists_num_ports(execlists) *
>  				       sizeof(*execlists->pending));
>  
> -			if (enable_timeslice(execlists))
> -				mod_timer(&execlists->timer, jiffies + 1);
> -			else
> -				cancel_timer(&execlists->timer);
> +			set_timeslice(engine);
>  
>  			WRITE_ONCE(execlists->pending[0], NULL);
>  		} else {
> diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> index 9507d750ae14..5ed275ef0984 100644
> --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c
> +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c
> @@ -440,6 +440,8 @@ static int live_timeslice_preempt(void *arg)
>  	 * need to preempt the current task and replace it with another
>  	 * ready task.
>  	 */
> +	if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
> +		return 0;
>  
>  	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
>  	if (IS_ERR(obj))
> @@ -514,6 +516,11 @@ static void wait_for_submit(struct intel_engine_cs *engine,
>  	} while (!i915_request_is_active(rq));
>  }
>  
> +static long timeslice_threshold(const struct intel_engine_cs *engine)
> +{
> +	return 2 * msecs_to_jiffies_timeout(timeslice(engine)) + 1;
> +}
> +
>  static int live_timeslice_queue(void *arg)
>  {
>  	struct intel_gt *gt = arg;
> @@ -531,6 +538,8 @@ static int live_timeslice_queue(void *arg)
>  	 * ELSP[1] is already occupied, so must rely on timeslicing to
>  	 * eject ELSP[0] in favour of the queue.)
>  	 */
> +	if (!CONFIG_DRM_I915_TIMESLICE_DURATION)
> +		return 0;
>  
>  	obj = i915_gem_object_create_internal(gt->i915, PAGE_SIZE);
>  	if (IS_ERR(obj))
> @@ -608,8 +617,8 @@ static int live_timeslice_queue(void *arg)
>  			err = -EINVAL;
>  		}
>  
> -		/* Timeslice every jiffie, so within 2 we should signal */
> -		if (i915_request_wait(rq, 0, 3) < 0) {
> +		/* Timeslice every jiffy, so within 2 we should signal */
> +		if (i915_request_wait(rq, 0, timeslice_threshold(engine)) < 0) {
>  			struct drm_printer p =
>  				drm_info_printer(gt->i915->drm.dev);
>  
> -- 
> 2.24.0.rc1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx