Re: [PATCH i-g-t v2] igt/gem_pipe_control_store_loop: Add qword write tests

From: Chris Wilson <chris@chris-wilson.co.uk>
To: "Michał Winiarski" <michal.winiarski@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH i-g-t v2] igt/gem_pipe_control_store_loop: Add qword write tests
Date: Wed, 16 Mar 2016 13:15:57 +0000	[thread overview]
Message-ID: <20160316131557.GL14143@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1458132730-23540-1-git-send-email-michal.winiarski@intel.com>

On Wed, Mar 16, 2016 at 01:52:10PM +0100, Michał Winiarski wrote:
> Test description suggested that all platforms were testing qword writes,
> while in fact only gen4-gen5 did.
> 
> v2: Test dword/qword writes for all available platforms
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Michał Winiarski <michal.winiarski@intel.com>
> ---
>  tests/gem_pipe_control_store_loop.c | 49 +++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 16 deletions(-)
> 
> diff --git a/tests/gem_pipe_control_store_loop.c b/tests/gem_pipe_control_store_loop.c
> index a155ad1..cab3ed5 100644
> --- a/tests/gem_pipe_control_store_loop.c
> +++ b/tests/gem_pipe_control_store_loop.c
> @@ -26,7 +26,7 @@
>   */
>  
>  /*
> - * Testcase: (TLB-)Coherency of pipe_control QW writes
> + * Testcase: (TLB-)Coherency of pipe_control writes
>   *
>   * Writes a counter-value into an always newly allocated target bo (by disabling
>   * buffer reuse). Decently trashes on tlb inconsistencies, too.
> @@ -43,7 +43,7 @@
>  #include "drm.h"
>  #include "intel_bufmgr.h"
>  
> -IGT_TEST_DESCRIPTION("Test (TLB-)Coherency of pipe_control QW writes.");
> +IGT_TEST_DESCRIPTION("Test (TLB-)Coherency of pipe_control writes.");
>  
>  static drm_intel_bufmgr *bufmgr;
>  struct intel_batchbuffer *batch;
> @@ -60,13 +60,20 @@ uint32_t devid;
>  #define   PIPE_CONTROL_CS_STALL	(1<<20)
>  #define   PIPE_CONTROL_GLOBAL_GTT (1<<2) /* in addr dword */
>  
> +#define PIPE_CONTROL_STATE_BUFFER_REUSED	(1 << 0)
> +#define PIPE_CONTROL_STATE_QWORD_WRITE	        (1 << 1)
> +#define PIPE_CONTROL_STATE_ALL_FLAGS (PIPE_CONTROL_STATE_BUFFER_REUSED | \
> +				      PIPE_CONTROL_STATE_QWORD_WRITE)

Let's not use PIPE_CONTROL for test flags. That was very confusing!

> +
>  /* Like the store dword test, but we create new command buffers each time */
>  static void
> -store_pipe_control_loop(bool preuse_buffer)
> +store_pipe_control_loop(uint32_t flags)
>  {
>  	int i, val = 0;
>  	uint32_t *buf;
>  	drm_intel_bo *target_bo;
> +	const bool preuse_buffer = flags & PIPE_CONTROL_STATE_BUFFER_REUSED;
> +	const bool qword_write = flags & PIPE_CONTROL_STATE_QWORD_WRITE;
>  
>  	for (i = 0; i < SLOW_QUICK(0x10000, 4); i++) {
>  		/* we want to check tlb consistency of the pipe_control target,
> @@ -98,15 +105,16 @@ store_pipe_control_loop(bool preuse_buffer)
>  		 * creating new batchbuffers - with buffer reuse disabled, the
>  		 * support code will do that for us. */
>  		if (batch->gen >= 8) {
> -			BEGIN_BATCH(4, 1);
> -			OUT_BATCH(GFX_OP_PIPE_CONTROL + 1);
> +			BEGIN_BATCH(4 + qword_write, 1);
> +			OUT_BATCH(GFX_OP_PIPE_CONTROL + 1 + qword_write);
>  			OUT_BATCH(PIPE_CONTROL_WRITE_IMMEDIATE);
>  			OUT_RELOC_FENCED(target_bo,
>  			     I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  			     PIPE_CONTROL_GLOBAL_GTT);
>  			OUT_BATCH(val); /* write data */
> +			if (qword_write)
> +				OUT_BATCH(~val); /* high dword */
>  			ADVANCE_BATCH();

Let's put a MI_NOOP | 0xabcd here to catch if the HW is reading past the
end of the packet and writing the upper dword anyway.

> -
>  		} else if (batch->gen >= 6) {
>  			/* work-around hw issue, see intel_emit_post_sync_nonzero_flush
>  			 * in mesa sources. */
> @@ -118,24 +126,27 @@ store_pipe_control_loop(bool preuse_buffer)
>  			OUT_BATCH(0); /* write data */
>  			ADVANCE_BATCH();
>  
> -			BEGIN_BATCH(4, 1);
> -			OUT_BATCH(GFX_OP_PIPE_CONTROL);
> +			BEGIN_BATCH(4 + qword_write, 1);
> +			OUT_BATCH(GFX_OP_PIPE_CONTROL + qword_write);
>  			OUT_BATCH(PIPE_CONTROL_WRITE_IMMEDIATE);
>  			OUT_RELOC(target_bo,
>  			     I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 
>  			     PIPE_CONTROL_GLOBAL_GTT);
>  			OUT_BATCH(val); /* write data */
> +			if (qword_write)
> +				OUT_BATCH(~val); /* high dword */
>  			ADVANCE_BATCH();
>  		} else if (batch->gen >= 4) {
> -			BEGIN_BATCH(4, 1);
> +			BEGIN_BATCH(3 + qword_write, 1);
>  			OUT_BATCH(GFX_OP_PIPE_CONTROL | PIPE_CONTROL_WC_FLUSH |
>  					PIPE_CONTROL_TC_FLUSH |
> -					PIPE_CONTROL_WRITE_IMMEDIATE | 2);
> +					PIPE_CONTROL_WRITE_IMMEDIATE | (1 + qword_write));

Ideally we should remove the extra flushes that are not required for
emitting the write. Part of the test is that the kernel is emitting
adequate flushes for a standalone pipecontrol batch.

>  			OUT_RELOC(target_bo,
>  				I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION,
>  				PIPE_CONTROL_GLOBAL_GTT);

Fiddle sticks. Get rid of the libdrm layer so that I can control the
placement of the batch completely to double check that the kernel w/a
happens with the batch and not by random happenstance because of
intel_batchbuffer.c

>  			OUT_BATCH(val);
> -			OUT_BATCH(0xdeadbeef);
> +			if (qword_write)
> +				OUT_BATCH(~val); /* high dword */
>  			ADVANCE_BATCH();
>  		}
>  
> @@ -145,6 +156,8 @@ store_pipe_control_loop(bool preuse_buffer)
>  
>  		buf = target_bo->virtual;
>  		igt_assert(buf[0] == val);
> +		if (qword_write)
> +			igt_assert(buf[1] == ~val);

else
	igt_assert_eq_u32(buf[1], 0);

We also want to test CPU coherency, since these instructions are
advertising as being compatable with snooped writes.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx