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

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

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@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.

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