[PATCH] drm/xe: Modify minimum of various scheduler timeout to 0

[Matthew Brost]

On Wed, May 08, 2024 at 08:06:52AM -0500, Lucas De Marchi wrote:
> On Mon, May 06, 2024 at 07:04:47PM GMT, Himal Prasad Ghimiray wrote:
> > Remove the min/max configs and change minimum values of preemption and
> > timeslice to 0. The sysman teams must deactivate preemption and
> > timeslice to assess exclusivity mode. They depend on configuring
> > timeslice_duration_us and preempt_timeout_us to 0, ensuring these
> > values are set at their minimum.
> 
> nack for now, based on this explanation. See below.
> 
> > 
> > Cc: Matthew Brost <matthew.brost at intel.com>
> > Cc: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray at intel.com>
> > ---
> > drivers/gpu/drm/xe/Kconfig.profile | 38 ------------------------------
> > drivers/gpu/drm/xe/xe_hw_engine.h  | 28 ++--------------------
> > 2 files changed, 2 insertions(+), 64 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/Kconfig.profile b/drivers/gpu/drm/xe/Kconfig.profile
> > index ba17a25e8db3..9bf7adb45eea 100644
> > --- a/drivers/gpu/drm/xe/Kconfig.profile
> > +++ b/drivers/gpu/drm/xe/Kconfig.profile
> > @@ -1,27 +1,3 @@
> > -config DRM_XE_JOB_TIMEOUT_MAX
> > -	int "Default max job timeout (ms)"
> > -	default 10000 # milliseconds
> > -	help
> > -	  Configures the default max job timeout after which job will
> > -	  be forcefully taken away from scheduler.
> > -config DRM_XE_JOB_TIMEOUT_MIN
> > -	int "Default min job timeout (ms)"
> > -	default 1 # milliseconds
> > -	help
> > -	  Configures the default min job timeout after which job will
> > -	  be forcefully taken away from scheduler.
> > -config DRM_XE_TIMESLICE_MAX
> > -	int "Default max timeslice duration (us)"
> > -	default 10000000 # microseconds
> > -	help
> > -	  Configures the default max timeslice duration between multiple
> > -	  contexts by guc scheduling.
> > -config DRM_XE_TIMESLICE_MIN
> > -	int "Default min timeslice duration (us)"
> > -	default 1 # microseconds
> > -	help
> > -	  Configures the default min timeslice duration between multiple
> > -	  contexts by guc scheduling.
> > config DRM_XE_PREEMPT_TIMEOUT
> > 	int "Preempt timeout (us, jiffy granularity)"
> > 	default 640000 # microseconds
> > @@ -30,20 +6,6 @@ config DRM_XE_PREEMPT_TIMEOUT
> > 	  when submitting a new context. If the current context does not hit
> > 	  an arbitration point and yield to HW before the timer expires, the
> > 	  HW will be reset to allow the more important context to execute.
> > -config DRM_XE_PREEMPT_TIMEOUT_MAX
> > -	int "Default max preempt timeout (us)"
> > -	default 10000000 # microseconds
> > -	help
> > -	  Configures the default max preempt timeout after which context
> > -	  will be forcefully taken away and higher priority context will
> > -	  run.
> > -config DRM_XE_PREEMPT_TIMEOUT_MIN
> > -	int "Default min preempt timeout (us)"
> > -	default 1 # microseconds
> > -	help
> > -	  Configures the default min preempt timeout after which context
> > -	  will be forcefully taken away and higher priority context will
> > -	  run.
> > config DRM_XE_ENABLE_SCHEDTIMEOUT_LIMIT
> > 	bool "Default configuration of limitation on scheduler timeout"
> > 	default y
> > diff --git a/drivers/gpu/drm/xe/xe_hw_engine.h b/drivers/gpu/drm/xe/xe_hw_engine.h
> > index 71968ee2f600..803555c601ec 100644
> > --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> > +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> > @@ -10,41 +10,17 @@
> > 
> > struct drm_printer;
> > 
> > -#ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > -#define XE_HW_ENGINE_JOB_TIMEOUT_MIN CONFIG_DRM_XE_JOB_TIMEOUT_MIN
> > -#else
> > #define XE_HW_ENGINE_JOB_TIMEOUT_MIN 1
> > -#endif
> > -#ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MAX
> > -#define XE_HW_ENGINE_JOB_TIMEOUT_MAX CONFIG_DRM_XE_JOB_TIMEOUT_MAX
> > -#else
> > #define XE_HW_ENGINE_JOB_TIMEOUT_MAX (10 * 1000)
> 
> previously one could configure a kernel with a different max.
> And now it's hardcoded. How is this better and related to the
> motivation in the commit message?
> 
> is this commit doing 2 different things?
> 
> 1) Removing the _MAX from config since nobody is or should change that
> 2) Modify the min value to 0... That seems weird as basically there is
>    no minimum.

This is true.

> 
> I'd like to hear Matt Brost's opinion on that.

I chatted with Lucas and second his nack. The requirements for this
change are coming from sysman and are unclear. If sysman needs to set
this to 0 (akin to no maximum), why can't sysman build the kernel with a
Kconfig to allow this? This use case appears to be an HPC/AI data center
scenario in which the kernel is probably built with a non-standard
Kconfig anyway. Why can't CONFIG_DRM_*_MIN be set to zero? This needs to
be addressed clearly and publicly.

Matt
 
> 
> Lucas De Marchi
> 
> > -#endif
> > -#ifdef CONFIG_DRM_XE_TIMESLICE_MIN
> > -#define XE_HW_ENGINE_TIMESLICE_MIN CONFIG_DRM_XE_TIMESLICE_MIN
> > -#else
> > -#define XE_HW_ENGINE_TIMESLICE_MIN 1
> > -#endif
> > -#ifdef CONFIG_DRM_XE_TIMESLICE_MAX
> > -#define XE_HW_ENGINE_TIMESLICE_MAX CONFIG_DRM_XE_TIMESLICE_MAX
> > -#else
> > +#define XE_HW_ENGINE_TIMESLICE_MIN 0
> > #define XE_HW_ENGINE_TIMESLICE_MAX (10 * 1000 * 1000)
> > -#endif
> > #ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT
> > #define XE_HW_ENGINE_PREEMPT_TIMEOUT CONFIG_DRM_XE_PREEMPT_TIMEOUT
> > #else
> > #define XE_HW_ENGINE_PREEMPT_TIMEOUT (640 * 1000)
> > #endif
> > -#ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN
> > -#define XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN
> > -#else
> > -#define XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN 1
> > -#endif
> > -#ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX
> > -#define XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX
> > -#else
> > +#define XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN 0
> > #define XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX (10 * 1000 * 1000)
> > -#endif
> > 
> > int xe_hw_engines_init_early(struct xe_gt *gt);
> > int xe_hw_engines_init(struct xe_gt *gt);
> > -- 
> > 2.25.1
> >