[Intel-xe] [PATCH 2/4] drm/xe: Add CONFIG_DRM_XE_PREEMPT_TIMEOUT

[Niranjana Vishwanathapura]

On Fri, Aug 11, 2023 at 11:57:31PM -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 2/4] drm/xe: Add CONFIG_DRM_XE_PREEMPT_TIMEOUT
>>
>> Allow preemption timeout to be specified as a config option.
>>
>> Signed-off-by: Niranjana Vishwanathapura
>> <niranjana.vishwanathapura at intel.com>
>> ---
>>  drivers/gpu/drm/xe/Kconfig.profile | 11 +++++++++++
>> drivers/gpu/drm/xe/xe_hw_engine.c  | 10 +++++++++-
>> drivers/gpu/drm/xe/xe_hw_engine.h  |  5 +++++
>>  3 files changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/Kconfig.profile
>> b/drivers/gpu/drm/xe/Kconfig.profile
>> index 9ad487647ca1..06742e192206 100644
>> --- a/drivers/gpu/drm/xe/Kconfig.profile
>> +++ b/drivers/gpu/drm/xe/Kconfig.profile
>> @@ -22,6 +22,17 @@ config DRM_XE_TIMESLICE_MIN
>>  	help
>>  	  Configures the default min timeslice duration between multiple
>>  	  contexts by guc scheduling.
>> +config DRM_XE_PREEMPT_TIMEOUT
>> +	int "Preempt timeout (ms, jiffy granularity)"
>> +	default 640 # milliseconds
>
>Why cant we represent this in us unit? At least it keeps uniformity with other code.
>

Thanks Tejas for the review.
Makes sense, will change.

>> +	help
>> +	  How long to wait (in milliseconds) for a preemption event to occur
>> +	  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.
>> +
>> +	  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
>>  	int "Default max  preempt timeout (us)"
>>  	default 10000000 # microseconds
>> diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
>> b/drivers/gpu/drm/xe/xe_hw_engine.c
>> index c44540684462..25bad007a85b 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> @@ -338,6 +338,11 @@ hw_engine_setup_default_state(struct
>> xe_hw_engine *hwe)
>>  	xe_rtp_process_to_sr(&ctx, engine_entries, &hwe->reg_sr);  }
>>
>> +static u32 get_default_preempt_timeout(struct xe_hw_engine *hwe) {
>> +	return XE_HW_ENGINE_PREEMPT_TIMEOUT;
>> +}
>> +
>>  static void hw_engine_init_early(struct xe_gt *gt, struct xe_hw_engine *hwe,
>>  				 enum xe_hw_engine_id id)
>>  {
>> @@ -370,7 +375,7 @@ static void hw_engine_init_early(struct xe_gt *gt,
>> struct xe_hw_engine *hwe,
>>  		hwe->eclass->sched_props.timeslice_us = 1 * 1000;
>>  		hwe->eclass->sched_props.timeslice_min =
>> XE_HW_ENGINE_TIMESLICE_MIN;
>>  		hwe->eclass->sched_props.timeslice_max =
>> XE_HW_ENGINE_TIMESLICE_MAX;
>> -		hwe->eclass->sched_props.preempt_timeout_us = 640 *
>> 1000;
>> +		hwe->eclass->sched_props.preempt_timeout_us =
>> +get_default_preempt_timeout(hwe);
>
>I don't see purpose of using this API, also adding hwe argument does not seem to be any useful.
>

AFAIK, the norm followed in a patch series is to introduce required functions
early in the series so that there is no deletion of added code later in the
series. Also, it is a static function not an API. Will make it inline.

Niranjana

>Thanks,
>Tejas
>>  		hwe->eclass->sched_props.preempt_timeout_min =
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN;
>>  		hwe->eclass->sched_props.preempt_timeout_max =
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX;
>>  		/* Record default props */
>> @@ -562,6 +567,9 @@ int xe_hw_engines_init_early(struct xe_gt *gt)
>>  	read_copy_fuses(gt);
>>  	read_compute_fuses(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);
>> +
>>  	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 3d37d6d44261..582be209355b 100644
>> --- a/drivers/gpu/drm/xe/xe_hw_engine.h
>> +++ b/drivers/gpu/drm/xe/xe_hw_engine.h
>> @@ -30,6 +30,11 @@ struct drm_printer;
>>  #else
>>  #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 *
>> +1000) #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
>> --
>> 2.21.0.rc0.32.g243a4c7e27
>