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

[Niranjana Vishwanathapura]

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.

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
>