[Intel-xe] [PATCH 3/4] drm/xe: Add CONFIG_DRM_XE_PREEMPT_TIMEOUT_COMPUTE

[Upadhyay, Tejas]

> -----Original Message-----
> From: Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
> Sent: Monday, August 14, 2023 8:37 PM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-xe at lists.freedesktop.org; Harrison, John C
> <john.c.harrison at intel.com>
> Subject: Re: [PATCH 3/4] drm/xe: Add
> CONFIG_DRM_XE_PREEMPT_TIMEOUT_COMPUTE
> 
> On Sat, Aug 12, 2023 at 12:08:05AM -0700, Upadhyay, Tejas wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vishwanathapura, Niranjana
> >> <niranjana.vishwanathapura at intel.com>
> >> Sent: Tuesday, August 8, 2023 11:54 PM
> >> To: intel-xe at lists.freedesktop.org
> >> Cc: Harrison, John C <john.c.harrison at intel.com>; Upadhyay, Tejas
> >> <tejas.upadhyay at intel.com>
> >> Subject: [PATCH 3/4] drm/xe: Add
> >> CONFIG_DRM_XE_PREEMPT_TIMEOUT_COMPUTE
> >>
> >> Allow preemption timeout for compute engines to be specified as a
> >> config option. The purpose is actually to give compute tasks longer
> >> time to reach a pre-emption point after a pre-emption request has
> >> been issued. This is necessary because Gen12 does not support
> >> mid-thread pre-emption and compute tasks can have long running
> threads.
> >>
> >> Signed-off-by: John Harrison <John.C.Harrison at Intel.com>
> >> Signed-off-by: Niranjana Vishwanathapura
> >> <niranjana.vishwanathapura at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/Kconfig.profile | 12 ++++++++++++
> >> drivers/gpu/drm/xe/xe_hw_engine.c  |  8 +++++++-
> >> drivers/gpu/drm/xe/xe_hw_engine.h  |  6 ++++++
> >>  3 files changed, 25 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/xe/Kconfig.profile
> >> b/drivers/gpu/drm/xe/Kconfig.profile
> >> index 06742e192206..5ca3305a22b6 100644
> >> --- a/drivers/gpu/drm/xe/Kconfig.profile
> >> +++ b/drivers/gpu/drm/xe/Kconfig.profile
> >> @@ -31,6 +31,18 @@ config DRM_XE_PREEMPT_TIMEOUT
> >>  	  an arbitration point and yield to HW before the timer expires, the
> >>  	  HW will be reset to allow the more important context to execute.
> >>
> >> +	  Value should be within the
> >> DRM_XE_PREEMPT_TIMEOUT_MIN/MAX limits.
> >> +	  This is adjustable via
> >> XE_ENGINE_SET_PROPERTY_PREEMPTION_TIMEOUT.
> >> +config DRM_XE_PREEMPT_TIMEOUT_COMPUTE
> >> +	int "Preempt timeout for compute engines (ms, jiffy granularity)"
> >> +	default 7500 # milliseconds
> >
> >Same comment as previous patch about us unit.
> >
> 
> Got it.
> 
> >> +	help
> >> +	  How long to wait (in milliseconds) for a preemption event to occur
> >> +	  when submitting a new context to a compute capable engine. 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.
> >> +
> >>  	  Value should be within the
> >> DRM_XE_PREEMPT_TIMEOUT_MIN/MAX limits.
> >>  	  This is adjustable via
> >> XE_ENGINE_SET_PROPERTY_PREEMPTION_TIMEOUT.
> >>  config DRM_XE_PREEMPT_TIMEOUT_MAX
> >> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
> >> b/drivers/gpu/drm/xe/xe_hw_engine.c
> >> index 25bad007a85b..57a0fb50852c 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> >> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> >> @@ -340,7 +340,11 @@ hw_engine_setup_default_state(struct
> >> xe_hw_engine *hwe)
> >>
> >>  static u32 get_default_preempt_timeout(struct xe_hw_engine *hwe)  {
> >> -	return XE_HW_ENGINE_PREEMPT_TIMEOUT;
> >> +	if (hwe->class == XE_ENGINE_CLASS_RENDER ||
> >> +	    hwe->class == XE_ENGINE_CLASS_COMPUTE)
> >> +		return XE_HW_ENGINE_PREEMPT_TIMEOUT_COMPUTE;
> >> +	else
> >> +		return XE_HW_ENGINE_PREEMPT_TIMEOUT;
> >>  }
> >
> >Probably API in previous patch continuation of this change, but it does not
> make sense to introduce change in previous patch at least.
> >
> 
> Responded in previous patch.
> 
> >>
> >>  static void hw_engine_init_early(struct xe_gt *gt, struct
> >> xe_hw_engine *hwe, @@ -569,6 +573,8 @@ int
> >> xe_hw_engines_init_early(struct xe_gt *gt)
> >>
> >>  	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT <
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN);
> >>  	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT >
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX);
> >> +	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT_COMPUTE <
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN);
> >> +	BUILD_BUG_ON(XE_HW_ENGINE_PREEMPT_TIMEOUT_COMPUTE >
> >> +XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX);
> >
> >Can this be done shorter with,
> >BUILD_BUG_ON(get_default_preempt_timeout(hwe) <
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN);
> >BUILD_BUG_ON(get_default_preempt_timeout(hwe) >
> >XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX);
> >
> 
> No, we can't. The BUILD_BUG_ON is to detect errors during build time, not
> the runtime. Any config issues must be caught during build time.

Oh, I got it. Thanks

> 
> Thanks,
> Niranjana
> 
> >Thanks,
> >Tejas
> >>
> >>  	for (i = 0; i < ARRAY_SIZE(gt->hw_engines); i++)
> >>  		hw_engine_init_early(gt, &gt->hw_engines[i], i); diff --git
> >> a/drivers/gpu/drm/xe/xe_hw_engine.h
> >> b/drivers/gpu/drm/xe/xe_hw_engine.h
> >> index 582be209355b..c4f0969efc06 100644
> >> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
> >> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
> >> @@ -35,6 +35,12 @@ struct drm_printer;  #else  #define
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT (640 * 1000)  #endif
> >> +#ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT_COMPUTE
> >> +#define XE_HW_ENGINE_PREEMPT_TIMEOUT_COMPUTE \
> >> +	(CONFIG_DRM_XE_PREEMPT_TIMEOUT_COMPUTE * 1000) #else
> >> #define
> >> +XE_HW_ENGINE_PREEMPT_TIMEOUT_COMPUTE
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT
> >> +#endif
> >>  #ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN  #define
> >> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN
> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN
> >> #else
> >> --
> >> 2.21.0.rc0.32.g243a4c7e27
> >