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

[Tvrtko Ursulin]

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.

Anyway, we can worry about that if it happens.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>

Regards,

Tvrtko