[1/3] drm/i915/guc: Limit scheduling properties to avoid overflow

Commit Message

John Harrison Feb. 18, 2022, 9:33 p.m. UTC

From: John Harrison <[e-mail]>

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.

Signed-off-by: John Harrison <[e-mail]>
---
 drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
 drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
 drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
 3 files changed, 38 insertions(+)

Comments

Tvrtko Ursulin Feb. 22, 2022, 9:52 a.m. UTC | #1

On 18/02/2022 21:33, [e-mail] wrote:
> From: John Harrison <[e-mail]>
> 
> 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

Where does 32-bit come from, the GuC side? We already use 64-bits so that something to fix to start with. Yep...

./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;

./gt/uc/intel_guc_submission.c: desc->execution_quantum = engine->props.timeslice_duration_ms * 1000;

./gt/intel_engine_types.h:              unsigned long timeslice_duration_ms;

timeslice_store/preempt_timeout_store:
err = kstrtoull(buf, 0, &duration);

So both kconfig and sysfs can already overflow GuC, not only because of tick conversion internally but because at backend level nothing was done for assigning 64-bit into 32-bit. Or I failed to find where it is handled.

> 110 seconds. Rather than allowing the user to set higher values and
> then get confused by early timeouts, add limits when setting these
> values.

Btw who is reviewing GuC patches these days - things have somehow gotten pretty quiet in activity and I don't think that's due absence of stuff to improve or fix? Asking since I think I noticed a few already which you posted and then crickets on the mailing list.

> Signed-off-by: John Harrison <[e-mail]>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>   3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index e53008b4dd05..2a1e9f36e6f5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
>   	if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>   		engine->props.preempt_timeout_ms = 0;
>   
> +	/* Cap timeouts to prevent overflow inside GuC */
> +	if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
> +		if (engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {

Hm "wanted".. There's been too much back and forth on the GuC load options over the years to keep track.. intel_engine_uses_guc work sounds like would work and read nicer.

And limit to class instead of applying to all engines looks like a miss.

> +			drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %d to prevent possibly overflow\n",
> +				 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
> +			engine->props.timeslice_duration_ms = GUC_POLICY_MAX_EXEC_QUANTUM_MS;

I am not sure logging such message during driver load is useful. Sounds more like a confused driver which starts with one value and then overrides itself. I'd just silently set the value appropriate for the active backend. Preemption timeout kconfig text already documents the fact timeouts can get overriden at runtime depending on platform+engine. So maybe just add same text to timeslice kconfig.

> +		}
> +
> +		if (engine->props.preempt_timeout_ms > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
> +			drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %d to prevent possibly overflow\n",
> +				 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
> +			engine->props.preempt_timeout_ms = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
> +		}
> +	}
> +
>   	engine->defaults = engine->props; /* never to change again */
>   
>   	engine->context_size = intel_engine_context_size(gt, engine->class);
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c b/drivers/gpu/drm/i915/gt/sysfs_engines.c
> index 967031056202..f57efe026474 100644
> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>   		return -EINVAL;
>   
> +	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
> +	    duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
> +		duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
> +		drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %lld to prevent possibly overflow\n",
> +			 duration);
> +	}

I would suggest to avoid duplicated clamping logic. Maybe hide the all backend logic into the helpers then, like maybe:

   d = intel_engine_validate_timeslice/preempt_timeout(engine, duration);
   if (d != duration)
	return -EINVAL:

Returning -EINVAL would be equivalent to existing behaviour:

	if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
		return -EINVAL;

That way userspace has explicit notification and read-back is identical to written in value. From engine setup you can just call the helper silently.

> +
>   	WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>   
>   	if (execlists_active(&engine->execlists))
> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>   		return -EINVAL;
>   
> +	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
> +	    timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
> +		timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
> +		drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %lld to prevent possibly overflow\n",
> +			 timeout);
> +	}
> +
>   	WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>   
>   	if (READ_ONCE(engine->execlists.pending[0]))
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 6a4612a852e2..ad131092f8df 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>   
>   #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>   
> +/*
> + * GuC converts the timeout to clock ticks internally. Different platforms have
> + * different GuC clocks. Thus, the maximum value before overflow is platform
> + * dependent. Current worst case scenario is about 110s. So, limit to 100s to be
> + * safe.
> + */
> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS		(100 * 1000)
> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS	(100 * 1000)

Most important question -
how will we know/notice if/when new GuC arrives where these timeouts would still overflow? Can this be queried somehow at runtime or where does the limit comes from? How is GuC told about it? Set in some field and it just allows too large values silently break things?

Regards,

Tvrtko

> +
>   struct guc_policies {
>   	u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
>   	/* In micro seconds. How much time to allow before DPC processing is

Tvrtko Ursulin Feb. 22, 2022, 10:39 a.m. UTC | #2

On 22/02/2022 09:52, Tvrtko Ursulin wrote:
> 
> On 18/02/2022 21:33, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> 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
> 
> Where does 32-bit come from, the GuC side? We already use 64-bits so 
> that something to fix to start with. Yep...
> 
> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
> 
> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
> engine->props.timeslice_duration_ms * 1000;
> 
> ./gt/intel_engine_types.h:              unsigned long 
> timeslice_duration_ms;
> 
> timeslice_store/preempt_timeout_store:
> err = kstrtoull(buf, 0, &duration);
> 
> So both kconfig and sysfs can already overflow GuC, not only because of 
> tick conversion internally but because at backend level nothing was done 
> for assigning 64-bit into 32-bit. Or I failed to find where it is handled.
> 
>> 110 seconds. Rather than allowing the user to set higher values and
>> then get confused by early timeouts, add limits when setting these
>> values.
> 
> Btw who is reviewing GuC patches these days - things have somehow gotten 
> pretty quiet in activity and I don't think that's due absence of stuff 
> to improve or fix? Asking since I think I noticed a few already which 
> you posted and then crickets on the mailing list.
> 
>> Signed-off-by: John Harrison <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>   3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index e53008b4dd05..2a1e9f36e6f5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>> *gt, enum intel_engine_id id,
>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>           engine->props.preempt_timeout_ms = 0;
>> +    /* Cap timeouts to prevent overflow inside GuC */
>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>> +        if (engine->props.timeslice_duration_ms > 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
> 
> Hm "wanted".. There's been too much back and forth on the GuC load 
> options over the years to keep track.. intel_engine_uses_guc work sounds 
> like would work and read nicer.
> 
> And limit to class instead of applying to all engines looks like a miss.

Sorry limit to class does not apply here, I confused this with the last 
patch.

Regards,

Tvrtko

> 
>> +            drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>> duration to %d to prevent possibly overflow\n",
>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>> +            engine->props.timeslice_duration_ms = 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
> 
> I am not sure logging such message during driver load is useful. Sounds 
> more like a confused driver which starts with one value and then 
> overrides itself. I'd just silently set the value appropriate for the 
> active backend. Preemption timeout kconfig text already documents the 
> fact timeouts can get overriden at runtime depending on platform+engine. 
> So maybe just add same text to timeslice kconfig.
> 
>> +        }
>> +
>> +        if (engine->props.preempt_timeout_ms > 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>> pre-emption timeout to %d to prevent possibly overflow\n",
>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>> +            engine->props.preempt_timeout_ms = 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        }
>> +    }
>> +
>>       engine->defaults = engine->props; /* never to change again */
>>       engine->context_size = intel_engine_context_size(gt, 
>> engine->class);
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 967031056202..f57efe026474 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>> kobj_attribute *attr,
>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>> +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>> duration to %lld to prevent possibly overflow\n",
>> +             duration);
>> +    }
> 
> I would suggest to avoid duplicated clamping logic. Maybe hide the all 
> backend logic into the helpers then, like maybe:
> 
>    d = intel_engine_validate_timeslice/preempt_timeout(engine, duration);
>    if (d != duration)
>      return -EINVAL:
> 
> Returning -EINVAL would be equivalent to existing behaviour:
> 
>      if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>          return -EINVAL;
> 
> That way userspace has explicit notification and read-back is identical 
> to written in value. From engine setup you can just call the helper 
> silently.
> 
>> +
>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>       if (execlists_active(&engine->execlists))
>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>> struct kobj_attribute *attr,
>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>> +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping pre-emption 
>> timeout to %lld to prevent possibly overflow\n",
>> +             timeout);
>> +    }
>> +
>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>       if (READ_ONCE(engine->execlists.pending[0]))
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> index 6a4612a852e2..ad131092f8df 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>   #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>> +/*
>> + * GuC converts the timeout to clock ticks internally. Different 
>> platforms have
>> + * different GuC clocks. Thus, the maximum value before overflow is 
>> platform
>> + * dependent. Current worst case scenario is about 110s. So, limit to 
>> 100s to be
>> + * safe.
>> + */
>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
> 
> Most important question -
> how will we know/notice if/when new GuC arrives where these timeouts 
> would still overflow? Can this be queried somehow at runtime or where 
> does the limit comes from? How is GuC told about it? Set in some field 
> and it just allows too large values silently break things?
> 
> Regards,
> 
> Tvrtko
> 
>> +
>>   struct guc_policies {
>>       u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
>>       /* In micro seconds. How much time to allow before DPC 
>> processing is

Daniele Ceraolo Spurio Feb. 23, 2022, 12:52 a.m. UTC | #3

On 2/18/2022 1:33 PM, [e-mail] wrote:
> From: John Harrison <[e-mail]>
>
> 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.
>
> Signed-off-by: John Harrison <[e-mail]>
> ---
>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>   3 files changed, 38 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index e53008b4dd05..2a1e9f36e6f5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
>   	if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>   		engine->props.preempt_timeout_ms = 0;
>   
> +	/* Cap timeouts to prevent overflow inside GuC */
> +	if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
> +		if (engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
> +			drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %d to prevent possibly overflow\n",

I'd drop the word "possibly"

> +				 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
> +			engine->props.timeslice_duration_ms = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
> +		}
> +
> +		if (engine->props.preempt_timeout_ms > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
> +			drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %d to prevent possibly overflow\n",
> +				 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
> +			engine->props.preempt_timeout_ms = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
> +		}
> +	}
> +
>   	engine->defaults = engine->props; /* never to change again */
>   
>   	engine->context_size = intel_engine_context_size(gt, engine->class);
> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c b/drivers/gpu/drm/i915/gt/sysfs_engines.c
> index 967031056202..f57efe026474 100644
> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>   		return -EINVAL;
>   
> +	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
> +	    duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
> +		duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
> +		drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %lld to prevent possibly overflow\n",
> +			 duration);
> +	}
> +
>   	WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>   
>   	if (execlists_active(&engine->execlists))
> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, struct kobj_attribute *attr,
>   	if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>   		return -EINVAL;
>   
> +	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
> +	    timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
> +		timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
> +		drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %lld to prevent possibly overflow\n",
> +			 timeout);
> +	}
> +
>   	WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>   
>   	if (READ_ONCE(engine->execlists.pending[0]))
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> index 6a4612a852e2..ad131092f8df 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>   
>   #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>   
> +/*
> + * GuC converts the timeout to clock ticks internally. Different platforms have
> + * different GuC clocks. Thus, the maximum value before overflow is platform
> + * dependent. Current worst case scenario is about 110s. So, limit to 100s to be
> + * safe.
> + */
> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS		(100 * 1000)
> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS	(100 * 1000)

Those values don't seem to be defined in the GuC interface. If I'm 
correct, IMO we need to ask the GuC team to add them in, because it 
shouldn't be our responsibility to convert from ms to GuC clocks, 
considering that the interface is in ms. Not a blocker for this patch.

Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>

Daniele

> +
>   struct guc_policies {
>   	u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
>   	/* In micro seconds. How much time to allow before DPC processing is

John Harrison Feb. 23, 2022, 2:11 a.m. UTC | #4

On 2/22/2022 01:52, Tvrtko Ursulin wrote:
> On 18/02/2022 21:33, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> 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
>
> Where does 32-bit come from, the GuC side? We already use 64-bits so 
> that something to fix to start with. Yep...
Yes, the GuC API is defined as 32bits only and then does a straight 
multiply by the clock speed with no range checking. We have requested 
64bit support but there was push back on the grounds that it is not 
something the GuC timer hardware supports and such long timeouts are not 
real world usable anyway.


>
> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>
> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
> engine->props.timeslice_duration_ms * 1000;
>
> ./gt/intel_engine_types.h:              unsigned long 
> timeslice_duration_ms;
>
> timeslice_store/preempt_timeout_store:
> err = kstrtoull(buf, 0, &duration);
>
> So both kconfig and sysfs can already overflow GuC, not only because 
> of tick conversion internally but because at backend level nothing was 
> done for assigning 64-bit into 32-bit. Or I failed to find where it is 
> handled.
That's why I'm adding this range check to make sure we don't allow 
overflows.

>
>> 110 seconds. Rather than allowing the user to set higher values and
>> then get confused by early timeouts, add limits when setting these
>> values.
>
> Btw who is reviewing GuC patches these days - things have somehow 
> gotten pretty quiet in activity and I don't think that's due absence 
> of stuff to improve or fix? Asking since I think I noticed a few 
> already which you posted and then crickets on the mailing list.
Too much work to do and not enough engineers to do it all :(.


>
>> Signed-off-by: John Harrison <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>   3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index e53008b4dd05..2a1e9f36e6f5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>> *gt, enum intel_engine_id id,
>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>           engine->props.preempt_timeout_ms = 0;
>>   +    /* Cap timeouts to prevent overflow inside GuC */
>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>> +        if (engine->props.timeslice_duration_ms > 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>
> Hm "wanted".. There's been too much back and forth on the GuC load 
> options over the years to keep track.. intel_engine_uses_guc work 
> sounds like would work and read nicer.
I'm not adding a new feature check here. I'm just using the existing 
one. If we want to rename it yet again then that would be a different 
patch set.

>
> And limit to class instead of applying to all engines looks like a miss.
As per follow up email, the class limit is not applied here.

>
>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice duration 
>> to %d to prevent possibly overflow\n",
>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>> +            engine->props.timeslice_duration_ms = 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>
> I am not sure logging such message during driver load is useful. 
> Sounds more like a confused driver which starts with one value and 
> then overrides itself. I'd just silently set the value appropriate for 
> the active backend. Preemption timeout kconfig text already documents 
> the fact timeouts can get overriden at runtime depending on 
> platform+engine. So maybe just add same text to timeslice kconfig.
The point is to make people aware if they compile with unsupported 
config options. As far as I know, there is no way to apply range 
checking or other limits to config defines. Which means that a user 
would silently get unwanted behaviour. That seems like a bad thing to 
me. If the driver is confused because the user built it in a confused 
manner then we should let them know.


>
>> +        }
>> +
>> +        if (engine->props.preempt_timeout_ms > 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>> pre-emption timeout to %d to prevent possibly overflow\n",
>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>> +            engine->props.preempt_timeout_ms = 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        }
>> +    }
>> +
>>       engine->defaults = engine->props; /* never to change again */
>>         engine->context_size = intel_engine_context_size(gt, 
>> engine->class);
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 967031056202..f57efe026474 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>> kobj_attribute *attr,
>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>> duration to %lld to prevent possibly overflow\n",
>> +             duration);
>> +    }
>
> I would suggest to avoid duplicated clamping logic. Maybe hide the all 
> backend logic into the helpers then, like maybe:
>
>   d = intel_engine_validate_timeslice/preempt_timeout(engine, duration);
>   if (d != duration)
>     return -EINVAL:
>
> Returning -EINVAL would be equivalent to existing behaviour:
>
>     if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>         return -EINVAL;
>
> That way userspace has explicit notification and read-back is 
> identical to written in value. From engine setup you can just call the 
> helper silently.
Sure, EINVAL rather than clamping works as well. And can certainly add 
helper wrappers. But as above, I don't like the idea of silently 
disregarding a user specified config option.

>
>> +
>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>         if (execlists_active(&engine->execlists))
>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>> struct kobj_attribute *attr,
>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping pre-emption 
>> timeout to %lld to prevent possibly overflow\n",
>> +             timeout);
>> +    }
>> +
>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>         if (READ_ONCE(engine->execlists.pending[0]))
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> index 6a4612a852e2..ad131092f8df 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>   +/*
>> + * GuC converts the timeout to clock ticks internally. Different 
>> platforms have
>> + * different GuC clocks. Thus, the maximum value before overflow is 
>> platform
>> + * dependent. Current worst case scenario is about 110s. So, limit 
>> to 100s to be
>> + * safe.
>> + */
>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>
> Most important question -
> how will we know/notice if/when new GuC arrives where these timeouts 
> would still overflow? Can this be queried somehow at runtime or where 
> does the limit comes from? How is GuC told about it? Set in some field 
> and it just allows too large values silently break things?
Currently, we don't notice except by debugging peculiar test failures :(.

These limits are not in any GuC spec. Indeed, it took a while to 
actually work out why increasing the value actually caused shorter 
timeouts to occur! As above, there is no range checking inside GuC 
itself. It does a truncated multiply which results in an effectively 
random number and just happily uses it.

John.



>
> Regards,
>
> Tvrtko
>
>> +
>>   struct guc_policies {
>>       u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
>>       /* In micro seconds. How much time to allow before DPC 
>> processing is

John Harrison Feb. 23, 2022, 2:15 a.m. UTC | #5

On 2/22/2022 16:52, Ceraolo Spurio, Daniele wrote:
> On 2/18/2022 1:33 PM, [e-mail] wrote:
>> From: John Harrison <[e-mail]>
>>
>> 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.
>>
>> Signed-off-by: John Harrison <[e-mail]>
>> ---
>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>   3 files changed, 38 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> index e53008b4dd05..2a1e9f36e6f5 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>> *gt, enum intel_engine_id id,
>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>           engine->props.preempt_timeout_ms = 0;
>>   +    /* Cap timeouts to prevent overflow inside GuC */
>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>> +        if (engine->props.timeslice_duration_ms > 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>> timeslice duration to %d to prevent possibly overflow\n",
>
> I'd drop the word "possibly"
>
>> + GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>> +            engine->props.timeslice_duration_ms = 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>> +        }
>> +
>> +        if (engine->props.preempt_timeout_ms > 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>> pre-emption timeout to %d to prevent possibly overflow\n",
>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>> +            engine->props.preempt_timeout_ms = 
>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        }
>> +    }
>> +
>>       engine->defaults = engine->props; /* never to change again */
>>         engine->context_size = intel_engine_context_size(gt, 
>> engine->class);
>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> index 967031056202..f57efe026474 100644
>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>> kobj_attribute *attr,
>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>> duration to %lld to prevent possibly overflow\n",
>> +             duration);
>> +    }
>> +
>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>         if (execlists_active(&engine->execlists))
>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>> struct kobj_attribute *attr,
>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>           return -EINVAL;
>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>> +        drm_info(&engine->i915->drm, "Warning, clamping pre-emption 
>> timeout to %lld to prevent possibly overflow\n",
>> +             timeout);
>> +    }
>> +
>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>         if (READ_ONCE(engine->execlists.pending[0]))
>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> index 6a4612a852e2..ad131092f8df 100644
>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>   +/*
>> + * GuC converts the timeout to clock ticks internally. Different 
>> platforms have
>> + * different GuC clocks. Thus, the maximum value before overflow is 
>> platform
>> + * dependent. Current worst case scenario is about 110s. So, limit 
>> to 100s to be
>> + * safe.
>> + */
>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>
> Those values don't seem to be defined in the GuC interface. If I'm 
> correct, IMO we need to ask the GuC team to add them in, because it 
> shouldn't be our responsibility to convert from ms to GuC clocks, 
> considering that the interface is in ms. Not a blocker for this patch.
>
As per other reply, no. GuC doesn't give us any hints or clues on any 
limits of these values. But yes, we can push them to at least document 
the limits.

John.


