Quoting Tvrtko Ursulin (2019-03-14 16:07:17)
> On 13/03/2019 14:43, Chris Wilson wrote:
> > In preparation to making the ppGTT binding for a context explicit (to
> > facilitate reusing the same ppGTT between different contexts), allow the
> > user to create and destroy named ppGTT.
> >
> > v2: Replace global barrier for swapping over the ppgtt and tlbs with a
> > local context barrier (Tvrtko)
> > v3: serialise with struct_mutex; it's lazy but required dammit
> > v4: Rewrite igt_ctx_shared_exec to be more different (aimed to be more
> > similarly, turned out different!)
> >
> > v2: Fix up test unwind for aliasing-ppgtt (snb)
> > v3: Tighten language for uapi struct drm_i915_gem_vm_control.
> > v4: Patch the context image for runtime ppgtt switching!
>
> -ECHANGELOGNOTMONOTONIC :))
I shall randomly delete half of them. Thanos rules.
> > +static void set_ppgtt_barrier(void *data)
> > +{
> > + struct i915_hw_ppgtt *old = data;
> > +
> > + if (INTEL_GEN(old->vm.i915) < 8)
> > + gen6_ppgtt_unpin_all(old);
>
> Is ppgtt sharing interesting for aliasing ppgtt? Ah HSW now has proper..
>
> > +
> > + i915_ppgtt_close(old);
> > + i915_ppgtt_put(old);
> > +}
> > +
> > +static int emit_ppgtt_update(struct i915_request *rq, void *data)
> > +{
> > + struct i915_hw_ppgtt *ppgtt = rq->gem_context->ppgtt;
> > + struct intel_engine_cs *engine = rq->engine;
> > + u32 *cs;
> > + int i;
> > +
> > + if (i915_vm_is_48bit(&ppgtt->vm)) {
> > + const dma_addr_t pd_daddr = px_dma(&ppgtt->pml4);
> > +
> > + cs = intel_ring_begin(rq, 6);
> > + if (IS_ERR(cs))
> > + return PTR_ERR(cs);
> > +
> > + *cs++ = MI_LOAD_REGISTER_IMM(2);
> > +
> > + *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(engine, 0));
> > + *cs++ = upper_32_bits(pd_daddr);
> > + *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_LDW(engine, 0));
> > + *cs++ = lower_32_bits(pd_daddr);
>
> Only one out of four these beasties needs to be updated? (In contrast to
> virtual_update_register_offsets.) As you can see I am not too familiar
> with pgtable management.
Our notes say that the HW ignores 1..3, so I was wondering if we could
condense these two into one with a bit of jiggery pokery to pluck the
right dma address. Might also simplify execlists_init_reg_state() that
tiny bit.
> > +
> > + *cs++ = MI_NOOP;
> > + intel_ring_advance(rq, cs);
> > + } else if (HAS_LOGICAL_RING_CONTEXTS(engine->i915)) {
> > + cs = intel_ring_begin(rq, 4 * GEN8_3LVL_PDPES + 2);
> > + if (IS_ERR(cs))
> > + return PTR_ERR(cs);
> > +
> > + *cs++ = MI_LOAD_REGISTER_IMM(2 * GEN8_3LVL_PDPES);
> > + for (i = GEN8_3LVL_PDPES; i--; ) {
> > + const dma_addr_t pd_daddr = i915_page_dir_dma_addr(ppgtt, i);
> > +
> > + *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_UDW(engine, i));
> > + *cs++ = upper_32_bits(pd_daddr);
> > + *cs++ = i915_mmio_reg_offset(GEN8_RING_PDP_LDW(engine, i));
> > + *cs++ = lower_32_bits(pd_daddr);
> > + }
> > + *cs++ = MI_NOOP;
> > + intel_ring_advance(rq, cs);
>
> So a running context will update it's own context image. And it doesn't
> get overwritten on context save?
Indeed. It is how we update PD at runtime.
> Are there tests which hit this path?
Yes. As always a serendipitous discovery. :)
The simplest of
gem_execbuf();
gem_context_set_param(NEW_VM);
gem_execbuf();
thankfully had a very recognisable GPU hang.
> > + } else {
> > + /* ppGTT is not part of the legacy context image */
> > + gen6_ppgtt_pin(ppgtt);
>
> What is happening on this path - why is the pin needed here only to be
> unpinned in the barrier callback? This is not handled by normal request
> flow?
It's because we pin the ppgtt as part of pinning the context (for
gen6/gen7). Since the context is already pinned, we don't notice the new
ctx->ppgtt. Hence we have to manually acquire a pin on ctx->ppgtt and
discard the old pins.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
