[Intel-xe] [PATCH V3 6/6] drm/xe: Add min/max cap for engine scheduler properties
Niranjana Vishwanathapura
niranjana.vishwanathapura at intel.com
Thu Jun 29 20:14:24 UTC 2023
On Wed, Jun 28, 2023 at 09:19:10PM -0700, Upadhyay, Tejas wrote:
>
>
>> -----Original Message-----
>> From: Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
>> Sent: Thursday, June 29, 2023 4:09 AM
>> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
>> Cc: intel-xe at lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH V3 6/6] drm/xe: Add min/max cap for engine
>> scheduler properties
>>
>> On Wed, Jun 28, 2023 at 05:17:32PM +0530, Tejas Upadhyay wrote:
>> >Add sysfs entries for the min, max, and defaults for each of engine
>> >scheduler controls for every hardware engine class.
>> >
>> >Non-elevated user IOCTLs to set these controls must be within the
>> >min-max ranges of the sysfs entries, elevated user can set these
>> >controls to any value. However, introduced compile time CONFIG min-max
>> >values which restricts elevated user to be in compile time min-max
>> >range if at all sysfs min/max are violated.
>> >
>> >Sysfs entries examples are,
>> >DUT# cat /sys/class/drm/cardX/device/gtN/engines/ccs/.defaults/
>> >job_timeout_max job_timeout_ms preempt_timeout_min
>> timeslice_duration_max timeslice_duration_us
>> >job_timeout_min preempt_timeout_max preempt_timeout_us
>> timeslice_duration_min
>> >
>> >DUT# cat /sys/class/drm/card1/device/gt1/engines/ccs/
>> >.defaults/ job_timeout_min preempt_timeout_max
>> preempt_timeout_us timeslice_duration_min
>> >job_timeout_max job_timeout_ms preempt_timeout_min
>> timeslice_duration_max timeslice_duration_us
>> >
>> >V3 :
>> > - Resolve CI hooks warning for kernel-doc
>> >V2 :
>> > - Restric min/max setting to #define default min/max for
>> > elevated user - Himal
>> > - Remove unrelated changes from patch - Niranjana
>> >
>> >Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
>> >---
>> > drivers/gpu/drm/xe/Kconfig | 7 +
>> > drivers/gpu/drm/xe/Kconfig.profile | 38 +++
>> > drivers/gpu/drm/xe/xe_engine.c | 58 +++-
>> > drivers/gpu/drm/xe/xe_gt_sysfs.c | 407 +++++++++++++++++++++++-
>> > drivers/gpu/drm/xe/xe_gt_sysfs.h | 1 +
>> > drivers/gpu/drm/xe/xe_hw_engine.c | 6 +
>> > drivers/gpu/drm/xe/xe_hw_engine.h | 31 ++
>> > drivers/gpu/drm/xe/xe_hw_engine_types.h | 16 +-
>> > 8 files changed, 550 insertions(+), 14 deletions(-) create mode
>> >100644 drivers/gpu/drm/xe/Kconfig.profile
>> >
>> >diff --git a/drivers/gpu/drm/xe/Kconfig b/drivers/gpu/drm/xe/Kconfig
>> >index d44794f99338..964c440a1135 100644
>> >--- a/drivers/gpu/drm/xe/Kconfig
>> >+++ b/drivers/gpu/drm/xe/Kconfig
>> >@@ -83,3 +83,10 @@ depends on DRM_XE
>> > depends on EXPERT
>> > source "drivers/gpu/drm/xe/Kconfig.debug"
>> > endmenu
>> >+
>> >+menu "drm/xe Profile Guided Optimisation"
>> >+ visible if EXPERT
>> >+ depends on DRM_XE
>> >+ source "drivers/gpu/drm/xe/Kconfig.profile"
>> >+endmenu
>> >+
>> >diff --git a/drivers/gpu/drm/xe/Kconfig.profile
>> >b/drivers/gpu/drm/xe/Kconfig.profile
>> >new file mode 100644
>> >index 000000000000..bc65326d52b4
>> >--- /dev/null
>> >+++ b/drivers/gpu/drm/xe/Kconfig.profile
>> >@@ -0,0 +1,38 @@
>> >+config DRM_XE_JOB_TIMEOUT_MAX
>> >+ int "Default max job timeout (ms)"
>> >+ default 10000 # milliseconds
>> >+ help
>> >+ Configures the default max job timeout after which job will
>> >+ be forcefully taken away from scheduler.
>>
>> Should we also mention 'default and configurable upper limit"?
>> Similarly for other parameters also.
>
>'default and configurable upper limit" are same. Do you mean we should mention that?
May be we can say "sets the upper limit configurable job timeout max value".
We need to document somewhere what you responded below on min/max values and CAP_SYS_NICE.
>
>>
>> >+config DRM_XE_JOB_TIMEOUT_MIN
>> >+ int "Default max job timeout (ms)"
>> >+ default 1 # milliseconds
>> >+ help
>> >+ Configures the default min job timeout after which job will
>> >+ be forcefully taken away from scheduler.
>> >+config DRM_XE_TIMESLICE_MAX
>> >+ int "Default max timeslice duration (us)"
>> >+ default 10000000 # microseconds
>> >+ help
>> >+ Configures the default max timeslice duration between multiple
>> >+ contexts by guc scheduling.
>> >+config DRM_XE_TIMESLICE_MIN
>> >+ int "Default min timeslice duration (us)"
>> >+ default 1 # microseconds
>> >+ help
>> >+ Configures the default min timeslice duration between multiple
>> >+ contexts by guc scheduling.
>> >+config DRM_XE_PREEMPT_TIMEOUT_MAX
>> >+ int "Default max preempt timeout (us)"
>> >+ default 10000000 # microseconds
>> >+ help
>> >+ Configures the default max preempt timeout after which context
>> >+ will be forcefully taken away and higher priority context will
>> >+ run.
>> >+config DRM_XE_PREEMPT_TIMEOUT_MIN
>> >+ int "Default min preempt timeout (us)"
>> >+ default 1 # microseconds
>> >+ help
>> >+ Configures the default min preempt timeout after which context
>> >+ will be forcefully taken away and higher priority context will
>> >+ run.
>> >diff --git a/drivers/gpu/drm/xe/xe_engine.c
>> >b/drivers/gpu/drm/xe/xe_engine.c index 33d1fbae560d..37c33a231f00
>> >100644
>> >--- a/drivers/gpu/drm/xe/xe_engine.c
>> >+++ b/drivers/gpu/drm/xe/xe_engine.c
>> >@@ -13,6 +13,7 @@
>> >
>> > #include "xe_device.h"
>> > #include "xe_gt.h"
>> >+#include "xe_gt_sysfs.h"
>> > #include "xe_hw_fence.h"
>> > #include "xe_lrc.h"
>> > #include "xe_macros.h"
>> >@@ -191,9 +192,26 @@ static int engine_set_priority(struct xe_device
>> >*xe, struct xe_engine *e, static int engine_set_timeslice(struct xe_device
>> *xe, struct xe_engine *e,
>> > u64 value, bool create)
>> > {
>> >- if (!capable(CAP_SYS_NICE))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ e->hwe->sched_props.timeslice_min,
>> >+ e->hwe->sched_props.timeslice_max))
>> > return -EPERM;
>> >
>> >+#if defined(CONFIG_DRM_XE_TIMESLICE_MIN) &&
>> defined(CONFIG_DRM_XE_TIMESLICE_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ CONFIG_DRM_XE_TIMESLICE_MIN,
>> >+ CONFIG_DRM_XE_TIMESLICE_MAX))
>> >+ return -EPERM;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ XE_HW_ENGINE_TIMESLICE_MIN,
>> >+ XE_HW_ENGINE_TIMESLICE_MAX))
>> >+ return -EPERM;
>> >+#endif
>> >+
>> > return e->ops->set_timeslice(e, value); }
>> >
>> >@@ -201,9 +219,26 @@ static int engine_set_preemption_timeout(struct
>> xe_device *xe,
>> > struct xe_engine *e, u64 value,
>> > bool create)
>> > {
>> >- if (!capable(CAP_SYS_NICE))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ e->hwe-
>> >sched_props.preempt_timeout_min,
>> >+ e->hwe-
>> >sched_props.preempt_timeout_max))
>> > return -EPERM;
>> >
>> >+#if defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EPERM;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EPERM;
>> >+#endif
>> >+
>> > return e->ops->set_preempt_timeout(e, value); }
>> >
>> >@@ -269,8 +304,25 @@ static int engine_set_job_timeout(struct xe_device
>> *xe, struct xe_engine *e,
>> > if (XE_IOCTL_ERR(xe, !create))
>> > return -EINVAL;
>> >
>> >- if (!capable(CAP_SYS_NICE))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ e->hwe->sched_props.job_timeout_min,
>> >+ e->hwe->sched_props.job_timeout_max))
>> >+ return -EPERM;
>> >+
>> >+#if defined(CONFIG_DRM_XE_JOB_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_JOB_TIMEOUT_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MIN,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MAX))
>> >+ return -EPERM;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(value,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MIN,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MAX))
>> > return -EPERM;
>> >+#endif
>> >
>> > return e->ops->set_job_timeout(e, value); } diff --git
>> >a/drivers/gpu/drm/xe/xe_gt_sysfs.c b/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> >index 4c8f589870ba..743ef9f105d0 100644
>> >--- a/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> >+++ b/drivers/gpu/drm/xe/xe_gt_sysfs.c
>> >@@ -34,6 +34,11 @@ static const struct kobj_type kobj_xe_engine_type = {
>> > .sysfs_ops = &kobj_sysfs_ops
>> > };
>> >
>> >+bool engine_timeout_in_range(u64 timeout, u64 min, u64 max) {
>> >+ return timeout >= min && timeout <= max; }
>> >+
>> > static ssize_t preempt_timeout_store(struct kobject *kobj,
>> > struct kobj_attribute *attr,
>> > const char *buf, size_t count) @@ -46,9
>> +51,26 @@ static
>> >ssize_t preempt_timeout_store(struct kobject *kobj,
>> > if (err)
>> > return err;
>> >
>> >- if (timeout > jiffies_to_usecs(MAX_SCHEDULE_TIMEOUT))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ hwe-
>> >sched_props.preempt_timeout_min,
>> >+ hwe-
>> >sched_props.preempt_timeout_max))
>> > return -EINVAL;
>> >
>> >+#if defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>>
>> It is still confusing to me as to,
>> 1. Given, hwe->sched_props.preempt_timeout_min/max is already
>> subjected to boundary
>> check, why we are checking again.
>> 2. What is the logic with CAP_SYS_NICE here?
>> More on this below.
>
>Non-elevated user : will have limit of what is configured through sysfs min/max, which might be same as default/compile configured min/max or different one under limit of default/compile configured min/max.
>
>Elevated user : Will also have limit but of default/compile configured min/max, which might be same or above range of configured through sysfs min/max. For example, sysfs_min = 2, sysfs_max = 5, default/compile_min = 1, default/compile_max = 100
>
>Non-elevated user tries :
>1. set timeout 3 --> in limit of sysfs min/max --> allowed
>2. set timeout 6 --> out of limit of sysfs min/max --> not allowed
>
>Elevated user tries :
>1. set timeout 3 --> in limit of sysfs min/max --> allowed
>2. set timeout 6 --> out of limit of sysfs min/max --> in limit of default/compile_min/max --> allowed
>3. set timeout 101 --> out of limit of sysfs min/max --> out of limit of default/compile_min/max --> not allowed
>
Ok. But all the '#if defined(CONFIG..." checks seems incorrect.
We should just check against the XE_HW_ENGINE*_MIN/MAX.
min = capable(CAP_SYS_NICE) ? XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN : hwe->sched_props.preempt_timeout_min;
max = capable(CAP_SYS_NICE) ? XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX : hwe->sched_props.preempt_timeout_max;
if (!engine_timeout_in_range(timeout, min, max))
return -EINVAL;
>>
>> > WRITE_ONCE(hwe->sched_props.preempt_timeout_us, timeout);
>> >
>> > return count;
>> >@@ -65,6 +87,90 @@ static ssize_t preempt_timeout_show(struct kobject
>> >*kobj, static struct kobj_attribute preempt_timeout_attr =
>> >__ATTR(preempt_timeout_us, 0644, preempt_timeout_show,
>> >preempt_timeout_store);
>> >
>> >+static ssize_t preempt_timeout_max_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count)
>> >+{
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 timeout;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &timeout);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX)
>> >+ if (!engine_timeout_in_range(timeout,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.preempt_timeout_max, timeout);
>> >+
>> >+ return count;
>> >+}
>>
>> I still have few concerns here.
>> 1. If the configs are defined, then we don't need CAP_SYS_NICE? If there is
>> some logic here,
>> can you add a comment with the explanation?
>
>As I explained scenario above for elevated user.
>
>> 2. XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN/MAX already checks the
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN/MAX
>> if they are defined. Then, we do we need separate checks here?
>
>This is correct, I had this change to use directly XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN/MAX locally, somehow missed to add it. I will address this.
>
Ok, also, here we can endup with min being greater than max.
We should check against that as well.
Niranjana
>>
>> >+
>> >+static ssize_t preempt_timeout_max_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf)
>> {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe-
>> >sched_props.preempt_timeout_max);
>> >+}
>> >+
>> >+static struct kobj_attribute preempt_timeout_max_attr =
>> >+ __ATTR(preempt_timeout_max, 0644, preempt_timeout_max_show,
>> >+ preempt_timeout_max_store);
>> >+
>> >+static ssize_t preempt_timeout_min_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count)
>> >+{
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 timeout;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &timeout);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX)
>> >+ if (!engine_timeout_in_range(timeout,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN,
>> >+
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN,
>> >+
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.preempt_timeout_min, timeout);
>> >+
>> >+ return count;
>> >+}
>> >+
>> >+static ssize_t preempt_timeout_min_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf)
>> {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe-
>> >sched_props.preempt_timeout_min);
>> >+}
>> >+
>> >+static struct kobj_attribute preempt_timeout_min_attr =
>> >+ __ATTR(preempt_timeout_min, 0644, preempt_timeout_min_show,
>> >+ preempt_timeout_min_store);
>> >+
>> > static ssize_t timeslice_duration_store(struct kobject *kobj,
>> > struct kobj_attribute *attr,
>> > const char *buf, size_t count)
>> >@@ -77,14 +183,116 @@ static ssize_t timeslice_duration_store(struct
>> kobject *kobj,
>> > if (err)
>> > return err;
>> >
>> >- if (duration > jiffies_to_usecs(MAX_SCHEDULE_TIMEOUT))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(duration,
>> >+ hwe->sched_props.timeslice_min,
>> >+ hwe->sched_props.timeslice_max))
>> > return -EINVAL;
>> >
>> >+#if defined(CONFIG_DRM_XE_TIMESLICE_MIN) &&
>> defined(CONFIG_DRM_XE_TIMESLICE_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(duration,
>> >+ CONFIG_DRM_XE_TIMESLICE_MIN,
>> >+ CONFIG_DRM_XE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(duration,
>> >+ XE_HW_ENGINE_TIMESLICE_MIN,
>> >+ XE_HW_ENGINE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> > WRITE_ONCE(hwe->sched_props.timeslice_us, duration);
>> >
>> > return count;
>> > }
>> >
>> >+static ssize_t timeslice_duration_max_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 duration;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &duration);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_TIMESLICE_MIN) &&
>> defined(CONFIG_DRM_XE_TIMESLICE_MAX)
>> >+ if (!engine_timeout_in_range(duration,
>> >+ CONFIG_DRM_XE_TIMESLICE_MIN,
>> >+ CONFIG_DRM_XE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(duration,
>> >+ XE_HW_ENGINE_TIMESLICE_MIN,
>> >+ XE_HW_ENGINE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.timeslice_max, duration);
>> >+
>> >+ return count;
>> >+}
>> >+
>> >+static ssize_t timeslice_duration_max_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ char *buf)
>> >+{
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->sched_props.timeslice_max); }
>> >+
>> >+static struct kobj_attribute timeslice_duration_max_attr =
>> >+ __ATTR(timeslice_duration_max, 0644,
>> timeslice_duration_max_show,
>> >+ timeslice_duration_max_store);
>> >+
>> >+static ssize_t timeslice_duration_min_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 duration;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &duration);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_TIMESLICE_MIN) &&
>> defined(CONFIG_DRM_XE_TIMESLICE_MAX)
>> >+ if (!engine_timeout_in_range(duration,
>> >+ CONFIG_DRM_XE_TIMESLICE_MIN,
>> >+ CONFIG_DRM_XE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(duration,
>> >+ XE_HW_ENGINE_TIMESLICE_MIN,
>> >+ XE_HW_ENGINE_TIMESLICE_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.timeslice_min, duration);
>> >+
>> >+ return count;
>> >+}
>> >+
>> >+static ssize_t timeslice_duration_min_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char
>> *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->sched_props.timeslice_min); }
>> >+
>> >+static struct kobj_attribute timeslice_duration_min_attr =
>> >+ __ATTR(timeslice_duration_min, 0644,
>> timeslice_duration_min_show,
>> >+ timeslice_duration_min_store);
>> >+
>> > static ssize_t timeslice_duration_show(struct kobject *kobj,
>> > struct kobj_attribute *attr, char *buf) {
>> @@ -109,8
>> >+317,25 @@ static ssize_t job_timeout_store(struct kobject *kobj,
>> > if (err)
>> > return err;
>> >
>> >- if (timeout > jiffies_to_usecs(MAX_SCHEDULE_TIMEOUT))
>> >+ if (!capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ hwe->sched_props.job_timeout_min,
>> >+ hwe->sched_props.job_timeout_max))
>> >+ return -EINVAL;
>> >+
>> >+#if defined(CONFIG_DRM_XE_JOB_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_JOB_TIMEOUT_MAX)
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MIN,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MIN,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MAX))
>> > return -EINVAL;
>> >+#endif
>> >
>> > WRITE_ONCE(hwe->sched_props.job_timeout_ms, timeout);
>> >
>> >@@ -128,13 +353,6 @@ static ssize_t job_timeout_show(struct kobject
>> >*kobj, static struct kobj_attribute job_timeout_attr =
>> >__ATTR(job_timeout_ms, 0644, job_timeout_show, job_timeout_store);
>> >
>> >-static const struct attribute *files[] = {
>> >- &job_timeout_attr.attr,
>> >- ×lice_duration_attr.attr,
>> >- &preempt_timeout_attr.attr,
>> >- NULL
>> >-};
>> >-
>> > static ssize_t job_timeout_default(struct kobject *kobj,
>> > struct kobj_attribute *attr, char *buf) {
>> @@ -143,6 +361,28 @@
>> >static ssize_t job_timeout_default(struct kobject *kobj,
>> > return sprintf(buf, "%u\n", hwe->defaults.job_timeout_ms); }
>> >
>> >+static ssize_t job_timeout_min_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.job_timeout_min); }
>> >+
>> >+static struct kobj_attribute job_timeout_min_def =
>> >+__ATTR(job_timeout_min, 0444, job_timeout_min_default, NULL);
>> >+
>> >+static ssize_t job_timeout_max_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.job_timeout_max); }
>> >+
>> >+static struct kobj_attribute job_timeout_max_def =
>> >+__ATTR(job_timeout_max, 0444, job_timeout_max_default, NULL);
>> >+
>> > static struct kobj_attribute job_timeout_def = __ATTR(job_timeout_ms,
>> > 0444, job_timeout_default, NULL);
>> >
>> >@@ -157,6 +397,123 @@ static ssize_t timeslice_default(struct kobject
>> >*kobj, static struct kobj_attribute timeslice_duration_def =
>> >__ATTR(timeslice_duration_us, 0444, timeslice_default, NULL);
>> >
>> >+static ssize_t timeslice_min_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.timeslice_min); }
>> >+
>> >+static struct kobj_attribute timeslice_duration_min_def =
>> >+__ATTR(timeslice_duration_min, 0444, timeslice_min_default, NULL);
>> >+
>> >+static ssize_t timeslice_max_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.timeslice_max); }
>> >+
>> >+static struct kobj_attribute timeslice_duration_max_def =
>> >+__ATTR(timeslice_duration_max, 0444, timeslice_max_default, NULL);
>> >+
>> >+static ssize_t job_timeout_max_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 timeout;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &timeout);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_JOB_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_JOB_TIMEOUT_MAX)
>> >+ if (!engine_timeout_in_range(timeout,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MIN,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MIN,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.job_timeout_max, timeout);
>> >+
>> >+ return count;
>> >+}
>> >+
>> >+static ssize_t job_timeout_max_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->sched_props.job_timeout_max);
>> >+}
>> >+
>> >+static struct kobj_attribute job_timeout_max_attr =
>> >+__ATTR(job_timeout_max, 0644, job_timeout_max_show,
>> >+job_timeout_max_store);
>> >+
>> >+static ssize_t job_timeout_min_store(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ const char *buf, size_t count) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+ u32 timeout;
>> >+ int err;
>> >+
>> >+ err = kstrtou32(buf, 0, &timeout);
>> >+ if (err)
>> >+ return err;
>> >+
>> >+#if defined(CONFIG_DRM_XE_JOB_TIMEOUT_MIN) &&
>> defined(CONFIG_DRM_XE_JOB_TIMEOUT_MAX)
>> >+ if (!engine_timeout_in_range(timeout,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MIN,
>> >+ CONFIG_DRM_XE_JOB_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#else
>> >+ if (capable(CAP_SYS_NICE) &&
>> >+ !engine_timeout_in_range(timeout,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MIN,
>> >+ XE_HW_ENGINE_JOB_TIMEOUT_MAX))
>> >+ return -EINVAL;
>> >+#endif
>> >+
>> >+ WRITE_ONCE(hwe->sched_props.job_timeout_min, timeout);
>> >+
>> >+ return count;
>> >+}
>> >+
>> >+static ssize_t job_timeout_min_show(struct kobject *kobj,
>> >+ struct kobj_attribute *attr, char *buf) {
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->sched_props.job_timeout_min);
>> >+}
>> >+
>> >+static struct kobj_attribute job_timeout_min_attr =
>> >+__ATTR(job_timeout_min, 0644, job_timeout_min_show,
>> >+job_timeout_min_store);
>> >+
>> >+static const struct attribute *files[] = {
>> >+ &job_timeout_attr.attr,
>> >+ &job_timeout_min_attr.attr,
>> >+ &job_timeout_max_attr.attr,
>> >+ ×lice_duration_attr.attr,
>> >+ ×lice_duration_min_attr.attr,
>> >+ ×lice_duration_max_attr.attr,
>> >+ &preempt_timeout_attr.attr,
>> >+ &preempt_timeout_min_attr.attr,
>> >+ &preempt_timeout_max_attr.attr,
>> >+ NULL
>> >+};
>>
>> Why is this moved down here?
>> Looks like things are not kept in order.
>>
>> >+
>> > static ssize_t preempt_timeout_default(struct kobject *kobj,
>> > struct kobj_attribute *attr,
>> > char *buf)
>> >@@ -169,10 +526,40 @@ static ssize_t preempt_timeout_default(struct
>> >kobject *kobj, static struct kobj_attribute preempt_timeout_def =
>> >__ATTR(preempt_timeout_us, 0444, preempt_timeout_default, NULL);
>> >
>> >+static ssize_t preempt_timeout_min_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ char *buf)
>> >+{
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.preempt_timeout_min);
>> >+}
>> >+
>> >+static struct kobj_attribute preempt_timeout_min_def =
>> >+__ATTR(preempt_timeout_min, 0444, preempt_timeout_min_default,
>> NULL);
>> >+
>> >+static ssize_t preempt_timeout_max_default(struct kobject *kobj,
>> >+ struct kobj_attribute *attr,
>> >+ char *buf)
>> >+{
>> >+ struct xe_hw_engine *hwe = kobj_to_hwe(kobj->parent);
>> >+
>> >+ return sprintf(buf, "%u\n", hwe->defaults.preempt_timeout_max);
>> >+}
>> >+
>> >+static struct kobj_attribute preempt_timeout_max_def =
>> >+__ATTR(preempt_timeout_max, 0444, preempt_timeout_max_default,
>> NULL);
>> >+
>> > static const struct attribute *defaults[] = {
>> > &job_timeout_def.attr,
>> >+ &job_timeout_min_def.attr,
>> >+ &job_timeout_max_def.attr,
>> > ×lice_duration_def.attr,
>> >+ ×lice_duration_min_def.attr,
>> >+ ×lice_duration_max_def.attr,
>> > &preempt_timeout_def.attr,
>> >+ &preempt_timeout_min_def.attr,
>> >+ &preempt_timeout_max_def.attr,
>> > NULL
>> > };
>> >
>> >diff --git a/drivers/gpu/drm/xe/xe_gt_sysfs.h
>> >b/drivers/gpu/drm/xe/xe_gt_sysfs.h
>> >index a539cf031c7d..a7ac7b9a9768 100644
>> >--- a/drivers/gpu/drm/xe/xe_gt_sysfs.h
>> >+++ b/drivers/gpu/drm/xe/xe_gt_sysfs.h
>> >@@ -12,6 +12,7 @@
>> >
>> > int xe_gt_sysfs_init(struct xe_gt *gt); int xe_gt_sysfs_engines(struct
>> > xe_gt *gt);
>> >+bool engine_timeout_in_range(u64 timeout, u64 min, u64 max);
>> >
>> > static inline struct xe_gt *
>> > kobj_to_gt(struct kobject *kobj)
>> >diff --git a/drivers/gpu/drm/xe/xe_hw_engine.c
>> >b/drivers/gpu/drm/xe/xe_hw_engine.c
>> >index bd94155a28b3..2b053ee0af80 100644
>> >--- a/drivers/gpu/drm/xe/xe_hw_engine.c
>> >+++ b/drivers/gpu/drm/xe/xe_hw_engine.c
>> >@@ -363,8 +363,14 @@ static void hw_engine_init_early(struct xe_gt *gt,
>> struct xe_hw_engine *hwe,
>> > hwe->engine_id = id;
>> > /* FIXME: Wire up to configurable default value */
>> > hwe->sched_props.job_timeout_ms = HZ * 5;
>> >+ hwe->sched_props.job_timeout_min =
>> XE_HW_ENGINE_JOB_TIMEOUT_MIN;
>> >+ hwe->sched_props.job_timeout_max =
>> XE_HW_ENGINE_JOB_TIMEOUT_MAX;
>> > hwe->sched_props.timeslice_us = 1 * 1000;
>> >+ hwe->sched_props.timeslice_min =
>> XE_HW_ENGINE_TIMESLICE_MIN;
>> >+ hwe->sched_props.timeslice_max =
>> XE_HW_ENGINE_TIMESLICE_MAX;
>> > hwe->sched_props.preempt_timeout_us = 640 * 1000;
>> >+ hwe->sched_props.preempt_timeout_min =
>> XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN;
>> >+ hwe->sched_props.preempt_timeout_max =
>> >+XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX;
>> > hwe->defaults = hwe->sched_props; /* Record default props */
>> >
>> > xe_reg_sr_init(&hwe->reg_sr, hwe->name, gt_to_xe(gt)); diff --git
>> >a/drivers/gpu/drm/xe/xe_hw_engine.h
>> b/drivers/gpu/drm/xe/xe_hw_engine.h
>> >index 7eca9d53c7b1..3d37d6d44261 100644
>> >--- a/drivers/gpu/drm/xe/xe_hw_engine.h
>> >+++ b/drivers/gpu/drm/xe/xe_hw_engine.h
>> >@@ -10,6 +10,37 @@
>> >
>> > struct drm_printer;
>> >
>> >+#ifdef CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>> >+#define XE_HW_ENGINE_JOB_TIMEOUT_MIN
>> CONFIG_DRM_XE_JOB_TIMEOUT_MIN
>> >+#else #define XE_HW_ENGINE_JOB_TIMEOUT_MIN 1 #endif #ifdef
>> >+CONFIG_DRM_XE_JOB_TIMEOUT_MAX #define
>> XE_HW_ENGINE_JOB_TIMEOUT_MAX
>> >+CONFIG_DRM_XE_JOB_TIMEOUT_MAX #else #define
>> >+XE_HW_ENGINE_JOB_TIMEOUT_MAX (10 * 1000) #endif #ifdef
>> >+CONFIG_DRM_XE_TIMESLICE_MIN #define
>> XE_HW_ENGINE_TIMESLICE_MIN
>> >+CONFIG_DRM_XE_TIMESLICE_MIN #else #define
>> XE_HW_ENGINE_TIMESLICE_MIN 1
>> >+#endif #ifdef CONFIG_DRM_XE_TIMESLICE_MAX #define
>> >+XE_HW_ENGINE_TIMESLICE_MAX CONFIG_DRM_XE_TIMESLICE_MAX
>> #else #define
>> >+XE_HW_ENGINE_TIMESLICE_MAX (10 * 1000 * 1000) #endif #ifdef
>> >+CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN
>> >+#define XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN
>> >+CONFIG_DRM_XE_PREEMPT_TIMEOUT_MIN #else #define
>> >+XE_HW_ENGINE_PREEMPT_TIMEOUT_MIN 1 #endif
>>
>> So, we can never have a preempt timeout of 0?
>> As I mentioned before, guc disables preempt timeout when it is set to 0.
>
>Yes I will add min as 0 for pre-empt timeout.
>
>Thanks,
>Tejas
>>
>> >+#ifdef CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX #define
>> >+XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX
>> CONFIG_DRM_XE_PREEMPT_TIMEOUT_MAX
>> >+#else #define XE_HW_ENGINE_PREEMPT_TIMEOUT_MAX (10 * 1000 *
>> 1000)
>> >+#endif
>> >+
>> > int xe_hw_engines_init_early(struct xe_gt *gt); int
>> >xe_hw_engines_init(struct xe_gt *gt); void
>> >xe_hw_engine_handle_irq(struct xe_hw_engine *hwe, u16 intr_vec); diff
>> >--git a/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> >b/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> >index 7a8d275df6db..d47e34e32a6c 100644
>> >--- a/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> >+++ b/drivers/gpu/drm/xe/xe_hw_engine_types.h
>> >@@ -107,14 +107,28 @@ struct xe_hw_engine {
>> > void (*irq_handler)(struct xe_hw_engine *, u16);
>> > /** @engine_id: id for this hw engine */
>> > enum xe_hw_engine_id engine_id;
>> >- /** @sched_props: scheduling properties */
>> >+ /** @sched_props: scheduling properties
>> >+ * @defaults: Default scheduling properties
>> >+ */
>>
>> This should be done patch #2.
>>
>> Niranjana
>>
>> > struct {
>> > /** @set_job_timeout: Set job timeout in ms for engine */
>> > u32 job_timeout_ms;
>> >+ /** @job_timeout_min: Min job timeout in ms for engine */
>> >+ u32 job_timeout_min;
>> >+ /** @job_timeout_max: Max job timeout in ms for engine */
>> >+ u32 job_timeout_max;
>> > /** @timeslice_us: timeslice period in micro-seconds */
>> > u32 timeslice_us;
>> >+ /** @timeslice_min: min timeslice period in micro-seconds
>> */
>> >+ u32 timeslice_min;
>> >+ /** @timeslice_max: max timeslice period in micro-seconds
>> */
>> >+ u32 timeslice_max;
>> > /** @preempt_timeout_us: preemption timeout in micro-
>> seconds */
>> > u32 preempt_timeout_us;
>> >+ /** @preempt_timeout_min: min preemption timeout in
>> micro-seconds */
>> >+ u32 preempt_timeout_min;
>> >+ /** @preempt_timeout_max: max preemption timeout in
>> micro-seconds */
>> >+ u32 preempt_timeout_max;
>> > } sched_props, defaults;
>> > };
>> >
>> >--
>> >2.25.1
>> >
More information about the Intel-xe
mailing list