| Message ID | 20200228082330.2411941-18-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/24] drm/i915/gt: Check engine-is-awake on reset later | expand |
Chris Wilson <chris@chris-wilson.co.uk> writes: > As we require a context switch to ensure that the current context is > switched out and saved to memory, perform an explicit switch to the > kernel context and wait for it. The patch subject is not incorrect. Just feels that the kernel context is a patsy in here. So I would s/kernel// on subject but keep in commit msg > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/gt/selftest_lrc.c | 37 +++++++++++++++++++------- > 1 file changed, 28 insertions(+), 9 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c > index d7f98aada626..95da6b880e3f 100644 > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c > @@ -4015,6 +4015,31 @@ static int emit_semaphore_signal(struct intel_context *ce, void *slot) > return 0; > } > > +static int context_sync(struct intel_context *ce) > +{ > + struct i915_request *rq; > + struct dma_fence *fence; > + int err = 0; > + > + rq = intel_engine_create_kernel_request(ce->engine); > + if (IS_ERR(rq)) > + return PTR_ERR(rq); > + > + fence = i915_active_fence_get(&ce->timeline->last_request); > + if (fence) { > + i915_request_await_dma_fence(rq, fence); > + dma_fence_put(fence); > + } > + > + rq = i915_request_get(rq); > + i915_request_add(rq); > + if (i915_request_wait(rq, 0, HZ / 2) < 0) > + err = -ETIME; > + i915_request_put(rq); > + > + return err; > +} > + > static int live_lrc_layout(void *arg) > { > struct intel_gt *gt = arg; > @@ -4638,16 +4663,10 @@ static int __lrc_timestamp(const struct lrc_timestamp *arg, bool preempt) > wmb(); > } > > - if (i915_request_wait(rq, 0, HZ / 2) < 0) { > - err = -ETIME; > - goto err; > - } > - > - /* and wait for switch to kernel */ > - if (igt_flush_test(arg->engine->i915)) { > - err = -EIO; > + /* and wait for switch to kernel (to save our context to memory) */ > + err = context_sync(arg->ce[0]); > + if (err) > goto err; > - } > > rmb(); For me the context_sync could be context_flush and it would allow the rmb() to be snuck inside. But I seem to gravitate towards lower resolution and apparently you prefer to be more fine grained and explicit on callsites so, Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > -- > 2.25.1
Quoting Mika Kuoppala (2020-02-28 15:09:28) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > As we require a context switch to ensure that the current context is > > switched out and saved to memory, perform an explicit switch to the > > kernel context and wait for it. > > The patch subject is not incorrect. Just feels that the kernel > context is a patsy in here. > > So I would s/kernel// on subject but keep in commit msg > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/gt/selftest_lrc.c | 37 +++++++++++++++++++------- > > 1 file changed, 28 insertions(+), 9 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c > > index d7f98aada626..95da6b880e3f 100644 > > --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c > > +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c > > @@ -4015,6 +4015,31 @@ static int emit_semaphore_signal(struct intel_context *ce, void *slot) > > return 0; > > } > > > > +static int context_sync(struct intel_context *ce) > > +{ > > + struct i915_request *rq; > > + struct dma_fence *fence; > > + int err = 0; > > + > > + rq = intel_engine_create_kernel_request(ce->engine); > > + if (IS_ERR(rq)) > > + return PTR_ERR(rq); > > + > > + fence = i915_active_fence_get(&ce->timeline->last_request); > > + if (fence) { > > + i915_request_await_dma_fence(rq, fence); > > + dma_fence_put(fence); > > + } > > + > > + rq = i915_request_get(rq); > > + i915_request_add(rq); > > + if (i915_request_wait(rq, 0, HZ / 2) < 0) > > + err = -ETIME; > > + i915_request_put(rq); > > + > > + return err; > > +} > > + > > static int live_lrc_layout(void *arg) > > { > > struct intel_gt *gt = arg; > > @@ -4638,16 +4663,10 @@ static int __lrc_timestamp(const struct lrc_timestamp *arg, bool preempt) > > wmb(); > > } > > > > - if (i915_request_wait(rq, 0, HZ / 2) < 0) { > > - err = -ETIME; > > - goto err; > > - } > > - > > - /* and wait for switch to kernel */ > > - if (igt_flush_test(arg->engine->i915)) { > > - err = -EIO; > > + /* and wait for switch to kernel (to save our context to memory) */ > > + err = context_sync(arg->ce[0]); > > + if (err) > > goto err; > > - } > > > > rmb(); > > For me the context_sync could be context_flush and it would > allow the rmb() to be snuck inside. I hear you. The rmb() is really associated with the action of confirming the switch and can justifiably be inside the context_sync/flush. The rmb is just there to placate inner daemons, so I didn't think about it when forcing the emission of the kernel request. -Chris
diff --git a/drivers/gpu/drm/i915/gt/selftest_lrc.c b/drivers/gpu/drm/i915/gt/selftest_lrc.c index d7f98aada626..95da6b880e3f 100644 --- a/drivers/gpu/drm/i915/gt/selftest_lrc.c +++ b/drivers/gpu/drm/i915/gt/selftest_lrc.c @@ -4015,6 +4015,31 @@ static int emit_semaphore_signal(struct intel_context *ce, void *slot) return 0; } +static int context_sync(struct intel_context *ce) +{ + struct i915_request *rq; + struct dma_fence *fence; + int err = 0; + + rq = intel_engine_create_kernel_request(ce->engine); + if (IS_ERR(rq)) + return PTR_ERR(rq); + + fence = i915_active_fence_get(&ce->timeline->last_request); + if (fence) { + i915_request_await_dma_fence(rq, fence); + dma_fence_put(fence); + } + + rq = i915_request_get(rq); + i915_request_add(rq); + if (i915_request_wait(rq, 0, HZ / 2) < 0) + err = -ETIME; + i915_request_put(rq); + + return err; +} + static int live_lrc_layout(void *arg) { struct intel_gt *gt = arg; @@ -4638,16 +4663,10 @@ static int __lrc_timestamp(const struct lrc_timestamp *arg, bool preempt) wmb(); } - if (i915_request_wait(rq, 0, HZ / 2) < 0) { - err = -ETIME; - goto err; - } - - /* and wait for switch to kernel */ - if (igt_flush_test(arg->engine->i915)) { - err = -EIO; + /* and wait for switch to kernel (to save our context to memory) */ + err = context_sync(arg->ce[0]); + if (err) goto err; - } rmb();
As we require a context switch to ensure that the current context is switched out and saved to memory, perform an explicit switch to the kernel context and wait for it. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/gt/selftest_lrc.c | 37 +++++++++++++++++++------- 1 file changed, 28 insertions(+), 9 deletions(-)