[Intel-xe] [PATCH V3 5/6] drm/xe: Add sysfs for preempt reset timeout

[Upadhyay, Tejas]

> -----Original Message-----
> From: Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
> Sent: Thursday, June 29, 2023 2:03 AM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [Intel-xe] [PATCH V3 5/6] drm/xe: Add sysfs for preempt reset
> timeout
> 
> On Wed, Jun 28, 2023 at 05:17:31PM +0530, Tejas Upadhyay wrote:
> >The preemption request and timeout is used for higher priority context
> >or kill hung context and reset gpu.
> >
> >The preempt timeout can be adjusted per-engine class using,
> >
> >/sys/class/drm/cardX/device/gtN/engines/ccs/preempt_timeout_us
> >
> >and can be disabled by setting it to 0.
> >
> >Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> >---
> > drivers/gpu/drm/xe/xe_gt_sysfs.c | 44 +++++++++++++++++++++++++++++-
> --
> > 1 file changed, 40 insertions(+), 4 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/xe/xe_gt_sysfs.c
> >b/drivers/gpu/drm/xe/xe_gt_sysfs.c
> >index 6ca77c5917cc..4c8f589870ba 100644
> >--- a/drivers/gpu/drm/xe/xe_gt_sysfs.c
> >+++ b/drivers/gpu/drm/xe/xe_gt_sysfs.c
> >@@ -34,6 +34,37 @@ static const struct kobj_type kobj_xe_engine_type = {
> > 	.sysfs_ops = &kobj_sysfs_ops
> > };
> >
> >+static ssize_t preempt_timeout_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 (timeout > jiffies_to_usecs(MAX_SCHEDULE_TIMEOUT))
> >+		return -EINVAL;
> >+
> >+	WRITE_ONCE(hwe->sched_props.preempt_timeout_us, timeout);
> >+
> >+	return count;
> >+}
> 
> So, these updated values does not affect the already created user engines.
> Maybe we can add that to commit description?

Sure

> 
> >+
> >+static ssize_t preempt_timeout_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_us);
> >+}
> >+
> >+static struct kobj_attribute preempt_timeout_attr =
> >+__ATTR(preempt_timeout_us, 0644, preempt_timeout_show,
> >+preempt_timeout_store);
> >+
> > static ssize_t timeslice_duration_store(struct kobject *kobj,
> > 					struct kobj_attribute *attr,
> > 					const char *buf, size_t count)
> >@@ -100,6 +131,7 @@ __ATTR(job_timeout_ms, 0644, job_timeout_show,
> >job_timeout_store);  static const struct attribute *files[] = {
> > 	&job_timeout_attr.attr,
> > 	&timeslice_duration_attr.attr,
> >+	&preempt_timeout_attr.attr,
> > 	NULL
> > };
> >
> >@@ -261,10 +293,14 @@ int xe_gt_sysfs_engines(struct xe_gt *gt)
> > 		}
> >
> > 		err = sysfs_create_files(&khwe->base, files);
> >-		if (err) {
> >-			kobject_put(kobj);
> >-			return err;
> >-		}
> >+		if (err)
> >+			goto err_object;
> >+	}
> >+
> >+	if (0) {
> >+err_object:
> >+		kobject_put(kobj);
> >+		return err;
> > 	}
> 
> I am not sure if we really need this 'if (0)' construct. We could instead do,
> 
>       return 0;
> err_object:
>       kobject_put(kobj);
>       return err;
> 
> In any case, we should do it in patch #3 (where this code is added) instead of
> here.

Ok, will check.

> 
> Niranjana
> 
> >
> > 	return err;
> >--
> >2.25.1
> >