Chris Wilson <ch...@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 <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@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.kuopp...@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

Reply via email to