> Reviewed-by: Daniele Ceraolo Spurio <[e-mail]>
>
> Daniele
>
>> +
>>   struct guc_policies {
>>       u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
>>       /* In micro seconds. How much time to allow before DPC 
>> processing is
>

Tvrtko Ursulin Feb. 23, 2022, 12:13 p.m. UTC | #6

On 23/02/2022 02:11, John Harrison wrote:
> On 2/22/2022 01:52, Tvrtko Ursulin wrote:
>> On 18/02/2022 21:33, [e-mail] wrote:
>>> From: John Harrison <[e-mail]>
>>>
>>> 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
>>
>> Where does 32-bit come from, the GuC side? We already use 64-bits so 
>> that something to fix to start with. Yep...
> Yes, the GuC API is defined as 32bits only and then does a straight 
> multiply by the clock speed with no range checking. We have requested 
> 64bit support but there was push back on the grounds that it is not 
> something the GuC timer hardware supports and such long timeouts are not 
> real world usable anyway.

As long as compute are happy with 100 seconds, then it "should be enough 
for everbody". :D

>>
>> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>>
>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>> engine->props.timeslice_duration_ms * 1000;
>>
>> ./gt/intel_engine_types.h:              unsigned long 
>> timeslice_duration_ms;
>>
>> timeslice_store/preempt_timeout_store:
>> err = kstrtoull(buf, 0, &duration);
>>
>> So both kconfig and sysfs can already overflow GuC, not only because 
>> of tick conversion internally but because at backend level nothing was 
>> done for assigning 64-bit into 32-bit. Or I failed to find where it is 
>> handled.
> That's why I'm adding this range check to make sure we don't allow 
> overflows.

Yes and no, this fixes it, but the first bug was not only due GuC 
internal tick conversion. It was present ever since the u64 from i915 
was shoved into u32 sent to GuC. So even if GuC used the value without 
additional multiplication, bug was be there. My point being when GuC 
backend was added timeout_ms values should have been limited/clamped to 
U32_MAX. The tick discovery is additional limit on top.

>>> 110 seconds. Rather than allowing the user to set higher values and
>>> then get confused by early timeouts, add limits when setting these
>>> values.
>>
>> Btw who is reviewing GuC patches these days - things have somehow 
>> gotten pretty quiet in activity and I don't think that's due absence 
>> of stuff to improve or fix? Asking since I think I noticed a few 
>> already which you posted and then crickets on the mailing list.
> Too much work to do and not enough engineers to do it all :(.
> 
> 
>>
>>> Signed-off-by: John Harrison <[e-mail]>
>>> ---
>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>   3 files changed, 38 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>>> *gt, enum intel_engine_id id,
>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>>           engine->props.preempt_timeout_ms = 0;
>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>> +        if (engine->props.timeslice_duration_ms > 
>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>
>> Hm "wanted".. There's been too much back and forth on the GuC load 
>> options over the years to keep track.. intel_engine_uses_guc work 
>> sounds like would work and read nicer.
> I'm not adding a new feature check here. I'm just using the existing 
> one. If we want to rename it yet again then that would be a different 
> patch set.

$ grep intel_engine_uses_guc . -rl
./i915_perf.c
./i915_request.c
./selftests/intel_scheduler_helpers.c
./gem/i915_gem_context.c
./gt/intel_context.c
./gt/intel_engine.h
./gt/intel_engine_cs.c
./gt/intel_engine_heartbeat.c
./gt/intel_engine_pm.c
./gt/intel_reset.c
./gt/intel_lrc.c
./gt/selftest_context.c
./gt/selftest_engine_pm.c
./gt/selftest_hangcheck.c
./gt/selftest_mocs.c
./gt/selftest_workarounds.c

Sounds better to me than intel_guc_submission_is_wanted. What does the 
reader know whether "is wanted" translates to "is actually used". Shrug 
on "is wanted".

>> And limit to class instead of applying to all engines looks like a miss.
> As per follow up email, the class limit is not applied here.
> 
>>
>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice duration 
>>> to %d to prevent possibly overflow\n",
>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>> +            engine->props.timeslice_duration_ms = 
>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>
>> I am not sure logging such message during driver load is useful. 
>> Sounds more like a confused driver which starts with one value and 
>> then overrides itself. I'd just silently set the value appropriate for 
>> the active backend. Preemption timeout kconfig text already documents 
>> the fact timeouts can get overriden at runtime depending on 
>> platform+engine. So maybe just add same text to timeslice kconfig.
> The point is to make people aware if they compile with unsupported 
> config options. As far as I know, there is no way to apply range 
> checking or other limits to config defines. Which means that a user 
> would silently get unwanted behaviour. That seems like a bad thing to 
> me. If the driver is confused because the user built it in a confused 
> manner then we should let them know.

Okay, but I think make it notice low level.

Also consider in patch 3/3 when you triple it, and then clamp back down 
here. That's even more confused state since tripling gets nerfed. I 
think that's also an argument to always account preempt timeout in 
heartbeat interval calculation. Haven't got to your reply on 2/3 yet 
though..

>>
>>> +        }
>>> +
>>> +        if (engine->props.preempt_timeout_ms > 
>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>>> pre-emption timeout to %d to prevent possibly overflow\n",
>>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>>> +            engine->props.preempt_timeout_ms = 
>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>> +        }
>>> +    }
>>> +
>>>       engine->defaults = engine->props; /* never to change again */
>>>         engine->context_size = intel_engine_context_size(gt, 
>>> engine->class);
>>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>> index 967031056202..f57efe026474 100644
>>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>>> kobj_attribute *attr,
>>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>           return -EINVAL;
>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>> duration to %lld to prevent possibly overflow\n",
>>> +             duration);
>>> +    }
>>
>> I would suggest to avoid duplicated clamping logic. Maybe hide the all 
>> backend logic into the helpers then, like maybe:
>>
>>   d = intel_engine_validate_timeslice/preempt_timeout(engine, duration);
>>   if (d != duration)
>>     return -EINVAL:
>>
>> Returning -EINVAL would be equivalent to existing behaviour:
>>
>>     if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>         return -EINVAL;
>>
>> That way userspace has explicit notification and read-back is 
>> identical to written in value. From engine setup you can just call the 
>> helper silently.
> Sure, EINVAL rather than clamping works as well. And can certainly add 
> helper wrappers. But as above, I don't like the idea of silently 
> disregarding a user specified config option.

Deal - with the open of heartbeat interval TBD.

> 
>>
>>> +
>>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>>         if (execlists_active(&engine->execlists))
>>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>>> struct kobj_attribute *attr,
>>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>           return -EINVAL;
>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>> +        drm_info(&engine->i915->drm, "Warning, clamping pre-emption 
>>> timeout to %lld to prevent possibly overflow\n",
>>> +             timeout);
>>> +    }
>>> +
>>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>>         if (READ_ONCE(engine->execlists.pending[0]))
>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>> index 6a4612a852e2..ad131092f8df 100644
>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>>   +/*
>>> + * GuC converts the timeout to clock ticks internally. Different 
>>> platforms have
>>> + * different GuC clocks. Thus, the maximum value before overflow is 
>>> platform
>>> + * dependent. Current worst case scenario is about 110s. So, limit 
>>> to 100s to be
>>> + * safe.
>>> + */
>>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>>
>> Most important question -
>> how will we know/notice if/when new GuC arrives where these timeouts 
>> would still overflow? Can this be queried somehow at runtime or where 
>> does the limit comes from? How is GuC told about it? Set in some field 
>> and it just allows too large values silently break things?
> Currently, we don't notice except by debugging peculiar test failures :(.
> 
> These limits are not in any GuC spec. Indeed, it took a while to 
> actually work out why increasing the value actually caused shorter 
> timeouts to occur! As above, there is no range checking inside GuC 
> itself. It does a truncated multiply which results in an effectively 
> random number and just happily uses it.

I will agree with what Daniele said - push on GuC fw folks to document 
the max values they guarantee to support in the interface spec. 
Otherwise it is too fragile.

Regards,

Tvrtko

John Harrison Feb. 23, 2022, 7:03 p.m. UTC | #7

On 2/23/2022 04:13, Tvrtko Ursulin wrote:
> On 23/02/2022 02:11, John Harrison wrote:
>> On 2/22/2022 01:52, Tvrtko Ursulin wrote:
>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>> From: John Harrison <[e-mail]>
>>>>
>>>> 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
>>>
>>> Where does 32-bit come from, the GuC side? We already use 64-bits so 
>>> that something to fix to start with. Yep...
>> Yes, the GuC API is defined as 32bits only and then does a straight 
>> multiply by the clock speed with no range checking. We have requested 
>> 64bit support but there was push back on the grounds that it is not 
>> something the GuC timer hardware supports and such long timeouts are 
>> not real world usable anyway.
>
> As long as compute are happy with 100 seconds, then it "should be 
> enough for everbody". :D
Compute disable all forms of reset and rely on manual kill. So yes.

But even if they aren't. That's all we can do at the moment. If there is 
a genuine customer requirement for more then we can push for full 64bit 
software implemented timers in the GuC but until that happens, we don't 
have much choice.

>
>>>
>>> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>>>
>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>> engine->props.timeslice_duration_ms * 1000;
>>>
>>> ./gt/intel_engine_types.h:              unsigned long 
>>> timeslice_duration_ms;
>>>
>>> timeslice_store/preempt_timeout_store:
>>> err = kstrtoull(buf, 0, &duration);
>>>
>>> So both kconfig and sysfs can already overflow GuC, not only because 
>>> of tick conversion internally but because at backend level nothing 
>>> was done for assigning 64-bit into 32-bit. Or I failed to find where 
>>> it is handled.
>> That's why I'm adding this range check to make sure we don't allow 
>> overflows.
>
> Yes and no, this fixes it, but the first bug was not only due GuC 
> internal tick conversion. It was present ever since the u64 from i915 
> was shoved into u32 sent to GuC. So even if GuC used the value without 
> additional multiplication, bug was be there. My point being when GuC 
> backend was added timeout_ms values should have been limited/clamped 
> to U32_MAX. The tick discovery is additional limit on top.
I'm not disagreeing. I'm just saying that the truncation wasn't noticed 
until I actually tried using very long timeouts to debug a particular 
problem. Now that it is noticed, we need some method of range checking 
and this simple clamp solves all the truncation problems.


