Re: [PATCH i-g-t 7/9] tests/perf_pmu: Tests for i915 PMU API

Tvrtko Ursulin
2017-10-10T13:38:13Z

On 10/10/2017 13:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-10-10 10:30:06)
>> +static void
>> +event_wait(int gem_fd, const struct intel_execution_engine2 *e)
>> +{
>> + struct drm_i915_gem_exec_object2 obj = { };
>> + struct drm_i915_gem_execbuffer2 eb = { };
>> + data_t data;
>> + igt_display_t *display = &data.display;
>> + const uint32_t DERRMR = 0x44050;
>> + unsigned int valid_tests = 0;
>> + uint32_t batch[8], *b;
>> + igt_output_t *output;
>> + uint32_t bb_handle;
>> + uint32_t reg;
>> + enum pipe p;
>> + int fd;
>> +
>> + igt_require(intel_gen(intel_get_drm_devid(gem_fd)) >= 6);
>> + igt_require(intel_register_access_init(intel_get_pci_device(),
>> + false, gem_fd) == 0);
>> +
>> + /**
>> + * We will use the display to render event forwarind so need to
>> + * program the DERRMR register and restore it at exit.
>
> DERRMR is always masked until we need it. If you really wanted to
> preserve the old value, SRM, LRM around the test. Not that fussed, just
> a general dislike of direct poking of registers.

Thought I can get away with it since the handle is drm master and also
there doesn't seem to be a patch in i915 which touches this register.

>> + *
>> + * We will emit a MI_WAIT_FOR_EVENT listening for vblank events,
>> + * have a background helper to indirectly enable vblank irqs, and
>> + * listen to the recorded time spent in engine wait state as reported
>> + * by the PMU.
>> + */
>> + reg = intel_register_read(DERRMR);
>> +
>> + kmstest_set_vt_graphics_mode();
>> + igt_display_init(&data.display, gem_fd);
>> +
>> + bb_handle = gem_create(gem_fd, 4096);
>> +
>> + b = batch;
>> + *b++ = MI_LOAD_REGISTER_IMM;
>> + *b++ = DERRMR;
>> + *b++ = reg & ~((1 << 3) | (1 << 11) | (1 << 21));
>> + *b++ = MI_WAIT_FOR_EVENT | MI_WAIT_FOR_PIPE_A_VBLANK;
>> + *b++ = MI_LOAD_REGISTER_IMM;
>> + *b++ = DERRMR;
>> + *b++ = reg;
>> + *b++ = MI_BATCH_BUFFER_END;
>
>> +static int chain_nop(int gem_fd, unsigned long sz, int in_fence, bool sync)
>> +{
>> + struct drm_i915_gem_exec_object2 obj = {};
>> + struct drm_i915_gem_execbuffer2 eb =
>> + { .buffer_count = 1, .buffers_ptr = (uintptr_t)&obj};
>> + const uint32_t bbe = 0xa << 23;
>> +
>> + sz = ALIGN(sz, sizeof(uint32_t));
>> +
>> + obj.handle = gem_create(gem_fd, sz);
>> + gem_write(gem_fd, obj.handle, sz - sizeof(bbe), &bbe, sizeof(bbe));
>> +
>> + eb.flags = I915_EXEC_RENDER | I915_EXEC_FENCE_OUT;
>> +
>> + if (in_fence >= 0) {
>> + eb.flags |= I915_EXEC_FENCE_IN;
>> + eb.rsvd2 = in_fence;
>> + }
>> +
>> + gem_execbuf_wr(gem_fd, &eb);
>
> On the same ctx/engine, this shouldn't be generating interrupts between
> the batches. The fence should be resolved to an i915 request and then we
> see that the requests are naturally ordered so the fence is elided.
>> +
>> + if (sync)
>> + gem_sync(gem_fd, obj.handle);
>
> So it looks like this will remain the only interrupt generator.
>
> If we exported the out-fence and then polled that, that is currently
> hooked up to an interrupt only path.

So you think I'm counting ctx switches and other stuff? Maybe we should
have separate PMU counters for all the different interrupts. :)

What do you mean by exporting the out fence? Looping it through via
something external to hide the source? Not sure how to do it. Dup the
fd? Or create a swfence and merge the out fence to it? (Talking from
memory here.)

P.S.
I did miss to subtract the previous count before the assert.

Regards,

Tvrtko

>
>
> I don't have anything else to say! Afaict you have everything covered,
> so the only way to find what's not is by letting it go live!
>
> Give or take more tuning of the interrupt test,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>