Re: [Intel-gfx] [PATCH 2/4] drm/i915/guc: Cancel requests immediately
On Wed, Jan 26, 2022 at 10:58:46AM -0800, John Harrison wrote:
> On 1/24/2022 07:01, Matthew Brost wrote:
> > Change the preemption timeout to the smallest possible value (1 us) when
> > disabling scheduling to cancel a request and restore it after
> > cancellation. This not only cancels the request as fast as possible, it
> > fixes a bug where the preemption timeout is 0 which results in the
> > schedule disable hanging forever.
> Shouldn't there be an 'if' in the above statement? The pre-emption timeout
> is not normally zero.
>
Yes. Will reword.
> >
> > Reported-by: Jani Saarinen
> > Fixes: 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation")
> > Link: https://gitlab.freedesktop.org/drm/intel/-/issues/4960
> > Signed-off-by: Matthew Brost
> > ---
> > drivers/gpu/drm/i915/gt/intel_context_types.h | 5 ++
> > .../gpu/drm/i915/gt/uc/intel_guc_submission.c | 46 +++
> > 2 files changed, 31 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > index 30cd81ad8911a..730998823dbea 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_context_types.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
> > @@ -198,6 +198,11 @@ struct intel_context {
> > * each priority bucket
> > */
> > u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
> > + /**
> > +* @preemption_timeout: preemption timeout of the context, used
> > +* to restore this value after request cancellation
> > +*/
> > + u32 preemption_timeout;
> > } guc_state;
> > struct {
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index 3918f1be114fa..966947c450253 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -2147,7 +2147,8 @@ static inline u32 get_children_join_value(struct
> > intel_context *ce,
> > return __get_parent_scratch(ce)->join[child_index].semaphore;
> > }
> > -static void guc_context_policy_init(struct intel_engine_cs *engine,
> > +static void guc_context_policy_init(struct intel_context *ce,
> > + struct intel_engine_cs *engine,
> > struct guc_lrc_desc *desc)
> Shouldn't engine be before ce? The more general structure usually goes
> first.
>
Sure. Fix fix this in the next rev.
Matt
> John.
>
> > {
> > desc->policy_flags = 0;
> > @@ -2157,7 +2158,8 @@ static void guc_context_policy_init(struct
> > intel_engine_cs *engine,
> > /* NB: For both of these, zero means disabled. */
> > desc->execution_quantum = engine->props.timeslice_duration_ms * 1000;
> > - desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
> > + ce->guc_state.preemption_timeout = engine->props.preempt_timeout_ms *
> > 1000;
> > + desc->preemption_timeout = ce->guc_state.preemption_timeout;
> > }
> > static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
> > @@ -2193,7 +2195,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
> > bool loop)
> > desc->hw_context_desc = ce->lrc.lrca;
> > desc->priority = ce->guc_state.prio;
> > desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> > - guc_context_policy_init(engine, desc);
> > + guc_context_policy_init(ce, engine, desc);
> > /*
> > * If context is a parent, we need to register a process descriptor
> > @@ -2226,7 +2228,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
> > bool loop)
> > desc->hw_context_desc = child->lrc.lrca;
> > desc->priority = ce->guc_state.prio;
> > desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
> > - guc_context_policy_init(engine, desc);
> > + guc_context_policy_init(child, engine, desc);
> > }
> > clear_children_join_go_memory(ce);
> > @@ -2409,6 +2411,19 @@ static u16 prep_context_pending_disable(struct
> > intel_context *ce)
> > return ce->guc_id.id;
> > }
> > +static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
> > +u16 guc_id,
> > +u32 preemption_timeout)
> > +{
> > + u32 action[] = {
> > + INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT,
> > + guc_id,
> > + preemption_timeout
> > + };
> > +
> > + intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
> > +}
> > +
> > static struct i915_sw_fence *guc_context_block(struct intel_context *ce)
> > {
> > struct intel_guc *guc = ce_to_guc(ce);
> > @@ -2442,8 +2457,10 @@ static struct i915_sw_fence
> > *guc_context_block(struct intel_context *ce)
> > spin_unlock_irqrestore(>guc_state.lock, flags);
> > -
Re: [Intel-gfx] [PATCH 2/4] drm/i915/guc: Cancel requests immediately
On 1/24/2022 07:01, Matthew Brost wrote:
Change the preemption timeout to the smallest possible value (1 us) when
disabling scheduling to cancel a request and restore it after
cancellation. This not only cancels the request as fast as possible, it
fixes a bug where the preemption timeout is 0 which results in the
schedule disable hanging forever.
Shouldn't there be an 'if' in the above statement? The pre-emption
timeout is not normally zero.
Reported-by: Jani Saarinen
Fixes: 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation")
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/4960
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_context_types.h | 5 ++
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 46 +++
2 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 30cd81ad8911a..730998823dbea 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -198,6 +198,11 @@ struct intel_context {
* each priority bucket
*/
u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
+ /**
+* @preemption_timeout: preemption timeout of the context, used
+* to restore this value after request cancellation
+*/
+ u32 preemption_timeout;
} guc_state;
struct {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 3918f1be114fa..966947c450253 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -2147,7 +2147,8 @@ static inline u32 get_children_join_value(struct
intel_context *ce,
return __get_parent_scratch(ce)->join[child_index].semaphore;
}
-static void guc_context_policy_init(struct intel_engine_cs *engine,
+static void guc_context_policy_init(struct intel_context *ce,
+ struct intel_engine_cs *engine,
struct guc_lrc_desc *desc)
Shouldn't engine be before ce? The more general structure usually goes
first.
John.
{
desc->policy_flags = 0;
@@ -2157,7 +2158,8 @@ static void guc_context_policy_init(struct
intel_engine_cs *engine,
/* NB: For both of these, zero means disabled. */
desc->execution_quantum = engine->props.timeslice_duration_ms * 1000;
- desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
+ ce->guc_state.preemption_timeout = engine->props.preempt_timeout_ms *
1000;
+ desc->preemption_timeout = ce->guc_state.preemption_timeout;
}
static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
@@ -2193,7 +2195,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
bool loop)
desc->hw_context_desc = ce->lrc.lrca;
desc->priority = ce->guc_state.prio;
desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
- guc_context_policy_init(engine, desc);
+ guc_context_policy_init(ce, engine, desc);
/*
* If context is a parent, we need to register a process descriptor
@@ -2226,7 +2228,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
bool loop)
desc->hw_context_desc = child->lrc.lrca;
desc->priority = ce->guc_state.prio;
desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
- guc_context_policy_init(engine, desc);
+ guc_context_policy_init(child, engine, desc);
}
clear_children_join_go_memory(ce);
@@ -2409,6 +2411,19 @@ static u16 prep_context_pending_disable(struct
intel_context *ce)
return ce->guc_id.id;
}
+static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
+u16 guc_id,
+u32 preemption_timeout)
+{
+ u32 action[] = {
+ INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT,
+ guc_id,
+ preemption_timeout
+ };
+
+ intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
+}
+
static struct i915_sw_fence *guc_context_block(struct intel_context *ce)
{
struct intel_guc *guc = ce_to_guc(ce);
@@ -2442,8 +2457,10 @@ static struct i915_sw_fence *guc_context_block(struct
intel_context *ce)
spin_unlock_irqrestore(>guc_state.lock, flags);
- with_intel_runtime_pm(runtime_pm, wakeref)
+ with_intel_runtime_pm(runtime_pm, wakeref) {
+ __guc_context_set_preemption_timeout(guc, guc_id, 1);
__guc_context_sched_disable(guc, ce, guc_id);
+ }
return >guc_state.blocked;
}
@@ -2492,8 +2509,10 @@ static void guc_context_unblock(struct intel_context *ce)
[Intel-gfx] [PATCH 2/4] drm/i915/guc: Cancel requests immediately
Change the preemption timeout to the smallest possible value (1 us) when
disabling scheduling to cancel a request and restore it after
cancellation. This not only cancels the request as fast as possible, it
fixes a bug where the preemption timeout is 0 which results in the
schedule disable hanging forever.
Reported-by: Jani Saarinen
Fixes: 62eaf0ae217d4 ("drm/i915/guc: Support request cancellation")
Link: https://gitlab.freedesktop.org/drm/intel/-/issues/4960
Signed-off-by: Matthew Brost
---
drivers/gpu/drm/i915/gt/intel_context_types.h | 5 ++
.../gpu/drm/i915/gt/uc/intel_guc_submission.c | 46 +++
2 files changed, 31 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/gt/intel_context_types.h
b/drivers/gpu/drm/i915/gt/intel_context_types.h
index 30cd81ad8911a..730998823dbea 100644
--- a/drivers/gpu/drm/i915/gt/intel_context_types.h
+++ b/drivers/gpu/drm/i915/gt/intel_context_types.h
@@ -198,6 +198,11 @@ struct intel_context {
* each priority bucket
*/
u32 prio_count[GUC_CLIENT_PRIORITY_NUM];
+ /**
+* @preemption_timeout: preemption timeout of the context, used
+* to restore this value after request cancellation
+*/
+ u32 preemption_timeout;
} guc_state;
struct {
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
index 3918f1be114fa..966947c450253 100644
--- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
@@ -2147,7 +2147,8 @@ static inline u32 get_children_join_value(struct
intel_context *ce,
return __get_parent_scratch(ce)->join[child_index].semaphore;
}
-static void guc_context_policy_init(struct intel_engine_cs *engine,
+static void guc_context_policy_init(struct intel_context *ce,
+ struct intel_engine_cs *engine,
struct guc_lrc_desc *desc)
{
desc->policy_flags = 0;
@@ -2157,7 +2158,8 @@ static void guc_context_policy_init(struct
intel_engine_cs *engine,
/* NB: For both of these, zero means disabled. */
desc->execution_quantum = engine->props.timeslice_duration_ms * 1000;
- desc->preemption_timeout = engine->props.preempt_timeout_ms * 1000;
+ ce->guc_state.preemption_timeout = engine->props.preempt_timeout_ms *
1000;
+ desc->preemption_timeout = ce->guc_state.preemption_timeout;
}
static int guc_lrc_desc_pin(struct intel_context *ce, bool loop)
@@ -2193,7 +2195,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
bool loop)
desc->hw_context_desc = ce->lrc.lrca;
desc->priority = ce->guc_state.prio;
desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
- guc_context_policy_init(engine, desc);
+ guc_context_policy_init(ce, engine, desc);
/*
* If context is a parent, we need to register a process descriptor
@@ -2226,7 +2228,7 @@ static int guc_lrc_desc_pin(struct intel_context *ce,
bool loop)
desc->hw_context_desc = child->lrc.lrca;
desc->priority = ce->guc_state.prio;
desc->context_flags = CONTEXT_REGISTRATION_FLAG_KMD;
- guc_context_policy_init(engine, desc);
+ guc_context_policy_init(child, engine, desc);
}
clear_children_join_go_memory(ce);
@@ -2409,6 +2411,19 @@ static u16 prep_context_pending_disable(struct
intel_context *ce)
return ce->guc_id.id;
}
+static void __guc_context_set_preemption_timeout(struct intel_guc *guc,
+u16 guc_id,
+u32 preemption_timeout)
+{
+ u32 action[] = {
+ INTEL_GUC_ACTION_SET_CONTEXT_PREEMPTION_TIMEOUT,
+ guc_id,
+ preemption_timeout
+ };
+
+ intel_guc_send_busy_loop(guc, action, ARRAY_SIZE(action), 0, true);
+}
+
static struct i915_sw_fence *guc_context_block(struct intel_context *ce)
{
struct intel_guc *guc = ce_to_guc(ce);
@@ -2442,8 +2457,10 @@ static struct i915_sw_fence *guc_context_block(struct
intel_context *ce)
spin_unlock_irqrestore(>guc_state.lock, flags);
- with_intel_runtime_pm(runtime_pm, wakeref)
+ with_intel_runtime_pm(runtime_pm, wakeref) {
+ __guc_context_set_preemption_timeout(guc, guc_id, 1);
__guc_context_sched_disable(guc, ce, guc_id);
+ }
return >guc_state.blocked;
}
@@ -2492,8 +2509,10 @@ static void guc_context_unblock(struct intel_context *ce)
spin_unlock_irqrestore(>guc_state.lock, flags);
- if (enable) {
- with_intel_runtime_pm(runtime_pm, wakeref)
+ with_intel_runtime_pm(runtime_pm, wakeref) {
+