>
>>>> 110 seconds. Rather than allowing the user to set higher values and
>>>> then get confused by early timeouts, add limits when setting these
>>>> values.
>>>
>>> Btw who is reviewing GuC patches these days - things have somehow 
>>> gotten pretty quiet in activity and I don't think that's due absence 
>>> of stuff to improve or fix? Asking since I think I noticed a few 
>>> already which you posted and then crickets on the mailing list.
>> Too much work to do and not enough engineers to do it all :(.
>>
>>
>>>
>>>> Signed-off-by: John Harrison <[e-mail]>
>>>> ---
>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>   3 files changed, 38 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>>>> *gt, enum intel_engine_id id,
>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>>>           engine->props.preempt_timeout_ms = 0;
>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>> +        if (engine->props.timeslice_duration_ms > 
>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>
>>> Hm "wanted".. There's been too much back and forth on the GuC load 
>>> options over the years to keep track.. intel_engine_uses_guc work 
>>> sounds like would work and read nicer.
>> I'm not adding a new feature check here. I'm just using the existing 
>> one. If we want to rename it yet again then that would be a different 
>> patch set.
>
> $ grep intel_engine_uses_guc . -rl
> ./i915_perf.c
> ./i915_request.c
> ./selftests/intel_scheduler_helpers.c
> ./gem/i915_gem_context.c
> ./gt/intel_context.c
> ./gt/intel_engine.h
> ./gt/intel_engine_cs.c
> ./gt/intel_engine_heartbeat.c
> ./gt/intel_engine_pm.c
> ./gt/intel_reset.c
> ./gt/intel_lrc.c
> ./gt/selftest_context.c
> ./gt/selftest_engine_pm.c
> ./gt/selftest_hangcheck.c
> ./gt/selftest_mocs.c
> ./gt/selftest_workarounds.c
>
> Sounds better to me than intel_guc_submission_is_wanted. What does the 
> reader know whether "is wanted" translates to "is actually used". 
> Shrug on "is wanted".
Yes, but isn't '_uses' the one that hits a BUG_ON if you call it too 
early in the boot up sequence? I never understood why that was necessary 
or why we need so many different ways to ask the same question. But this 
version already exists and definitely works without hitting any explosions.

>
>>> And limit to class instead of applying to all engines looks like a 
>>> miss.
>> As per follow up email, the class limit is not applied here.
>>
>>>
>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>> duration to %d to prevent possibly overflow\n",
>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>> +            engine->props.timeslice_duration_ms = 
>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>
>>> I am not sure logging such message during driver load is useful. 
>>> Sounds more like a confused driver which starts with one value and 
>>> then overrides itself. I'd just silently set the value appropriate 
>>> for the active backend. Preemption timeout kconfig text already 
>>> documents the fact timeouts can get overriden at runtime depending 
>>> on platform+engine. So maybe just add same text to timeslice kconfig.
>> The point is to make people aware if they compile with unsupported 
>> config options. As far as I know, there is no way to apply range 
>> checking or other limits to config defines. Which means that a user 
>> would silently get unwanted behaviour. That seems like a bad thing to 
>> me. If the driver is confused because the user built it in a confused 
>> manner then we should let them know.
>
> Okay, but I think make it notice low level.
>
> Also consider in patch 3/3 when you triple it, and then clamp back 
> down here. That's even more confused state since tripling gets nerfed. 
> I think that's also an argument to always account preempt timeout in 
> heartbeat interval calculation. Haven't got to your reply on 2/3 yet 
> though..
That sounds like even more reason to make sure the warning gets seen. 
The more complex the system and the more chances there are to get it 
wrong, the more important it is to have a nice easy to see and 
understand notification that it did go wrong.


>
>>>
>>>> +        }
>>>> +
>>>> +        if (engine->props.preempt_timeout_ms > 
>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>>>> pre-emption timeout to %d to prevent possibly overflow\n",
>>>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>>>> +            engine->props.preempt_timeout_ms = 
>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>> +        }
>>>> +    }
>>>> +
>>>>       engine->defaults = engine->props; /* never to change again */
>>>>         engine->context_size = intel_engine_context_size(gt, 
>>>> engine->class);
>>>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>>>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>> index 967031056202..f57efe026474 100644
>>>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>>>> kobj_attribute *attr,
>>>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>           return -EINVAL;
>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>> duration to %lld to prevent possibly overflow\n",
>>>> +             duration);
>>>> +    }
>>>
>>> I would suggest to avoid duplicated clamping logic. Maybe hide the 
>>> all backend logic into the helpers then, like maybe:
>>>
>>>   d = intel_engine_validate_timeslice/preempt_timeout(engine, 
>>> duration);
>>>   if (d != duration)
>>>     return -EINVAL:
>>>
>>> Returning -EINVAL would be equivalent to existing behaviour:
>>>
>>>     if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>         return -EINVAL;
>>>
>>> That way userspace has explicit notification and read-back is 
>>> identical to written in value. From engine setup you can just call 
>>> the helper silently.
>> Sure, EINVAL rather than clamping works as well. And can certainly 
>> add helper wrappers. But as above, I don't like the idea of silently 
>> disregarding a user specified config option.
>
> Deal - with the open of heartbeat interval TBD.
>
>>
>>>
>>>> +
>>>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>>>         if (execlists_active(&engine->execlists))
>>>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>>>> struct kobj_attribute *attr,
>>>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>           return -EINVAL;
>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>> +        drm_info(&engine->i915->drm, "Warning, clamping 
>>>> pre-emption timeout to %lld to prevent possibly overflow\n",
>>>> +             timeout);
>>>> +    }
>>>> +
>>>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>>>         if (READ_ONCE(engine->execlists.pending[0]))
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>> index 6a4612a852e2..ad131092f8df 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>>>   +/*
>>>> + * GuC converts the timeout to clock ticks internally. Different 
>>>> platforms have
>>>> + * different GuC clocks. Thus, the maximum value before overflow 
>>>> is platform
>>>> + * dependent. Current worst case scenario is about 110s. So, limit 
>>>> to 100s to be
>>>> + * safe.
>>>> + */
>>>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>>>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>>>
>>> Most important question -
>>> how will we know/notice if/when new GuC arrives where these timeouts 
>>> would still overflow? Can this be queried somehow at runtime or 
>>> where does the limit comes from? How is GuC told about it? Set in 
>>> some field and it just allows too large values silently break things?
>> Currently, we don't notice except by debugging peculiar test failures 
>> :(.
>>
>> These limits are not in any GuC spec. Indeed, it took a while to 
>> actually work out why increasing the value actually caused shorter 
>> timeouts to occur! As above, there is no range checking inside GuC 
>> itself. It does a truncated multiply which results in an effectively 
>> random number and just happily uses it.
>
> I will agree with what Daniele said - push on GuC fw folks to document 
> the max values they guarantee to support in the interface spec. 
> Otherwise it is too fragile.
I do agree. But that is going to take time. I would like to get 
something merged now while we fight over spec updates.

John.

>
> Regards,
>
> Tvrtko

Tvrtko Ursulin Feb. 24, 2022, 9:59 a.m. UTC | #8

On 23/02/2022 19:03, John Harrison wrote:
> On 2/23/2022 04:13, Tvrtko Ursulin wrote:
>> On 23/02/2022 02:11, John Harrison wrote:
>>> On 2/22/2022 01:52, Tvrtko Ursulin wrote:
>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>> From: John Harrison <[e-mail]>
>>>>>
>>>>> 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
>>>>
>>>> Where does 32-bit come from, the GuC side? We already use 64-bits so 
>>>> that something to fix to start with. Yep...
>>> Yes, the GuC API is defined as 32bits only and then does a straight 
>>> multiply by the clock speed with no range checking. We have requested 
>>> 64bit support but there was push back on the grounds that it is not 
>>> something the GuC timer hardware supports and such long timeouts are 
>>> not real world usable anyway.
>>
>> As long as compute are happy with 100 seconds, then it "should be 
>> enough for everbody". :D
> Compute disable all forms of reset and rely on manual kill. So yes.
> 
> But even if they aren't. That's all we can do at the moment. If there is 
> a genuine customer requirement for more then we can push for full 64bit 
> software implemented timers in the GuC but until that happens, we don't 
> have much choice.

Yeah.

> 
>>
>>>>
>>>> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>>>>
>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>> engine->props.timeslice_duration_ms * 1000;
>>>>
>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>> timeslice_duration_ms;
>>>>
>>>> timeslice_store/preempt_timeout_store:
>>>> err = kstrtoull(buf, 0, &duration);
>>>>
>>>> So both kconfig and sysfs can already overflow GuC, not only because 
>>>> of tick conversion internally but because at backend level nothing 
>>>> was done for assigning 64-bit into 32-bit. Or I failed to find where 
>>>> it is handled.
>>> That's why I'm adding this range check to make sure we don't allow 
>>> overflows.
>>
>> Yes and no, this fixes it, but the first bug was not only due GuC 
>> internal tick conversion. It was present ever since the u64 from i915 
>> was shoved into u32 sent to GuC. So even if GuC used the value without 
>> additional multiplication, bug was be there. My point being when GuC 
>> backend was added timeout_ms values should have been limited/clamped 
>> to U32_MAX. The tick discovery is additional limit on top.
> I'm not disagreeing. I'm just saying that the truncation wasn't noticed 
> until I actually tried using very long timeouts to debug a particular 
> problem. Now that it is noticed, we need some method of range checking 
> and this simple clamp solves all the truncation problems.

Agreed in principle, just please mention in the commit message all aspects of the problem.

I think we can get away without a Fixes: tag since it requires user fiddling to break things in unexpected ways.

I would though put in a code a clamping which expresses both, something like min(u32, ..GUC LIMIT..). So the full story is documented forever. Or "if > u32 || > ..GUC LIMIT..) return -EINVAL". Just in case GuC limit one day changes but u32 stays. Perhaps internal ticks go away or anything and we are left with plain 1:1 millisecond relationship.

>>>>> 110 seconds. Rather than allowing the user to set higher values and
>>>>> then get confused by early timeouts, add limits when setting these
>>>>> values.
>>>>
>>>> Btw who is reviewing GuC patches these days - things have somehow 
>>>> gotten pretty quiet in activity and I don't think that's due absence 
>>>> of stuff to improve or fix? Asking since I think I noticed a few 
>>>> already which you posted and then crickets on the mailing list.
>>> Too much work to do and not enough engineers to do it all :(.
>>>
>>>
>>>>
>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>> ---
>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>>   3 files changed, 38 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct intel_gt 
>>>>> *gt, enum intel_engine_id id,
>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>
>>>> Hm "wanted".. There's been too much back and forth on the GuC load 
>>>> options over the years to keep track.. intel_engine_uses_guc work 
>>>> sounds like would work and read nicer.
>>> I'm not adding a new feature check here. I'm just using the existing 
>>> one. If we want to rename it yet again then that would be a different 
>>> patch set.
>>
>> $ grep intel_engine_uses_guc . -rl
>> ./i915_perf.c
>> ./i915_request.c
>> ./selftests/intel_scheduler_helpers.c
>> ./gem/i915_gem_context.c
>> ./gt/intel_context.c
>> ./gt/intel_engine.h
>> ./gt/intel_engine_cs.c
>> ./gt/intel_engine_heartbeat.c
>> ./gt/intel_engine_pm.c
>> ./gt/intel_reset.c
>> ./gt/intel_lrc.c
>> ./gt/selftest_context.c
>> ./gt/selftest_engine_pm.c
>> ./gt/selftest_hangcheck.c
>> ./gt/selftest_mocs.c
>> ./gt/selftest_workarounds.c
>>
>> Sounds better to me than intel_guc_submission_is_wanted. What does the 
>> reader know whether "is wanted" translates to "is actually used". 
>> Shrug on "is wanted".
> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it too 
> early in the boot up sequence? I never understood why that was necessary 
> or why we need so many different ways to ask the same question. But this 
> version already exists and definitely works without hitting any explosions.

No idea if it causes a bug on, doesn't in the helper itself so maybe you are saying it is called too early? Might be.. I think over time the nice idea we had that "setup" and "init" phases of engine setup clearly separated got destroyed a bit. There would always be an option to move this clamping in a later phase, once the submission method is known. One could argue that if the submission method is not yet known at this point, it is even wrong to clamp based on something which will only be decided later. Because:

int intel_engines_init(struct intel_gt *gt)
{
	int (*setup)(struct intel_engine_cs *engine);
	struct intel_engine_cs *engine;
	enum intel_engine_id id;
	int err;

	if (intel_uc_uses_guc_submission(&gt->uc)) {
		gt->submission_method = INTEL_SUBMISSION_GUC;

So this uses "uses", not "wanted". Presumably the point for having "wanted" and "uses" is that they can disagree, in which case if you clamp early based on "wanted" that suggests it could be wrong.

>>>> And limit to class instead of applying to all engines looks like a 
>>>> miss.
>>> As per follow up email, the class limit is not applied here.
>>>
>>>>
>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>> duration to %d to prevent possibly overflow\n",
>>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>> +            engine->props.timeslice_duration_ms = 
>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>
>>>> I am not sure logging such message during driver load is useful. 
>>>> Sounds more like a confused driver which starts with one value and 
>>>> then overrides itself. I'd just silently set the value appropriate 
>>>> for the active backend. Preemption timeout kconfig text already 
>>>> documents the fact timeouts can get overriden at runtime depending 
>>>> on platform+engine. So maybe just add same text to timeslice kconfig.
>>> The point is to make people aware if they compile with unsupported 
>>> config options. As far as I know, there is no way to apply range 
>>> checking or other limits to config defines. Which means that a user 
>>> would silently get unwanted behaviour. That seems like a bad thing to 
>>> me. If the driver is confused because the user built it in a confused 
>>> manner then we should let them know.
>>
>> Okay, but I think make it notice low level.
>>
>> Also consider in patch 3/3 when you triple it, and then clamp back 
>> down here. That's even more confused state since tripling gets nerfed. 
>> I think that's also an argument to always account preempt timeout in 
>> heartbeat interval calculation. Haven't got to your reply on 2/3 yet 
>> though..
> That sounds like even more reason to make sure the warning gets seen. 
> The more complex the system and the more chances there are to get it 
> wrong, the more important it is to have a nice easy to see and 
> understand notification that it did go wrong.

I did not disagree, just said make it notice, one level higher than info! :)

But also think how, if we agree to go with tripling, that you'd have to consider that in the sysfs store when hearbeat timeout is written, to consider whether or not to triple and error out if preemption timeout is over limit.

>>>>> +        }
>>>>> +
>>>>> +        if (engine->props.preempt_timeout_ms > 
>>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>>>>> pre-emption timeout to %d to prevent possibly overflow\n",
>>>>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>>>>> +            engine->props.preempt_timeout_ms = 
>>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>>> +        }
>>>>> +    }
>>>>> +
>>>>>       engine->defaults = engine->props; /* never to change again */
>>>>>         engine->context_size = intel_engine_context_size(gt, 
>>>>> engine->class);
>>>>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>>>>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>> index 967031056202..f57efe026474 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>>>>> kobj_attribute *attr,
>>>>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>>           return -EINVAL;
>>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>> +        drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>> duration to %lld to prevent possibly overflow\n",
>>>>> +             duration);
>>>>> +    }
>>>>
>>>> I would suggest to avoid duplicated clamping logic. Maybe hide the 
>>>> all backend logic into the helpers then, like maybe:
>>>>
>>>>   d = intel_engine_validate_timeslice/preempt_timeout(engine, 
>>>> duration);
>>>>   if (d != duration)
>>>>     return -EINVAL:
>>>>
>>>> Returning -EINVAL would be equivalent to existing behaviour:
>>>>
>>>>     if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>         return -EINVAL;
>>>>
>>>> That way userspace has explicit notification and read-back is 
>>>> identical to written in value. From engine setup you can just call 
>>>> the helper silently.
>>> Sure, EINVAL rather than clamping works as well. And can certainly 
>>> add helper wrappers. But as above, I don't like the idea of silently 
>>> disregarding a user specified config option.
>>
>> Deal - with the open of heartbeat interval TBD.
>>
>>>
>>>>
>>>>> +
>>>>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>>>>         if (execlists_active(&engine->execlists))
>>>>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>>>>> struct kobj_attribute *attr,
>>>>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>>           return -EINVAL;
>>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>>> +        drm_info(&engine->i915->drm, "Warning, clamping 
>>>>> pre-emption timeout to %lld to prevent possibly overflow\n",
>>>>> +             timeout);
>>>>> +    }
>>>>> +
>>>>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>>>>         if (READ_ONCE(engine->execlists.pending[0]))
>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>> index 6a4612a852e2..ad131092f8df 100644
>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>>>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>>>>   +/*
>>>>> + * GuC converts the timeout to clock ticks internally. Different 
>>>>> platforms have
>>>>> + * different GuC clocks. Thus, the maximum value before overflow 
>>>>> is platform
>>>>> + * dependent. Current worst case scenario is about 110s. So, limit 
>>>>> to 100s to be
>>>>> + * safe.
>>>>> + */
>>>>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>>>>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>>>>
>>>> Most important question -
>>>> how will we know/notice if/when new GuC arrives where these timeouts 
>>>> would still overflow? Can this be queried somehow at runtime or 
>>>> where does the limit comes from? How is GuC told about it? Set in 
>>>> some field and it just allows too large values silently break things?
>>> Currently, we don't notice except by debugging peculiar test failures 
>>> :(.
>>>
>>> These limits are not in any GuC spec. Indeed, it took a while to 
>>> actually work out why increasing the value actually caused shorter 
>>> timeouts to occur! As above, there is no range checking inside GuC 
>>> itself. It does a truncated multiply which results in an effectively 
>>> random number and just happily uses it.
>>
>> I will agree with what Daniele said - push on GuC fw folks to document 
>> the max values they guarantee to support in the interface spec. 
>> Otherwise it is too fragile.
> I do agree. But that is going to take time. I would like to get 
> something merged now while we fight over spec updates.

Yeah that's okay, did not mean to imply I am against a quick fix. "Otherwise it is too fragile, *in the long run*" should have written or something like that.

Regards,

Tvrtko

John Harrison Feb. 24, 2022, 7:19 p.m. UTC | #9

On 2/24/2022 01:59, Tvrtko Ursulin wrote:
> On 23/02/2022 19:03, John Harrison wrote:
>> On 2/23/2022 04:13, Tvrtko Ursulin wrote:
>>> On 23/02/2022 02:11, John Harrison wrote:
>>>> On 2/22/2022 01:52, Tvrtko Ursulin wrote:
>>>>> On 18/02/2022 21:33, [e-mail] wrote:
>>>>>> From: John Harrison <[e-mail]>
>>>>>>
>>>>>> 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
>>>>>
>>>>> Where does 32-bit come from, the GuC side? We already use 64-bits 
>>>>> so that something to fix to start with. Yep...
>>>> Yes, the GuC API is defined as 32bits only and then does a straight 
>>>> multiply by the clock speed with no range checking. We have 
>>>> requested 64bit support but there was push back on the grounds that 
>>>> it is not something the GuC timer hardware supports and such long 
>>>> timeouts are not real world usable anyway.
>>>
>>> As long as compute are happy with 100 seconds, then it "should be 
>>> enough for everbody". :D
>> Compute disable all forms of reset and rely on manual kill. So yes.
>>
>> But even if they aren't. That's all we can do at the moment. If there 
>> is a genuine customer requirement for more then we can push for full 
>> 64bit software implemented timers in the GuC but until that happens, 
>> we don't have much choice.
>
> Yeah.
>
>>
>>>
>>>>>
>>>>> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>>>>>
>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>
>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>> timeslice_duration_ms;
>>>>>
>>>>> timeslice_store/preempt_timeout_store:
>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>
>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>> because of tick conversion internally but because at backend level 
>>>>> nothing was done for assigning 64-bit into 32-bit. Or I failed to 
>>>>> find where it is handled.
>>>> That's why I'm adding this range check to make sure we don't allow 
>>>> overflows.
>>>
>>> Yes and no, this fixes it, but the first bug was not only due GuC 
>>> internal tick conversion. It was present ever since the u64 from 
>>> i915 was shoved into u32 sent to GuC. So even if GuC used the value 
>>> without additional multiplication, bug was be there. My point being 
>>> when GuC backend was added timeout_ms values should have been 
>>> limited/clamped to U32_MAX. The tick discovery is additional limit 
>>> on top.
>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>> noticed until I actually tried using very long timeouts to debug a 
>> particular problem. Now that it is noticed, we need some method of 
>> range checking and this simple clamp solves all the truncation problems.
>
> Agreed in principle, just please mention in the commit message all 
> aspects of the problem.
>
> I think we can get away without a Fixes: tag since it requires user 
> fiddling to break things in unexpected ways.
>
> I would though put in a code a clamping which expresses both, 
> something like min(u32, ..GUC LIMIT..). So the full story is 
> documented forever. Or "if > u32 || > ..GUC LIMIT..) return -EINVAL". 
> Just in case GuC limit one day changes but u32 stays. Perhaps internal 
> ticks go away or anything and we are left with plain 1:1 millisecond 
> relationship.
Can certainly add a comment along the lines of "GuC API only takes a 
32bit field but that is further reduced to GUC_LIMIT due to internal 
calculations which would otherwise overflow".

But if the GuC limit is > u32 then, by definition, that means the GuC 
API has changed to take a u64 instead of a u32. So there will no u32 
truncation any more. So I'm not seeing a need to explicitly test the 
integer size when the value check covers that.

>
>>>>>> 110 seconds. Rather than allowing the user to set higher values and
>>>>>> then get confused by early timeouts, add limits when setting these
>>>>>> values.
>>>>>
>>>>> Btw who is reviewing GuC patches these days - things have somehow 
>>>>> gotten pretty quiet in activity and I don't think that's due 
>>>>> absence of stuff to improve or fix? Asking since I think I noticed 
>>>>> a few already which you posted and then crickets on the mailing list.
>>>> Too much work to do and not enough engineers to do it all :(.
>>>>
>>>>
>>>>>
>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>> ---
>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>>>   3 files changed, 38 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>
>>>>> Hm "wanted".. There's been too much back and forth on the GuC load 
>>>>> options over the years to keep track.. intel_engine_uses_guc work 
>>>>> sounds like would work and read nicer.
>>>> I'm not adding a new feature check here. I'm just using the 
>>>> existing one. If we want to rename it yet again then that would be 
>>>> a different patch set.
>>>
>>> $ grep intel_engine_uses_guc . -rl
>>> ./i915_perf.c
>>> ./i915_request.c
>>> ./selftests/intel_scheduler_helpers.c
>>> ./gem/i915_gem_context.c
>>> ./gt/intel_context.c
>>> ./gt/intel_engine.h
>>> ./gt/intel_engine_cs.c
>>> ./gt/intel_engine_heartbeat.c
>>> ./gt/intel_engine_pm.c
>>> ./gt/intel_reset.c
>>> ./gt/intel_lrc.c
>>> ./gt/selftest_context.c
>>> ./gt/selftest_engine_pm.c
>>> ./gt/selftest_hangcheck.c
>>> ./gt/selftest_mocs.c
>>> ./gt/selftest_workarounds.c
>>>
>>> Sounds better to me than intel_guc_submission_is_wanted. What does 
>>> the reader know whether "is wanted" translates to "is actually 
>>> used". Shrug on "is wanted".
>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it too 
>> early in the boot up sequence? I never understood why that was 
>> necessary or why we need so many different ways to ask the same 
>> question. But this version already exists and definitely works 
>> without hitting any explosions.
>
> No idea if it causes a bug on, doesn't in the helper itself so maybe 
> you are saying it is called too early? Might be.. I think over time 
> the nice idea we had that "setup" and "init" phases of engine setup 
> clearly separated got destroyed a bit. There would always be an option 
> to move this clamping in a later phase, once the submission method is 
> known. One could argue that if the submission method is not yet known 
> at this point, it is even wrong to clamp based on something which will 
> only be decided later. Because:
>
> int intel_engines_init(struct intel_gt *gt)
> {
>     int (*setup)(struct intel_engine_cs *engine);
>     struct intel_engine_cs *engine;
>     enum intel_engine_id id;
>     int err;
>
>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>         gt->submission_method = INTEL_SUBMISSION_GUC;
>
> So this uses "uses", not "wanted". Presumably the point for having 
> "wanted" and "uses" is that they can disagree, in which case if you 
> clamp early based on "wanted" that suggests it could be wrong.

Okay, looks like I was getting confused with intel_guc_is_used(). That 
one blows up if called too early.

I'll change it to _uses_ and repost, then.

>
>>>>> And limit to class instead of applying to all engines looks like a 
>>>>> miss.
>>>> As per follow up email, the class limit is not applied here.
>>>>
>>>>>
>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>> +            engine->props.timeslice_duration_ms = 
>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>
>>>>> I am not sure logging such message during driver load is useful. 
>>>>> Sounds more like a confused driver which starts with one value and 
>>>>> then overrides itself. I'd just silently set the value appropriate 
>>>>> for the active backend. Preemption timeout kconfig text already 
>>>>> documents the fact timeouts can get overriden at runtime depending 
>>>>> on platform+engine. So maybe just add same text to timeslice kconfig.
>>>> The point is to make people aware if they compile with unsupported 
>>>> config options. As far as I know, there is no way to apply range 
>>>> checking or other limits to config defines. Which means that a user 
>>>> would silently get unwanted behaviour. That seems like a bad thing 
>>>> to me. If the driver is confused because the user built it in a 
>>>> confused manner then we should let them know.
>>>
>>> Okay, but I think make it notice low level.
>>>
>>> Also consider in patch 3/3 when you triple it, and then clamp back 
>>> down here. That's even more confused state since tripling gets 
>>> nerfed. I think that's also an argument to always account preempt 
>>> timeout in heartbeat interval calculation. Haven't got to your reply 
>>> on 2/3 yet though..
>> That sounds like even more reason to make sure the warning gets seen. 
>> The more complex the system and the more chances there are to get it 
>> wrong, the more important it is to have a nice easy to see and 
>> understand notification that it did go wrong.
>
> I did not disagree, just said make it notice, one level higher than 
> info! :)
But then it won't appear unless you have explicitly said an elevated 
debug level. Whereas info appears in dmesg by default (but is still not 
classed as an error by CI and such).

>
> But also think how, if we agree to go with tripling, that you'd have 
> to consider that in the sysfs store when hearbeat timeout is written, 
> to consider whether or not to triple and error out if preemption 
> timeout is over limit.
I see this as just setting the default values. If an end user is 
explicitly overriding the defaults then we should obey what they have 
requested. If they are changing the heartbeat interval then they can 
also change the pre-emption timeout appropriately.

John.


>
>>>>>> +        }
>>>>>> +
>>>>>> +        if (engine->props.preempt_timeout_ms > 
>>>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>>>> +            drm_info(&engine->i915->drm, "Warning, clamping 
>>>>>> pre-emption timeout to %d to prevent possibly overflow\n",
>>>>>> +                 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
>>>>>> +            engine->props.preempt_timeout_ms = 
>>>>>> GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>>>> +        }
>>>>>> +    }
>>>>>> +
>>>>>>       engine->defaults = engine->props; /* never to change again */
>>>>>>         engine->context_size = intel_engine_context_size(gt, 
>>>>>> engine->class);
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c 
>>>>>> b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>>> index 967031056202..f57efe026474 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>>> +++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
>>>>>> @@ -221,6 +221,13 @@ timeslice_store(struct kobject *kobj, struct 
>>>>>> kobj_attribute *attr,
>>>>>>       if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>>>           return -EINVAL;
>>>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>>>> +        duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>> +        duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>> +        drm_info(&engine->i915->drm, "Warning, clamping 
>>>>>> timeslice duration to %lld to prevent possibly overflow\n",
>>>>>> +             duration);
>>>>>> +    }
>>>>>
>>>>> I would suggest to avoid duplicated clamping logic. Maybe hide the 
>>>>> all backend logic into the helpers then, like maybe:
>>>>>
>>>>>   d = intel_engine_validate_timeslice/preempt_timeout(engine, 
>>>>> duration);
>>>>>   if (d != duration)
>>>>>     return -EINVAL:
>>>>>
>>>>> Returning -EINVAL would be equivalent to existing behaviour:
>>>>>
>>>>>     if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>>         return -EINVAL;
>>>>>
>>>>> That way userspace has explicit notification and read-back is 
>>>>> identical to written in value. From engine setup you can just call 
>>>>> the helper silently.
>>>> Sure, EINVAL rather than clamping works as well. And can certainly 
>>>> add helper wrappers. But as above, I don't like the idea of 
>>>> silently disregarding a user specified config option.
>>>
>>> Deal - with the open of heartbeat interval TBD.
>>>
>>>>
>>>>>
>>>>>> +
>>>>>>       WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
>>>>>>         if (execlists_active(&engine->execlists))
>>>>>> @@ -325,6 +332,13 @@ preempt_timeout_store(struct kobject *kobj, 
>>>>>> struct kobj_attribute *attr,
>>>>>>       if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
>>>>>>           return -EINVAL;
>>>>>>   +    if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
>>>>>> +        timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
>>>>>> +        timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
>>>>>> +        drm_info(&engine->i915->drm, "Warning, clamping 
>>>>>> pre-emption timeout to %lld to prevent possibly overflow\n",
>>>>>> +             timeout);
>>>>>> +    }
>>>>>> +
>>>>>>       WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
>>>>>>         if (READ_ONCE(engine->execlists.pending[0]))
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h 
>>>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>>> index 6a4612a852e2..ad131092f8df 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
>>>>>> @@ -248,6 +248,15 @@ struct guc_lrc_desc {
>>>>>>     #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
>>>>>>   +/*
>>>>>> + * GuC converts the timeout to clock ticks internally. Different 
>>>>>> platforms have
>>>>>> + * different GuC clocks. Thus, the maximum value before overflow 
>>>>>> is platform
>>>>>> + * dependent. Current worst case scenario is about 110s. So, 
>>>>>> limit to 100s to be
>>>>>> + * safe.
>>>>>> + */
>>>>>> +#define GUC_POLICY_MAX_EXEC_QUANTUM_MS        (100 * 1000)
>>>>>> +#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS    (100 * 1000)
>>>>>
>>>>> Most important question -
>>>>> how will we know/notice if/when new GuC arrives where these 
>>>>> timeouts would still overflow? Can this be queried somehow at 
>>>>> runtime or where does the limit comes from? How is GuC told about 
>>>>> it? Set in some field and it just allows too large values silently 
>>>>> break things?
>>>> Currently, we don't notice except by debugging peculiar test 
>>>> failures :(.
>>>>
>>>> These limits are not in any GuC spec. Indeed, it took a while to 
>>>> actually work out why increasing the value actually caused shorter 
>>>> timeouts to occur! As above, there is no range checking inside GuC 
>>>> itself. It does a truncated multiply which results in an 
>>>> effectively random number and just happily uses it.
>>>
>>> I will agree with what Daniele said - push on GuC fw folks to 
>>> document the max values they guarantee to support in the interface 
>>> spec. Otherwise it is too fragile.
>> I do agree. But that is going to take time. I would like to get 
>> something merged now while we fight over spec updates.
>
> Yeah that's okay, did not mean to imply I am against a quick fix. 
> "Otherwise it is too fragile, *in the long run*" should have written 
> or something like that.
>
> Regards,
>
> Tvrtko

John Harrison Feb. 24, 2022, 7:51 p.m. UTC | #10

On 2/24/2022 11:19, John Harrison wrote:
> [snip]
>
> I'll change it to _uses_ and repost, then.
>
[    7.683149] kernel BUG at drivers/gpu/drm/i915/gt/uc/intel_guc.h:367!

Told you that one went bang.

John.

Tvrtko Ursulin Feb. 25, 2022, 5:06 p.m. UTC | #11

On 24/02/2022 19:19, John Harrison wrote:

[snip]

>>>>>> ./gt/uc/intel_guc_fwif.h:       u32 execution_quantum;
>>>>>>
>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>
>>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>>> timeslice_duration_ms;
>>>>>>
>>>>>> timeslice_store/preempt_timeout_store:
>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>
>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>> because of tick conversion internally but because at backend level 
>>>>>> nothing was done for assigning 64-bit into 32-bit. Or I failed to 
>>>>>> find where it is handled.
>>>>> That's why I'm adding this range check to make sure we don't allow 
>>>>> overflows.
>>>>
>>>> Yes and no, this fixes it, but the first bug was not only due GuC 
>>>> internal tick conversion. It was present ever since the u64 from 
>>>> i915 was shoved into u32 sent to GuC. So even if GuC used the value 
>>>> without additional multiplication, bug was be there. My point being 
>>>> when GuC backend was added timeout_ms values should have been 
>>>> limited/clamped to U32_MAX. The tick discovery is additional limit 
>>>> on top.
>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>> noticed until I actually tried using very long timeouts to debug a 
>>> particular problem. Now that it is noticed, we need some method of 
>>> range checking and this simple clamp solves all the truncation problems.
>>
>> Agreed in principle, just please mention in the commit message all 
>> aspects of the problem.
>>
>> I think we can get away without a Fixes: tag since it requires user 
>> fiddling to break things in unexpected ways.
>>
>> I would though put in a code a clamping which expresses both, 
>> something like min(u32, ..GUC LIMIT..). So the full story is 
>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return -EINVAL". 
>> Just in case GuC limit one day changes but u32 stays. Perhaps internal 
>> ticks go away or anything and we are left with plain 1:1 millisecond 
>> relationship.
> Can certainly add a comment along the lines of "GuC API only takes a 
> 32bit field but that is further reduced to GUC_LIMIT due to internal 
> calculations which would otherwise overflow".
> 
> But if the GuC limit is > u32 then, by definition, that means the GuC 
> API has changed to take a u64 instead of a u32. So there will no u32 
> truncation any more. So I'm not seeing a need to explicitly test the 
> integer size when the value check covers that.

Hmm I was thinking if the internal conversion in the GuC fw changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be extra safe by documenting in code there is the additional limit of the data structure field. Say the field was changed to take some unit larger than a millisecond. Then the check against the GuC MAX limit define would not be enough, unless that would account both for internal implementation and u32 in the protocol. Maybe that is overdefensive but I don't see that it harms. 50-50, but it's do it once and forget so I'd do it.

>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
>>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>
>>>>>> Hm "wanted".. There's been too much back and forth on the GuC load 
>>>>>> options over the years to keep track.. intel_engine_uses_guc work 
>>>>>> sounds like would work and read nicer.
>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>> existing one. If we want to rename it yet again then that would be 
>>>>> a different patch set.
>>>>
>>>> $ grep intel_engine_uses_guc . -rl
>>>> ./i915_perf.c
>>>> ./i915_request.c
>>>> ./selftests/intel_scheduler_helpers.c
>>>> ./gem/i915_gem_context.c
>>>> ./gt/intel_context.c
>>>> ./gt/intel_engine.h
>>>> ./gt/intel_engine_cs.c
>>>> ./gt/intel_engine_heartbeat.c
>>>> ./gt/intel_engine_pm.c
>>>> ./gt/intel_reset.c
>>>> ./gt/intel_lrc.c
>>>> ./gt/selftest_context.c
>>>> ./gt/selftest_engine_pm.c
>>>> ./gt/selftest_hangcheck.c
>>>> ./gt/selftest_mocs.c
>>>> ./gt/selftest_workarounds.c
>>>>
>>>> Sounds better to me than intel_guc_submission_is_wanted. What does 
>>>> the reader know whether "is wanted" translates to "is actually 
>>>> used". Shrug on "is wanted".
>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it too 
>>> early in the boot up sequence? I never understood why that was 
>>> necessary or why we need so many different ways to ask the same 
>>> question. But this version already exists and definitely works 
>>> without hitting any explosions.
>>
>> No idea if it causes a bug on, doesn't in the helper itself so maybe 
>> you are saying it is called too early? Might be.. I think over time 
>> the nice idea we had that "setup" and "init" phases of engine setup 
>> clearly separated got destroyed a bit. There would always be an option 
>> to move this clamping in a later phase, once the submission method is 
>> known. One could argue that if the submission method is not yet known 
>> at this point, it is even wrong to clamp based on something which will 
>> only be decided later. Because:
>>
>> int intel_engines_init(struct intel_gt *gt)
>> {
>>     int (*setup)(struct intel_engine_cs *engine);
>>     struct intel_engine_cs *engine;
>>     enum intel_engine_id id;
>>     int err;
>>
>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>
>> So this uses "uses", not "wanted". Presumably the point for having 
>> "wanted" and "uses" is that they can disagree, in which case if you 
>> clamp early based on "wanted" that suggests it could be wrong.
> 
> Okay, looks like I was getting confused with intel_guc_is_used(). That 
> one blows up if called too early.
> 
> I'll change it to _uses_ and repost, then.

Check that it isn't called too early, before gt->submission_setup is set.

> 
>>
>>>>>> And limit to class instead of applying to all engines looks like a 
>>>>>> miss.
>>>>> As per follow up email, the class limit is not applied here.
>>>>>
>>>>>>
>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>> +            engine->props.timeslice_duration_ms = 
>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>
>>>>>> I am not sure logging such message during driver load is useful. 
>>>>>> Sounds more like a confused driver which starts with one value and 
>>>>>> then overrides itself. I'd just silently set the value appropriate 
>>>>>> for the active backend. Preemption timeout kconfig text already 
>>>>>> documents the fact timeouts can get overriden at runtime depending 
>>>>>> on platform+engine. So maybe just add same text to timeslice kconfig.
>>>>> The point is to make people aware if they compile with unsupported 
>>>>> config options. As far as I know, there is no way to apply range 
>>>>> checking or other limits to config defines. Which means that a user 
>>>>> would silently get unwanted behaviour. That seems like a bad thing 
>>>>> to me. If the driver is confused because the user built it in a 
>>>>> confused manner then we should let them know.
>>>>
>>>> Okay, but I think make it notice low level.
>>>>
>>>> Also consider in patch 3/3 when you triple it, and then clamp back 
>>>> down here. That's even more confused state since tripling gets 
>>>> nerfed. I think that's also an argument to always account preempt 
>>>> timeout in heartbeat interval calculation. Haven't got to your reply 
>>>> on 2/3 yet though..
>>> That sounds like even more reason to make sure the warning gets seen. 
>>> The more complex the system and the more chances there are to get it 
>>> wrong, the more important it is to have a nice easy to see and 
>>> understand notification that it did go wrong.
>>
>> I did not disagree, just said make it notice, one level higher than 
>> info! :)
> But then it won't appear unless you have explicitly said an elevated 
> debug level. Whereas info appears in dmesg by default (but is still not 
> classed as an error by CI and such).

Notice is higher than info! :) If info appears by default so does notice, warning, err, etc...

#define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
#define KERN_ALERT      KERN_SOH "1"    /* action must be taken immediately */
#define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
#define KERN_ERR        KERN_SOH "3"    /* error conditions */
#define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
#define KERN_NOTICE     KERN_SOH "5"    /* normal but significant condition */
#define KERN_INFO       KERN_SOH "6"    /* informational */
#define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */

>> But also think how, if we agree to go with tripling, that you'd have 
>> to consider that in the sysfs store when hearbeat timeout is written, 
>> to consider whether or not to triple and error out if preemption 
>> timeout is over limit.
> I see this as just setting the default values. If an end user is 
> explicitly overriding the defaults then we should obey what they have 
> requested. If they are changing the heartbeat interval then they can 
> also change the pre-emption timeout appropriately.

Question is can they unknowingly and without any feedback configure a much worse state than they expect? Like when they set heartbeats up to some value, everything is configured as you intended - but if you go over a certain hidden limit the overall scheme degrades in some way. What is the failure mode here if you silently let them do that?

Regards,

Tvrtko

John Harrison Feb. 25, 2022, 5:39 p.m. UTC | #12

On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>
> On 24/02/2022 19:19, John Harrison wrote:
>
> [snip]
>
>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>
>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>
>>>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>>>> timeslice_duration_ms;
>>>>>>>
>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>
>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>> because of tick conversion internally but because at backend 
>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or I 
>>>>>>> failed to find where it is handled.
>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>> allow overflows.
>>>>>
>>>>> Yes and no, this fixes it, but the first bug was not only due GuC 
>>>>> internal tick conversion. It was present ever since the u64 from 
>>>>> i915 was shoved into u32 sent to GuC. So even if GuC used the 
>>>>> value without additional multiplication, bug was be there. My 
>>>>> point being when GuC backend was added timeout_ms values should 
>>>>> have been limited/clamped to U32_MAX. The tick discovery is 
>>>>> additional limit on top.
>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>> noticed until I actually tried using very long timeouts to debug a 
>>>> particular problem. Now that it is noticed, we need some method of 
>>>> range checking and this simple clamp solves all the truncation 
>>>> problems.
>>>
>>> Agreed in principle, just please mention in the commit message all 
>>> aspects of the problem.
>>>
>>> I think we can get away without a Fixes: tag since it requires user 
>>> fiddling to break things in unexpected ways.
>>>
>>> I would though put in a code a clamping which expresses both, 
>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>> Perhaps internal ticks go away or anything and we are left with 
>>> plain 1:1 millisecond relationship.
>> Can certainly add a comment along the lines of "GuC API only takes a 
>> 32bit field but that is further reduced to GUC_LIMIT due to internal 
>> calculations which would otherwise overflow".
>>
>> But if the GuC limit is > u32 then, by definition, that means the GuC 
>> API has changed to take a u64 instead of a u32. So there will no u32 
>> truncation any more. So I'm not seeing a need to explicitly test the 
>> integer size when the value check covers that.
>
> Hmm I was thinking if the internal conversion in the GuC fw changes so 
> that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be 
> extra safe by documenting in code there is the additional limit of the 
> data structure field. Say the field was changed to take some unit 
> larger than a millisecond. Then the check against the GuC MAX limit 
> define would not be enough, unless that would account both for 
> internal implementation and u32 in the protocol. Maybe that is 
> overdefensive but I don't see that it harms. 50-50, but it's do it 
> once and forget so I'd do it.
Huh?

How can the limit be greater than a u32 if the interface only takes a 
u32? By definition the limit would be clamped to u32 size.

If you mean that the GuC policy is in different units and those units 
might not overflow but ms units do, then actually that is already the 
case. The GuC works in us not ms. That's part of why the wrap around is 
so low, we have to multiply by 1000 before sending to GuC. However, that 
is actually irrelevant because the comparison is being done on the i915 
side in i915's units. We have to scale the GuC limit to match what i915 
is using. And the i915 side is u64 so if the scaling to i915 numbers 
overflows a u32 then who cares because that comparison can be done at 64 
bits wide.

If the units change then that is a backwards breaking API change that 
will require a manual driver code update. You can't just recompile with 
a new header and magically get an ms to us or ms to s conversion in your 
a = b assignment. The code will need to be changed to do the new unit 
conversion (note we already convert from ms to us, the GuC API is all 
expressed in us). And that code change will mean having to revisit any 
and all scaling, type conversions, etc. I.e. any pre-existing checks 
will not necessarily be valid and will need to be re-visted anyway. But 
as above, any scaling to GuC units has to be incorporated into the limit 
already because otherwise the limit would not fit in the GuC's own API.

John.

>
>>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == 
>>>>>>>> RENDER_CLASS)
>>>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>>
>>>>>>> Hm "wanted".. There's been too much back and forth on the GuC 
>>>>>>> load options over the years to keep track.. 
>>>>>>> intel_engine_uses_guc work sounds like would work and read nicer.
>>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>>> existing one. If we want to rename it yet again then that would 
>>>>>> be a different patch set.
>>>>>
>>>>> $ grep intel_engine_uses_guc . -rl
>>>>> ./i915_perf.c
>>>>> ./i915_request.c
>>>>> ./selftests/intel_scheduler_helpers.c
>>>>> ./gem/i915_gem_context.c
>>>>> ./gt/intel_context.c
>>>>> ./gt/intel_engine.h
>>>>> ./gt/intel_engine_cs.c
>>>>> ./gt/intel_engine_heartbeat.c
>>>>> ./gt/intel_engine_pm.c
>>>>> ./gt/intel_reset.c
>>>>> ./gt/intel_lrc.c
>>>>> ./gt/selftest_context.c
>>>>> ./gt/selftest_engine_pm.c
>>>>> ./gt/selftest_hangcheck.c
>>>>> ./gt/selftest_mocs.c
>>>>> ./gt/selftest_workarounds.c
>>>>>
>>>>> Sounds better to me than intel_guc_submission_is_wanted. What does 
>>>>> the reader know whether "is wanted" translates to "is actually 
>>>>> used". Shrug on "is wanted".
>>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it 
>>>> too early in the boot up sequence? I never understood why that was 
>>>> necessary or why we need so many different ways to ask the same 
>>>> question. But this version already exists and definitely works 
>>>> without hitting any explosions.
>>>
>>> No idea if it causes a bug on, doesn't in the helper itself so maybe 
>>> you are saying it is called too early? Might be.. I think over time 
>>> the nice idea we had that "setup" and "init" phases of engine setup 
>>> clearly separated got destroyed a bit. There would always be an 
>>> option to move this clamping in a later phase, once the submission 
>>> method is known. One could argue that if the submission method is 
>>> not yet known at this point, it is even wrong to clamp based on 
>>> something which will only be decided later. Because:
>>>
>>> int intel_engines_init(struct intel_gt *gt)
>>> {
>>>     int (*setup)(struct intel_engine_cs *engine);
>>>     struct intel_engine_cs *engine;
>>>     enum intel_engine_id id;
>>>     int err;
>>>
>>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>>
>>> So this uses "uses", not "wanted". Presumably the point for having 
>>> "wanted" and "uses" is that they can disagree, in which case if you 
>>> clamp early based on "wanted" that suggests it could be wrong.
>>
>> Okay, looks like I was getting confused with intel_guc_is_used(). 
>> That one blows up if called too early.
>>
>> I'll change it to _uses_ and repost, then.
>
> Check that it isn't called too early, before gt->submission_setup is set.
Obviously it is because it blew up. But I am not re-writing the driver 
start up sequence just to use the word 'use' instead of 'want'.

>
>>
>>>
>>>>>>> And limit to class instead of applying to all engines looks like 
>>>>>>> a miss.
>>>>>> As per follow up email, the class limit is not applied here.
>>>>>>
>>>>>>>
>>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>>> +            engine->props.timeslice_duration_ms = 
>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>>
>>>>>>> I am not sure logging such message during driver load is useful. 
>>>>>>> Sounds more like a confused driver which starts with one value 
>>>>>>> and then overrides itself. I'd just silently set the value 
>>>>>>> appropriate for the active backend. Preemption timeout kconfig 
>>>>>>> text already documents the fact timeouts can get overriden at 
>>>>>>> runtime depending on platform+engine. So maybe just add same 
>>>>>>> text to timeslice kconfig.
>>>>>> The point is to make people aware if they compile with 
>>>>>> unsupported config options. As far as I know, there is no way to 
>>>>>> apply range checking or other limits to config defines. Which 
>>>>>> means that a user would silently get unwanted behaviour. That 
>>>>>> seems like a bad thing to me. If the driver is confused because 
>>>>>> the user built it in a confused manner then we should let them know.
>>>>>
>>>>> Okay, but I think make it notice low level.
>>>>>
>>>>> Also consider in patch 3/3 when you triple it, and then clamp back 
>>>>> down here. That's even more confused state since tripling gets 
>>>>> nerfed. I think that's also an argument to always account preempt 
>>>>> timeout in heartbeat interval calculation. Haven't got to your 
>>>>> reply on 2/3 yet though..
>>>> That sounds like even more reason to make sure the warning gets 
>>>> seen. The more complex the system and the more chances there are to 
>>>> get it wrong, the more important it is to have a nice easy to see 
>>>> and understand notification that it did go wrong.
>>>
>>> I did not disagree, just said make it notice, one level higher than 
>>> info! :)
>> But then it won't appear unless you have explicitly said an elevated 
>> debug level. Whereas info appears in dmesg by default (but is still 
>> not classed as an error by CI and such).
>
> Notice is higher than info! :) If info appears by default so does 
> notice, warning, err, etc...
Doh! I could have sworn those were the other way around.

