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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4252 bytes --]

Hi Niranjana,

On 16-08-2023 17:22, Niranjana Vishwanathapura wrote:
> Allow preemption timeout to be specified as a config option.
>
> v2: Change unit to microseconds (Tejas)
> v3: Remove get_default_preempt_timeout()
>
> Signed-off-by: Niranjana Vishwanathapura<niranjana.vishwanathapura@intel.com>
> ---
>   drivers/gpu/drm/xe/Kconfig.profile | 11 +++++++++++
>   drivers/gpu/drm/xe/xe_hw_engine.c  |  5 ++++-
>   drivers/gpu/drm/xe/xe_hw_engine.h  |  5 +++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/xe/Kconfig.profile b/drivers/gpu/drm/xe/Kconfig.profile
> index 9ad487647ca1..a3655ee6978e 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 (us, jiffy granularity)"
> +	default 640000 # microseconds
> +	help
> +	  How long to wait (in microseconds) 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.

Is there plan to make this config dependent on 
CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN/CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX ?

If not, value should be within 
XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN/XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX.

> +	  This is adjustable via XE_ENGINE_SET_PROPERTY_PREEMPTION_TIMEOUT.

If IIRC ioctl calls names are changed from XE_ENGINE_* to 
XE_EXEC_QUUEUE_*. Please confirm.

And this statement seems misleading, ioctl callwill set the properties 
for specific exec_queue and config is meant to be for all exec_queues 
unless changed by ioctl.

IMO better description will be "preemption property for exec_queue can 
be adjusted by XE_EXEC_QUEUE_SET_PROPERTY_PREEMPTION_TIMEOUT" or we can 
skip this

info altogether in kconfig because ioctl description might be already 
covering it.

BR

Himal Ghimiray

>   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..4c812d04e182 100644
> --- a/drivers/gpu/drm/xe/xe_hw_engine.c
> +++ b/drivers/gpu/drm/xe/xe_hw_engine.c
> @@ -370,7 +370,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 = XE_HW_ENGINE_PREEMPT_TIMEOUT;
>   		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 +562,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..71968ee2f600 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
> +#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

[-- Attachment #2: Type: text/html, Size: 5384 bytes --]