[Intel-xe] [PATCH V3 6/6] drm/xe: Add min/max cap for engine scheduler properties
Upadhyay, Tejas
tejas.upadhyay at intel.com
Fri Jun 30 04:32:17 UTC 2023
> -----Original Message-----
> From: Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
> Sent: Friday, June 30, 2023 1:44 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 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.
Ok.
>
> >
> >>
> >> >+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;
Looks good, I will add this change.
>
> >>
> >> > 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.
Ok, looks like we need to check that as well. I will see that.
>
> 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
I think we cant set 0 here, as Matt Brost commented below to me, please see :
"0 is disabled, right? i think per dma-fence rules we can't let anyone do that, I believe Thomas H had a rant about that somewhere"
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