Okay. Will update to use notice :).

>
> #define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
> #define KERN_ALERT      KERN_SOH "1"    /* action must be taken 
> immediately */
> #define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
> #define KERN_ERR        KERN_SOH "3"    /* error conditions */
> #define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
> #define KERN_NOTICE     KERN_SOH "5"    /* normal but significant 
> condition */
> #define KERN_INFO       KERN_SOH "6"    /* informational */
> #define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */
>
>>> But also think how, if we agree to go with tripling, that you'd have 
>>> to consider that in the sysfs store when hearbeat timeout is 
>>> written, to consider whether or not to triple and error out if 
>>> preemption timeout is over limit.
>> I see this as just setting the default values. If an end user is 
>> explicitly overriding the defaults then we should obey what they have 
>> requested. If they are changing the heartbeat interval then they can 
>> also change the pre-emption timeout appropriately.
>
> Question is can they unknowingly and without any feedback configure a 
> much worse state than they expect? Like when they set heartbeats up to 
> some value, everything is configured as you intended - but if you go 
> over a certain hidden limit the overall scheme degrades in some way. 
> What is the failure mode here if you silently let them do that?
You can always configure things to be worse than expected. If you don't 
understand what you are doing then any control can make things worse 
instead of better. The assumption is that if a user is savvy enough to 
be writing to sysfs overrides of kernel parameters then they know what 
those parameters are and what their implications are. If they want to 
set a very short heartbeat with a very long pre-emption timeout then its 
their problem if they hit frequent TDRs. Conversely, if they want to set 
a very long heartbeat with a very short pre-emption timeout then its 
still their problem if they hit frequent TDRs.

