Quoting Tvrtko Ursulin (2017-11-24 08:55:13)
>
> On 23/11/2017 08:22, Chris Wilson wrote:
> > Program the MI_WAIT_FOR_EVENT without reference to DERRMR by knowing its
> > state is ~0u when not in use, and is only in use when userspace requires
> > it. By not touching intel_regsiter_access we completely eliminate the
> > risk that we leak the forcewake ref, which can cause later rc6 to fail.
> >
> > At the same time, note that vlv/chv use a different mechanism (read
> > none) for coupling between the render engine and display.
> >
> > Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> > tests/perf_pmu.c | 63 ++++++++++++++++++++++++++++----------------------------
> > 1 file changed, 32 insertions(+), 31 deletions(-)
> >
> > diff --git a/tests/perf_pmu.c b/tests/perf_pmu.c
> > index 5118c023..7fd1506e 100644
> > --- a/tests/perf_pmu.c
> > +++ b/tests/perf_pmu.c
> > @@ -498,20 +498,22 @@ 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;
> > + const uint32_t FORCEWAKE_MT = 0xa188;
> > unsigned int valid_tests = 0;
> > - uint32_t batch[8], *b;
> > + uint32_t batch[16], *b;
> > + uint16_t devid;
> > igt_output_t *output;
> > - uint32_t bb_handle;
> > - uint32_t reg;
> > + data_t data;
> > 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);
> > + devid = intel_get_drm_devid(gem_fd);
> > + igt_require(intel_gen(devid) >= 7);
> > + igt_skip_on(IS_VALLEYVIEW(devid) || IS_CHERRYVIEW(devid));
> > +
> > + kmstest_set_vt_graphics_mode();
> > + igt_display_init(&data.display, gem_fd);
> >
> > /**
> > * We will use the display to render event forwarind so need to
> > @@ -522,51 +524,52 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> > * 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);
> > + obj.handle = gem_create(gem_fd, 4096);
> >
> > b = batch;
> > *b++ = MI_LOAD_REGISTER_IMM;
> > + *b++ = FORCEWAKE_MT;
> > + *b++ = 2 << 16 | 2;
> > + *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++ = ~0u;
> > + *b++ = MI_WAIT_FOR_EVENT;
> > *b++ = MI_LOAD_REGISTER_IMM;
> > *b++ = DERRMR;
> > - *b++ = reg;
> > + *b++ = ~0u;
> > + *b++ = MI_LOAD_REGISTER_IMM;
> > + *b++ = FORCEWAKE_MT;
> > + *b++ = 2 << 16;
> > *b++ = MI_BATCH_BUFFER_END;
> >
> > - obj.handle = bb_handle;
> > -
> > eb.buffer_count = 1;
> > eb.buffers_ptr = to_user_pointer(&obj);
> > eb.flags = e2ring(gem_fd, e) | I915_EXEC_SECURE;
> >
> > - for_each_pipe_with_valid_output(display, p, output) {
> > + for_each_pipe_with_valid_output(&data.display, p, output) {
> > struct igt_helper_process waiter = { };
> > const unsigned int frames = 3;
> > - unsigned int frame;
> > uint64_t val[2];
> >
> > - batch[3] = MI_WAIT_FOR_EVENT;
> > + batch[6] = MI_WAIT_FOR_EVENT;
> > switch (p) {
> > case PIPE_A:
> > - batch[3] |= MI_WAIT_FOR_PIPE_A_VBLANK;
> > + batch[6] |= MI_WAIT_FOR_PIPE_A_VBLANK;
> > + batch[5] = ~(1 << 3);
> > break;
> > case PIPE_B:
> > - batch[3] |= MI_WAIT_FOR_PIPE_B_VBLANK;
> > + batch[6] |= MI_WAIT_FOR_PIPE_B_VBLANK;
> > + batch[5] = ~(1 << 11);
> > break;
> > case PIPE_C:
> > - batch[3] |= MI_WAIT_FOR_PIPE_C_VBLANK;
> > + batch[6] |= MI_WAIT_FOR_PIPE_C_VBLANK;
> > + batch[5] = ~(1 << 21);
> > break;
> > default:
> > continue;
> > }
> >
> > - gem_write(gem_fd, bb_handle, 0, batch, sizeof(batch));
> > + gem_write(gem_fd, obj.handle, 0, batch, sizeof(batch));
> >
> > data.pipe = p;
> > prepare_crtc(&data, gem_fd, output);
> > @@ -589,9 +592,9 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> > }
> > }
> >
> > - for (frame = 0; frame < frames; frame++) {
> > + for (unsigned int frame = 0; frame < frames; frame++) {
> > gem_execbuf(gem_fd, &eb);
> > - gem_sync(gem_fd, bb_handle);
> > + gem_sync(gem_fd, obj.handle);
> > }
> >
> > igt_stop_helper(&waiter);
> > @@ -606,9 +609,7 @@ event_wait(int gem_fd, const struct intel_execution_engine2 *e)
> > igt_assert(val[1] - val[0] > 0);
> > }
> >
> > - gem_close(gem_fd, bb_handle);
> > -
> > - intel_register_access_fini();
> > + gem_close(gem_fd, obj.handle);
> >
> > igt_require_f(valid_tests,
> > "no valid crtc/connector combinations found\n");
> >
>
> It looks fine apart from the assumption that DERRMR always have to
> remain at default ~0. Worst I can imagine is that in the future we have
> to make this test force disconnect all displays, should some of the
> reserved bits be used for something which we will be turning on by default.
>
> But in that world sampling DERRMR value at test start is also only
> marginally better.
Added a comment that I am lazy, and pushed. Thanks,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
