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

[Joonas Lahtinen]

Quoting Matthew Brost (2024-05-09 19:00:43)
> 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>

<SNIP>

> > > +++ 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.

Maybe I've missed some discussion but what is wrong with the previously
agreed approach where KConfig would determine hard _MIN/_MAX bounds
(and maybe _DEFAULT) that the kernel can work with, then there would be
sysfs min/max (+ default) which the sysadmin sets as comfortable with
considering the expected use-cases on the system and application then
choose within those bounds (if it wants expedited hang detection for an
example)?

> > 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.

I also thought we agreed on doing different build for such scenario
where the interactive desktop is not expected to be used, so larger
delays are more allowable.

The default build simply can't allow unlimited blocking of the HW by
certain workloads in order to make sure the desktop/compositor stays
operational. That can only be enabled by default build when the long-running
workloads can be isolated so they won't interfere with the workloads
using dma-fences. And I don't think that code exists yet.

Sysman should be able to report a feature missing from the kernel if it
detects that it can't set the limits wanted. This can be reported to
user via error code and/or error message asking to install a different
kernel build.

Same strategy has been applied in the past for various kernel features
like realtime/virtualization etc. 

Regards, Joonas

> 
> 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
> > >