But if the user explicitly requests a heartbeat period of 3s and a 
pre-emption timeout of 2s and the i915 arbitrarily splats their 2s and 
makes it 9s then that is wrong.

We should give the driver defaults that work for the majority of users 
and then let the minority specify exactly what they need.

And there is no silent or hidden limit. If the user specifies a value 
too large then they will get -EINVAL. Nothing hidden or silent about 
that. Any other values are legal and the behaviour will be whatever has 
been requested.

John.


>
> Regards,
>
> Tvrtko

Tvrtko Ursulin Feb. 25, 2022, 5:44 p.m. UTC | #13

On 24/02/2022 19:51, John Harrison wrote:
> On 2/24/2022 11:19, John Harrison wrote:
>> [snip]
>>
>> I'll change it to _uses_ and repost, then.
>>
> [    7.683149] kernel BUG at drivers/gpu/drm/i915/gt/uc/intel_guc.h:367!
> 
> Told you that one went bang.

intel_guc_is_used ?

My suggestion was intel_engine_uses_guc. But do note I think it would 
not work either because of setup vs init ordering. Not sure that it 
makes sense at engine granularity anyway.

Still I do think "is wanted" is quite bad.

Regards,

Tvrtko

Tvrtko Ursulin Feb. 28, 2022, 4:11 p.m. UTC | #14

On 25/02/2022 17:39, John Harrison wrote:
> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>
>> On 24/02/2022 19:19, John Harrison wrote:
>>
>> [snip]
>>
>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>
>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>
>>>>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>>>>> timeslice_duration_ms;
>>>>>>>>
>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>
>>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>>> because of tick conversion internally but because at backend 
>>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or I 
>>>>>>>> failed to find where it is handled.
>>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>>> allow overflows.
>>>>>>
>>>>>> Yes and no, this fixes it, but the first bug was not only due GuC 
>>>>>> internal tick conversion. It was present ever since the u64 from 
>>>>>> i915 was shoved into u32 sent to GuC. So even if GuC used the 
>>>>>> value without additional multiplication, bug was be there. My 
>>>>>> point being when GuC backend was added timeout_ms values should 
>>>>>> have been limited/clamped to U32_MAX. The tick discovery is 
>>>>>> additional limit on top.
>>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>>> noticed until I actually tried using very long timeouts to debug a 
>>>>> particular problem. Now that it is noticed, we need some method of 
>>>>> range checking and this simple clamp solves all the truncation 
>>>>> problems.
>>>>
>>>> Agreed in principle, just please mention in the commit message all 
>>>> aspects of the problem.
>>>>
>>>> I think we can get away without a Fixes: tag since it requires user 
>>>> fiddling to break things in unexpected ways.
>>>>
>>>> I would though put in a code a clamping which expresses both, 
>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>>> Perhaps internal ticks go away or anything and we are left with 
>>>> plain 1:1 millisecond relationship.
>>> Can certainly add a comment along the lines of "GuC API only takes a 
>>> 32bit field but that is further reduced to GUC_LIMIT due to internal 
>>> calculations which would otherwise overflow".
>>>
>>> But if the GuC limit is > u32 then, by definition, that means the GuC 
>>> API has changed to take a u64 instead of a u32. So there will no u32 
>>> truncation any more. So I'm not seeing a need to explicitly test the 
>>> integer size when the value check covers that.
>>
>> Hmm I was thinking if the internal conversion in the GuC fw changes so 
>> that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be 
>> extra safe by documenting in code there is the additional limit of the 
>> data structure field. Say the field was changed to take some unit 
>> larger than a millisecond. Then the check against the GuC MAX limit 
>> define would not be enough, unless that would account both for 
>> internal implementation and u32 in the protocol. Maybe that is 
>> overdefensive but I don't see that it harms. 50-50, but it's do it 
>> once and forget so I'd do it.
> Huh?
> 
> How can the limit be greater than a u32 if the interface only takes a 
> u32? By definition the limit would be clamped to u32 size.
> 
> If you mean that the GuC policy is in different units and those units 
> might not overflow but ms units do, then actually that is already the 
> case. The GuC works in us not ms. That's part of why the wrap around is 
> so low, we have to multiply by 1000 before sending to GuC. However, that 
> is actually irrelevant because the comparison is being done on the i915 
> side in i915's units. We have to scale the GuC limit to match what i915 
> is using. And the i915 side is u64 so if the scaling to i915 numbers 
> overflows a u32 then who cares because that comparison can be done at 64 
> bits wide.
> 
> If the units change then that is a backwards breaking API change that 
> will require a manual driver code update. You can't just recompile with 
> a new header and magically get an ms to us or ms to s conversion in your 
> a = b assignment. The code will need to be changed to do the new unit 
> conversion (note we already convert from ms to us, the GuC API is all 
> expressed in us). And that code change will mean having to revisit any 
> and all scaling, type conversions, etc. I.e. any pre-existing checks 
> will not necessarily be valid and will need to be re-visted anyway. But 
> as above, any scaling to GuC units has to be incorporated into the limit 
> already because otherwise the limit would not fit in the GuC's own API.

Yes I get that, I was just worried that u32 field in the protocol and 
GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the source code 
and then how to protect against forgetting to update both in sync.

Like if the protocol was changed to take nanoseconds, and firmware 
implementation changed to support the full range, but define 
left/forgotten at 100s. That would then overflow u32.

Regards,

Tvrtko

> John.
> 
>>
>>>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 +++++++++++++++
>>>>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 ++++++++++++++
>>>>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h |  9 +++++++++
>>>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == 
>>>>>>>>> RENDER_CLASS)
>>>>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>>>
>>>>>>>> Hm "wanted".. There's been too much back and forth on the GuC 
>>>>>>>> load options over the years to keep track.. 
>>>>>>>> intel_engine_uses_guc work sounds like would work and read nicer.
>>>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>>>> existing one. If we want to rename it yet again then that would 
>>>>>>> be a different patch set.
>>>>>>
>>>>>> $ grep intel_engine_uses_guc . -rl
>>>>>> ./i915_perf.c
>>>>>> ./i915_request.c
>>>>>> ./selftests/intel_scheduler_helpers.c
>>>>>> ./gem/i915_gem_context.c
>>>>>> ./gt/intel_context.c
>>>>>> ./gt/intel_engine.h
>>>>>> ./gt/intel_engine_cs.c
>>>>>> ./gt/intel_engine_heartbeat.c
>>>>>> ./gt/intel_engine_pm.c
>>>>>> ./gt/intel_reset.c
>>>>>> ./gt/intel_lrc.c
>>>>>> ./gt/selftest_context.c
>>>>>> ./gt/selftest_engine_pm.c
>>>>>> ./gt/selftest_hangcheck.c
>>>>>> ./gt/selftest_mocs.c
>>>>>> ./gt/selftest_workarounds.c
>>>>>>
>>>>>> Sounds better to me than intel_guc_submission_is_wanted. What does 
>>>>>> the reader know whether "is wanted" translates to "is actually 
>>>>>> used". Shrug on "is wanted".
>>>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it 
>>>>> too early in the boot up sequence? I never understood why that was 
>>>>> necessary or why we need so many different ways to ask the same 
>>>>> question. But this version already exists and definitely works 
>>>>> without hitting any explosions.
>>>>
>>>> No idea if it causes a bug on, doesn't in the helper itself so maybe 
>>>> you are saying it is called too early? Might be.. I think over time 
>>>> the nice idea we had that "setup" and "init" phases of engine setup 
>>>> clearly separated got destroyed a bit. There would always be an 
>>>> option to move this clamping in a later phase, once the submission 
>>>> method is known. One could argue that if the submission method is 
>>>> not yet known at this point, it is even wrong to clamp based on 
>>>> something which will only be decided later. Because:
>>>>
>>>> int intel_engines_init(struct intel_gt *gt)
>>>> {
>>>>     int (*setup)(struct intel_engine_cs *engine);
>>>>     struct intel_engine_cs *engine;
>>>>     enum intel_engine_id id;
>>>>     int err;
>>>>
>>>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>>>
>>>> So this uses "uses", not "wanted". Presumably the point for having 
>>>> "wanted" and "uses" is that they can disagree, in which case if you 
>>>> clamp early based on "wanted" that suggests it could be wrong.
>>>
>>> Okay, looks like I was getting confused with intel_guc_is_used(). 
>>> That one blows up if called too early.
>>>
>>> I'll change it to _uses_ and repost, then.
>>
>> Check that it isn't called too early, before gt->submission_setup is set.
> Obviously it is because it blew up. But I am not re-writing the driver 
> start up sequence just to use the word 'use' instead of 'want'.
> 
>>
>>>
>>>>
>>>>>>>> And limit to class instead of applying to all engines looks like 
>>>>>>>> a miss.
>>>>>>> As per follow up email, the class limit is not applied here.
>>>>>>>
>>>>>>>>
>>>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>>>> +                 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>>>> +            engine->props.timeslice_duration_ms = 
>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>>>
>>>>>>>> I am not sure logging such message during driver load is useful. 
>>>>>>>> Sounds more like a confused driver which starts with one value 
>>>>>>>> and then overrides itself. I'd just silently set the value 
>>>>>>>> appropriate for the active backend. Preemption timeout kconfig 
>>>>>>>> text already documents the fact timeouts can get overriden at 
>>>>>>>> runtime depending on platform+engine. So maybe just add same 
>>>>>>>> text to timeslice kconfig.
>>>>>>> The point is to make people aware if they compile with 
>>>>>>> unsupported config options. As far as I know, there is no way to 
>>>>>>> apply range checking or other limits to config defines. Which 
>>>>>>> means that a user would silently get unwanted behaviour. That 
>>>>>>> seems like a bad thing to me. If the driver is confused because 
>>>>>>> the user built it in a confused manner then we should let them know.
>>>>>>
>>>>>> Okay, but I think make it notice low level.
>>>>>>
>>>>>> Also consider in patch 3/3 when you triple it, and then clamp back 
>>>>>> down here. That's even more confused state since tripling gets 
>>>>>> nerfed. I think that's also an argument to always account preempt 
>>>>>> timeout in heartbeat interval calculation. Haven't got to your 
>>>>>> reply on 2/3 yet though..
>>>>> That sounds like even more reason to make sure the warning gets 
>>>>> seen. The more complex the system and the more chances there are to 
>>>>> get it wrong, the more important it is to have a nice easy to see 
>>>>> and understand notification that it did go wrong.
>>>>
>>>> I did not disagree, just said make it notice, one level higher than 
>>>> info! :)
>>> But then it won't appear unless you have explicitly said an elevated 
>>> debug level. Whereas info appears in dmesg by default (but is still 
>>> not classed as an error by CI and such).
>>
>> Notice is higher than info! :) If info appears by default so does 
>> notice, warning, err, etc...
> Doh! I could have sworn those were the other way around.
> 
> Okay. Will update to use notice :).
> 
>>
>> #define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
>> #define KERN_ALERT      KERN_SOH "1"    /* action must be taken 
>> immediately */
>> #define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
>> #define KERN_ERR        KERN_SOH "3"    /* error conditions */
>> #define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
>> #define KERN_NOTICE     KERN_SOH "5"    /* normal but significant 
>> condition */
>> #define KERN_INFO       KERN_SOH "6"    /* informational */
>> #define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */
>>
>>>> But also think how, if we agree to go with tripling, that you'd have 
>>>> to consider that in the sysfs store when hearbeat timeout is 
>>>> written, to consider whether or not to triple and error out if 
>>>> preemption timeout is over limit.
>>> I see this as just setting the default values. If an end user is 
>>> explicitly overriding the defaults then we should obey what they have 
>>> requested. If they are changing the heartbeat interval then they can 
>>> also change the pre-emption timeout appropriately.
>>
>> Question is can they unknowingly and without any feedback configure a 
>> much worse state than they expect? Like when they set heartbeats up to 
>> some value, everything is configured as you intended - but if you go 
>> over a certain hidden limit the overall scheme degrades in some way. 
>> What is the failure mode here if you silently let them do that?
> You can always configure things to be worse than expected. If you don't 
> understand what you are doing then any control can make things worse 
> instead of better. The assumption is that if a user is savvy enough to 
> be writing to sysfs overrides of kernel parameters then they know what 
> those parameters are and what their implications are. If they want to 
> set a very short heartbeat with a very long pre-emption timeout then its 
> their problem if they hit frequent TDRs. Conversely, if they want to set 
> a very long heartbeat with a very short pre-emption timeout then its 
> still their problem if they hit frequent TDRs.
> 
> But if the user explicitly requests a heartbeat period of 3s and a 
> pre-emption timeout of 2s and the i915 arbitrarily splats their 2s and 
> makes it 9s then that is wrong.
> 
> We should give the driver defaults that work for the majority of users 
> and then let the minority specify exactly what they need.
> 
> And there is no silent or hidden limit. If the user specifies a value 
> too large then they will get -EINVAL. Nothing hidden or silent about 
> that. Any other values are legal and the behaviour will be whatever has 
> been requested.
> 
> John.
> 
> 
>>
>> Regards,
>>
>> Tvrtko
>

John Harrison Feb. 28, 2022, 6:32 p.m. UTC | #15

On 2/28/2022 08:11, Tvrtko Ursulin wrote:
> On 25/02/2022 17:39, John Harrison wrote:
>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>
>>> On 24/02/2022 19:19, John Harrison wrote:
>>>
>>> [snip]
>>>
>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>
>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>
>>>>>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>>>>>> timeslice_duration_ms;
>>>>>>>>>
>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>
>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>>>> because of tick conversion internally but because at backend 
>>>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or I 
>>>>>>>>> failed to find where it is handled.
>>>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>>>> allow overflows.
>>>>>>>
>>>>>>> Yes and no, this fixes it, but the first bug was not only due 
>>>>>>> GuC internal tick conversion. It was present ever since the u64 
>>>>>>> from i915 was shoved into u32 sent to GuC. So even if GuC used 
>>>>>>> the value without additional multiplication, bug was be there. 
>>>>>>> My point being when GuC backend was added timeout_ms values 
>>>>>>> should have been limited/clamped to U32_MAX. The tick discovery 
>>>>>>> is additional limit on top.
>>>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>>>> noticed until I actually tried using very long timeouts to debug 
>>>>>> a particular problem. Now that it is noticed, we need some method 
>>>>>> of range checking and this simple clamp solves all the truncation 
>>>>>> problems.
>>>>>
>>>>> Agreed in principle, just please mention in the commit message all 
>>>>> aspects of the problem.
>>>>>
>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>> user fiddling to break things in unexpected ways.
>>>>>
>>>>> I would though put in a code a clamping which expresses both, 
>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>>>> Perhaps internal ticks go away or anything and we are left with 
>>>>> plain 1:1 millisecond relationship.
>>>> Can certainly add a comment along the lines of "GuC API only takes 
>>>> a 32bit field but that is further reduced to GUC_LIMIT due to 
>>>> internal calculations which would otherwise overflow".
>>>>
>>>> But if the GuC limit is > u32 then, by definition, that means the 
>>>> GuC API has changed to take a u64 instead of a u32. So there will 
>>>> no u32 truncation any more. So I'm not seeing a need to explicitly 
>>>> test the integer size when the value check covers that.
>>>
>>> Hmm I was thinking if the internal conversion in the GuC fw changes 
>>> so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be 
>>> extra safe by documenting in code there is the additional limit of 
>>> the data structure field. Say the field was changed to take some 
>>> unit larger than a millisecond. Then the check against the GuC MAX 
>>> limit define would not be enough, unless that would account both for 
>>> internal implementation and u32 in the protocol. Maybe that is 
>>> overdefensive but I don't see that it harms. 50-50, but it's do it 
>>> once and forget so I'd do it.
>> Huh?
>>
>> How can the limit be greater than a u32 if the interface only takes a 
>> u32? By definition the limit would be clamped to u32 size.
>>
>> If you mean that the GuC policy is in different units and those units 
>> might not overflow but ms units do, then actually that is already the 
>> case. The GuC works in us not ms. That's part of why the wrap around 
>> is so low, we have to multiply by 1000 before sending to GuC. 
>> However, that is actually irrelevant because the comparison is being 
>> done on the i915 side in i915's units. We have to scale the GuC limit 
>> to match what i915 is using. And the i915 side is u64 so if the 
>> scaling to i915 numbers overflows a u32 then who cares because that 
>> comparison can be done at 64 bits wide.
>>
>> If the units change then that is a backwards breaking API change that 
>> will require a manual driver code update. You can't just recompile 
>> with a new header and magically get an ms to us or ms to s conversion 
>> in your a = b assignment. The code will need to be changed to do the 
>> new unit conversion (note we already convert from ms to us, the GuC 
>> API is all expressed in us). And that code change will mean having to 
>> revisit any and all scaling, type conversions, etc. I.e. any 
>> pre-existing checks will not necessarily be valid and will need to be 
>> re-visted anyway. But as above, any scaling to GuC units has to be 
>> incorporated into the limit already because otherwise the limit would 
>> not fit in the GuC's own API.
>
> Yes I get that, I was just worried that u32 field in the protocol and 
> GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the source code 
> and then how to protect against forgetting to update both in sync.
>
> Like if the protocol was changed to take nanoseconds, and firmware 
> implementation changed to support the full range, but define 
> left/forgotten at 100s. That would then overflow u32.
Huh? If the API was updated to 'support the full range' then how can you 
get overflow by forgetting to update the limit? You could get 
unnecessary clamping, which hopefully would be noticed by whoever is 
testing the new API and/or whoever requested the change. But you can't 
get u32 overflow errors if all the code has been updated to u64.

John.

