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

[Lucas De Marchi]

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.

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

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
>