Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Keep guc submission permanently engaged

[Chris Wilson]

Quoting Michal Wajdeczko (2018-05-17 18:23:23)
> On Thu, 17 May 2018 18:56:41 +0200, Chris Wilson  
>  wrote:
> 
> > We make a decision at module load whether to use the GuC backend or not,
> > but lose that setup across set-wedge. Currently, the guc doesn't
> > override the engine->set_default_submission hook letting execlists sneak
> > back in temporarily on unwedging leading to an unbalanced park/unpark.
> >
> > Testcase: igt/gem_eio
> > Signed-off-by: Chris Wilson 
> > Cc: Michał Winiarski 
> > Cc: Michal Wajdeczko 
> > ---
> >  drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++--
> >  drivers/gpu/drm/i915/intel_lrc.c|  2 +-
> >  drivers/gpu/drm/i915/intel_lrc.h|  2 ++
> >  3 files changed, 28 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c  
> > b/drivers/gpu/drm/i915/intel_guc_submission.c
> > index 637e852888ec..cbd8caffd271 100644
> > --- a/drivers/gpu/drm/i915/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/intel_guc_submission.c
> > @@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct  
> > intel_engine_cs *engine)
> >   intel_engine_pin_breadcrumbs_irq(engine);
> >  }
> > +static void guc_set_default_submission(struct intel_engine_cs *engine)
> > +{
> > + /*
> > +  * We inherit a bunch of functions from execlists that we'd like
> > +  * to keep using:
> > +  *
> > +  *engine->submit_request = execlists_submit_request;
> > +  *engine->cancel_requests = execlists_cancel_requests;
> > +  *engine->schedule = execlists_schedule;
> > +  *
> > +  * But we need to override the actual submission backend in order
> > +  * to talk to the GuC.
> > +  */
> > + execlists_set_default_submission(engine);
> > +
> > + engine->execlists.tasklet.func = guc_submission_tasklet;
> > +
> > + engine->park = guc_submission_park;
> > + engine->unpark = guc_submission_unpark;
> > +
> > + engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
> > +}
> > +
> >  int intel_guc_submission_enable(struct intel_guc *guc)
> >  {
> >   struct drm_i915_private *dev_priv = guc_to_i915(guc);
> > @@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc  
> > *guc)
> >   guc_interrupts_capture(dev_priv);
> >   for_each_engine(engine, dev_priv, id) {
> > - struct intel_engine_execlists * const execlists =
> > - >execlists;
> > -
> > - execlists->tasklet.func = guc_submission_tasklet;
> > -
> >   engine->reset.prepare = guc_reset_prepare;
> > + engine->set_default_submission = guc_set_default_submission;
> > - engine->park = guc_submission_park;
> > - engine->unpark = guc_submission_unpark;
> > -
> > - engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
> > + engine->set_default_submission(engine);
> 
> While this part looks ok (and maybe will help with my [1])
> but what about intel_guc_submission_disable(), where we
> will call intel_engines_reset_default_submission() and
> 'revert' to GuC again... time for nop_submission() ?

If we apply the "once you go GuC, you won't go back" rule, then we might
as well just call i915_gem_set_wedged() (or leave it wedged) on disabling.
So long as any re-enable path goes through a i915_reset, we will be
fine.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Keep guc submission permanently engaged

[Michal Wajdeczko]

On Thu, 17 May 2018 18:56:41 +0200, Chris Wilson  
 wrote:



We make a decision at module load whether to use the GuC backend or not,
but lose that setup across set-wedge. Currently, the guc doesn't
override the engine->set_default_submission hook letting execlists sneak
back in temporarily on unwedging leading to an unbalanced park/unpark.

Testcase: igt/gem_eio
Signed-off-by: Chris Wilson 
Cc: Michał Winiarski 
Cc: Michal Wajdeczko 
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++--
 drivers/gpu/drm/i915/intel_lrc.c|  2 +-
 drivers/gpu/drm/i915/intel_lrc.h|  2 ++
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c  
b/drivers/gpu/drm/i915/intel_guc_submission.c

index 637e852888ec..cbd8caffd271 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct  
intel_engine_cs *engine)

intel_engine_pin_breadcrumbs_irq(engine);
 }
+static void guc_set_default_submission(struct intel_engine_cs *engine)
+{
+   /*
+* We inherit a bunch of functions from execlists that we'd like
+* to keep using:
+*
+*engine->submit_request = execlists_submit_request;
+*engine->cancel_requests = execlists_cancel_requests;
+*engine->schedule = execlists_schedule;
+*
+* But we need to override the actual submission backend in order
+* to talk to the GuC.
+*/
+   execlists_set_default_submission(engine);
+
+   engine->execlists.tasklet.func = guc_submission_tasklet;
+
+   engine->park = guc_submission_park;
+   engine->unpark = guc_submission_unpark;
+
+   engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+}
+
 int intel_guc_submission_enable(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc  
*guc)