>
> Regards,
>
> Tvrtko
>
>> John.
>>
>>>
>>>>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>>>>> ---
>>>>>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 
>>>>>>>>>> +++++++++++++++
>>>>>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 
>>>>>>>>>> ++++++++++++++
>>>>>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 9 +++++++++
>>>>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == 
>>>>>>>>>> RENDER_CLASS)
>>>>>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>>>>
>>>>>>>>> Hm "wanted".. There's been too much back and forth on the GuC 
>>>>>>>>> load options over the years to keep track.. 
>>>>>>>>> intel_engine_uses_guc work sounds like would work and read nicer.
>>>>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>>>>> existing one. If we want to rename it yet again then that would 
>>>>>>>> be a different patch set.
>>>>>>>
>>>>>>> $ grep intel_engine_uses_guc . -rl
>>>>>>> ./i915_perf.c
>>>>>>> ./i915_request.c
>>>>>>> ./selftests/intel_scheduler_helpers.c
>>>>>>> ./gem/i915_gem_context.c
>>>>>>> ./gt/intel_context.c
>>>>>>> ./gt/intel_engine.h
>>>>>>> ./gt/intel_engine_cs.c
>>>>>>> ./gt/intel_engine_heartbeat.c
>>>>>>> ./gt/intel_engine_pm.c
>>>>>>> ./gt/intel_reset.c
>>>>>>> ./gt/intel_lrc.c
>>>>>>> ./gt/selftest_context.c
>>>>>>> ./gt/selftest_engine_pm.c
>>>>>>> ./gt/selftest_hangcheck.c
>>>>>>> ./gt/selftest_mocs.c
>>>>>>> ./gt/selftest_workarounds.c
>>>>>>>
>>>>>>> Sounds better to me than intel_guc_submission_is_wanted. What 
>>>>>>> does the reader know whether "is wanted" translates to "is 
>>>>>>> actually used". Shrug on "is wanted".
>>>>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it 
>>>>>> too early in the boot up sequence? I never understood why that 
>>>>>> was necessary or why we need so many different ways to ask the 
>>>>>> same question. But this version already exists and definitely 
>>>>>> works without hitting any explosions.
>>>>>
>>>>> No idea if it causes a bug on, doesn't in the helper itself so 
>>>>> maybe you are saying it is called too early? Might be.. I think 
>>>>> over time the nice idea we had that "setup" and "init" phases of 
>>>>> engine setup clearly separated got destroyed a bit. There would 
>>>>> always be an option to move this clamping in a later phase, once 
>>>>> the submission method is known. One could argue that if the 
>>>>> submission method is not yet known at this point, it is even wrong 
>>>>> to clamp based on something which will only be decided later. 
>>>>> Because:
>>>>>
>>>>> int intel_engines_init(struct intel_gt *gt)
>>>>> {
>>>>>     int (*setup)(struct intel_engine_cs *engine);
>>>>>     struct intel_engine_cs *engine;
>>>>>     enum intel_engine_id id;
>>>>>     int err;
>>>>>
>>>>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>>>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>>>>
>>>>> So this uses "uses", not "wanted". Presumably the point for having 
>>>>> "wanted" and "uses" is that they can disagree, in which case if 
>>>>> you clamp early based on "wanted" that suggests it could be wrong.
>>>>
>>>> Okay, looks like I was getting confused with intel_guc_is_used(). 
>>>> That one blows up if called too early.
>>>>
>>>> I'll change it to _uses_ and repost, then.
>>>
>>> Check that it isn't called too early, before gt->submission_setup is 
>>> set.
>> Obviously it is because it blew up. But I am not re-writing the 
>> driver start up sequence just to use the word 'use' instead of 'want'.
>>
>>>
>>>>
>>>>>
>>>>>>>>> And limit to class instead of applying to all engines looks 
>>>>>>>>> like a miss.
>>>>>>>> As per follow up email, the class limit is not applied here.
>>>>>>>>
>>>>>>>>>
>>>>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>>>>> + GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>>>>> + engine->props.timeslice_duration_ms = 
>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>>>>
>>>>>>>>> I am not sure logging such message during driver load is 
>>>>>>>>> useful. Sounds more like a confused driver which starts with 
>>>>>>>>> one value and then overrides itself. I'd just silently set the 
>>>>>>>>> value appropriate for the active backend. Preemption timeout 
>>>>>>>>> kconfig text already documents the fact timeouts can get 
>>>>>>>>> overriden at runtime depending on platform+engine. So maybe 
>>>>>>>>> just add same text to timeslice kconfig.
>>>>>>>> The point is to make people aware if they compile with 
>>>>>>>> unsupported config options. As far as I know, there is no way 
>>>>>>>> to apply range checking or other limits to config defines. 
>>>>>>>> Which means that a user would silently get unwanted behaviour. 
>>>>>>>> That seems like a bad thing to me. If the driver is confused 
>>>>>>>> because the user built it in a confused manner then we should 
>>>>>>>> let them know.
>>>>>>>
>>>>>>> Okay, but I think make it notice low level.
>>>>>>>
>>>>>>> Also consider in patch 3/3 when you triple it, and then clamp 
>>>>>>> back down here. That's even more confused state since tripling 
>>>>>>> gets nerfed. I think that's also an argument to always account 
>>>>>>> preempt timeout in heartbeat interval calculation. Haven't got 
>>>>>>> to your reply on 2/3 yet though..
>>>>>> That sounds like even more reason to make sure the warning gets 
>>>>>> seen. The more complex the system and the more chances there are 
>>>>>> to get it wrong, the more important it is to have a nice easy to 
>>>>>> see and understand notification that it did go wrong.
>>>>>
>>>>> I did not disagree, just said make it notice, one level higher 
>>>>> than info! :)
>>>> But then it won't appear unless you have explicitly said an 
>>>> elevated debug level. Whereas info appears in dmesg by default (but 
>>>> is still not classed as an error by CI and such).
>>>
>>> Notice is higher than info! :) If info appears by default so does 
>>> notice, warning, err, etc...
>> Doh! I could have sworn those were the other way around.
>>
>> Okay. Will update to use notice :).
>>
>>>
>>> #define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
>>> #define KERN_ALERT      KERN_SOH "1"    /* action must be taken 
>>> immediately */
>>> #define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
>>> #define KERN_ERR        KERN_SOH "3"    /* error conditions */
>>> #define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
>>> #define KERN_NOTICE     KERN_SOH "5"    /* normal but significant 
>>> condition */
>>> #define KERN_INFO       KERN_SOH "6"    /* informational */
>>> #define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */
>>>
>>>>> But also think how, if we agree to go with tripling, that you'd 
>>>>> have to consider that in the sysfs store when hearbeat timeout is 
>>>>> written, to consider whether or not to triple and error out if 
>>>>> preemption timeout is over limit.
>>>> I see this as just setting the default values. If an end user is 
>>>> explicitly overriding the defaults then we should obey what they 
>>>> have requested. If they are changing the heartbeat interval then 
>>>> they can also change the pre-emption timeout appropriately.
>>>
>>> Question is can they unknowingly and without any feedback configure 
>>> a much worse state than they expect? Like when they set heartbeats 
>>> up to some value, everything is configured as you intended - but if 
>>> you go over a certain hidden limit the overall scheme degrades in 
>>> some way. What is the failure mode here if you silently let them do 
>>> that?
>> You can always configure things to be worse than expected. If you 
>> don't understand what you are doing then any control can make things 
>> worse instead of better. The assumption is that if a user is savvy 
>> enough to be writing to sysfs overrides of kernel parameters then 
>> they know what those parameters are and what their implications are. 
>> If they want to set a very short heartbeat with a very long 
>> pre-emption timeout then its their problem if they hit frequent TDRs. 
>> Conversely, if they want to set a very long heartbeat with a very 
>> short pre-emption timeout then its still their problem if they hit 
>> frequent TDRs.
>>
>> But if the user explicitly requests a heartbeat period of 3s and a 
>> pre-emption timeout of 2s and the i915 arbitrarily splats their 2s 
>> and makes it 9s then that is wrong.
>>
>> We should give the driver defaults that work for the majority of 
>> users and then let the minority specify exactly what they need.
>>
>> And there is no silent or hidden limit. If the user specifies a value 
>> too large then they will get -EINVAL. Nothing hidden or silent about 
>> that. Any other values are legal and the behaviour will be whatever 
>> has been requested.
>>
>> John.
>>
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>

Tvrtko Ursulin March 1, 2022, 10:50 a.m. UTC | #16

On 28/02/2022 18:32, John Harrison wrote:
> On 2/28/2022 08:11, Tvrtko Ursulin wrote:
>> On 25/02/2022 17:39, John Harrison wrote:
>>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>>
>>>> On 24/02/2022 19:19, John Harrison wrote:
>>>>
>>>> [snip]
>>>>
>>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>>
>>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>>
>>>>>>>>>> ./gt/intel_engine_types.h:              unsigned long 
>>>>>>>>>> timeslice_duration_ms;
>>>>>>>>>>
>>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>>
>>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>>>>> because of tick conversion internally but because at backend 
>>>>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or I 
>>>>>>>>>> failed to find where it is handled.
>>>>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>>>>> allow overflows.
>>>>>>>>
>>>>>>>> Yes and no, this fixes it, but the first bug was not only due 
>>>>>>>> GuC internal tick conversion. It was present ever since the u64 
>>>>>>>> from i915 was shoved into u32 sent to GuC. So even if GuC used 
>>>>>>>> the value without additional multiplication, bug was be there. 
>>>>>>>> My point being when GuC backend was added timeout_ms values 
>>>>>>>> should have been limited/clamped to U32_MAX. The tick discovery 
>>>>>>>> is additional limit on top.
>>>>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>>>>> noticed until I actually tried using very long timeouts to debug 
>>>>>>> a particular problem. Now that it is noticed, we need some method 
>>>>>>> of range checking and this simple clamp solves all the truncation 
>>>>>>> problems.
>>>>>>
>>>>>> Agreed in principle, just please mention in the commit message all 
>>>>>> aspects of the problem.
>>>>>>
>>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>>> user fiddling to break things in unexpected ways.
>>>>>>
>>>>>> I would though put in a code a clamping which expresses both, 
>>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>>>>> Perhaps internal ticks go away or anything and we are left with 
>>>>>> plain 1:1 millisecond relationship.
>>>>> Can certainly add a comment along the lines of "GuC API only takes 
>>>>> a 32bit field but that is further reduced to GUC_LIMIT due to 
>>>>> internal calculations which would otherwise overflow".
>>>>>
>>>>> But if the GuC limit is > u32 then, by definition, that means the 
>>>>> GuC API has changed to take a u64 instead of a u32. So there will 
>>>>> no u32 truncation any more. So I'm not seeing a need to explicitly 
>>>>> test the integer size when the value check covers that.
>>>>
>>>> Hmm I was thinking if the internal conversion in the GuC fw changes 
>>>> so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, then to be 
>>>> extra safe by documenting in code there is the additional limit of 
>>>> the data structure field. Say the field was changed to take some 
>>>> unit larger than a millisecond. Then the check against the GuC MAX 
>>>> limit define would not be enough, unless that would account both for 
>>>> internal implementation and u32 in the protocol. Maybe that is 
>>>> overdefensive but I don't see that it harms. 50-50, but it's do it 
>>>> once and forget so I'd do it.
>>> Huh?
>>>
>>> How can the limit be greater than a u32 if the interface only takes a 
>>> u32? By definition the limit would be clamped to u32 size.
>>>
>>> If you mean that the GuC policy is in different units and those units 
>>> might not overflow but ms units do, then actually that is already the 
>>> case. The GuC works in us not ms. That's part of why the wrap around 
>>> is so low, we have to multiply by 1000 before sending to GuC. 
>>> However, that is actually irrelevant because the comparison is being 
>>> done on the i915 side in i915's units. We have to scale the GuC limit 
>>> to match what i915 is using. And the i915 side is u64 so if the 
>>> scaling to i915 numbers overflows a u32 then who cares because that 
>>> comparison can be done at 64 bits wide.
>>>
>>> If the units change then that is a backwards breaking API change that 
>>> will require a manual driver code update. You can't just recompile 
>>> with a new header and magically get an ms to us or ms to s conversion 
>>> in your a = b assignment. The code will need to be changed to do the 
>>> new unit conversion (note we already convert from ms to us, the GuC 
>>> API is all expressed in us). And that code change will mean having to 
>>> revisit any and all scaling, type conversions, etc. I.e. any 
>>> pre-existing checks will not necessarily be valid and will need to be 
>>> re-visted anyway. But as above, any scaling to GuC units has to be 
>>> incorporated into the limit already because otherwise the limit would 
>>> not fit in the GuC's own API.
>>
>> Yes I get that, I was just worried that u32 field in the protocol and 
>> GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the source code 
>> and then how to protect against forgetting to update both in sync.
>>
>> Like if the protocol was changed to take nanoseconds, and firmware 
>> implementation changed to support the full range, but define 
>> left/forgotten at 100s. That would then overflow u32.
> Huh? If the API was updated to 'support the full range' then how can you 
> get overflow by forgetting to update the limit? You could get 
> unnecessary clamping, which hopefully would be noticed by whoever is 
> testing the new API and/or whoever requested the change. But you can't 
> get u32 overflow errors if all the code has been updated to u64.

1)
Change the protocol so that "u32 desc->execution_quantum" now takes nano seconds.

This now makes the maximum time 4.29.. seconds.

2)
Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for instance that part at that point still not part of the interface contract.

3)
User passes in 5 seconds.

Clamping check says all is good.

"engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"

4)

Assignment was updated:

gt/uc/intel_guc_submission.c:

   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;

But someone did not realize field is u32.

   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;

Defensive solution:

   if (overflows_type(engine->props.timeslice_duration_ms * 1e6, desc->execution_quantum))
	drm_WARN_ON...

   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;

Regards,

Tvrtko

  
> John.
> 
>>
>> Regards,
>>
>> Tvrtko
>>
>>> John.
>>>
>>>>
>>>>>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>   drivers/gpu/drm/i915/gt/intel_engine_cs.c   | 15 
>>>>>>>>>>> +++++++++++++++
>>>>>>>>>>>   drivers/gpu/drm/i915/gt/sysfs_engines.c     | 14 
>>>>>>>>>>> ++++++++++++++
>>>>>>>>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 9 +++++++++
>>>>>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == 
>>>>>>>>>>> RENDER_CLASS)
>>>>>>>>>>>           engine->props.preempt_timeout_ms = 0;
>>>>>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>>>>>
>>>>>>>>>> Hm "wanted".. There's been too much back and forth on the GuC 
>>>>>>>>>> load options over the years to keep track.. 
>>>>>>>>>> intel_engine_uses_guc work sounds like would work and read nicer.
>>>>>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>>>>>> existing one. If we want to rename it yet again then that would 
>>>>>>>>> be a different patch set.
>>>>>>>>
>>>>>>>> $ grep intel_engine_uses_guc . -rl
>>>>>>>> ./i915_perf.c
>>>>>>>> ./i915_request.c
>>>>>>>> ./selftests/intel_scheduler_helpers.c
>>>>>>>> ./gem/i915_gem_context.c
>>>>>>>> ./gt/intel_context.c
>>>>>>>> ./gt/intel_engine.h
>>>>>>>> ./gt/intel_engine_cs.c
>>>>>>>> ./gt/intel_engine_heartbeat.c
>>>>>>>> ./gt/intel_engine_pm.c
>>>>>>>> ./gt/intel_reset.c
>>>>>>>> ./gt/intel_lrc.c
>>>>>>>> ./gt/selftest_context.c
>>>>>>>> ./gt/selftest_engine_pm.c
>>>>>>>> ./gt/selftest_hangcheck.c
>>>>>>>> ./gt/selftest_mocs.c
>>>>>>>> ./gt/selftest_workarounds.c
>>>>>>>>
>>>>>>>> Sounds better to me than intel_guc_submission_is_wanted. What 
>>>>>>>> does the reader know whether "is wanted" translates to "is 
>>>>>>>> actually used". Shrug on "is wanted".
>>>>>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call it 
>>>>>>> too early in the boot up sequence? I never understood why that 
>>>>>>> was necessary or why we need so many different ways to ask the 
>>>>>>> same question. But this version already exists and definitely 
>>>>>>> works without hitting any explosions.
>>>>>>
>>>>>> No idea if it causes a bug on, doesn't in the helper itself so 
>>>>>> maybe you are saying it is called too early? Might be.. I think 
>>>>>> over time the nice idea we had that "setup" and "init" phases of 
>>>>>> engine setup clearly separated got destroyed a bit. There would 
>>>>>> always be an option to move this clamping in a later phase, once 
>>>>>> the submission method is known. One could argue that if the 
>>>>>> submission method is not yet known at this point, it is even wrong 
>>>>>> to clamp based on something which will only be decided later. 
>>>>>> Because:
>>>>>>
>>>>>> int intel_engines_init(struct intel_gt *gt)
>>>>>> {
>>>>>>     int (*setup)(struct intel_engine_cs *engine);
>>>>>>     struct intel_engine_cs *engine;
>>>>>>     enum intel_engine_id id;
>>>>>>     int err;
>>>>>>
>>>>>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>>>>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>>>>>
>>>>>> So this uses "uses", not "wanted". Presumably the point for having 
>>>>>> "wanted" and "uses" is that they can disagree, in which case if 
>>>>>> you clamp early based on "wanted" that suggests it could be wrong.
>>>>>
>>>>> Okay, looks like I was getting confused with intel_guc_is_used(). 
>>>>> That one blows up if called too early.
>>>>>
>>>>> I'll change it to _uses_ and repost, then.
>>>>
>>>> Check that it isn't called too early, before gt->submission_setup is 
>>>> set.
>>> Obviously it is because it blew up. But I am not re-writing the 
>>> driver start up sequence just to use the word 'use' instead of 'want'.
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>>>> And limit to class instead of applying to all engines looks 
>>>>>>>>>> like a miss.
>>>>>>>>> As per follow up email, the class limit is not applied here.
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>>>>>> + GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>>>>>> + engine->props.timeslice_duration_ms = 
>>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>>>>>
>>>>>>>>>> I am not sure logging such message during driver load is 
>>>>>>>>>> useful. Sounds more like a confused driver which starts with 
>>>>>>>>>> one value and then overrides itself. I'd just silently set the 
>>>>>>>>>> value appropriate for the active backend. Preemption timeout 
>>>>>>>>>> kconfig text already documents the fact timeouts can get 
>>>>>>>>>> overriden at runtime depending on platform+engine. So maybe 
>>>>>>>>>> just add same text to timeslice kconfig.
>>>>>>>>> The point is to make people aware if they compile with 
>>>>>>>>> unsupported config options. As far as I know, there is no way 
>>>>>>>>> to apply range checking or other limits to config defines. 
>>>>>>>>> Which means that a user would silently get unwanted behaviour. 
>>>>>>>>> That seems like a bad thing to me. If the driver is confused 
>>>>>>>>> because the user built it in a confused manner then we should 
>>>>>>>>> let them know.
>>>>>>>>
>>>>>>>> Okay, but I think make it notice low level.
>>>>>>>>
>>>>>>>> Also consider in patch 3/3 when you triple it, and then clamp 
>>>>>>>> back down here. That's even more confused state since tripling 
>>>>>>>> gets nerfed. I think that's also an argument to always account 
>>>>>>>> preempt timeout in heartbeat interval calculation. Haven't got 
>>>>>>>> to your reply on 2/3 yet though..
>>>>>>> That sounds like even more reason to make sure the warning gets 
>>>>>>> seen. The more complex the system and the more chances there are 
>>>>>>> to get it wrong, the more important it is to have a nice easy to 
>>>>>>> see and understand notification that it did go wrong.
>>>>>>
>>>>>> I did not disagree, just said make it notice, one level higher 
>>>>>> than info! :)
>>>>> But then it won't appear unless you have explicitly said an 
>>>>> elevated debug level. Whereas info appears in dmesg by default (but 
>>>>> is still not classed as an error by CI and such).
>>>>
>>>> Notice is higher than info! :) If info appears by default so does 
>>>> notice, warning, err, etc...
>>> Doh! I could have sworn those were the other way around.
>>>
>>> Okay. Will update to use notice :).
>>>
>>>>
>>>> #define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
>>>> #define KERN_ALERT      KERN_SOH "1"    /* action must be taken 
>>>> immediately */
>>>> #define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
>>>> #define KERN_ERR        KERN_SOH "3"    /* error conditions */
>>>> #define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
>>>> #define KERN_NOTICE     KERN_SOH "5"    /* normal but significant 
>>>> condition */
>>>> #define KERN_INFO       KERN_SOH "6"    /* informational */
>>>> #define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */
>>>>
>>>>>> But also think how, if we agree to go with tripling, that you'd 
>>>>>> have to consider that in the sysfs store when hearbeat timeout is 
>>>>>> written, to consider whether or not to triple and error out if 
>>>>>> preemption timeout is over limit.
>>>>> I see this as just setting the default values. If an end user is 
>>>>> explicitly overriding the defaults then we should obey what they 
>>>>> have requested. If they are changing the heartbeat interval then 
>>>>> they can also change the pre-emption timeout appropriately.
>>>>
>>>> Question is can they unknowingly and without any feedback configure 
>>>> a much worse state than they expect? Like when they set heartbeats 
>>>> up to some value, everything is configured as you intended - but if 
>>>> you go over a certain hidden limit the overall scheme degrades in 
>>>> some way. What is the failure mode here if you silently let them do 
>>>> that?
>>> You can always configure things to be worse than expected. If you 
>>> don't understand what you are doing then any control can make things 
>>> worse instead of better. The assumption is that if a user is savvy 
>>> enough to be writing to sysfs overrides of kernel parameters then 
>>> they know what those parameters are and what their implications are. 
>>> If they want to set a very short heartbeat with a very long 
>>> pre-emption timeout then its their problem if they hit frequent TDRs. 
>>> Conversely, if they want to set a very long heartbeat with a very 
>>> short pre-emption timeout then its still their problem if they hit 
>>> frequent TDRs.
>>>
>>> But if the user explicitly requests a heartbeat period of 3s and a 
>>> pre-emption timeout of 2s and the i915 arbitrarily splats their 2s 
>>> and makes it 9s then that is wrong.
>>>
>>> We should give the driver defaults that work for the majority of 
>>> users and then let the minority specify exactly what they need.
>>>
>>> And there is no silent or hidden limit. If the user specifies a value 
>>> too large then they will get -EINVAL. Nothing hidden or silent about 
>>> that. Any other values are legal and the behaviour will be whatever 
>>> has been requested.
>>>
>>> John.
>>>
>>>
>>>>
>>>> Regards,
>>>>
>>>> Tvrtko
>>>
>

