[Intel-gfx] [PATCH igt 2/3] igt/perf_pmu: Stop peeking at intel_mmio registers

Chris Wilson chris at chris-wilson.co.uk
Fri Nov 24 10:47:22 UTC 2017


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 at chris-wilson.co.uk>
> > ---
> >   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


More information about the Intel-gfx mailing list