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

[Niranjana Vishwanathapura]

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.

Niranjana

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