guc_interrupts_capture(dev_priv);
for_each_engine(engine, dev_priv, id) {
-   struct intel_engine_execlists * const execlists =
-   >execlists;
-
-   execlists->tasklet.func = guc_submission_tasklet;
-
engine->reset.prepare = guc_reset_prepare;
+   engine->set_default_submission = guc_set_default_submission;
-   engine->park = guc_submission_park;
-   engine->unpark = guc_submission_unpark;
-
-   engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+   engine->set_default_submission(engine);


While this part looks ok (and maybe will help with my [1])
but what about intel_guc_submission_disable(), where we
will call intel_engines_reset_default_submission() and
'revert' to GuC again... time for nop_submission() ?

Michal


[1] https://patchwork.freedesktop.org/series/41304/


}
return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c  
b/drivers/gpu/drm/i915/intel_lrc.c

index 646ecf267411..853fb0b5f73e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2291,7 +2291,7 @@ void intel_logical_ring_cleanup(struct  
intel_engine_cs *engine)

kfree(engine);
 }
-static void execlists_set_default_submission(struct intel_engine_cs  
*engine)

+void execlists_set_default_submission(struct intel_engine_cs *engine)
 {
engine->submit_request = execlists_submit_request;
engine->cancel_requests = execlists_cancel_requests;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h  
b/drivers/gpu/drm/i915/intel_lrc.h

index 4ec7d8dd13c8..e64f47e612f4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -111,4 +111,6 @@ intel_lr_context_descriptor(struct i915_gem_context  
*ctx,

return to_intel_context(ctx, engine)->lrc_desc;
 }
+void execlists_set_default_submission(struct intel_engine_cs *engine);
+
 #endif /* _INTEL_LRC_H_ */

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/2] drm/i915/guc: Keep guc submission permanently engaged

[Chris Wilson]

We make a decision at module load whether to use the GuC backend or not,
but lose that setup across set-wedge. Currently, the guc doesn't
override the engine->set_default_submission hook letting execlists sneak
back in temporarily on unwedging leading to an unbalanced park/unpark.

Testcase: igt/gem_eio
Signed-off-by: Chris Wilson 
Cc: Michał Winiarski 
Cc: Michal Wajdeczko 
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 34 +++--
 drivers/gpu/drm/i915/intel_lrc.c|  2 +-
 drivers/gpu/drm/i915/intel_lrc.h|  2 ++
 3 files changed, 28 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c 
b/drivers/gpu/drm/i915/intel_guc_submission.c
index 637e852888ec..cbd8caffd271 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -1264,6 +1264,29 @@ static void guc_submission_unpark(struct intel_engine_cs 
*engine)
intel_engine_pin_breadcrumbs_irq(engine);
 }
 
+static void guc_set_default_submission(struct intel_engine_cs *engine)
+{
+   /*
+* We inherit a bunch of functions from execlists that we'd like
+* to keep using:
+*
+*engine->submit_request = execlists_submit_request;
+*engine->cancel_requests = execlists_cancel_requests;
+*engine->schedule = execlists_schedule;
+*
+* But we need to override the actual submission backend in order
+* to talk to the GuC.
+*/
+   execlists_set_default_submission(engine);
+
+   engine->execlists.tasklet.func = guc_submission_tasklet;
+
+   engine->park = guc_submission_park;
+   engine->unpark = guc_submission_unpark;
+
+   engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+}
+
 int intel_guc_submission_enable(struct intel_guc *guc)
 {
struct drm_i915_private *dev_priv = guc_to_i915(guc);
@@ -1302,17 +1325,10 @@ int intel_guc_submission_enable(struct intel_guc *guc)
guc_interrupts_capture(dev_priv);
 
for_each_engine(engine, dev_priv, id) {
-   struct intel_engine_execlists * const execlists =
-   >execlists;
-
-   execlists->tasklet.func = guc_submission_tasklet;
-
engine->reset.prepare = guc_reset_prepare;
+   engine->set_default_submission = guc_set_default_submission;
 
-   engine->park = guc_submission_park;
-   engine->unpark = guc_submission_unpark;
-
-   engine->flags &= ~I915_ENGINE_SUPPORTS_STATS;
+   engine->set_default_submission(engine);
}
 
return 0;
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 646ecf267411..853fb0b5f73e 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -2291,7 +2291,7 @@ void intel_logical_ring_cleanup(struct intel_engine_cs 
*engine)
kfree(engine);
 }
 
-static void execlists_set_default_submission(struct intel_engine_cs *engine)
+void execlists_set_default_submission(struct intel_engine_cs *engine)
 {
engine->submit_request = execlists_submit_request;
engine->cancel_requests = execlists_cancel_requests;
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 4ec7d8dd13c8..e64f47e612f4 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -111,4 +111,6 @@ intel_lr_context_descriptor(struct i915_gem_context *ctx,
return to_intel_context(ctx, engine)->lrc_desc;
 }
 
+void execlists_set_default_submission(struct intel_engine_cs *engine);
+
 #endif /* _INTEL_LRC_H_ */
-- 
2.17.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx