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

Wed Aug 16 16:41:04 UTC 2023

On Wed, Aug 16, 2023 at 10:00:59PM +0530, Ghimiray, Himal Prasad wrote:
>Hi Niranjana,
>
>On 16-08-2023 21:27, Niranjana Vishwanathapura wrote:
>>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 they can be independently set we can have scenario where there is 
>no DRM_XE_PREEMPT_TIMEOUT_MIN/MAX limits
>
>and above statement "Value should be within the 
>DRM_XE_PREEMPT_TIMEOUT_MIN/MAX limits"  becomes somewhat wrong.
>
>>
>>>  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.
>
>Yes that is correct. I had only pointed this from description in 
>Kconfig perspective.
>

Ok. May be we are overthinking here. It doesn't sound right to
force MIN/MAX to be defined here and we have BUILD_BUG_ON to
report any configuration issue anyway.

Let me remove this line as well and MIN/MAX documentation should
be suffecient here I think.

Niranjana

>BR
>
>Himal Ghimiray
>
>>
>>>+         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