Re: [PATCH 08/39] drm/i915: Create/destroy VM (ppGTT) for use with contexts

[Chris Wilson <chris@chris-wilson.co.uk>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx