[igt-dev] [PATCH i-g-t v1 1/1] gem_ctx_isolation.c - Gen11 enabling for context isolation test

[Dale B Stimson]

On Mon, Feb 18, 2019 at 01:36:26PM +0000, Chris Wilson wrote:
> Quoting Dale B Stimson via igt-dev (2019-02-15 21:31:53)
> > Anticipated for the future:
> > ...
> > Reduce memory requirements by avoiding sparse register-space arrays.
>
> Is that a real issue? Would you even care for doing anything other than
> finding the range of interest in each engine-set? Or even just
> subtracting off the engine->mmio_base.

Is it a real issue?  Maybe not.  I ask for your opinion on this.

Consider allocations done in function read_regs.
As far as I can see without drilling down too far into the gem object creation
process, the expanded address space (for the new mmio offsets and up to four
VCS engines) would change the allocation from around:
    user allocation   -- about 2MB
    kernel allocation -- about 3MB
to:
    user allocation   -- about 16MB
    kernel allocation -- about 24MB

Is it worthwhile doing anything about that?

    For reference, a code fragment from read_regs:
        reloc = calloc(NUM_REGS, sizeof(*reloc));
        igt_assert(reloc);

        regs_size = NUM_REGS * sizeof(uint32_t);
        regs_size = PAGE_ALIGN(regs_size);

        batch_size = NUM_REGS * 4 * sizeof(uint32_t) + 4;
        batch_size = PAGE_ALIGN(batch_size);

        memset(obj, 0, sizeof(obj));
        obj[0].handle = gem_create(fd, regs_size);
        obj[1].handle = gem_create(fd, batch_size);

...

You had several comments about the nonpriv_registers table, which I have
addressed and which will be shown in the next reroll.

> > +       { "VCS0_GPR", GEN11, VCS0, 0x1c0600, 32 },
> > +       { "VCS1_GPR", GEN11, VCS1, 0x1c4600, 32 },
> > +       { "VCS2_GPR", GEN11, VCS2, 0x1d0600, 32 },
> > +       { "VCS3_GPR", GEN11, VCS3, 0x1d4600, 32 },
> > +       { "VECS_GPR", GEN11, VECS0, 0x1c8600, 32 },
>
> Would we not want to keep this as part of per-engine sets? That makes
> more sense to me.

I agree, which is why I wrote in the commit message:

    Anticipated for the future:
    Take advantage of the new infrastructure to dynamically determine which
    engines are present and test the same register(s) in each of them.

Granted that something could be done about this without waiting for the new
query infrastructure.

I would prefer to do this as a separate patch to avoid complications and
further delays in getting this patch merged.

Are you aware of any plans to have any centralized data describing the MMIO
offsets for each engine?  If not, I would expect that I would propose doing
that in support of the per-engine handling.

Do you object to doing this part later?

> Part of me wants to add GEN11_xCSy_MMIO etc (and the other part wants
> that from the query iface).

I would like to see it from the query interface.  Single point of truth.
>
> Otherwise looks ok, and I just want someone else with an actual icl to
> confirm it does what it says on the tin.

My test output from an actual icl is appended below.  I will re-test with
the revised patch.

> I still think we should add a readback inside the test to confirm that
> writes to these registers are actually sticking before confirming that
> they are reset between ctx. That should have caught the issue with the
> previous patch, so a useful piece of sanitychecking.

I agree.  I'll do this and submit a separate patch.

I have prepared a revised version of this patch and am submitting it to
the list.

-Dale

================
Test output of 2019-02-13:

$ build/tests/gem_ctx_isolation
IGT-Version: 1.23-g4b08ac86 (x86_64) (Linux: 5.0.0-rc6-ci-ci-dii-1765+ x86_64)
Starting subtest: rcs0-clean
Subtest rcs0-clean: SUCCESS (0.034s)
Starting subtest: rcs0-dirty-create
Subtest rcs0-dirty-create: SUCCESS (0.274s)
Starting subtest: rcs0-dirty-switch
Subtest rcs0-dirty-switch: SUCCESS (0.311s)
Starting subtest: rcs0-none
Subtest rcs0-none: SUCCESS (0.207s)
Starting subtest: rcs0-reset
Subtest rcs0-reset: SUCCESS (0.209s)
Starting subtest: bcs0-clean
Subtest bcs0-clean: SUCCESS (0.064s)
Starting subtest: bcs0-dirty-create
Subtest bcs0-dirty-create: SUCCESS (0.265s)
Starting subtest: bcs0-dirty-switch
Subtest bcs0-dirty-switch: SUCCESS (0.287s)
Starting subtest: bcs0-none
Subtest bcs0-none: SUCCESS (0.238s)
Starting subtest: bcs0-reset
Subtest bcs0-reset: SUCCESS (0.210s)
Starting subtest: vcs0-clean
Subtest vcs0-clean: SUCCESS (0.061s)
Starting subtest: vcs0-dirty-create
Subtest vcs0-dirty-create: SUCCESS (0.277s)
Starting subtest: vcs0-dirty-switch
Subtest vcs0-dirty-switch: SUCCESS (0.292s)
Starting subtest: vcs0-none
Subtest vcs0-none: SUCCESS (0.227s)
Starting subtest: vcs0-reset
Subtest vcs0-reset: SUCCESS (0.190s)
Test requirement not met in function gem_require_engine, file ../lib/igt_gt.h:114:
Test requirement: gem_has_engine(gem_fd, class, instance)
Subtest vcs1-clean: SKIP
Subtest vcs1-dirty-create: SKIP
Subtest vcs1-dirty-switch: SKIP
Subtest vcs1-none: SKIP
Subtest vcs1-reset: SKIP
Starting subtest: vecs0-clean
Subtest vecs0-clean: SUCCESS (0.073s)
Starting subtest: vecs0-dirty-create
Subtest vecs0-dirty-create: SUCCESS (0.270s)
Starting subtest: vecs0-dirty-switch
Subtest vecs0-dirty-switch: SUCCESS (0.333s)
Starting subtest: vecs0-none
Subtest vecs0-none: SUCCESS (0.212s)
Starting subtest: vecs0-reset
Subtest vecs0-reset: SUCCESS (0.197s)