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

[Niranjana Vishwanathapura]

On Wed, Aug 16, 2023 at 09:18:12PM +0530, Ghimiray, Himal Prasad wrote:
>   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 at 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 ?
>

No, it can be independently set. The _MIN and _MAX can also be independently set.

>   If not, value should be within
>   XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN/XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX.
>

Yes, the patch includes a BUILD_BUG_ON() to ensure that.

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

Yah, the for to change the name from Xe_ENGINE* to XE_EXEC_QUEUE.
Yah, makes sense to remove that line altogether. Will post a fixup patch.

Niranajna

>   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