[Intel-xe] [PATCH 6/6] drm/xe: Add min/max cap for engine scheduler properties

[Upadhyay, Tejas]

> -----Original Message-----
> From: Vishwanathapura, Niranjana <niranjana.vishwanathapura at intel.com>
> Sent: Wednesday, June 21, 2023 9:25 AM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> Cc: Brost, Matthew <matthew.brost at intel.com>; intel-
> xe at lists.freedesktop.org; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Ghimiray, Himal Prasad
> <himal.prasad.ghimiray at intel.com>
> Subject: Re: [PATCH 6/6] drm/xe: Add min/max cap for engine scheduler
> properties
> 
> On Sun, Jun 18, 2023 at 09:43:51PM -0700, Upadhyay, Tejas wrote:
> >
> >
> >> -----Original Message-----
> >> From: Vishwanathapura, Niranjana
> >> <niranjana.vishwanathapura at intel.com>
> >> Sent: Saturday, June 17, 2023 2:26 AM
> >> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> >> Cc: intel-xe at lists.freedesktop.org; Iddamsetty, Aravind
> >> <aravind.iddamsetty at intel.com>; Ghimiray, Himal Prasad
> >> <himal.prasad.ghimiray at intel.com>; Brost, Matthew
> >> <matthew.brost at intel.com>
> >> Subject: Re: [PATCH 6/6] drm/xe: Add min/max cap for engine scheduler
> >> properties
> >>
> >> On Thu, Jun 15, 2023 at 07:49:33PM +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.
> >> >
> >> >Introducing 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
> >> >
> >> >Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> 
> <snip>
> 
> >> >diff --git a/drivers/gpu/drm/xe/xe_engine.c
> >> >b/drivers/gpu/drm/xe/xe_engine.c index e72423bc398a..c368ffd5e461
> >> >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,20 @@ 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;
> >> >+#endif
> >>
> >> Do we really need this second boundary check given the in the first
> >> check above, the e->hwe->sched_props.timeslice_min/max values are
> >> already subjected to these boundaries?
> >> Applies for other below parameters also.
> >
> >Yes we need because,
> >If elevated user exceeds configured sysfs min/max (he is allowed to do so),
> which number could be below or above compile config min max.
> >As user can try any value through IOCTL/sysfs being elevated user, we need
> to check with second check.
> >
> 
> I thought CONFIG_DRM_XE_TIMESLICE_MIN/MAX are the default values of
> hwe->sched_props.timeslice_min/max
> which user can override. But looking at timeslice_duration_min/max_store(),
> looks like these configs are not default min/max values but a boundary for
> min/max values. I am confused.

Yes it is boundary for elevated user to config min/max as well as default value if not configured by user.

Tejas
> 
> Niranjana
> 
> >>
> >> >+
> >> > 	return e->ops->set_timeslice(e, value); }
> >> >