Re: [PATCH v5 1/4] drm/i915/guc: Limit scheduling properties to avoid overflow

2022-10-17 Thread Ceraolo Spurio, Daniele 




On 10/6/2022 2:38 PM, john.c.harri...@intel.com wrote:

From: John Harrison 

GuC converts the pre-emption timeout and timeslice quantum values into
clock ticks internally. That significantly reduces the point of 32bit
overflow. On current platforms, worst case scenario is approximately
110 seconds. Rather than allowing the user to set higher values and
then get confused by early timeouts, add limits when setting these
values.

v2: Add helper functions for clamping (review feedback from Tvrtko).
v3: Add a bunch of BUG_ON range checks in addition to the checks
already in the clamping functions (Tvrtko)

Signed-off-by: John Harrison 
Reviewed-by: Daniele Ceraolo Spurio  (v1)


r-b stands

Daniele


Acked-by: Tvrtko Ursulin 
---
  drivers/gpu/drm/i915/gt/intel_engine.h|  6 ++
  drivers/gpu/drm/i915/gt/intel_engine_cs.c | 69 +++
  drivers/gpu/drm/i915/gt/sysfs_engines.c   | 25 ---
  drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   | 21 ++
  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  8 +++
  5 files changed, 119 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h 
b/drivers/gpu/drm/i915/gt/intel_engine.h
index 04e435bce79bd..cbc8b857d5f7a 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -348,4 +348,10 @@ intel_engine_get_hung_context(struct intel_engine_cs 
*engine)
return engine->hung_ce;
  }
  
+u64 intel_clamp_heartbeat_interval_ms(struct intel_engine_cs *engine, u64 value);

+u64 intel_clamp_max_busywait_duration_ns(struct intel_engine_cs *engine, u64 
value);
+u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
+u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
+u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 
value);
+
  #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 2ddcad497fa30..8f16955f0821e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -512,6 +512,26 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id,
engine->flags |= I915_ENGINE_HAS_EU_PRIORITY;
}
  
+	/* Cap properties according to any system limits */

+#define CLAMP_PROP(field) \
+   do { \
+   u64 clamp = intel_clamp_##field(engine, engine->props.field); \
+   if (clamp != engine->props.field) { \
+   drm_notice(>i915->drm, \
+  "Warning, clamping %s to %lld to prevent 
overflow\n", \
+  #field, clamp); \
+   engine->props.field = clamp; \
+   } \
+   } while (0)
+
+   CLAMP_PROP(heartbeat_interval_ms);
+   CLAMP_PROP(max_busywait_duration_ns);
+   CLAMP_PROP(preempt_timeout_ms);
+   CLAMP_PROP(stop_timeout_ms);
+   CLAMP_PROP(timeslice_duration_ms);
+
+#undef CLAMP_PROP
+
engine->defaults = engine->props; /* never to change again */
  
  	engine->context_size = intel_engine_context_size(gt, engine->class);

@@ -534,6 +554,55 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id,
return 0;
  }
  
+u64 intel_clamp_heartbeat_interval_ms(struct intel_engine_cs *engine, u64 value)

+{
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_max_busywait_duration_ns(struct intel_engine_cs *engine, u64 
value)
+{
+   value = min(value, jiffies_to_nsecs(2));
+
+   return value;
+}
+
+u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value)
+{
+   /*
+* NB: The GuC API only supports 32bit values. However, the limit is 
further
+* reduced due to internal calculations which would otherwise overflow.
+*/
+   if (intel_guc_submission_is_wanted(>gt->uc.guc))
+   value = min_t(u64, value, guc_policy_max_preempt_timeout_ms());
+
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value)
+{
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 
value)
+{
+   /*
+* NB: The GuC API only supports 32bit values. However, the limit is 
further
+* reduced due to internal calculations which would otherwise overflow.
+*/
+   if (intel_guc_submission_is_wanted(>gt->uc.guc))
+   value = min_t(u64, value, guc_policy_max_exec_quantum_ms());
+
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
  static void __setup_engine_capabilities(struct intel_engine_cs *engine)
  {
struct

[PATCH v5 1/4] drm/i915/guc: Limit scheduling properties to avoid overflow

2022-10-06 Thread John . C . Harrison 
From: John Harrison 

GuC converts the pre-emption timeout and timeslice quantum values into
clock ticks internally. That significantly reduces the point of 32bit
overflow. On current platforms, worst case scenario is approximately
110 seconds. Rather than allowing the user to set higher values and
then get confused by early timeouts, add limits when setting these
values.

v2: Add helper functions for clamping (review feedback from Tvrtko).
v3: Add a bunch of BUG_ON range checks in addition to the checks
already in the clamping functions (Tvrtko)

Signed-off-by: John Harrison 
Reviewed-by: Daniele Ceraolo Spurio  (v1)
Acked-by: Tvrtko Ursulin 
---
 drivers/gpu/drm/i915/gt/intel_engine.h|  6 ++
 drivers/gpu/drm/i915/gt/intel_engine_cs.c | 69 +++
 drivers/gpu/drm/i915/gt/sysfs_engines.c   | 25 ---
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h   | 21 ++
 .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  8 +++
 5 files changed, 119 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_engine.h 
b/drivers/gpu/drm/i915/gt/intel_engine.h
index 04e435bce79bd..cbc8b857d5f7a 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine.h
+++ b/drivers/gpu/drm/i915/gt/intel_engine.h
@@ -348,4 +348,10 @@ intel_engine_get_hung_context(struct intel_engine_cs 
*engine)
return engine->hung_ce;
 }
 
+u64 intel_clamp_heartbeat_interval_ms(struct intel_engine_cs *engine, u64 
value);
+u64 intel_clamp_max_busywait_duration_ns(struct intel_engine_cs *engine, u64 
value);
+u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value);
+u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value);
+u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 
value);
+
 #endif /* _INTEL_RINGBUFFER_H_ */
diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index 2ddcad497fa30..8f16955f0821e 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -512,6 +512,26 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id,
engine->flags |= I915_ENGINE_HAS_EU_PRIORITY;
}
 
+   /* Cap properties according to any system limits */
+#define CLAMP_PROP(field) \
+   do { \
+   u64 clamp = intel_clamp_##field(engine, engine->props.field); \
+   if (clamp != engine->props.field) { \
+   drm_notice(>i915->drm, \
+  "Warning, clamping %s to %lld to prevent 
overflow\n", \
+  #field, clamp); \
+   engine->props.field = clamp; \
+   } \
+   } while (0)
+
+   CLAMP_PROP(heartbeat_interval_ms);
+   CLAMP_PROP(max_busywait_duration_ns);
+   CLAMP_PROP(preempt_timeout_ms);
+   CLAMP_PROP(stop_timeout_ms);
+   CLAMP_PROP(timeslice_duration_ms);
+
+#undef CLAMP_PROP
+
engine->defaults = engine->props; /* never to change again */
 
engine->context_size = intel_engine_context_size(gt, engine->class);
@@ -534,6 +554,55 @@ static int intel_engine_setup(struct intel_gt *gt, enum 
intel_engine_id id,
return 0;
 }
 
+u64 intel_clamp_heartbeat_interval_ms(struct intel_engine_cs *engine, u64 
value)
+{
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_max_busywait_duration_ns(struct intel_engine_cs *engine, u64 
value)
+{
+   value = min(value, jiffies_to_nsecs(2));
+
+   return value;
+}
+
+u64 intel_clamp_preempt_timeout_ms(struct intel_engine_cs *engine, u64 value)
+{
+   /*
+* NB: The GuC API only supports 32bit values. However, the limit is 
further
+* reduced due to internal calculations which would otherwise overflow.
+*/
+   if (intel_guc_submission_is_wanted(>gt->uc.guc))
+   value = min_t(u64, value, guc_policy_max_preempt_timeout_ms());
+
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_stop_timeout_ms(struct intel_engine_cs *engine, u64 value)
+{
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
+u64 intel_clamp_timeslice_duration_ms(struct intel_engine_cs *engine, u64 
value)
+{
+   /*
+* NB: The GuC API only supports 32bit values. However, the limit is 
further
+* reduced due to internal calculations which would otherwise overflow.
+*/
+   if (intel_guc_submission_is_wanted(>gt->uc.guc))
+   value = min_t(u64, value, guc_policy_max_exec_quantum_ms());
+
+   value = min_t(u64, value, jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT));
+
+   return value;
+}
+
 static void __setup_engine_capabilities(struct intel_engine_cs *engine)
 {
struct drm_i915_private *i915 = engine->i915;
diff --git