On Thu, Oct 05, 2017 at 03:00:31PM +0000, Michal Wajdeczko wrote:
> On Thu, 05 Oct 2017 11:20:04 +0200, Michał Winiarski
> <michal.winiarski@xxxxxxxxx> wrote:
>
> > Pretty similar to what we have on execlists.
> > We're reusing most of the GEM code, however, due to GuC quirks we need a
> > couple of extra bits.
> > Preemption is implemented as GuC action, and actions can be pretty slow.
> > Because of that, we're using a mutex to serialize them. Since we're
> > requesting preemption from the tasklet, the task of creating a workitem
> > and wrapping it in GuC action is delegated to a worker.
> >
> > To distinguish that preemption has finished, we're using additional
> > piece of HWSP, and since we're not getting context switch interrupts,
> > we're also adding a user interrupt.
> >
> > The fact that our special preempt context has completed unfortunately
> > doesn't mean that we're ready to submit new work. We also need to wait
> > for GuC to finish its own processing.
> >
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > Cc: Jeff Mcgee <jeff.mcgee@xxxxxxxxx>
> > Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> > Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Signed-off-by: Michał Winiarski <michal.winiarski@xxxxxxxxx>
> > ---
>
> <snip>
>
> > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c
> > b/drivers/gpu/drm/i915/i915_guc_submission.c
> > index 0f36bba9fc9e..21412ea3dc0e 100644
> > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > @@ -498,6 +498,88 @@ static void guc_add_request(struct intel_guc *guc,
> > spin_unlock(&client->wq_lock);
> > }
> > +#define GUC_PREEMPT_FINISHED 0x1
> > +#define GUC_PREEMPT_BREADCRUMB_DWORDS 0x8
> > +static void inject_preempt_context(struct work_struct *work)
> > +{
> > + struct intel_engine_cs *engine =
> > + container_of(work, typeof(*engine), guc_preempt_work);
> > + struct drm_i915_private *dev_priv = engine->i915;
> > + struct intel_guc *guc = &engine->i915->guc;
>
> guc = &dev_priv->guc;
>
> > + struct i915_guc_client *client = guc->preempt_client;
> > + struct intel_context *ce = &client->owner->engine[engine->id];
> > + u32 ctx_desc = lower_32_bits(intel_lr_context_descriptor(client->owner,
> > + engine));
> > + u32 *cs = ce->ring->vaddr + ce->ring->tail;
> > + u32 data[7];
> > +
> > + if (engine->id == RCS) {
> > + cs = gen8_emit_ggtt_write_render(
> > + intel_hws_preempt_done_address(engine),
> > + GUC_PREEMPT_FINISHED, cs);
> > + *cs++ = MI_USER_INTERRUPT;
> > + *cs++ = MI_NOOP;
> > + } else {
> > + cs = gen8_emit_ggtt_write(
> > + intel_hws_preempt_done_address(engine),
> > + GUC_PREEMPT_FINISHED, cs);
> > + *cs++ = MI_USER_INTERRUPT;
> > + *cs++ = MI_NOOP;
> > + *cs++ = MI_NOOP;
> > + *cs++ = MI_NOOP;
> > + }
>
> Maybe like this:
>
> cs = gen8_emit_ggtt_write_render(
> intel_hws_preempt_done_address(engine),
> GUC_PREEMPT_FINISHED, cs);
> *cs++ = MI_USER_INTERRUPT;
> *cs++ = MI_NOOP;
> if (engine->id != RCS) {
> *cs++ = MI_NOOP;
> *cs++ = MI_NOOP;
> }
Did you mean:
if (engine->id == RCS) {
cs = gen8_emit_ggtt_write_render()
} else {
cs = gen8_emit_ggtt_write()
*cs++ = MI_NOOP;
*cs++ = MI_NOOP;
}
*cs++ = MI_USER_INTERRUPT
*cs++ = MI_NOOP
?
[SNIP]
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h
> > b/drivers/gpu/drm/i915/intel_uc.h
> > index c6c6f8513bbf..8e2818e5741e 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -124,6 +124,9 @@ struct intel_guc {
> > /* Kernel context state ggtt offset, first page is shared with GuC */
> > u32 shared_data_offset;
> > + void *shared_data_addr;
> > +
> > + struct workqueue_struct *preempt_wq;
>
> Please pick better place (see my comment in 1/10)
I'll go with a helper for shared_data_* instead.
-Michał
>
> > /* GuC's FW specific send function */
> > int (*send)(struct intel_guc *guc, const u32 *data, u32 len);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