John Harrison March 1, 2022, 7:57 p.m. UTC | #17

On 3/1/2022 02:50, Tvrtko Ursulin wrote:
> On 28/02/2022 18:32, John Harrison wrote:
>> On 2/28/2022 08:11, Tvrtko Ursulin wrote:
>>> On 25/02/2022 17:39, John Harrison wrote:
>>>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>>>
>>>>> On 24/02/2022 19:19, John Harrison wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>>>
>>>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>>>
>>>>>>>>>>> ./gt/intel_engine_types.h: unsigned long timeslice_duration_ms;
>>>>>>>>>>>
>>>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>>>
>>>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>>>>>> because of tick conversion internally but because at backend 
>>>>>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or 
>>>>>>>>>>> I failed to find where it is handled.
>>>>>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>>>>>> allow overflows.
>>>>>>>>>
>>>>>>>>> Yes and no, this fixes it, but the first bug was not only due 
>>>>>>>>> GuC internal tick conversion. It was present ever since the 
>>>>>>>>> u64 from i915 was shoved into u32 sent to GuC. So even if GuC 
>>>>>>>>> used the value without additional multiplication, bug was be 
>>>>>>>>> there. My point being when GuC backend was added timeout_ms 
>>>>>>>>> values should have been limited/clamped to U32_MAX. The tick 
>>>>>>>>> discovery is additional limit on top.
>>>>>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>>>>>> noticed until I actually tried using very long timeouts to 
>>>>>>>> debug a particular problem. Now that it is noticed, we need 
>>>>>>>> some method of range checking and this simple clamp solves all 
>>>>>>>> the truncation problems.
>>>>>>>
>>>>>>> Agreed in principle, just please mention in the commit message 
>>>>>>> all aspects of the problem.
>>>>>>>
>>>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>>>> user fiddling to break things in unexpected ways.
>>>>>>>
>>>>>>> I would though put in a code a clamping which expresses both, 
>>>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>>>>>> Perhaps internal ticks go away or anything and we are left with 
>>>>>>> plain 1:1 millisecond relationship.
>>>>>> Can certainly add a comment along the lines of "GuC API only 
>>>>>> takes a 32bit field but that is further reduced to GUC_LIMIT due 
>>>>>> to internal calculations which would otherwise overflow".
>>>>>>
>>>>>> But if the GuC limit is > u32 then, by definition, that means the 
>>>>>> GuC API has changed to take a u64 instead of a u32. So there will 
>>>>>> no u32 truncation any more. So I'm not seeing a need to 
>>>>>> explicitly test the integer size when the value check covers that.
>>>>>
>>>>> Hmm I was thinking if the internal conversion in the GuC fw 
>>>>> changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, 
>>>>> then to be extra safe by documenting in code there is the 
>>>>> additional limit of the data structure field. Say the field was 
>>>>> changed to take some unit larger than a millisecond. Then the 
>>>>> check against the GuC MAX limit define would not be enough, unless 
>>>>> that would account both for internal implementation and u32 in the 
>>>>> protocol. Maybe that is overdefensive but I don't see that it 
>>>>> harms. 50-50, but it's do it once and forget so I'd do it.
>>>> Huh?
>>>>
>>>> How can the limit be greater than a u32 if the interface only takes 
>>>> a u32? By definition the limit would be clamped to u32 size.
>>>>
>>>> If you mean that the GuC policy is in different units and those 
>>>> units might not overflow but ms units do, then actually that is 
>>>> already the case. The GuC works in us not ms. That's part of why 
>>>> the wrap around is so low, we have to multiply by 1000 before 
>>>> sending to GuC. However, that is actually irrelevant because the 
>>>> comparison is being done on the i915 side in i915's units. We have 
>>>> to scale the GuC limit to match what i915 is using. And the i915 
>>>> side is u64 so if the scaling to i915 numbers overflows a u32 then 
>>>> who cares because that comparison can be done at 64 bits wide.
>>>>
>>>> If the units change then that is a backwards breaking API change 
>>>> that will require a manual driver code update. You can't just 
>>>> recompile with a new header and magically get an ms to us or ms to 
>>>> s conversion in your a = b assignment. The code will need to be 
>>>> changed to do the new unit conversion (note we already convert from 
>>>> ms to us, the GuC API is all expressed in us). And that code change 
>>>> will mean having to revisit any and all scaling, type conversions, 
>>>> etc. I.e. any pre-existing checks will not necessarily be valid and 
>>>> will need to be re-visted anyway. But as above, any scaling to GuC 
>>>> units has to be incorporated into the limit already because 
>>>> otherwise the limit would not fit in the GuC's own API.
>>>
>>> Yes I get that, I was just worried that u32 field in the protocol 
>>> and GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the 
>>> source code and then how to protect against forgetting to update 
>>> both in sync.
>>>
>>> Like if the protocol was changed to take nanoseconds, and firmware 
>>> implementation changed to support the full range, but define 
>>> left/forgotten at 100s. That would then overflow u32.
>> Huh? If the API was updated to 'support the full range' then how can 
>> you get overflow by forgetting to update the limit? You could get 
>> unnecessary clamping, which hopefully would be noticed by whoever is 
>> testing the new API and/or whoever requested the change. But you 
>> can't get u32 overflow errors if all the code has been updated to u64.
>
> 1)
> Change the protocol so that "u32 desc->execution_quantum" now takes 
> nano seconds.
>
> This now makes the maximum time 4.29.. seconds.
You seriously think this is likely to happen?

That the GuC people would force an API change on us that is completely 
backwards from what we have been asking? And that such a massive 
backwards step would not get implemented correctly because someone 
didn't notice just how huge an impact it was?

>
> 2)
> Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for 
> instance that part at that point still not part of the interface 
> contract.
There is zero chance of the us -> ns change occurring in the foreseeable 
future whereas the expectation is to have the limits be part of the spec 
in the next firmware release. So this scenario is just not going to 
happen. And as above, it would be such a big change with such a huge 
amount of push back and discussion going on that it would be impossible 
for the limit update to be missed/forgotten.

>
> 3)
> User passes in 5 seconds.
>
> Clamping check says all is good.
>
> "engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"
>
> 4)
>
> Assignment was updated:
>
> gt/uc/intel_guc_submission.c:
>
>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>
> But someone did not realize field is u32.
>
>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>
> Defensive solution:
>
>   if (overflows_type(engine->props.timeslice_duration_ms * 1e6, 
> desc->execution_quantum))
>     drm_WARN_ON...

All you are saying is that bugs can happen. The above is just one more 
place to have a bug.

The purpose of the limit is to take into account all reasons for there 
being a limit. Having a bunch of different tests that are all testing 
the same thing is pointless.

John.

>
>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>
> Regards,
>
> Tvrtko
>
>
>> John.
>>
>>>
>>> Regards,
>>>
>>> Tvrtko
>>>
>>>> John.
>>>>
>>>>>
>>>>>>>>>>>> Signed-off-by: John Harrison <[e-mail]>
>>>>>>>>>>>> ---
>>>>>>>>>>>> drivers/gpu/drm/i915/gt/intel_engine_cs.c | 15 +++++++++++++++
>>>>>>>>>>>> drivers/gpu/drm/i915/gt/sysfs_engines.c | 14 ++++++++++++++
>>>>>>>>>>>> drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h | 9 +++++++++
>>>>>>>>>>>>   3 files changed, 38 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c 
>>>>>>>>>>>> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>>> index e53008b4dd05..2a1e9f36e6f5 100644
>>>>>>>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
>>>>>>>>>>>> @@ -389,6 +389,21 @@ static int intel_engine_setup(struct 
>>>>>>>>>>>> intel_gt *gt, enum intel_engine_id id,
>>>>>>>>>>>>       if (GRAPHICS_VER(i915) == 12 && engine->class == 
>>>>>>>>>>>> RENDER_CLASS)
>>>>>>>>>>>> engine->props.preempt_timeout_ms = 0;
>>>>>>>>>>>>   +    /* Cap timeouts to prevent overflow inside GuC */
>>>>>>>>>>>> +    if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
>>>>>>>>>>>> +        if (engine->props.timeslice_duration_ms > 
>>>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
>>>>>>>>>>>
>>>>>>>>>>> Hm "wanted".. There's been too much back and forth on the 
>>>>>>>>>>> GuC load options over the years to keep track.. 
>>>>>>>>>>> intel_engine_uses_guc work sounds like would work and read 
>>>>>>>>>>> nicer.
>>>>>>>>>> I'm not adding a new feature check here. I'm just using the 
>>>>>>>>>> existing one. If we want to rename it yet again then that 
>>>>>>>>>> would be a different patch set.
>>>>>>>>>
>>>>>>>>> $ grep intel_engine_uses_guc . -rl
>>>>>>>>> ./i915_perf.c
>>>>>>>>> ./i915_request.c
>>>>>>>>> ./selftests/intel_scheduler_helpers.c
>>>>>>>>> ./gem/i915_gem_context.c
>>>>>>>>> ./gt/intel_context.c
>>>>>>>>> ./gt/intel_engine.h
>>>>>>>>> ./gt/intel_engine_cs.c
>>>>>>>>> ./gt/intel_engine_heartbeat.c
>>>>>>>>> ./gt/intel_engine_pm.c
>>>>>>>>> ./gt/intel_reset.c
>>>>>>>>> ./gt/intel_lrc.c
>>>>>>>>> ./gt/selftest_context.c
>>>>>>>>> ./gt/selftest_engine_pm.c
>>>>>>>>> ./gt/selftest_hangcheck.c
>>>>>>>>> ./gt/selftest_mocs.c
>>>>>>>>> ./gt/selftest_workarounds.c
>>>>>>>>>
>>>>>>>>> Sounds better to me than intel_guc_submission_is_wanted. What 
>>>>>>>>> does the reader know whether "is wanted" translates to "is 
>>>>>>>>> actually used". Shrug on "is wanted".
>>>>>>>> Yes, but isn't '_uses' the one that hits a BUG_ON if you call 
>>>>>>>> it too early in the boot up sequence? I never understood why 
>>>>>>>> that was necessary or why we need so many different ways to ask 
>>>>>>>> the same question. But this version already exists and 
>>>>>>>> definitely works without hitting any explosions.
>>>>>>>
>>>>>>> No idea if it causes a bug on, doesn't in the helper itself so 
>>>>>>> maybe you are saying it is called too early? Might be.. I think 
>>>>>>> over time the nice idea we had that "setup" and "init" phases of 
>>>>>>> engine setup clearly separated got destroyed a bit. There would 
>>>>>>> always be an option to move this clamping in a later phase, once 
>>>>>>> the submission method is known. One could argue that if the 
>>>>>>> submission method is not yet known at this point, it is even 
>>>>>>> wrong to clamp based on something which will only be decided 
>>>>>>> later. Because:
>>>>>>>
>>>>>>> int intel_engines_init(struct intel_gt *gt)
>>>>>>> {
>>>>>>>     int (*setup)(struct intel_engine_cs *engine);
>>>>>>>     struct intel_engine_cs *engine;
>>>>>>>     enum intel_engine_id id;
>>>>>>>     int err;
>>>>>>>
>>>>>>>     if (intel_uc_uses_guc_submission(&gt->uc)) {
>>>>>>>         gt->submission_method = INTEL_SUBMISSION_GUC;
>>>>>>>
>>>>>>> So this uses "uses", not "wanted". Presumably the point for 
>>>>>>> having "wanted" and "uses" is that they can disagree, in which 
>>>>>>> case if you clamp early based on "wanted" that suggests it could 
>>>>>>> be wrong.
>>>>>>
>>>>>> Okay, looks like I was getting confused with intel_guc_is_used(). 
>>>>>> That one blows up if called too early.
>>>>>>
>>>>>> I'll change it to _uses_ and repost, then.
>>>>>
>>>>> Check that it isn't called too early, before gt->submission_setup 
>>>>> is set.
>>>> Obviously it is because it blew up. But I am not re-writing the 
>>>> driver start up sequence just to use the word 'use' instead of 'want'.
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>>>>> And limit to class instead of applying to all engines looks 
>>>>>>>>>>> like a miss.
>>>>>>>>>> As per follow up email, the class limit is not applied here.
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>> + drm_info(&engine->i915->drm, "Warning, clamping timeslice 
>>>>>>>>>>>> duration to %d to prevent possibly overflow\n",
>>>>>>>>>>>> + GUC_POLICY_MAX_EXEC_QUANTUM_MS);
>>>>>>>>>>>> + engine->props.timeslice_duration_ms = 
>>>>>>>>>>>> GUC_POLICY_MAX_EXEC_QUANTUM_MS;
>>>>>>>>>>>
>>>>>>>>>>> I am not sure logging such message during driver load is 
>>>>>>>>>>> useful. Sounds more like a confused driver which starts with 
>>>>>>>>>>> one value and then overrides itself. I'd just silently set 
>>>>>>>>>>> the value appropriate for the active backend. Preemption 
>>>>>>>>>>> timeout kconfig text already documents the fact timeouts can 
>>>>>>>>>>> get overriden at runtime depending on platform+engine. So 
>>>>>>>>>>> maybe just add same text to timeslice kconfig.
>>>>>>>>>> The point is to make people aware if they compile with 
>>>>>>>>>> unsupported config options. As far as I know, there is no way 
>>>>>>>>>> to apply range checking or other limits to config defines. 
>>>>>>>>>> Which means that a user would silently get unwanted 
>>>>>>>>>> behaviour. That seems like a bad thing to me. If the driver 
>>>>>>>>>> is confused because the user built it in a confused manner 
>>>>>>>>>> then we should let them know.
>>>>>>>>>
>>>>>>>>> Okay, but I think make it notice low level.
>>>>>>>>>
>>>>>>>>> Also consider in patch 3/3 when you triple it, and then clamp 
>>>>>>>>> back down here. That's even more confused state since tripling 
>>>>>>>>> gets nerfed. I think that's also an argument to always account 
>>>>>>>>> preempt timeout in heartbeat interval calculation. Haven't got 
>>>>>>>>> to your reply on 2/3 yet though..
>>>>>>>> That sounds like even more reason to make sure the warning gets 
>>>>>>>> seen. The more complex the system and the more chances there 
>>>>>>>> are to get it wrong, the more important it is to have a nice 
>>>>>>>> easy to see and understand notification that it did go wrong.
>>>>>>>
>>>>>>> I did not disagree, just said make it notice, one level higher 
>>>>>>> than info! :)
>>>>>> But then it won't appear unless you have explicitly said an 
>>>>>> elevated debug level. Whereas info appears in dmesg by default 
>>>>>> (but is still not classed as an error by CI and such).
>>>>>
>>>>> Notice is higher than info! :) If info appears by default so does 
>>>>> notice, warning, err, etc...
>>>> Doh! I could have sworn those were the other way around.
>>>>
>>>> Okay. Will update to use notice :).
>>>>
>>>>>
>>>>> #define KERN_EMERG      KERN_SOH "0"    /* system is unusable */
>>>>> #define KERN_ALERT      KERN_SOH "1"    /* action must be taken 
>>>>> immediately */
>>>>> #define KERN_CRIT       KERN_SOH "2"    /* critical conditions */
>>>>> #define KERN_ERR        KERN_SOH "3"    /* error conditions */
>>>>> #define KERN_WARNING    KERN_SOH "4"    /* warning conditions */
>>>>> #define KERN_NOTICE     KERN_SOH "5"    /* normal but significant 
>>>>> condition */
>>>>> #define KERN_INFO       KERN_SOH "6"    /* informational */
>>>>> #define KERN_DEBUG      KERN_SOH "7"    /* debug-level messages */
>>>>>
>>>>>>> But also think how, if we agree to go with tripling, that you'd 
>>>>>>> have to consider that in the sysfs store when hearbeat timeout 
>>>>>>> is written, to consider whether or not to triple and error out 
>>>>>>> if preemption timeout is over limit.
>>>>>> I see this as just setting the default values. If an end user is 
>>>>>> explicitly overriding the defaults then we should obey what they 
>>>>>> have requested. If they are changing the heartbeat interval then 
>>>>>> they can also change the pre-emption timeout appropriately.
>>>>>
>>>>> Question is can they unknowingly and without any feedback 
>>>>> configure a much worse state than they expect? Like when they set 
>>>>> heartbeats up to some value, everything is configured as you 
>>>>> intended - but if you go over a certain hidden limit the overall 
>>>>> scheme degrades in some way. What is the failure mode here if you 
>>>>> silently let them do that?
>>>> You can always configure things to be worse than expected. If you 
>>>> don't understand what you are doing then any control can make 
>>>> things worse instead of better. The assumption is that if a user is 
>>>> savvy enough to be writing to sysfs overrides of kernel parameters 
>>>> then they know what those parameters are and what their 
>>>> implications are. If they want to set a very short heartbeat with a 
>>>> very long pre-emption timeout then its their problem if they hit 
>>>> frequent TDRs. Conversely, if they want to set a very long 
>>>> heartbeat with a very short pre-emption timeout then its still 
>>>> their problem if they hit frequent TDRs.
>>>>
>>>> But if the user explicitly requests a heartbeat period of 3s and a 
>>>> pre-emption timeout of 2s and the i915 arbitrarily splats their 2s 
>>>> and makes it 9s then that is wrong.
>>>>
>>>> We should give the driver defaults that work for the majority of 
>>>> users and then let the minority specify exactly what they need.
>>>>
>>>> And there is no silent or hidden limit. If the user specifies a 
>>>> value too large then they will get -EINVAL. Nothing hidden or 
>>>> silent about that. Any other values are legal and the behaviour 
>>>> will be whatever has been requested.
>>>>
>>>> John.
>>>>
>>>>
>>>>>
>>>>> Regards,
>>>>>
>>>>> Tvrtko
>>>>
>>

Tvrtko Ursulin March 2, 2022, 9:20 a.m. UTC | #18

On 01/03/2022 19:57, John Harrison wrote:
> On 3/1/2022 02:50, Tvrtko Ursulin wrote:
>> On 28/02/2022 18:32, John Harrison wrote:
>>> On 2/28/2022 08:11, Tvrtko Ursulin wrote:
>>>> On 25/02/2022 17:39, John Harrison wrote:
>>>>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 24/02/2022 19:19, John Harrison wrote:
>>>>>>
>>>>>> [snip]
>>>>>>
>>>>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>>>>
>>>>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>>>>
>>>>>>>>>>>> ./gt/intel_engine_types.h: unsigned long timeslice_duration_ms;
>>>>>>>>>>>>
>>>>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>>>>
>>>>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not only 
>>>>>>>>>>>> because of tick conversion internally but because at backend 
>>>>>>>>>>>> level nothing was done for assigning 64-bit into 32-bit. Or 
>>>>>>>>>>>> I failed to find where it is handled.
>>>>>>>>>>> That's why I'm adding this range check to make sure we don't 
>>>>>>>>>>> allow overflows.
>>>>>>>>>>
>>>>>>>>>> Yes and no, this fixes it, but the first bug was not only due 
>>>>>>>>>> GuC internal tick conversion. It was present ever since the 
>>>>>>>>>> u64 from i915 was shoved into u32 sent to GuC. So even if GuC 
>>>>>>>>>> used the value without additional multiplication, bug was be 
>>>>>>>>>> there. My point being when GuC backend was added timeout_ms 
>>>>>>>>>> values should have been limited/clamped to U32_MAX. The tick 
>>>>>>>>>> discovery is additional limit on top.
>>>>>>>>> I'm not disagreeing. I'm just saying that the truncation wasn't 
>>>>>>>>> noticed until I actually tried using very long timeouts to 
>>>>>>>>> debug a particular problem. Now that it is noticed, we need 
>>>>>>>>> some method of range checking and this simple clamp solves all 
>>>>>>>>> the truncation problems.
>>>>>>>>
>>>>>>>> Agreed in principle, just please mention in the commit message 
>>>>>>>> all aspects of the problem.
>>>>>>>>
>>>>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>>>>> user fiddling to break things in unexpected ways.
>>>>>>>>
>>>>>>>> I would though put in a code a clamping which expresses both, 
>>>>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>>>>> -EINVAL". Just in case GuC limit one day changes but u32 stays. 
>>>>>>>> Perhaps internal ticks go away or anything and we are left with 
>>>>>>>> plain 1:1 millisecond relationship.
>>>>>>> Can certainly add a comment along the lines of "GuC API only 
>>>>>>> takes a 32bit field but that is further reduced to GUC_LIMIT due 
>>>>>>> to internal calculations which would otherwise overflow".
>>>>>>>
>>>>>>> But if the GuC limit is > u32 then, by definition, that means the 
>>>>>>> GuC API has changed to take a u64 instead of a u32. So there will 
>>>>>>> no u32 truncation any more. So I'm not seeing a need to 
>>>>>>> explicitly test the integer size when the value check covers that.
>>>>>>
>>>>>> Hmm I was thinking if the internal conversion in the GuC fw 
>>>>>> changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above u32, 
>>>>>> then to be extra safe by documenting in code there is the 
>>>>>> additional limit of the data structure field. Say the field was 
>>>>>> changed to take some unit larger than a millisecond. Then the 
>>>>>> check against the GuC MAX limit define would not be enough, unless 
>>>>>> that would account both for internal implementation and u32 in the 
>>>>>> protocol. Maybe that is overdefensive but I don't see that it 
>>>>>> harms. 50-50, but it's do it once and forget so I'd do it.
>>>>> Huh?
>>>>>
>>>>> How can the limit be greater than a u32 if the interface only takes 
>>>>> a u32? By definition the limit would be clamped to u32 size.
>>>>>
>>>>> If you mean that the GuC policy is in different units and those 
>>>>> units might not overflow but ms units do, then actually that is 
>>>>> already the case. The GuC works in us not ms. That's part of why 
>>>>> the wrap around is so low, we have to multiply by 1000 before 
>>>>> sending to GuC. However, that is actually irrelevant because the 
>>>>> comparison is being done on the i915 side in i915's units. We have 
>>>>> to scale the GuC limit to match what i915 is using. And the i915 
>>>>> side is u64 so if the scaling to i915 numbers overflows a u32 then 
>>>>> who cares because that comparison can be done at 64 bits wide.
>>>>>
>>>>> If the units change then that is a backwards breaking API change 
>>>>> that will require a manual driver code update. You can't just 
>>>>> recompile with a new header and magically get an ms to us or ms to 
>>>>> s conversion in your a = b assignment. The code will need to be 
>>>>> changed to do the new unit conversion (note we already convert from 
>>>>> ms to us, the GuC API is all expressed in us). And that code change 
>>>>> will mean having to revisit any and all scaling, type conversions, 
>>>>> etc. I.e. any pre-existing checks will not necessarily be valid and 
>>>>> will need to be re-visted anyway. But as above, any scaling to GuC 
>>>>> units has to be incorporated into the limit already because 
>>>>> otherwise the limit would not fit in the GuC's own API.
>>>>
>>>> Yes I get that, I was just worried that u32 field in the protocol 
>>>> and GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the 
>>>> source code and then how to protect against forgetting to update 
>>>> both in sync.
>>>>
>>>> Like if the protocol was changed to take nanoseconds, and firmware 
>>>> implementation changed to support the full range, but define 
>>>> left/forgotten at 100s. That would then overflow u32.
>>> Huh? If the API was updated to 'support the full range' then how can 
>>> you get overflow by forgetting to update the limit? You could get 
>>> unnecessary clamping, which hopefully would be noticed by whoever is 
>>> testing the new API and/or whoever requested the change. But you 
>>> can't get u32 overflow errors if all the code has been updated to u64.
>>
>> 1)
>> Change the protocol so that "u32 desc->execution_quantum" now takes 
>> nano seconds.
>>
>> This now makes the maximum time 4.29.. seconds.
> You seriously think this is likely to happen?
> 
> That the GuC people would force an API change on us that is completely 
> backwards from what we have been asking? And that such a massive 
> backwards step would not get implemented correctly because someone 
> didn't notice just how huge an impact it was?

I don't know what we have been asking or what GuC people would do.

>> 2)
>> Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for 
>> instance that part at that point still not part of the interface 
>> contract.
> There is zero chance of the us -> ns change occurring in the foreseeable 
> future whereas the expectation is to have the limits be part of the spec 
> in the next firmware release. So this scenario is just not going to 
> happen. And as above, it would be such a big change with such a huge 
> amount of push back and discussion going on that it would be impossible 
> for the limit update to be missed/forgotten.
> 
>>
>> 3)
>> User passes in 5 seconds.
>>
>> Clamping check says all is good.
>>
>> "engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"
>>
>> 4)
>>
>> Assignment was updated:
>>
>> gt/uc/intel_guc_submission.c:
>>
>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>
>> But someone did not realize field is u32.
>>
>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>
>> Defensive solution:
>>
>>   if (overflows_type(engine->props.timeslice_duration_ms * 1e6, 
>> desc->execution_quantum))
>>     drm_WARN_ON...
> 
> All you are saying is that bugs can happen. The above is just one more 
> place to have a bug.
> 
> The purpose of the limit is to take into account all reasons for there 
> being a limit. Having a bunch of different tests that are all testing 
> the same thing is pointless.

I am saying this:

1)
The code I pointed out is a boundary layer between two components which 
have independent design and development teams.

2)
The limit in question is currently not explicitly defined by the 
interface provider.

3)
The limit in question is also implicitly defined by the hidden internal 
firmware implementation details not relating to the units of the interface.

4)
The source code location of the clamping check is far away (different 
file, different layer) from the assignment to the interface data structure.

 From this it sounds plausible to me to have the check at the assignment 
site and don't have to think about it further.

Regards,

Tvrtko

John Harrison March 2, 2022, 6:07 p.m. UTC | #19

On 3/2/2022 01:20, Tvrtko Ursulin wrote:
> On 01/03/2022 19:57, John Harrison wrote:
>> On 3/1/2022 02:50, Tvrtko Ursulin wrote:
>>> On 28/02/2022 18:32, John Harrison wrote:
>>>> On 2/28/2022 08:11, Tvrtko Ursulin wrote:
>>>>> On 25/02/2022 17:39, John Harrison wrote:
>>>>>> On 2/25/2022 09:06, Tvrtko Ursulin wrote:
>>>>>>>
>>>>>>> On 24/02/2022 19:19, John Harrison wrote:
>>>>>>>
>>>>>>> [snip]
>>>>>>>
>>>>>>>>>>>>> ./gt/uc/intel_guc_fwif.h: u32 execution_quantum;
>>>>>>>>>>>>>
>>>>>>>>>>>>> ./gt/uc/intel_guc_submission.c: desc->execution_quantum = 
>>>>>>>>>>>>> engine->props.timeslice_duration_ms * 1000;
>>>>>>>>>>>>>
>>>>>>>>>>>>> ./gt/intel_engine_types.h: unsigned long 
>>>>>>>>>>>>> timeslice_duration_ms;
>>>>>>>>>>>>>
>>>>>>>>>>>>> timeslice_store/preempt_timeout_store:
>>>>>>>>>>>>> err = kstrtoull(buf, 0, &duration);
>>>>>>>>>>>>>
>>>>>>>>>>>>> So both kconfig and sysfs can already overflow GuC, not 
>>>>>>>>>>>>> only because of tick conversion internally but because at 
>>>>>>>>>>>>> backend level nothing was done for assigning 64-bit into 
>>>>>>>>>>>>> 32-bit. Or I failed to find where it is handled.
>>>>>>>>>>>> That's why I'm adding this range check to make sure we 
>>>>>>>>>>>> don't allow overflows.
>>>>>>>>>>>
>>>>>>>>>>> Yes and no, this fixes it, but the first bug was not only 
>>>>>>>>>>> due GuC internal tick conversion. It was present ever since 
>>>>>>>>>>> the u64 from i915 was shoved into u32 sent to GuC. So even 
>>>>>>>>>>> if GuC used the value without additional multiplication, bug 
>>>>>>>>>>> was be there. My point being when GuC backend was added 
>>>>>>>>>>> timeout_ms values should have been limited/clamped to 
>>>>>>>>>>> U32_MAX. The tick discovery is additional limit on top.
>>>>>>>>>> I'm not disagreeing. I'm just saying that the truncation 
>>>>>>>>>> wasn't noticed until I actually tried using very long 
>>>>>>>>>> timeouts to debug a particular problem. Now that it is 
>>>>>>>>>> noticed, we need some method of range checking and this 
>>>>>>>>>> simple clamp solves all the truncation problems.
>>>>>>>>>
>>>>>>>>> Agreed in principle, just please mention in the commit message 
>>>>>>>>> all aspects of the problem.
>>>>>>>>>
>>>>>>>>> I think we can get away without a Fixes: tag since it requires 
>>>>>>>>> user fiddling to break things in unexpected ways.
>>>>>>>>>
>>>>>>>>> I would though put in a code a clamping which expresses both, 
>>>>>>>>> something like min(u32, ..GUC LIMIT..). So the full story is 
>>>>>>>>> documented forever. Or "if > u32 || > ..GUC LIMIT..) return 
>>>>>>>>> -EINVAL". Just in case GuC limit one day changes but u32 
>>>>>>>>> stays. Perhaps internal ticks go away or anything and we are 
>>>>>>>>> left with plain 1:1 millisecond relationship.
>>>>>>>> Can certainly add a comment along the lines of "GuC API only 
>>>>>>>> takes a 32bit field but that is further reduced to GUC_LIMIT 
>>>>>>>> due to internal calculations which would otherwise overflow".
>>>>>>>>
>>>>>>>> But if the GuC limit is > u32 then, by definition, that means 
>>>>>>>> the GuC API has changed to take a u64 instead of a u32. So 
>>>>>>>> there will no u32 truncation any more. So I'm not seeing a need 
>>>>>>>> to explicitly test the integer size when the value check covers 
>>>>>>>> that.
>>>>>>>
>>>>>>> Hmm I was thinking if the internal conversion in the GuC fw 
>>>>>>> changes so that GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS goes above 
>>>>>>> u32, then to be extra safe by documenting in code there is the 
>>>>>>> additional limit of the data structure field. Say the field was 
>>>>>>> changed to take some unit larger than a millisecond. Then the 
>>>>>>> check against the GuC MAX limit define would not be enough, 
>>>>>>> unless that would account both for internal implementation and 
>>>>>>> u32 in the protocol. Maybe that is overdefensive but I don't see 
>>>>>>> that it harms. 50-50, but it's do it once and forget so I'd do it.
>>>>>> Huh?
>>>>>>
>>>>>> How can the limit be greater than a u32 if the interface only 
>>>>>> takes a u32? By definition the limit would be clamped to u32 size.
>>>>>>
>>>>>> If you mean that the GuC policy is in different units and those 
>>>>>> units might not overflow but ms units do, then actually that is 
>>>>>> already the case. The GuC works in us not ms. That's part of why 
>>>>>> the wrap around is so low, we have to multiply by 1000 before 
>>>>>> sending to GuC. However, that is actually irrelevant because the 
>>>>>> comparison is being done on the i915 side in i915's units. We 
>>>>>> have to scale the GuC limit to match what i915 is using. And the 
>>>>>> i915 side is u64 so if the scaling to i915 numbers overflows a 
>>>>>> u32 then who cares because that comparison can be done at 64 bits 
>>>>>> wide.
>>>>>>
>>>>>> If the units change then that is a backwards breaking API change 
>>>>>> that will require a manual driver code update. You can't just 
>>>>>> recompile with a new header and magically get an ms to us or ms 
>>>>>> to s conversion in your a = b assignment. The code will need to 
>>>>>> be changed to do the new unit conversion (note we already convert 
>>>>>> from ms to us, the GuC API is all expressed in us). And that code 
>>>>>> change will mean having to revisit any and all scaling, type 
>>>>>> conversions, etc. I.e. any pre-existing checks will not 
>>>>>> necessarily be valid and will need to be re-visted anyway. But as 
>>>>>> above, any scaling to GuC units has to be incorporated into the 
>>>>>> limit already because otherwise the limit would not fit in the 
>>>>>> GuC's own API.
>>>>>
>>>>> Yes I get that, I was just worried that u32 field in the protocol 
>>>>> and GUC_POLICY_MAX_EXEC_QUANTUM_MS defines are separate in the 
>>>>> source code and then how to protect against forgetting to update 
>>>>> both in sync.
>>>>>
>>>>> Like if the protocol was changed to take nanoseconds, and firmware 
>>>>> implementation changed to support the full range, but define 
>>>>> left/forgotten at 100s. That would then overflow u32.
>>>> Huh? If the API was updated to 'support the full range' then how 
>>>> can you get overflow by forgetting to update the limit? You could 
>>>> get unnecessary clamping, which hopefully would be noticed by 
>>>> whoever is testing the new API and/or whoever requested the change. 
>>>> But you can't get u32 overflow errors if all the code has been 
>>>> updated to u64.
>>>
>>> 1)
>>> Change the protocol so that "u32 desc->execution_quantum" now takes 
>>> nano seconds.
>>>
>>> This now makes the maximum time 4.29.. seconds.
>> You seriously think this is likely to happen?
>>
>> That the GuC people would force an API change on us that is 
>> completely backwards from what we have been asking? And that such a 
>> massive backwards step would not get implemented correctly because 
>> someone didn't notice just how huge an impact it was?
>
> I don't know what we have been asking or what GuC people would do.
Despite the views of some in the community, the GuC team are not evil 
monsters out for world domination. We are their customers and their task 
is to provide a usable offload device that makes the Linux experience 
better not worse.

Just from this discussion alone, ignoring any internal forums, it has 
been made clear that the (long standing) request from the i915 team is 
to support 64bit policy values and (more recently) to officially 
document any and all limits involved in the policies. By definition, 
that also means that there would be significant push back and argument 
if the GuC team proposed making this interface worse.

>
>>> 2)
>>> Forget to update GUC_POLICY_MAX_EXEC_QUANTUM_MS from 100s, since for 
>>> instance that part at that point still not part of the interface 
>>> contract.
>> There is zero chance of the us -> ns change occurring in the 
>> foreseeable future whereas the expectation is to have the limits be 
>> part of the spec in the next firmware release. So this scenario is 
>> just not going to happen. And as above, it would be such a big change 
>> with such a huge amount of push back and discussion going on that it 
>> would be impossible for the limit update to be missed/forgotten.
>>
>>>
>>> 3)
>>> User passes in 5 seconds.
>>>
>>> Clamping check says all is good.
>>>
>>> "engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS"
>>>
>>> 4)
>>>
>>> Assignment was updated:
>>>
>>> gt/uc/intel_guc_submission.c:
>>>
>>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>>
>>> But someone did not realize field is u32.
>>>
>>>   desc->execution_quantum = engine->props.timeslice_duration_ms * 1e6;
>>>
>>> Defensive solution:
>>>
>>>   if (overflows_type(engine->props.timeslice_duration_ms * 1e6, 
>>> desc->execution_quantum))
>>>     drm_WARN_ON...
>>
>> All you are saying is that bugs can happen. The above is just one 
>> more place to have a bug.
>>
>> The purpose of the limit is to take into account all reasons for 
>> there being a limit. Having a bunch of different tests that are all 
>> testing the same thing is pointless.
>
> I am saying this:
>
> 1)
> The code I pointed out is a boundary layer between two components 
> which have independent design and development teams.
>
> 2)
> The limit in question is currently not explicitly defined by the 
> interface provider.
>
> 3)
> The limit in question is also implicitly defined by the hidden 
> internal firmware implementation details not relating to the units of 
> the interface.
>
> 4)
> The source code location of the clamping check is far away (different 
> file, different layer) from the assignment to the interface data 
> structure.
>
> From this it sounds plausible to me to have the check at the 
> assignment site and don't have to think about it further.
It also sounds plausible to use the concept of consolidation. Rather 
than scattering random different limit tests in random different places, 
it all goes into a single helper function that can be used at the top 
level and report any range issues before you get to the lower levels 
where errors might not be allowed. This was your own feedback (and is 
currently implemented in the v2 post).

John.

>
> Regards,
>
> Tvrtko

Patch

diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
index e53008b4dd05..2a1e9f36e6f5 100644
--- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
@@ -389,6 +389,21 @@  static int intel_engine_setup(struct intel_gt *gt, enum intel_engine_id id,
 	if (GRAPHICS_VER(i915) == 12 && engine->class == RENDER_CLASS)
 		engine->props.preempt_timeout_ms = 0;
 
+	/* Cap timeouts to prevent overflow inside GuC */
+	if (intel_guc_submission_is_wanted(&gt->uc.guc)) {
+		if (engine->props.timeslice_duration_ms > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
+			drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %d to prevent possibly overflow\n",
+				 GUC_POLICY_MAX_EXEC_QUANTUM_MS);
+			engine->props.timeslice_duration_ms = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
+		}
+
+		if (engine->props.preempt_timeout_ms > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
+			drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %d to prevent possibly overflow\n",
+				 GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS);
+			engine->props.preempt_timeout_ms = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
+		}
+	}
+
 	engine->defaults = engine->props; /* never to change again */
 
 	engine->context_size = intel_engine_context_size(gt, engine->class);
diff --git a/drivers/gpu/drm/i915/gt/sysfs_engines.c b/drivers/gpu/drm/i915/gt/sysfs_engines.c
index 967031056202..f57efe026474 100644
--- a/drivers/gpu/drm/i915/gt/sysfs_engines.c
+++ b/drivers/gpu/drm/i915/gt/sysfs_engines.c
@@ -221,6 +221,13 @@  timeslice_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (duration > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
 		return -EINVAL;
 
+	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
+	    duration > GUC_POLICY_MAX_EXEC_QUANTUM_MS) {
+		duration = GUC_POLICY_MAX_EXEC_QUANTUM_MS;
+		drm_info(&engine->i915->drm, "Warning, clamping timeslice duration to %lld to prevent possibly overflow\n",
+			 duration);
+	}
+
 	WRITE_ONCE(engine->props.timeslice_duration_ms, duration);
 
 	if (execlists_active(&engine->execlists))
@@ -325,6 +332,13 @@  preempt_timeout_store(struct kobject *kobj, struct kobj_attribute *attr,
 	if (timeout > jiffies_to_msecs(MAX_SCHEDULE_TIMEOUT))
 		return -EINVAL;
 
+	if (intel_uc_uses_guc_submission(&engine->gt->uc) &&
+	    timeout > GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS) {
+		timeout = GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS;
+		drm_info(&engine->i915->drm, "Warning, clamping pre-emption timeout to %lld to prevent possibly overflow\n",
+			 timeout);
+	}
+
 	WRITE_ONCE(engine->props.preempt_timeout_ms, timeout);
 
 	if (READ_ONCE(engine->execlists.pending[0]))
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
index 6a4612a852e2..ad131092f8df 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fwif.h
@@ -248,6 +248,15 @@  struct guc_lrc_desc {
 
 #define GLOBAL_POLICY_DEFAULT_DPC_PROMOTE_TIME_US 500000
 
+/*
+ * GuC converts the timeout to clock ticks internally. Different platforms have
+ * different GuC clocks. Thus, the maximum value before overflow is platform
+ * dependent. Current worst case scenario is about 110s. So, limit to 100s to be
+ * safe.
+ */
+#define GUC_POLICY_MAX_EXEC_QUANTUM_MS		(100 * 1000)
+#define GUC_POLICY_MAX_PREEMPT_TIMEOUT_MS	(100 * 1000)
+
 struct guc_policies {
 	u32 submission_queue_depth[GUC_MAX_ENGINE_CLASSES];
 	/* In micro seconds. How much time to allow before DPC processing is