[PATCH i-g-t v3 0/6] Scheduler tests

[PATCH i-g-t v3 0/6] Scheduler tests

[Derek Morton]

This patch set adds scheduler tests.
Patch 1 Makes gem_has_ring() non static as the test will need to call it
Patch 2 adds library code used by the tests. There are other tests under
development which are planned to reuse some of these libraries.
Patch 3 adds some basic tests, read dependency tests and write dependency tests.
Patch 4 Is the patch previously submitted by John Harrison to update
gem_ctx_param_basic with ioctls to set context priorities. It is included as
part of this patch set as Patch 6 & 7 are dependant on it.
Patch 5 adds subtests to check sheduler behaviour for batch buffers submitted at
differing priorities.
Patch 6 adds subtests to check priority bumping behaviour.

v2: Updates for comments from Daniele Ceraolo Spurio and Daniel Vetter
    Added tests requested by Joonas Lahtinen

v3: Removed the patch to update gem_has_ring() and updated the test to
    use BSD if there is 1 ring and BSD1 / BSD2 if there are two.

Derek Morton (5):
  ioctl_wrappers: make gem_has_ring non static
  lib/intel_batchbuffer: Add functions to be used in the scheduler test
  tests/gem_scheduler: Add gem_scheduler test
  tests/gem_scheduler: Add subtests to test batch priority behaviour
  gem_scheduler: Added subtests to test priority bumping

John Harrison (1):
  igt/gem_ctx_param_basic: Updated to support scheduler priority
    interface

 lib/intel_batchbuffer.c     | 384 ++++++++++++++++++++++++++-
 lib/intel_batchbuffer.h     |  14 +
 lib/ioctl_wrappers.c        |   2 +-
 lib/ioctl_wrappers.h        |   2 +
 tests/Makefile.sources      |   1 +
 tests/gem_ctx_param_basic.c |  34 ++-
 tests/gem_scheduler.c       | 630 ++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 1060 insertions(+), 7 deletions(-)
 create mode 100644 tests/gem_scheduler.c

-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 1/6] ioctl_wrappers: make gem_has_ring non static

[Derek Morton]

For tests that use multiple rings to test interactions it is
useful to know if a ring exists without triggering the test to skip.

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 lib/ioctl_wrappers.c | 2 +-
 lib/ioctl_wrappers.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
index 5d49729..f42e2c9 100644
--- a/lib/ioctl_wrappers.c
+++ b/lib/ioctl_wrappers.c
@@ -1396,7 +1396,7 @@ void gem_require_caching(int fd)
 	errno = 0;
 }
 
-static int gem_has_ring(int fd, int ring)
+int gem_has_ring(int fd, int ring)
 {
 	uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_execbuffer2 execbuf;
diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index f59eafb..e095c41 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -150,6 +150,7 @@ uint64_t gem_global_aperture_size(int fd);
 uint64_t gem_mappable_aperture_size(void);
 bool gem_has_softpin(int fd);
 
+int gem_has_ring(int fd, int ring);
 /* check functions which auto-skip tests by calling igt_skip() */
 void gem_require_caching(int fd);
 void gem_require_ring(int fd, int ring_id);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 1/6] ioctl_wrappers: make gem_has_ring non static

[Daniele Ceraolo Spurio]

On 10/03/16 11:03, Derek Morton wrote:
> For tests that use multiple rings to test interactions it is
> useful to know if a ring exists without triggering the test to skip.
>
> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
> ---
>   lib/ioctl_wrappers.c | 2 +-
>   lib/ioctl_wrappers.h | 1 +
>   2 files changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c
> index 5d49729..f42e2c9 100644
> --- a/lib/ioctl_wrappers.c
> +++ b/lib/ioctl_wrappers.c
> @@ -1396,7 +1396,7 @@ void gem_require_caching(int fd)
>   	errno = 0;
>   }
>   
> -static int gem_has_ring(int fd, int ring)
> +int gem_has_ring(int fd, int ring)

If this becomes non-static it will need documentation, also to explain 
how it differs from gem_has_enable_ring.

Regards,
Daniele

>   {
>   	uint32_t bbe = MI_BATCH_BUFFER_END;
>   	struct drm_i915_gem_execbuffer2 execbuf;
> diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
> index f59eafb..e095c41 100644
> --- a/lib/ioctl_wrappers.h
> +++ b/lib/ioctl_wrappers.h
> @@ -150,6 +150,7 @@ uint64_t gem_global_aperture_size(int fd);
>   uint64_t gem_mappable_aperture_size(void);
>   bool gem_has_softpin(int fd);
>   
> +int gem_has_ring(int fd, int ring);
>   /* check functions which auto-skip tests by calling igt_skip() */
>   void gem_require_caching(int fd);
>   void gem_require_ring(int fd, int ring_id);

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 1/6] ioctl_wrappers: make gem_has_ring non static

[Morton, Derek J]

>
>-----Original Message-----
>From: Ceraolo Spurio, Daniele 
>Sent: Monday, March 14, 2016 12:30 PM
>To: Morton, Derek J <derek.j.morton@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [PATCH i-g-t v3 1/6] ioctl_wrappers: make gem_has_ring non static
>
>
>
>On 10/03/16 11:03, Derek Morton wrote:
>> For tests that use multiple rings to test interactions it is useful to 
>> know if a ring exists without triggering the test to skip.
>>
>> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
>> ---
>>   lib/ioctl_wrappers.c | 2 +-
>>   lib/ioctl_wrappers.h | 1 +
>>   2 files changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 
>> 5d49729..f42e2c9 100644
>> --- a/lib/ioctl_wrappers.c
>> +++ b/lib/ioctl_wrappers.c
>> @@ -1396,7 +1396,7 @@ void gem_require_caching(int fd)
>>   	errno = 0;
>>   }
>>   
>> -static int gem_has_ring(int fd, int ring)
>> +int gem_has_ring(int fd, int ring)
>
>If this becomes non-static it will need documentation, also to explain how it differs from gem_has_enable_ring.

This patch is no longer needed. Chris Wilson has separately pushed a patch which makes this function non static. Chase him for the documentation  :-)

//Derek

>
>Regards,
>Daniele
>
>>   {
>>   	uint32_t bbe = MI_BATCH_BUFFER_END;
>>   	struct drm_i915_gem_execbuffer2 execbuf; diff --git 
>> a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index f59eafb..e095c41 
>> 100644
>> --- a/lib/ioctl_wrappers.h
>> +++ b/lib/ioctl_wrappers.h
>> @@ -150,6 +150,7 @@ uint64_t gem_global_aperture_size(int fd);
>>   uint64_t gem_mappable_aperture_size(void);
>>   bool gem_has_softpin(int fd);
>>   
>> +int gem_has_ring(int fd, int ring);
>>   /* check functions which auto-skip tests by calling igt_skip() */
>>   void gem_require_caching(int fd);
>>   void gem_require_ring(int fd, int ring_id);
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 2/6] lib/intel_batchbuffer: Add functions to be used in the scheduler test

[Derek Morton]

Adds functions to create a number of different batch buffers to perform
several functions including:
Batch buffer which will run for a long duration to provide a delay on a
specified ring.
Function to calibrate the delay batch buffer to run for a specified period
of time.
Function to create a batch buffer which writes timestamps to a buffer object.
Function to compare timestamps allowing for wrapping of the values.

v2: Moved code to intel_batchbuffer (Daniel Vetter)
    Addressed review comments from Daniele Ceraolo Spurio

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 lib/intel_batchbuffer.c | 384 +++++++++++++++++++++++++++++++++++++++++++++++-
 lib/intel_batchbuffer.h |  14 ++
 2 files changed, 393 insertions(+), 5 deletions(-)

diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
index 692521f..30e78c5 100644
--- a/lib/intel_batchbuffer.c
+++ b/lib/intel_batchbuffer.c
@@ -1,8 +1,8 @@
 /**************************************************************************
- * 
+ *
  * Copyright 2006 Tungsten Graphics, Inc., Cedar Park, Texas.
  * All Rights Reserved.
- * 
+ *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the
  * "Software"), to deal in the Software without restriction, including
@@ -10,11 +10,11 @@
  * distribute, sub license, and/or sell copies of the Software, and to
  * permit persons to whom the Software is furnished to do so, subject to
  * the following conditions:
- * 
+ *
  * The above copyright notice and this permission notice (including the
  * next paragraph) shall be included in all copies or substantial portions
  * of the Software.
- * 
+ *
  * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
  * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
  * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
@@ -22,7 +22,7 @@
  * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
  * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
  * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
- * 
+ *
  **************************************************************************/
 
 #include <inttypes.h>
@@ -30,6 +30,8 @@
 #include <stdio.h>
 #include <string.h>
 #include <assert.h>
+#include <stdint.h>
+#include <time.h>
 
 #include "drm.h"
 #include "drmtest.h"
@@ -42,6 +44,7 @@
 #include "ioctl_wrappers.h"
 #include "media_spin.h"
 #include "gpgpu_fill.h"
+#include "igt_gt.h"
 
 #include <i915_drm.h>
 
@@ -817,3 +820,374 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int devid)
 
 	return spin;
 }
+
+#define SEC_TO_NSEC (1000 * 1000 * 1000)
+#define DWORDS_TO_BYTES(x) ((x)*4)
+
+#define MI_STORE_REGISTER_MEM(LENGTH)   ((0x024 << 23) | ((LENGTH - 2) & 0xff))
+#define MI_MATH(NrInst)                 ((0x01A << 23) | ((NrInst - 1) & 0x3f))
+#define MI_CONDITIONAL_BATCH_BUFFER_END ((0x036 << 23) | (1 << 21) | 2)
+#define MI_COPY_MEM_MEM                 ((0x02E << 23) | (3))
+
+#define ALU_LOAD(TO, FROM)  ((0x080 << 20) | ((TO) << 10) | (FROM))
+#define ALU_SUB             ( 0x101 << 20)
+#define ALU_STORE(TO, FROM) ((0x180 << 20) | ((TO) << 10) | (FROM))
+
+#define TIMESTAMP_offset      (0x358) /* Elapsed time from system start */
+#define CTX_TIMESTAMP_offset  (0x3A8) /* Elapsed Time from context creation */
+#define ALU_GPU_R0_LSB_offset (0x600)
+#define ALU_GPU_R0_MSB_offset (0x604)
+#define ALU_GPU_R1_LSB_offset (0x608)
+#define ALU_GPU_R1_MSB_offset (0x60C)
+#define ALU_GPU_R2_LSB_offset (0x610)
+#define ALU_GPU_R2_MSB_offset (0x614)
+
+#define ALU_R0_ENCODING   (0x00)
+#define ALU_R1_ENCODING   (0x01)
+#define ALU_SRCA_ENCODING (0x20)
+#define ALU_SRCB_ENCODING (0x21)
+#define ALU_ACCU_ENCODING (0x31)
+
+static int bb_address_size_dw(int fd)
+{
+	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
+		return 2;
+	else
+		return 1;
+}
+
+static uint32_t get_mmio_base(int ringid)
+{
+	switch (ringid) {
+	case I915_EXEC_RENDER:
+		return 0x02000;
+	case I915_EXEC_BSD:
+	case I915_EXEC_BSD | 1<<13: /* BSD1 */
+		return 0x12000;
+	case I915_EXEC_BSD | 2<<13: /* BSD2 */
+		return 0x1c000;
+	case I915_EXEC_BLT:
+		return 0x22000;
+	case I915_EXEC_VEBOX:
+		return 0x1A000;
+	default:
+		igt_assert_f(0, "Invalid ringid %d passed to get_mmio_base()\n", ringid);
+	}
+}
+
+/**
+ * igt_batch_used
+ * @batch batchbuffer to get offset from
+ *
+ * This returns the number of bytes of the batchbuffer that have been used.
+ * e.g. The offset into the batchbuffer that the next OUT_BATCH would write to.
+ *
+ * Returns:
+ * The number of bytes of the batchbuffer that have been used.
+ */
+uint32_t igt_batch_used(struct intel_batchbuffer *batch)
+{
+	return batch->ptr - batch->buffer;
+}
+
+/**
+ * igt_create_delay_bb:
+ * @fd: file descriptor for i915 driver instance
+ * @batch: Batch buffer to write to
+ * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER
+ * loops: Number of times to loop
+ * dest: Buffer to use for saving the current loop count and timestamp.
+ *
+ * This creates a batch buffer which will iterate a loop a specified number
+ * of times. Intended for creating batch buffers which take an arbitarily
+ * long time to execute. This can be useful to keep a ring busy while
+ * constructing a test scenario.
+ *
+ * The dest buffer will have a number of Dwords written by the batch buffer
+ * when it runs. They are:
+ * DW0 & DW1 - These are loaded with the value of 'loops' and are decremented
+ *             as the batch buffer executes. They will be 0 after the batch
+ *             buffer completes if it finished succesfully.
+ * DW2 Timestamp - An indication of when the batch buffer ran allowing a
+ *                 comparison between batch buffers to show execution order.
+ *                 May wrap so igt_compare_timestamps() should be used to
+ *                 compare timestamps.
+ *                 The timestamp will wrap every few minutes.
+ *
+ */
+void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,
+                         int ringid, uint32_t loops, drm_intel_bo *dest)
+{
+	int addr_size_dw;
+	uint32_t mmio_base, jump_offset;
+
+	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
+	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
+
+	addr_size_dw = bb_address_size_dw(fd);
+	mmio_base = get_mmio_base(ringid);
+	igt_assert(batch);
+	BEGIN_BATCH(32, 5);
+
+	/* store current timestamp in DW2 */
+	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
+	OUT_BATCH(mmio_base + TIMESTAMP_offset);
+	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
+
+	/* Load R0 with loops */
+	OUT_BATCH(MI_LOAD_REGISTER_IMM);
+	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
+	OUT_BATCH(loops);
+	OUT_BATCH(MI_LOAD_REGISTER_IMM);
+	OUT_BATCH(mmio_base + ALU_GPU_R0_MSB_offset);
+	OUT_BATCH(0x00000000);
+	/* Load R1 with 1 */
+	OUT_BATCH(MI_LOAD_REGISTER_IMM);
+	OUT_BATCH(mmio_base + ALU_GPU_R1_LSB_offset);
+	OUT_BATCH(0x00000001);
+	OUT_BATCH(MI_LOAD_REGISTER_IMM);
+	OUT_BATCH(mmio_base + ALU_GPU_R1_MSB_offset);
+	OUT_BATCH(0x00000000);
+	/* Copy R0 / R1 into SRCA / SRCB, Perform R0 - R1, Store result in R0 */
+	/* e.g. R0 -= 1 */
+	jump_offset=igt_batch_used(batch);
+	OUT_BATCH(MI_MATH(4));
+	OUT_BATCH(ALU_LOAD(ALU_SRCA_ENCODING, ALU_R0_ENCODING));
+	OUT_BATCH(ALU_LOAD(ALU_SRCB_ENCODING, ALU_R1_ENCODING));
+	OUT_BATCH(ALU_SUB);
+	OUT_BATCH(ALU_STORE(ALU_R0_ENCODING, ALU_ACCU_ENCODING));
+	/* Copy R0 to dest
+	 * On Gen8 MI_CONDITIONAL_BATCH_BUFFER_END BSD ring Compare address
+	 * points to 2 Dwords, a mask (DW0) and data (DW1) which are ANDed
+	 * together.
+	 * On Gen9+, and the other rings on Gen8 Compare address points to
+	 * just Data (DW0). For simplicity always copy R0 LSB to DW0 and DW1.
+	 */
+	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
+	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
+	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
+	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
+	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
+	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(1));
+	/* Repeat until R0 == 0 */
+	OUT_BATCH(MI_CONDITIONAL_BATCH_BUFFER_END);
+	OUT_BATCH(0x00000000);
+	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
+	OUT_BATCH(MI_BATCH_BUFFER_START | (addr_size_dw - 1));
+	OUT_RELOC(batch->bo, I915_GEM_DOMAIN_INSTRUCTION, 0, jump_offset);
+
+	/* Should never get here, but end if it happens */
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	ADVANCE_BATCH();
+}
+
+/**
+ * igt_create_timestamp_bb:
+ * @fd: file descriptor for i915 driver instance
+ * @batch: Batch buffer to write to
+ * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER
+ * dest: Buffer to use for saving the timestamps.
+ * load: Buffer to access. Set NULL if not required.
+ * write: If true and load is not NULL, will also write a timestamp to load
+ * buffer. If false and load is not NULL, will read from load buffer into dest.
+ * Intended for dependency checking.
+ *
+ * This creates a batch buffer which writes timestamps into a buffer object.
+ * If 'load' is non null, data is either written to 'load' or copied from 'load'
+ * depending on whether 'write' is set.
+ *
+ * The dest buffer will have a number of Dwords written by the batch buffer
+ * when it runs. They are:
+ * DW0 Reported timestamp - An indication of when the batch buffer ran allowing a
+ *                          comparison between batch buffers to show execution order.
+ *                          May wrap so igt_compare_timestamps() should be used to
+ *                          compare timestamps.
+ *                          The timestamp will wrap every few minutes.
+ * DW2 Value copied from DW0 of load if write == false
+ *
+ */
+void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
+                             drm_intel_bo *dest, drm_intel_bo *load, bool write)
+{
+	int addr_size_dw;
+	uint32_t mmio_base;
+
+	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
+	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
+
+	addr_size_dw = bb_address_size_dw(fd);
+	mmio_base = get_mmio_base(ringid);
+	igt_assert(batch);
+
+	BEGIN_BATCH(3, 1);
+	/* store current reported timestamp in DW0 */
+	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
+	OUT_BATCH(mmio_base + TIMESTAMP_offset);
+	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
+
+	ADVANCE_BATCH();
+
+	if(load != NULL) {
+		if(write) {
+			BEGIN_BATCH(3, 1);
+			OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
+			OUT_BATCH(mmio_base + TIMESTAMP_offset);
+			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
+			ADVANCE_BATCH();
+		}
+		else {
+			BEGIN_BATCH(3, 2);
+			OUT_BATCH(MI_COPY_MEM_MEM);
+			OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
+			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, 0, DWORDS_TO_BYTES(0));
+			ADVANCE_BATCH();
+		}
+	}
+
+	BEGIN_BATCH(1, 0);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	ADVANCE_BATCH();
+}
+
+/**
+ * igt_create_noop_bb:
+ * @batch: Batch buffer to write to
+ * noops: Number of MI_NOOP instructions to add to the batch buffer.
+ *
+ * This creates a batch buffer with a specified number of MI_NOOP instructions.
+ */
+void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops)
+{
+	int loop;
+
+	igt_assert(batch);
+
+	BEGIN_BATCH(noops + 1, 0);
+	for(loop = 0; loop < noops; loop++)
+		OUT_BATCH(MI_NOOP);
+	OUT_BATCH(MI_BATCH_BUFFER_END);
+	ADVANCE_BATCH();
+
+}
+
+/* Store calibrated values so they only need calculating once.
+ * Use intel_execution_engines array as list of supported rings
+ */
+static uint32_t *calibrated_ring_value = NULL;
+
+/**
+ * igt_calibrate_delay_bb:
+ * @fd: file descriptor for i915 driver instance
+ * @bufmgr: Buffer manager to be used for creation of batch buffers
+ * ringid: Ring to calibrate. e.g. I915_EXEC_RENDER
+ *
+ * This calculates the value of loops that would need to be passed to
+ * igt_create_delay_bb() to create a delay of about 1 second on the specified
+ * ring.
+ *
+ * Returns:
+ * uint32_t to be passed to igt_create_delay_bb().
+ */
+/* 0x100000 will run for about 0.6 - 0.8 seconds (dependant on ring) on BXT HW */
+#define CAL_SEED (0x100000)
+uint32_t igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int ringid)
+{
+	uint32_t buf[2];
+	struct intel_batchbuffer *bb;
+	struct timespec start, end;
+	uint64_t duration;
+	uint64_t calibrated;
+	drm_intel_bo *target_bo;
+	int ring_index=0;
+
+/*	igt_assert(ringid < 8);
+	if(calibrated_ring_value[ringid] != 0)
+		return calibrated_ring_value[ringid];*/
+
+	if(calibrated_ring_value == NULL) {
+		int count;
+		for(count = 0; intel_execution_engines[count].name != NULL; count++) {}
+		calibrated_ring_value = calloc(count, sizeof(uint32_t));
+		igt_assert(calibrated_ring_value);
+	}
+
+	/* Check if there is already a calibration value for this ring */
+	while(intel_execution_engines[ring_index].name != NULL) {
+		if((intel_execution_engines[ring_index].exec_id |
+		    intel_execution_engines[ring_index].flags) == ringid) {
+			if(calibrated_ring_value[ring_index] != 0) {
+				return calibrated_ring_value[ring_index];
+			}
+			break;
+		}
+		ring_index++;
+	}
+
+	target_bo = drm_intel_bo_alloc(bufmgr, "target bo", BATCH_SZ, BATCH_SZ);
+	igt_assert(target_bo);
+
+	/* Put some non zero values in the target bo */
+	{
+		uint32_t data=0xffffffff;
+		drm_intel_bo_subdata(target_bo, 0, 4, &data);
+	}
+
+	bb = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
+	igt_assert(bb);
+	igt_create_delay_bb(fd, bb, ringid, CAL_SEED, target_bo);
+
+	gem_quiescent_gpu(fd);
+	clock_gettime(CLOCK_MONOTONIC, &start);
+	intel_batchbuffer_flush_on_ring(bb, ringid);
+	/* This will not return until the bo has finished executing */
+	drm_intel_bo_wait_rendering(target_bo);
+	clock_gettime(CLOCK_MONOTONIC, &end);
+
+	drm_intel_bo_get_subdata(target_bo, 0, 4, (void*)buf);
+
+	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
+	igt_assert_f(buf[0] == 0, "buf[0] expected 0x0, got 0x%x\n", buf[0]);
+
+	duration = ((((uint64_t)end.tv_sec - (uint64_t)start.tv_sec) * SEC_TO_NSEC)
+	           + (uint64_t)end.tv_nsec) - (uint64_t)start.tv_nsec;
+
+	calibrated = (((uint64_t)(CAL_SEED) * SEC_TO_NSEC) / duration);
+	igt_debug("Uncalibrated run took %" PRIu64 ".%04" PRIu64 "s\n",
+	          duration / SEC_TO_NSEC,
+	          (duration % SEC_TO_NSEC) / 100000);
+	drm_intel_bo_unreference(target_bo);
+	intel_batchbuffer_free(bb);
+
+	/* Sanity check. If duration < 100ms, something has clearly gone wrong */
+	igt_assert(duration > (SEC_TO_NSEC  / 10));
+
+	igt_assert_f(calibrated <= UINT32_MAX, "Calibrated value > UINT32_MAX\n");
+
+	if(intel_execution_engines[ring_index].name != NULL)
+		calibrated_ring_value[ring_index] = (uint32_t)calibrated;
+	return (uint32_t)calibrated;
+}
+
+/**
+ * igt_compare_timestamps:
+ * @ts1: timestamp 1
+ * @ts2: timestamp 2
+ *
+ * This compares two uint32_t timestamps. To handle wrapping it assumes the
+ * difference between the two timestamps is less than 1/4 the max elapsed time
+ * represented by the counters.
+ * It also assumes the timestamps are samples from the same counter.
+ *
+ * Returns:
+ * True if ts2 > ts1, allowing for counter wrapping, false otherwise.
+ */
+
+bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2)
+{
+	if (ts2 > ts1)
+		return true;
+	else if ((ts1 > 0x80000000) && (ts2 < 0x40000000))
+		return true; /* Assuming timestamp counter wrapped */
+	else
+		return false;
+}
diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
index 869747d..5b66fa3 100644
--- a/lib/intel_batchbuffer.h
+++ b/lib/intel_batchbuffer.h
@@ -323,4 +323,18 @@ typedef void (*igt_media_spinfunc_t)(struct intel_batchbuffer *batch,
 
 igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
 
+uint32_t igt_batch_used(struct intel_batchbuffer *batch);
+
+void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,
+		int ringid, uint32_t loops, drm_intel_bo *dest);
+
+void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
+			drm_intel_bo *dest, drm_intel_bo *load, bool write);
+
+void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops);
+
+uint32_t igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int ringid);
+
+bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2);
+
 #endif
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 2/6] lib/intel_batchbuffer: Add functions to be used in the scheduler test

[Daniele Ceraolo Spurio]

On 10/03/16 11:03, Derek Morton wrote:
> Adds functions to create a number of different batch buffers to perform
> several functions including:
> Batch buffer which will run for a long duration to provide a delay on a
> specified ring.
> Function to calibrate the delay batch buffer to run for a specified period
> of time.
> Function to create a batch buffer which writes timestamps to a buffer object.
> Function to compare timestamps allowing for wrapping of the values.
>
> v2: Moved code to intel_batchbuffer (Daniel Vetter)
>      Addressed review comments from Daniele Ceraolo Spurio
>
> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
> ---
>   lib/intel_batchbuffer.c | 384 +++++++++++++++++++++++++++++++++++++++++++++++-
>   lib/intel_batchbuffer.h |  14 ++
>   2 files changed, 393 insertions(+), 5 deletions(-)
>
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index 692521f..30e78c5 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -1,8 +1,8 @@
>   /**************************************************************************
> - *
> + *
>    * Copyright 2006 Tungsten Graphics, Inc., Cedar Park, Texas.
>    * All Rights Reserved.
> - *
> + *
>    * Permission is hereby granted, free of charge, to any person obtaining a
>    * copy of this software and associated documentation files (the
>    * "Software"), to deal in the Software without restriction, including
> @@ -10,11 +10,11 @@
>    * distribute, sub license, and/or sell copies of the Software, and to
>    * permit persons to whom the Software is furnished to do so, subject to
>    * the following conditions:
> - *
> + *
>    * The above copyright notice and this permission notice (including the
>    * next paragraph) shall be included in all copies or substantial portions
>    * of the Software.
> - *
> + *
>    * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>    * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>    * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
> @@ -22,7 +22,7 @@
>    * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>    * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>    * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
> - *
> + *
>    **************************************************************************/
>   
>   #include <inttypes.h>
> @@ -30,6 +30,8 @@
>   #include <stdio.h>
>   #include <string.h>
>   #include <assert.h>
> +#include <stdint.h>
> +#include <time.h>
>   
>   #include "drm.h"
>   #include "drmtest.h"
> @@ -42,6 +44,7 @@
>   #include "ioctl_wrappers.h"
>   #include "media_spin.h"
>   #include "gpgpu_fill.h"
> +#include "igt_gt.h"
>   
>   #include <i915_drm.h>
>   
> @@ -817,3 +820,374 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int devid)
>   
>   	return spin;
>   }
> +
> +#define SEC_TO_NSEC (1000 * 1000 * 1000)
> +#define DWORDS_TO_BYTES(x) ((x)*4)
> +
> +#define MI_STORE_REGISTER_MEM(LENGTH)   ((0x024 << 23) | ((LENGTH - 2) & 0xff))
> +#define MI_MATH(NrInst)                 ((0x01A << 23) | ((NrInst - 1) & 0x3f))
> +#define MI_CONDITIONAL_BATCH_BUFFER_END ((0x036 << 23) | (1 << 21) | 2)
> +#define MI_COPY_MEM_MEM                 ((0x02E << 23) | (3))
> +
> +#define ALU_LOAD(TO, FROM)  ((0x080 << 20) | ((TO) << 10) | (FROM))
> +#define ALU_SUB             ( 0x101 << 20)
> +#define ALU_STORE(TO, FROM) ((0x180 << 20) | ((TO) << 10) | (FROM))
> +
> +#define TIMESTAMP_offset      (0x358) /* Elapsed time from system start */
> +#define CTX_TIMESTAMP_offset  (0x3A8) /* Elapsed Time from context creation */
> +#define ALU_GPU_R0_LSB_offset (0x600)
> +#define ALU_GPU_R0_MSB_offset (0x604)
> +#define ALU_GPU_R1_LSB_offset (0x608)
> +#define ALU_GPU_R1_MSB_offset (0x60C)
> +#define ALU_GPU_R2_LSB_offset (0x610)
> +#define ALU_GPU_R2_MSB_offset (0x614)
> +
> +#define ALU_R0_ENCODING   (0x00)
> +#define ALU_R1_ENCODING   (0x01)
> +#define ALU_SRCA_ENCODING (0x20)
> +#define ALU_SRCB_ENCODING (0x21)
> +#define ALU_ACCU_ENCODING (0x31)
> +
> +static int bb_address_size_dw(int fd)
> +{
> +	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
> +		return 2;
> +	else
> +		return 1;
> +}
> +
> +static uint32_t get_mmio_base(int ringid)
> +{
> +	switch (ringid) {
> +	case I915_EXEC_RENDER:
> +		return 0x02000;
> +	case I915_EXEC_BSD:
> +	case I915_EXEC_BSD | 1<<13: /* BSD1 */
> +		return 0x12000;
> +	case I915_EXEC_BSD | 2<<13: /* BSD2 */
> +		return 0x1c000;
> +	case I915_EXEC_BLT:
> +		return 0x22000;
> +	case I915_EXEC_VEBOX:
> +		return 0x1A000;
> +	default:
> +		igt_assert_f(0, "Invalid ringid %d passed to get_mmio_base()\n", ringid);
> +	}
> +}
> +
> +/**
> + * igt_batch_used
> + * @batch batchbuffer to get offset from
> + *
> + * This returns the number of bytes of the batchbuffer that have been used.
> + * e.g. The offset into the batchbuffer that the next OUT_BATCH would write to.
> + *
> + * Returns:
> + * The number of bytes of the batchbuffer that have been used.
> + */
> +uint32_t igt_batch_used(struct intel_batchbuffer *batch)
> +{
> +	return batch->ptr - batch->buffer;
> +}
> +
> +/**
> + * igt_create_delay_bb:
> + * @fd: file descriptor for i915 driver instance
> + * @batch: Batch buffer to write to
> + * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER

ringid is the execution flag of the ring

> + * loops: Number of times to loop
> + * dest: Buffer to use for saving the current loop count and timestamp.
> + *
> + * This creates a batch buffer which will iterate a loop a specified number
> + * of times. Intended for creating batch buffers which take an arbitarily
> + * long time to execute. This can be useful to keep a ring busy while
> + * constructing a test scenario.
> + *
> + * The dest buffer will have a number of Dwords written by the batch buffer
> + * when it runs. They are:
> + * DW0 & DW1 - These are loaded with the value of 'loops' and are decremented
> + *             as the batch buffer executes. They will be 0 after the batch
> + *             buffer completes if it finished succesfully.
> + * DW2 Timestamp - An indication of when the batch buffer ran allowing a
> + *                 comparison between batch buffers to show execution order.
> + *                 May wrap so igt_compare_timestamps() should be used to
> + *                 compare timestamps.
> + *                 The timestamp will wrap every few minutes.
> + *
> + */
> +void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,

Now that you're passing the batch from the outside you're only using the 
fd to get the gen. However, the gen is already saved inside the batch so 
you could get it from there and drop the fd parameter. the 
bb_address_size_dw would need to be modified to get the batch or the gen 
as a parameter, but it looks like an ok change to me. Same reasoning 
applies to other functions you've added.

> +                         int ringid, uint32_t loops, drm_intel_bo *dest)
> +{
> +	int addr_size_dw;
> +	uint32_t mmio_base, jump_offset;
> +
> +	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
> +	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
> +
> +	addr_size_dw = bb_address_size_dw(fd);
> +	mmio_base = get_mmio_base(ringid);
> +	igt_assert(batch);
> +	BEGIN_BATCH(32, 5);
> +
> +	/* store current timestamp in DW2 */
> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
> +	OUT_BATCH(mmio_base + TIMESTAMP_offset);
> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
> +
> +	/* Load R0 with loops */
> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
> +	OUT_BATCH(loops);
> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
> +	OUT_BATCH(mmio_base + ALU_GPU_R0_MSB_offset);
> +	OUT_BATCH(0x00000000);
> +	/* Load R1 with 1 */
> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
> +	OUT_BATCH(mmio_base + ALU_GPU_R1_LSB_offset);
> +	OUT_BATCH(0x00000001);
> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
> +	OUT_BATCH(mmio_base + ALU_GPU_R1_MSB_offset);
> +	OUT_BATCH(0x00000000);
> +	/* Copy R0 / R1 into SRCA / SRCB, Perform R0 - R1, Store result in R0 */
> +	/* e.g. R0 -= 1 */
> +	jump_offset=igt_batch_used(batch);

for clarity the sampling of jump offset could go above the ALU comment 
(and have a comment of its own)

> +	OUT_BATCH(MI_MATH(4));
> +	OUT_BATCH(ALU_LOAD(ALU_SRCA_ENCODING, ALU_R0_ENCODING));
> +	OUT_BATCH(ALU_LOAD(ALU_SRCB_ENCODING, ALU_R1_ENCODING));
> +	OUT_BATCH(ALU_SUB);
> +	OUT_BATCH(ALU_STORE(ALU_R0_ENCODING, ALU_ACCU_ENCODING));
> +	/* Copy R0 to dest
> +	 * On Gen8 MI_CONDITIONAL_BATCH_BUFFER_END BSD ring Compare address
> +	 * points to 2 Dwords, a mask (DW0) and data (DW1) which are ANDed
> +	 * together.
> +	 * On Gen9+, and the other rings on Gen8 Compare address points to
> +	 * just Data (DW0). For simplicity always copy R0 LSB to DW0 and DW1.
> +	 */
> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(1));
> +	/* Repeat until R0 == 0 */
> +	OUT_BATCH(MI_CONDITIONAL_BATCH_BUFFER_END);
> +	OUT_BATCH(0x00000000);
> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
> +	OUT_BATCH(MI_BATCH_BUFFER_START | (addr_size_dw - 1));
> +	OUT_RELOC(batch->bo, I915_GEM_DOMAIN_INSTRUCTION, 0, jump_offset);
> +
> +	/* Should never get here, but end if it happens */
> +	OUT_BATCH(MI_BATCH_BUFFER_END);
> +	ADVANCE_BATCH();
> +}
> +
> +/**
> + * igt_create_timestamp_bb:
> + * @fd: file descriptor for i915 driver instance
> + * @batch: Batch buffer to write to
> + * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER
> + * dest: Buffer to use for saving the timestamps.
> + * load: Buffer to access. Set NULL if not required.
> + * write: If true and load is not NULL, will also write a timestamp to load
> + * buffer. If false and load is not NULL, will read from load buffer into dest.
> + * Intended for dependency checking.
> + *
> + * This creates a batch buffer which writes timestamps into a buffer object.
> + * If 'load' is non null, data is either written to 'load' or copied from 'load'
> + * depending on whether 'write' is set.
> + *
> + * The dest buffer will have a number of Dwords written by the batch buffer
> + * when it runs. They are:
> + * DW0 Reported timestamp - An indication of when the batch buffer ran allowing a
> + *                          comparison between batch buffers to show execution order.
> + *                          May wrap so igt_compare_timestamps() should be used to
> + *                          compare timestamps.
> + *                          The timestamp will wrap every few minutes.
> + * DW2 Value copied from DW0 of load if write == false
> + *
> + */
> +void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
> +                             drm_intel_bo *dest, drm_intel_bo *load, bool write)

I still think that the timestamp capture should be its own function, 
because it could be useful to add it multiple time in the same batch. 
You could split that out in a separate function and then call it from 
igt_create_timestamp_bb, i.e. something like:

igt_emit_timestamp_sample(struct intel_batchbuffer *batch, int ringid, 
drm_intel_bo *dest, unsigned dest_offset);

Personally I also still don't really like the "load" handling, which is 
not related to timestamps in any way. won't oppose it if other people 
are ok with it though.

> +{
> +	int addr_size_dw;
> +	uint32_t mmio_base;
> +
> +	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
> +	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
> +
> +	addr_size_dw = bb_address_size_dw(fd);
> +	mmio_base = get_mmio_base(ringid);
> +	igt_assert(batch);
> +
> +	BEGIN_BATCH(3, 1);
> +	/* store current reported timestamp in DW0 */
> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
> +	OUT_BATCH(mmio_base + TIMESTAMP_offset);
> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
> +
> +	ADVANCE_BATCH();
> +
> +	if(load != NULL) {

style: we tend to put a space between if/while/for and the parenthesis, 
but you didn't anywhere in this patch

> +		if(write) {
> +			BEGIN_BATCH(3, 1);
> +			OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
> +			OUT_BATCH(mmio_base + TIMESTAMP_offset);
> +			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
> +			ADVANCE_BATCH();
> +		}
> +		else {

weird indent

> +			BEGIN_BATCH(3, 2);
> +			OUT_BATCH(MI_COPY_MEM_MEM);
> +			OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
> +			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, 0, DWORDS_TO_BYTES(0));
> +			ADVANCE_BATCH();
> +		}
> +	}
> +
> +	BEGIN_BATCH(1, 0);
> +	OUT_BATCH(MI_BATCH_BUFFER_END);
> +	ADVANCE_BATCH();
> +}
> +
> +/**
> + * igt_create_noop_bb:
> + * @batch: Batch buffer to write to
> + * noops: Number of MI_NOOP instructions to add to the batch buffer.
> + *
> + * This creates a batch buffer with a specified number of MI_NOOP instructions.
> + */
> +void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops)
> +{
> +	int loop;
> +
> +	igt_assert(batch);
> +
> +	BEGIN_BATCH(noops + 1, 0);
> +	for(loop = 0; loop < noops; loop++)
> +		OUT_BATCH(MI_NOOP);
> +	OUT_BATCH(MI_BATCH_BUFFER_END);
> +	ADVANCE_BATCH();
> +
> +}
> +
> +/* Store calibrated values so they only need calculating once.
> + * Use intel_execution_engines array as list of supported rings
> + */
> +static uint32_t *calibrated_ring_value = NULL;
> +
> +/**
> + * igt_calibrate_delay_bb:
> + * @fd: file descriptor for i915 driver instance
> + * @bufmgr: Buffer manager to be used for creation of batch buffers
> + * ringid: Ring to calibrate. e.g. I915_EXEC_RENDER
> + *
> + * This calculates the value of loops that would need to be passed to
> + * igt_create_delay_bb() to create a delay of about 1 second on the specified
> + * ring.
> + *
> + * Returns:
> + * uint32_t to be passed to igt_create_delay_bb().
> + */
> +/* 0x100000 will run for about 0.6 - 0.8 seconds (dependant on ring) on BXT HW */
> +#define CAL_SEED (0x100000)
> +uint32_t igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int ringid)
> +{
> +	uint32_t buf[2];
> +	struct intel_batchbuffer *bb;
> +	struct timespec start, end;
> +	uint64_t duration;
> +	uint64_t calibrated;
> +	drm_intel_bo *target_bo;
> +	int ring_index=0;
> +
> +/*	igt_assert(ringid < 8);
> +	if(calibrated_ring_value[ringid] != 0)
> +		return calibrated_ring_value[ringid];*/

dead code

> +
> +	if(calibrated_ring_value == NULL) {
> +		int count;
> +		for(count = 0; intel_execution_engines[count].name != NULL; count++) {}

Can you add a comment to explain why this loop is required to count the 
engines? it could also be possible to add a function in igt_gt to return 
the exec_engines count, but I'm not sure that's worth the effort. Also, 
BSD1 and BSD2 will have the same calibration value so you can calibrate 
only once for I915_EXEC_BSD

> +		calibrated_ring_value = calloc(count, sizeof(uint32_t));
> +		igt_assert(calibrated_ring_value);
> +	}
> +
> +	/* Check if there is already a calibration value for this ring */
> +	while(intel_execution_engines[ring_index].name != NULL) {
> +		if((intel_execution_engines[ring_index].exec_id |
> +		    intel_execution_engines[ring_index].flags) == ringid) {
> +			if(calibrated_ring_value[ring_index] != 0) {
> +				return calibrated_ring_value[ring_index];
> +			}
> +			break;
> +		}
> +		ring_index++;

Could use a for loop instead of incrementing on each iteration.

> +	}
> +
> +	target_bo = drm_intel_bo_alloc(bufmgr, "target bo", BATCH_SZ, BATCH_SZ);
> +	igt_assert(target_bo);
> +
> +	/* Put some non zero values in the target bo */
> +	{

This bracket used to give scope to the data variable doesn't look 
essential as there are no other uses of a variable named data in this 
function

> +		uint32_t data=0xffffffff;

style: spaces around "="

> +		drm_intel_bo_subdata(target_bo, 0, 4, &data);

the subdata call can fail. In the tree we don't check the return code on 
every usage, but I believe it is safer to do it. do_or_die() should do 
the trick.

> +	}
> +
> +	bb = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
> +	igt_assert(bb);
> +	igt_create_delay_bb(fd, bb, ringid, CAL_SEED, target_bo);
> +
> +	gem_quiescent_gpu(fd);
> +	clock_gettime(CLOCK_MONOTONIC, &start);
> +	intel_batchbuffer_flush_on_ring(bb, ringid);
> +	/* This will not return until the bo has finished executing */
> +	drm_intel_bo_wait_rendering(target_bo);
> +	clock_gettime(CLOCK_MONOTONIC, &end);
> +
> +	drm_intel_bo_get_subdata(target_bo, 0, 4, (void*)buf);

Why are you using a 2 words array to read only 1 word? what's the 
purpose of buf[1]? if you don't need it you could reuse the data 
variable from above

> +
> +	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
> +	igt_assert_f(buf[0] == 0, "buf[0] expected 0x0, got 0x%x\n", buf[0]);
> +
> +	duration = ((((uint64_t)end.tv_sec - (uint64_t)start.tv_sec) * SEC_TO_NSEC)
> +	           + (uint64_t)end.tv_nsec) - (uint64_t)start.tv_nsec;
> +
> +	calibrated = (((uint64_t)(CAL_SEED) * SEC_TO_NSEC) / duration);
> +	igt_debug("Uncalibrated run took %" PRIu64 ".%04" PRIu64 "s\n",
> +	          duration / SEC_TO_NSEC,
> +	          (duration % SEC_TO_NSEC) / 100000);
> +	drm_intel_bo_unreference(target_bo);
> +	intel_batchbuffer_free(bb);
> +
> +	/* Sanity check. If duration < 100ms, something has clearly gone wrong */
> +	igt_assert(duration > (SEC_TO_NSEC  / 10));
> +
> +	igt_assert_f(calibrated <= UINT32_MAX, "Calibrated value > UINT32_MAX\n");
> +
> +	if(intel_execution_engines[ring_index].name != NULL)
> +		calibrated_ring_value[ring_index] = (uint32_t)calibrated;
> +	return (uint32_t)calibrated;
> +}
> +
> +/**
> + * igt_compare_timestamps:
> + * @ts1: timestamp 1
> + * @ts2: timestamp 2
> + *
> + * This compares two uint32_t timestamps. To handle wrapping it assumes the
> + * difference between the two timestamps is less than 1/4 the max elapsed time
> + * represented by the counters.
> + * It also assumes the timestamps are samples from the same counter.
> + *
> + * Returns:
> + * True if ts2 > ts1, allowing for counter wrapping, false otherwise.
> + */
> +
> +bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2)
> +{
> +	if (ts2 > ts1)

This will return true if ts2 = 0xFFFFFFFF and ts1 = 0x00000001 even if 
ts1 came after ts2 due to wrapping.

> +		return true;
> +	else if ((ts1 > 0x80000000) && (ts2 < 0x40000000))
> +		return true; /* Assuming timestamp counter wrapped */
> +	else
> +		return false;
> +}

You could use something like:

     return (int32_t)(ts2 - ts1) > 0;

This should give you the right result as long as the 2 samples are not 
more than ~50% of the timestamp period apart

Regards,
Daniele

> diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
> index 869747d..5b66fa3 100644
> --- a/lib/intel_batchbuffer.h
> +++ b/lib/intel_batchbuffer.h
> @@ -323,4 +323,18 @@ typedef void (*igt_media_spinfunc_t)(struct intel_batchbuffer *batch,
>   
>   igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
>   
> +uint32_t igt_batch_used(struct intel_batchbuffer *batch);
> +
> +void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,
> +		int ringid, uint32_t loops, drm_intel_bo *dest);
> +
> +void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
> +			drm_intel_bo *dest, drm_intel_bo *load, bool write);
> +
> +void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops);
> +
> +uint32_t igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int ringid);
> +
> +bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2);
> +
>   #endif

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 2/6] lib/intel_batchbuffer: Add functions to be used in the scheduler test

[Morton, Derek J]

>
>
>-----Original Message-----
>From: Ceraolo Spurio, Daniele 
>Sent: Monday, March 14, 2016 12:16 PM
>To: Morton, Derek J <derek.j.morton@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [PATCH i-g-t v3 2/6] lib/intel_batchbuffer: Add functions to be used in the scheduler test
>
>
>
>On 10/03/16 11:03, Derek Morton wrote:
>> Adds functions to create a number of different batch buffers to 
>> perform several functions including:
>> Batch buffer which will run for a long duration to provide a delay on 
>> a specified ring.
>> Function to calibrate the delay batch buffer to run for a specified 
>> period of time.
>> Function to create a batch buffer which writes timestamps to a buffer object.
>> Function to compare timestamps allowing for wrapping of the values.
>>
>> v2: Moved code to intel_batchbuffer (Daniel Vetter)
>>      Addressed review comments from Daniele Ceraolo Spurio
>>
>> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
>> ---
>>   lib/intel_batchbuffer.c | 384 +++++++++++++++++++++++++++++++++++++++++++++++-
>>   lib/intel_batchbuffer.h |  14 ++
>>   2 files changed, 393 insertions(+), 5 deletions(-)
>>
>> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c index 
>> 692521f..30e78c5 100644
>> --- a/lib/intel_batchbuffer.c
>> +++ b/lib/intel_batchbuffer.c
>> @@ -1,8 +1,8 @@
>>   
>> /*********************************************************************
>> *****
>> - *
>> + *
>>    * Copyright 2006 Tungsten Graphics, Inc., Cedar Park, Texas.
>>    * All Rights Reserved.
>> - *
>> + *
>>    * Permission is hereby granted, free of charge, to any person obtaining a
>>    * copy of this software and associated documentation files (the
>>    * "Software"), to deal in the Software without restriction, 
>> including @@ -10,11 +10,11 @@
>>    * distribute, sub license, and/or sell copies of the Software, and to
>>    * permit persons to whom the Software is furnished to do so, subject to
>>    * the following conditions:
>> - *
>> + *
>>    * The above copyright notice and this permission notice (including the
>>    * next paragraph) shall be included in all copies or substantial portions
>>    * of the Software.
>> - *
>> + *
>>    * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
>>    * OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
>>    * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT.
>> @@ -22,7 +22,7 @@
>>    * ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
>>    * TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
>>    * SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
>> - *
>> + *
>>    
>> **********************************************************************
>> ****/
>>   
>>   #include <inttypes.h>
>> @@ -30,6 +30,8 @@
>>   #include <stdio.h>
>>   #include <string.h>
>>   #include <assert.h>
>> +#include <stdint.h>
>> +#include <time.h>
>>   
>>   #include "drm.h"
>>   #include "drmtest.h"
>> @@ -42,6 +44,7 @@
>>   #include "ioctl_wrappers.h"
>>   #include "media_spin.h"
>>   #include "gpgpu_fill.h"
>> +#include "igt_gt.h"
>>   
>>   #include <i915_drm.h>
>>   
>> @@ -817,3 +820,374 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int 
>> devid)
>>   
>>   	return spin;
>>   }
>> +
>> +#define SEC_TO_NSEC (1000 * 1000 * 1000) #define DWORDS_TO_BYTES(x) 
>> +((x)*4)
>> +
>> +#define MI_STORE_REGISTER_MEM(LENGTH)   ((0x024 << 23) | ((LENGTH - 2) & 0xff))
>> +#define MI_MATH(NrInst)                 ((0x01A << 23) | ((NrInst - 1) & 0x3f))
>> +#define MI_CONDITIONAL_BATCH_BUFFER_END ((0x036 << 23) | (1 << 21) | 2)
>> +#define MI_COPY_MEM_MEM                 ((0x02E << 23) | (3))
>> +
>> +#define ALU_LOAD(TO, FROM)  ((0x080 << 20) | ((TO) << 10) | (FROM))
>> +#define ALU_SUB             ( 0x101 << 20)
>> +#define ALU_STORE(TO, FROM) ((0x180 << 20) | ((TO) << 10) | (FROM))
>> +
>> +#define TIMESTAMP_offset      (0x358) /* Elapsed time from system start */
>> +#define CTX_TIMESTAMP_offset  (0x3A8) /* Elapsed Time from context 
>> +creation */ #define ALU_GPU_R0_LSB_offset (0x600) #define 
>> +ALU_GPU_R0_MSB_offset (0x604) #define ALU_GPU_R1_LSB_offset (0x608) 
>> +#define ALU_GPU_R1_MSB_offset (0x60C) #define ALU_GPU_R2_LSB_offset 
>> +(0x610) #define ALU_GPU_R2_MSB_offset (0x614)
>> +
>> +#define ALU_R0_ENCODING   (0x00)
>> +#define ALU_R1_ENCODING   (0x01)
>> +#define ALU_SRCA_ENCODING (0x20)
>> +#define ALU_SRCB_ENCODING (0x21)
>> +#define ALU_ACCU_ENCODING (0x31)
>> +
>> +static int bb_address_size_dw(int fd) {
>> +	if (intel_gen(intel_get_drm_devid(fd)) >= 8)
>> +		return 2;
>> +	else
>> +		return 1;
>> +}
>> +
>> +static uint32_t get_mmio_base(int ringid) {
>> +	switch (ringid) {
>> +	case I915_EXEC_RENDER:
>> +		return 0x02000;
>> +	case I915_EXEC_BSD:
>> +	case I915_EXEC_BSD | 1<<13: /* BSD1 */
>> +		return 0x12000;
>> +	case I915_EXEC_BSD | 2<<13: /* BSD2 */
>> +		return 0x1c000;
>> +	case I915_EXEC_BLT:
>> +		return 0x22000;
>> +	case I915_EXEC_VEBOX:
>> +		return 0x1A000;
>> +	default:
>> +		igt_assert_f(0, "Invalid ringid %d passed to get_mmio_base()\n", ringid);
>> +	}
>> +}
>> +
>> +/**
>> + * igt_batch_used
>> + * @batch batchbuffer to get offset from
>> + *
>> + * This returns the number of bytes of the batchbuffer that have been used.
>> + * e.g. The offset into the batchbuffer that the next OUT_BATCH would write to.
>> + *
>> + * Returns:
>> + * The number of bytes of the batchbuffer that have been used.
>> + */
>> +uint32_t igt_batch_used(struct intel_batchbuffer *batch) {
>> +	return batch->ptr - batch->buffer;
>> +}
>> +
>> +/**
>> + * igt_create_delay_bb:
>> + * @fd: file descriptor for i915 driver instance
>> + * @batch: Batch buffer to write to
>> + * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER
>
>ringid is the execution flag of the ring

Ok

>
>> + * loops: Number of times to loop
>> + * dest: Buffer to use for saving the current loop count and timestamp.
>> + *
>> + * This creates a batch buffer which will iterate a loop a specified 
>> +number
>> + * of times. Intended for creating batch buffers which take an 
>> +arbitarily
>> + * long time to execute. This can be useful to keep a ring busy while
>> + * constructing a test scenario.
>> + *
>> + * The dest buffer will have a number of Dwords written by the batch 
>> +buffer
>> + * when it runs. They are:
>> + * DW0 & DW1 - These are loaded with the value of 'loops' and are decremented
>> + *             as the batch buffer executes. They will be 0 after the batch
>> + *             buffer completes if it finished succesfully.
>> + * DW2 Timestamp - An indication of when the batch buffer ran allowing a
>> + *                 comparison between batch buffers to show execution order.
>> + *                 May wrap so igt_compare_timestamps() should be used to
>> + *                 compare timestamps.
>> + *                 The timestamp will wrap every few minutes.
>> + *
>> + */
>> +void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,
>
>Now that you're passing the batch from the outside you're only using the fd to get the gen. However, the gen is already saved inside the batch so you could get it from there and drop the fd parameter. the bb_address_size_dw would need to be modified to get the batch or the gen as a parameter, but it looks like an ok change to me. Same reasoning applies to other functions you've added.

Ok

>
>> +                         int ringid, uint32_t loops, drm_intel_bo 
>> +*dest) {
>> +	int addr_size_dw;
>> +	uint32_t mmio_base, jump_offset;
>> +
>> +	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
>> +	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
>> +
>> +	addr_size_dw = bb_address_size_dw(fd);
>> +	mmio_base = get_mmio_base(ringid);
>> +	igt_assert(batch);
>> +	BEGIN_BATCH(32, 5);
>> +
>> +	/* store current timestamp in DW2 */
>> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
>> +	OUT_BATCH(mmio_base + TIMESTAMP_offset);
>> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, 
>> +I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
>> +
>> +	/* Load R0 with loops */
>> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
>> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
>> +	OUT_BATCH(loops);
>> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
>> +	OUT_BATCH(mmio_base + ALU_GPU_R0_MSB_offset);
>> +	OUT_BATCH(0x00000000);
>> +	/* Load R1 with 1 */
>> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
>> +	OUT_BATCH(mmio_base + ALU_GPU_R1_LSB_offset);
>> +	OUT_BATCH(0x00000001);
>> +	OUT_BATCH(MI_LOAD_REGISTER_IMM);
>> +	OUT_BATCH(mmio_base + ALU_GPU_R1_MSB_offset);
>> +	OUT_BATCH(0x00000000);
>> +	/* Copy R0 / R1 into SRCA / SRCB, Perform R0 - R1, Store result in R0 */
>> +	/* e.g. R0 -= 1 */
>> +	jump_offset=igt_batch_used(batch);
>
>for clarity the sampling of jump offset could go above the ALU comment (and have a comment of its own)

Ok

>
>> +	OUT_BATCH(MI_MATH(4));
>> +	OUT_BATCH(ALU_LOAD(ALU_SRCA_ENCODING, ALU_R0_ENCODING));
>> +	OUT_BATCH(ALU_LOAD(ALU_SRCB_ENCODING, ALU_R1_ENCODING));
>> +	OUT_BATCH(ALU_SUB);
>> +	OUT_BATCH(ALU_STORE(ALU_R0_ENCODING, ALU_ACCU_ENCODING));
>> +	/* Copy R0 to dest
>> +	 * On Gen8 MI_CONDITIONAL_BATCH_BUFFER_END BSD ring Compare address
>> +	 * points to 2 Dwords, a mask (DW0) and data (DW1) which are ANDed
>> +	 * together.
>> +	 * On Gen9+, and the other rings on Gen8 Compare address points to
>> +	 * just Data (DW0). For simplicity always copy R0 LSB to DW0 and DW1.
>> +	 */
>> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
>> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
>> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
>> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
>> +	OUT_BATCH(mmio_base + ALU_GPU_R0_LSB_offset);
>> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(1));
>> +	/* Repeat until R0 == 0 */
>> +	OUT_BATCH(MI_CONDITIONAL_BATCH_BUFFER_END);
>> +	OUT_BATCH(0x00000000);
>> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, 0);
>> +	OUT_BATCH(MI_BATCH_BUFFER_START | (addr_size_dw - 1));
>> +	OUT_RELOC(batch->bo, I915_GEM_DOMAIN_INSTRUCTION, 0, jump_offset);
>> +
>> +	/* Should never get here, but end if it happens */
>> +	OUT_BATCH(MI_BATCH_BUFFER_END);
>> +	ADVANCE_BATCH();
>> +}
>> +
>> +/**
>> + * igt_create_timestamp_bb:
>> + * @fd: file descriptor for i915 driver instance
>> + * @batch: Batch buffer to write to
>> + * ringid: Ring to create batch buffer for. e.g. I915_EXEC_RENDER
>> + * dest: Buffer to use for saving the timestamps.
>> + * load: Buffer to access. Set NULL if not required.
>> + * write: If true and load is not NULL, will also write a timestamp 
>> +to load
>> + * buffer. If false and load is not NULL, will read from load buffer into dest.
>> + * Intended for dependency checking.
>> + *
>> + * This creates a batch buffer which writes timestamps into a buffer object.
>> + * If 'load' is non null, data is either written to 'load' or copied from 'load'
>> + * depending on whether 'write' is set.
>> + *
>> + * The dest buffer will have a number of Dwords written by the batch 
>> +buffer
>> + * when it runs. They are:
>> + * DW0 Reported timestamp - An indication of when the batch buffer ran allowing a
>> + *                          comparison between batch buffers to show execution order.
>> + *                          May wrap so igt_compare_timestamps() should be used to
>> + *                          compare timestamps.
>> + *                          The timestamp will wrap every few minutes.
>> + * DW2 Value copied from DW0 of load if write == false
>> + *
>> + */
>> +void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
>> +                             drm_intel_bo *dest, drm_intel_bo *load, 
>> +bool write)
>
>I still think that the timestamp capture should be its own function, because it could be useful to add it multiple time in the same batch. 
>You could split that out in a separate function and then call it from igt_create_timestamp_bb, i.e. something like:
>
>igt_emit_timestamp_sample(struct intel_batchbuffer *batch, int ringid, drm_intel_bo *dest, unsigned dest_offset);

I can create a separate function. Will make it static for now as it is not needed outside this file.
>
>Personally I also still don't really like the "load" handling, which is not related to timestamps in any way. won't oppose it if other people are ok with it though.
>
>> +{
>> +	int addr_size_dw;
>> +	uint32_t mmio_base;
>> +
>> +	/* CMD parser blocks reading TIMESTAMP register on gen 7.5 */
>> +	igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8);
>> +
>> +	addr_size_dw = bb_address_size_dw(fd);
>> +	mmio_base = get_mmio_base(ringid);
>> +	igt_assert(batch);
>> +
>> +	BEGIN_BATCH(3, 1);
>> +	/* store current reported timestamp in DW0 */
>> +	OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
>> +	OUT_BATCH(mmio_base + TIMESTAMP_offset);
>> +	OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, 
>> +I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
>> +
>> +	ADVANCE_BATCH();
>> +
>> +	if(load != NULL) {
>
>style: we tend to put a space between if/while/for and the parenthesis, but you didn't anywhere in this patch

Ok

>
>> +		if(write) {
>> +			BEGIN_BATCH(3, 1);
>> +			OUT_BATCH(MI_STORE_REGISTER_MEM(addr_size_dw + 2));
>> +			OUT_BATCH(mmio_base + TIMESTAMP_offset);
>> +			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(0));
>> +			ADVANCE_BATCH();
>> +		}
>> +		else {
>
>weird indent

What is weird? The if and else line up and it is all tabs.

>
>> +			BEGIN_BATCH(3, 2);
>> +			OUT_BATCH(MI_COPY_MEM_MEM);
>> +			OUT_RELOC(dest, I915_GEM_DOMAIN_INSTRUCTION, I915_GEM_DOMAIN_INSTRUCTION, DWORDS_TO_BYTES(2));
>> +			OUT_RELOC(load, I915_GEM_DOMAIN_INSTRUCTION, 0, DWORDS_TO_BYTES(0));
>> +			ADVANCE_BATCH();
>> +		}
>> +	}
>> +
>> +	BEGIN_BATCH(1, 0);
>> +	OUT_BATCH(MI_BATCH_BUFFER_END);
>> +	ADVANCE_BATCH();
>> +}
>> +
>> +/**
>> + * igt_create_noop_bb:
>> + * @batch: Batch buffer to write to
>> + * noops: Number of MI_NOOP instructions to add to the batch buffer.
>> + *
>> + * This creates a batch buffer with a specified number of MI_NOOP instructions.
>> + */
>> +void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops) {
>> +	int loop;
>> +
>> +	igt_assert(batch);
>> +
>> +	BEGIN_BATCH(noops + 1, 0);
>> +	for(loop = 0; loop < noops; loop++)
>> +		OUT_BATCH(MI_NOOP);
>> +	OUT_BATCH(MI_BATCH_BUFFER_END);
>> +	ADVANCE_BATCH();
>> +
>> +}
>> +
>> +/* Store calibrated values so they only need calculating once.
>> + * Use intel_execution_engines array as list of supported rings  */ 
>> +static uint32_t *calibrated_ring_value = NULL;
>> +
>> +/**
>> + * igt_calibrate_delay_bb:
>> + * @fd: file descriptor for i915 driver instance
>> + * @bufmgr: Buffer manager to be used for creation of batch buffers
>> + * ringid: Ring to calibrate. e.g. I915_EXEC_RENDER
>> + *
>> + * This calculates the value of loops that would need to be passed to
>> + * igt_create_delay_bb() to create a delay of about 1 second on the 
>> +specified
>> + * ring.
>> + *
>> + * Returns:
>> + * uint32_t to be passed to igt_create_delay_bb().
>> + */
>> +/* 0x100000 will run for about 0.6 - 0.8 seconds (dependant on ring) 
>> +on BXT HW */ #define CAL_SEED (0x100000) uint32_t 
>> +igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int ringid) 
>> +{
>> +	uint32_t buf[2];
>> +	struct intel_batchbuffer *bb;
>> +	struct timespec start, end;
>> +	uint64_t duration;
>> +	uint64_t calibrated;
>> +	drm_intel_bo *target_bo;
>> +	int ring_index=0;
>> +
>> +/*	igt_assert(ringid < 8);
>> +	if(calibrated_ring_value[ringid] != 0)
>> +		return calibrated_ring_value[ringid];*/
>
>dead code

Removed

>
>> +
>> +	if(calibrated_ring_value == NULL) {
>> +		int count;
>> +		for(count = 0; intel_execution_engines[count].name != NULL; 
>> +count++) {}
>
>Can you add a comment to explain why this loop is required to count the engines? it could also be possible to add a function in igt_gt to return the exec_engines count, but I'm not sure that's worth the effort. Also,
>BSD1 and BSD2 will have the same calibration value so you can calibrate only once for I915_EXEC_BSD

Can add a comment. It will require adding a load of logic to use a single calibration value for BSD1 and BSD2, and that would then make the assumption that they would have the same calibration value. I would prefer not to make such assumptions.

>
>> +		calibrated_ring_value = calloc(count, sizeof(uint32_t));
>> +		igt_assert(calibrated_ring_value);
>> +	}
>> +
>> +	/* Check if there is already a calibration value for this ring */
>> +	while(intel_execution_engines[ring_index].name != NULL) {
>> +		if((intel_execution_engines[ring_index].exec_id |
>> +		    intel_execution_engines[ring_index].flags) == ringid) {
>> +			if(calibrated_ring_value[ring_index] != 0) {
>> +				return calibrated_ring_value[ring_index];
>> +			}
>> +			break;
>> +		}
>> +		ring_index++;
>
>Could use a for loop instead of incrementing on each iteration.

Ok

>
>> +	}
>> +
>> +	target_bo = drm_intel_bo_alloc(bufmgr, "target bo", BATCH_SZ, BATCH_SZ);
>> +	igt_assert(target_bo);
>> +
>> +	/* Put some non zero values in the target bo */
>> +	{
>
>This bracket used to give scope to the data variable doesn't look essential as there are no other uses of a variable named data in this function
>
>> +		uint32_t data=0xffffffff;
>
>style: spaces around "="
>
>> +		drm_intel_bo_subdata(target_bo, 0, 4, &data);
>
>the subdata call can fail. In the tree we don't check the return code on every usage, but I believe it is safer to do it. do_or_die() should do the trick.
>
>> +	}
>> +
>> +	bb = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
>> +	igt_assert(bb);
>> +	igt_create_delay_bb(fd, bb, ringid, CAL_SEED, target_bo);
>> +
>> +	gem_quiescent_gpu(fd);
>> +	clock_gettime(CLOCK_MONOTONIC, &start);
>> +	intel_batchbuffer_flush_on_ring(bb, ringid);
>> +	/* This will not return until the bo has finished executing */
>> +	drm_intel_bo_wait_rendering(target_bo);
>> +	clock_gettime(CLOCK_MONOTONIC, &end);
>> +
>> +	drm_intel_bo_get_subdata(target_bo, 0, 4, (void*)buf);
>
>Why are you using a 2 words array to read only 1 word? what's the purpose of buf[1]? if you don't need it you could reuse the data variable from above
>
>> +
>> +	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
>> +	igt_assert_f(buf[0] == 0, "buf[0] expected 0x0, got 0x%x\n", 
>> +buf[0]);
>> +
>> +	duration = ((((uint64_t)end.tv_sec - (uint64_t)start.tv_sec) * SEC_TO_NSEC)
>> +	           + (uint64_t)end.tv_nsec) - (uint64_t)start.tv_nsec;
>> +
>> +	calibrated = (((uint64_t)(CAL_SEED) * SEC_TO_NSEC) / duration);
>> +	igt_debug("Uncalibrated run took %" PRIu64 ".%04" PRIu64 "s\n",
>> +	          duration / SEC_TO_NSEC,
>> +	          (duration % SEC_TO_NSEC) / 100000);
>> +	drm_intel_bo_unreference(target_bo);
>> +	intel_batchbuffer_free(bb);
>> +
>> +	/* Sanity check. If duration < 100ms, something has clearly gone wrong */
>> +	igt_assert(duration > (SEC_TO_NSEC  / 10));
>> +
>> +	igt_assert_f(calibrated <= UINT32_MAX, "Calibrated value > 
>> +UINT32_MAX\n");
>> +
>> +	if(intel_execution_engines[ring_index].name != NULL)
>> +		calibrated_ring_value[ring_index] = (uint32_t)calibrated;
>> +	return (uint32_t)calibrated;
>> +}
>> +
>> +/**
>> + * igt_compare_timestamps:
>> + * @ts1: timestamp 1
>> + * @ts2: timestamp 2
>> + *
>> + * This compares two uint32_t timestamps. To handle wrapping it 
>> +assumes the
>> + * difference between the two timestamps is less than 1/4 the max 
>> +elapsed time
>> + * represented by the counters.
>> + * It also assumes the timestamps are samples from the same counter.
>> + *
>> + * Returns:
>> + * True if ts2 > ts1, allowing for counter wrapping, false otherwise.
>> + */
>> +
>> +bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2) {
>> +	if (ts2 > ts1)
>
>This will return true if ts2 = 0xFFFFFFFF and ts1 = 0x00000001 even if
>ts1 came after ts2 due to wrapping.
>
>> +		return true;
>> +	else if ((ts1 > 0x80000000) && (ts2 < 0x40000000))
>> +		return true; /* Assuming timestamp counter wrapped */
>> +	else
>> +		return false;
>> +}
>
>You could use something like:
>
>     return (int32_t)(ts2 - ts1) > 0;

Ok

>
>This should give you the right result as long as the 2 samples are not more than ~50% of the timestamp period apart
>
>Regards,
>Daniele
>
>> diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h index 
>> 869747d..5b66fa3 100644
>> --- a/lib/intel_batchbuffer.h
>> +++ b/lib/intel_batchbuffer.h
>> @@ -323,4 +323,18 @@ typedef void (*igt_media_spinfunc_t)(struct 
>> intel_batchbuffer *batch,
>>   
>>   igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
>>   
>> +uint32_t igt_batch_used(struct intel_batchbuffer *batch);
>> +
>> +void igt_create_delay_bb(int fd, struct intel_batchbuffer *batch,
>> +		int ringid, uint32_t loops, drm_intel_bo *dest);
>> +
>> +void igt_create_timestamp_bb(int fd, struct intel_batchbuffer *batch, int ringid,
>> +			drm_intel_bo *dest, drm_intel_bo *load, bool write);
>> +
>> +void igt_create_noop_bb(struct intel_batchbuffer *batch, int noops);
>> +
>> +uint32_t igt_calibrate_delay_bb(int fd, drm_intel_bufmgr *bufmgr, int 
>> +ringid);
>> +
>> +bool igt_compare_timestamps(uint32_t ts1, uint32_t ts2);
>> +
>>   #endif
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 3/6] tests/gem_scheduler: Add gem_scheduler test

[Derek Morton]

This is intended to test the scheduler behaviour is correct.
The subtests are
<ring>-basic
Tests that batch buffers of the same priority submitted to a ring
execute in the order they are submitted.
<ring>-read
Submits a batch buffer with a read dependency to a buffer object to
a ring which is held in the scheduler queue by a long running batch
buffer. Submit batch buffers to other rings that have a read dependency
to the same buffer object. Ensure they execute before the batch buffer
being held up behind the long running batch buffer.
<ring>-write
Submits a batch buffer with a write dependency to a buffer object to
a ring which is held in the scheduler queue by a long running batch
buffer. Submit batch buffers to other rings that have a write dependency
to the same buffer object. Submit batch buffers with no interdependencies
to all rings. Ensure the batch buffers that have write dependencies are
executed in submission order but the batch buffers without interdependencies
do not get held up.

v2: Addressed review comments from Daniele Ceraolo Spurio

v3: Added logic to use generic BSD ring if there is 1 and BSD1 / BSD2 if there
    are 2.

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 tests/Makefile.sources |   1 +
 tests/gem_scheduler.c  | 459 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 460 insertions(+)
 create mode 100644 tests/gem_scheduler.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index f8b18b0..c88e045 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -66,6 +66,7 @@ TESTS_progs_M = \
 	gem_request_retire \
 	gem_reset_stats \
 	gem_ringfill \
+	gem_scheduler \
 	gem_set_tiling_vs_blt \
 	gem_softpin \
 	gem_stolen \
diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
new file mode 100644
index 0000000..436440a
--- /dev/null
+++ b/tests/gem_scheduler.c
@@ -0,0 +1,459 @@
+/*
+ * Copyright © 2016 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ *    Derek Morton <derek.j.morton@intel.com>
+ *
+ */
+
+#include "igt.h"
+#include <unistd.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <stdio.h>
+#include <string.h>
+#include <inttypes.h>
+#include <time.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+
+IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant "
+                     "batch buffers of the same priority are executed in "
+                     "submission order. Read-read tests ensure "
+                     "batch buffers with a read dependency to the same buffer "
+                     "object do not block each other. Write-write dependency "
+                     "tests ensure batch buffers with a write dependency to a "
+                     "buffer object will be executed in submission order but "
+                     "will not block execution of other independant batch "
+                     "buffers.");
+
+#define SEC_TO_NSEC (1000 * 1000 * 1000)
+
+static struct ring {
+	const char *name;
+	int id;
+	bool exists;
+} rings[] = {
+	{ "render", I915_EXEC_RENDER, false },
+	{ "bsd",    I915_EXEC_BSD , false },
+	{ "bsd2",    I915_EXEC_BSD | 2<<13, false },
+	{ "blt",    I915_EXEC_BLT, false },
+	{ "vebox",  I915_EXEC_VEBOX, false },
+};
+
+#define NBR_RINGS (sizeof(rings)/sizeof(struct ring))
+
+static void check_rings(int fd) {
+	int loop;
+	for(loop=0; loop < NBR_RINGS; loop++) {
+		if(gem_has_ring(fd, rings[loop].id)) {
+			if(rings[loop].id == (I915_EXEC_BSD | 2<<13)) {
+				rings[loop].exists = gem_has_bsd2(fd);
+			}
+			else {
+				rings[loop].exists = true;
+				if(rings[loop].id == I915_EXEC_BSD)
+					/* If there is BSD2, need to make BSD1
+					 * explicit.
+					 */
+					if(gem_has_bsd2(fd))
+						rings[loop].id |= (1<<13);
+			}
+		}
+	}
+}
+
+static drm_intel_bo *create_and_check_bo(drm_intel_bufmgr *bufmgr, const char *desc)
+{
+	drm_intel_bo *bo = drm_intel_bo_alloc(bufmgr, desc, BATCH_SZ, BATCH_SZ);
+	igt_assert_f(bo, "Failed allocating %s\n", desc);
+	return bo;
+}
+
+static struct intel_batchbuffer *create_delay_bb(int fd, drm_intel_bufmgr *bufmgr,
+                                                 int ringid, uint32_t loops,
+                                                 drm_intel_bo *dest)
+{
+	struct intel_batchbuffer *buffer;
+	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
+	igt_assert(buffer);
+	igt_create_delay_bb(fd, buffer, ringid, loops, dest);
+	return buffer;
+}
+
+static struct intel_batchbuffer *create_timestamp_bb(int fd, drm_intel_bufmgr *bufmgr,
+                                                     int ringid, drm_intel_bo *dest,
+                                                     drm_intel_bo *load, bool write)
+{
+	struct intel_batchbuffer *buffer;
+	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
+	igt_assert(buffer);
+	igt_create_timestamp_bb(fd, buffer, ringid, dest, load, write);
+	return buffer;
+}
+
+static struct intel_batchbuffer *create_noop_bb(int fd, drm_intel_bufmgr *bufmgr,
+                                                int noops)
+{
+	struct intel_batchbuffer *buffer;
+	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
+	igt_assert(buffer);
+	igt_create_noop_bb(buffer, noops);
+	return buffer;
+}
+
+static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int ringid)
+{
+	struct intel_batchbuffer *noop_bb;
+	*fd = drm_open_driver(DRIVER_INTEL);
+	igt_assert(*fd >= 0);
+	*bufmgr = drm_intel_bufmgr_gem_init(*fd, BATCH_SZ);
+	igt_assert(*bufmgr);
+	drm_intel_bufmgr_gem_enable_reuse(*bufmgr);
+	/* Send a noop batch buffer to force any deferred initialisation */
+	noop_bb = create_noop_bb(*fd, *bufmgr, 5);
+	intel_batchbuffer_flush_on_ring(noop_bb, ringid);
+	intel_batchbuffer_free(noop_bb);
+}
+
+/* Basic test. Check batch buffers of the same priority and with no dependencies
+ * are executed in the order they are submitted.
+ */
+#define NBR_BASIC_FDs (3)
+static void run_test_basic(int in_flight, int ringid)
+{
+	int fd[NBR_BASIC_FDs];
+	int loop;
+	drm_intel_bufmgr *bufmgr[NBR_BASIC_FDs];
+	uint32_t *delay_buf, *ts1_buf, *ts2_buf;
+	struct intel_batchbuffer *ts1_bb, *ts2_bb;
+	struct intel_batchbuffer **in_flight_bbs;
+	uint32_t calibrated_1s;
+	drm_intel_bo *delay_bo, *ts1_bo, *ts2_bo;
+
+	in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *));
+	igt_assert(in_flight_bbs);
+
+	/* Need multiple i915 fd's. Scheduler will not change execution order of
+	 * batch buffers from the same context.
+	 */
+	for(loop=0; loop < NBR_BASIC_FDs; loop++)
+		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
+
+
+	/* Create buffer objects */
+	delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
+	ts1_bo = create_and_check_bo(bufmgr[1], "ts1 bo");
+	ts2_bo = create_and_check_bo(bufmgr[2], "ts2 bo");
+
+	/* Put some non zero values in the delay bo */
+	{
+		uint32_t data=0xffffffff;
+		drm_intel_bo_subdata(delay_bo, 0, 4, &data);
+	}
+
+	calibrated_1s = igt_calibrate_delay_bb(fd[0], bufmgr[0], ringid);
+
+	/* Batch buffers to fill the in flight queue */
+	in_flight_bbs[0] = create_delay_bb(fd[0], bufmgr[0], ringid, calibrated_1s, delay_bo);
+	for(loop = 1; loop < in_flight; loop++)
+		in_flight_bbs[loop] = create_noop_bb(fd[0], bufmgr[0], 5);
+
+	/* Extra batch buffers in the scheduler queue */
+	ts1_bb = create_timestamp_bb(fd[1], bufmgr[1], ringid, ts1_bo, NULL, false);
+	ts2_bb = create_timestamp_bb(fd[2], bufmgr[2], ringid, ts2_bo, NULL, false);
+
+	/* Flush batchbuffers */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], ringid);
+	intel_batchbuffer_flush_on_ring(ts1_bb, ringid);
+	intel_batchbuffer_flush_on_ring(ts2_bb, ringid);
+
+	/* This will not return until the bo has finished executing */
+	drm_intel_bo_map(delay_bo, 0);
+	drm_intel_bo_map(ts1_bo, 0);
+	drm_intel_bo_map(ts2_bo, 0);
+
+	delay_buf = delay_bo->virtual;
+	ts1_buf = ts1_bo->virtual;
+	ts2_buf = ts2_bo->virtual;
+
+	igt_debug("Delay Timestamp = 0x%08" PRIx32 "\n", delay_buf[2]);
+	igt_debug("TS1 Timestamp = 0x%08" PRIx32 "\n", ts1_buf[0]);
+	igt_debug("TS2 Timestamp = 0x%08" PRIx32 "\n", ts2_buf[0]);
+
+	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
+	igt_assert_f(delay_buf[0] == 0,
+	             "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n", delay_buf[0]);
+
+	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
+	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
+	             delay_buf[2], ts1_buf[0]);
+	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
+	             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
+	             ts1_buf[0], ts2_buf[0]);
+
+	/* Cleanup */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_free(in_flight_bbs[loop]);
+	intel_batchbuffer_free(ts1_bb);
+	intel_batchbuffer_free(ts2_bb);
+
+	drm_intel_bo_unreference(delay_bo);
+	drm_intel_bo_unreference(ts1_bo);
+	drm_intel_bo_unreference(ts2_bo);
+	for(loop = 0; loop < NBR_BASIC_FDs; loop++) {
+		drm_intel_bufmgr_destroy(bufmgr[loop]);
+		close(fd[loop]);
+	}
+	free(in_flight_bbs);
+}
+
+/* Dependency test.
+ * write=0, Submit batch buffers with read dependencies to all rings. Delay one
+ * with a long executing batch buffer. Check the others are not held up.
+ * write=1, Submit batch buffers with write dependencies to all rings. Delay one
+ * with a long executing batch buffer. Also submit batch buffers with no
+ * dependencies to all rings. Batch buffers with write dependencies should be
+ * executed in submission order. The batch buffers with no dependencies should
+ * not be held up.
+ */
+static void run_test_dependency(int in_flight, int ring, bool write)
+{
+	int fd[NBR_RINGS], fd2[NBR_RINGS];
+	int loop;
+	int prime_fd;
+	uint32_t *delay_buf, *ts_buf[NBR_RINGS], *ts2_buf[NBR_RINGS], *shared_buf;
+	uint32_t calibrated_1s;
+	drm_intel_bufmgr *bufmgr[NBR_RINGS], *bufmgr2[NBR_RINGS];
+	struct intel_batchbuffer *ts_bb[NBR_RINGS], *ts2_bb[NBR_RINGS], **in_flight_bbs;
+	drm_intel_bo *delay_bo, *ts_bo[NBR_RINGS], *ts2_bo[NBR_RINGS], *shared_bo[NBR_RINGS];
+
+	in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *));
+	igt_assert(in_flight_bbs);
+
+	/* Need multiple i915 fd's. Scheduler will not change execution order of
+	 * batch buffers from the same context.
+	 */
+	for(loop=0; loop < NBR_RINGS; loop++) {
+		init_context(&(fd[loop]), &(bufmgr[loop]), rings[loop].id);
+		if(write)
+			init_context(&(fd2[loop]), &(bufmgr2[loop]), rings[loop].id);
+	}
+
+	/* Create buffer objects */
+	delay_bo = create_and_check_bo(bufmgr[ring], "delay bo");
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		ts_bo[loop] = create_and_check_bo(bufmgr[loop], "ts bo");
+		if(write)
+			ts2_bo[loop] = create_and_check_bo(bufmgr2[loop], "ts2 bo");
+	}
+
+	/* Create shared buffer object */
+	shared_bo[0] = create_and_check_bo(bufmgr[0], "shared bo");
+
+	drm_intel_bo_gem_export_to_prime(shared_bo[0], &prime_fd);
+	for(loop = 1; loop < NBR_RINGS; loop++) {
+		shared_bo[loop] = drm_intel_bo_gem_create_from_prime(bufmgr[loop],
+		                                                     prime_fd, BATCH_SZ);
+		igt_assert(shared_bo[loop]);
+	}
+	close(prime_fd);
+
+	/* Put some non zero values in the delay and shared bo */
+	drm_intel_bo_map(delay_bo, 1);
+	delay_buf = delay_bo->virtual;
+	delay_buf[0] = 0xff;
+	drm_intel_bo_unmap(delay_bo);
+	drm_intel_bo_map(shared_bo[0], 1);
+	shared_buf = shared_bo[0]->virtual;
+	shared_buf[0] = 0xff00ff00;
+	drm_intel_bo_unmap(shared_bo[0]);
+
+	calibrated_1s = igt_calibrate_delay_bb(fd[ring], bufmgr[ring], rings[ring].id);
+
+	/* Batch buffers to fill the in flight queue */
+	in_flight_bbs[0] = create_delay_bb(fd[ring], bufmgr[ring], rings[ring].id,
+	                                   calibrated_1s, delay_bo);
+	for(loop = 1; loop < in_flight; loop++)
+		in_flight_bbs[loop] = create_noop_bb(fd[ring], bufmgr[ring], 5);
+
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		ts_bb[loop] = create_timestamp_bb(fd[loop], bufmgr[loop],
+		                                  rings[loop].id, ts_bo[loop],
+		                                  shared_bo[loop], write);
+		if(write)
+			ts2_bb[loop] = create_timestamp_bb(fd2[loop], bufmgr2[loop],
+			                                   rings[loop].id, ts2_bo[loop],
+			                                   NULL, false);
+	}
+
+	/* Flush batchbuffers */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], rings[ring].id);
+
+	intel_batchbuffer_flush_on_ring(ts_bb[ring], rings[ring].id);
+	for(loop = 0; loop < NBR_RINGS; loop++)
+		if((loop != ring) && rings[loop].exists)
+			intel_batchbuffer_flush_on_ring(ts_bb[loop], rings[loop].id);
+
+	if(write) {
+		intel_batchbuffer_flush_on_ring(ts2_bb[ring], rings[ring].id);
+		for(loop = 0; loop < NBR_RINGS; loop++)
+			if((loop != ring) && rings[loop].exists)
+				intel_batchbuffer_flush_on_ring(ts2_bb[loop], rings[loop].id);
+	}
+
+	/* This will not return until the bo has finished executing */
+	drm_intel_bo_map(delay_bo, 0);
+	delay_buf = delay_bo->virtual;
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		drm_intel_bo_map(ts_bo[loop], 0);
+		ts_buf[loop] = ts_bo[loop]->virtual;
+		if(write) {
+			drm_intel_bo_map(ts2_bo[loop], 0);
+			ts2_buf[loop] = ts2_bo[loop]->virtual;
+		}
+	}
+
+	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
+	igt_assert_f(delay_buf[0] == 0,
+	             "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n",
+	             delay_buf[0]);
+
+	igt_debug("%6s delay timestamp = 0x%08" PRIx32 "\n",
+	          rings[ring].name, delay_buf[2]);
+	for(loop = 0; loop < NBR_RINGS; loop++)
+		igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
+		          rings[loop].name, ts_buf[loop][0]);
+
+	if(write) {
+		igt_debug("Independant batch buffers\n");
+		for(loop = 0; loop < NBR_RINGS; loop++)
+			igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
+			          rings[loop].name, ts2_buf[loop][0]);
+	}
+
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		if((loop != ring) && rings[loop].exists) {
+			if(write) {
+				/* Write dependency, delayed ring should run first */
+				igt_assert_f(igt_compare_timestamps(ts_buf[ring][0], ts_buf[loop][0]),
+				             "%s ran before %s - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
+				             rings[loop].name, rings[ring].name,
+				             ts_buf[loop][0], ts_buf[ring][0]);
+				/* Second bb without dependency should run first */
+				igt_assert_f(igt_compare_timestamps(ts2_buf[loop][0], ts_buf[loop][0]),
+				             "(%s) independant bb was held up - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
+				             rings[loop].name, ts_buf[loop][0], ts2_buf[loop][0]);
+			}
+			else
+				/* Read dependency, delayed ring should run last */
+				igt_assert_f(igt_compare_timestamps(ts_buf[loop][0], ts_buf[ring][0]),
+				             "%s ran after %s - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
+				             rings[loop].name, rings[ring].name,
+				             ts_buf[loop][0], ts_buf[ring][0]);
+		}
+	}
+
+	/* Cleanup */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_free(in_flight_bbs[loop]);
+
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		intel_batchbuffer_free(ts_bb[loop]);
+		drm_intel_bo_unreference(ts_bo[loop]);
+		drm_intel_bo_unreference(shared_bo[loop]);
+		if(write) {
+			intel_batchbuffer_free(ts2_bb[loop]);
+			drm_intel_bo_unreference(ts2_bo[loop]);
+		}
+	}
+
+	drm_intel_bo_unreference(delay_bo);
+
+	for(loop = 0; loop < NBR_RINGS; loop++) {
+		drm_intel_bufmgr_destroy(bufmgr[loop]);
+		close(fd[loop]);
+		if(write) {
+			drm_intel_bufmgr_destroy(bufmgr2[loop]);
+			close(fd2[loop]);
+		}
+	}
+
+	free(in_flight_bbs);
+}
+
+igt_main
+{
+	int loop;
+	int in_flight;
+	int fd;
+
+	igt_fixture {
+		int debug_fd;
+		int l;
+		char buf[6];
+		/* Get nbr of batch buffers that the scheduler will queue in the
+		 * HW. If this debugfs file does not exist there is no scheduler
+		 * so skip the test.
+		 */
+		debug_fd = igt_debugfs_open("i915_scheduler_min_flying", O_RDONLY);
+		igt_skip_on(debug_fd == -1);
+		l = read(debug_fd, buf, sizeof(buf)-1);
+		igt_assert(l > 0);
+		igt_assert(l < sizeof(buf));
+		buf[l] = '\0';
+		/* May be a decimal or hex number depending on scheduler version */
+		if(sscanf(buf, "0x%2x", &in_flight) != 1)
+			igt_assert_f(sscanf(buf, "%2d", &in_flight) == 1,
+			             "Error reading from i915_scheduler_min_flying\n");
+		close(debug_fd);
+		igt_debug("in flight = %d\n", in_flight);
+		fd = drm_open_driver(DRIVER_INTEL);
+		igt_assert(fd >= 0);
+		check_rings(fd);
+	}
+
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-basic", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_basic(in_flight, rings[loop].id);
+		}
+
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-read", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_dependency(in_flight, loop, false);
+		}
+
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-write", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_dependency(in_flight, loop, true);
+		}
+
+	igt_fixture {
+		close(fd);
+	}
+}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 3/6] tests/gem_scheduler: Add gem_scheduler test

[Daniele Ceraolo Spurio]

On 10/03/16 11:03, Derek Morton wrote:
> This is intended to test the scheduler behaviour is correct.
> The subtests are
> <ring>-basic
> Tests that batch buffers of the same priority submitted to a ring
> execute in the order they are submitted.
> <ring>-read
> Submits a batch buffer with a read dependency to a buffer object to
> a ring which is held in the scheduler queue by a long running batch
> buffer. Submit batch buffers to other rings that have a read dependency
> to the same buffer object. Ensure they execute before the batch buffer
> being held up behind the long running batch buffer.
> <ring>-write
> Submits a batch buffer with a write dependency to a buffer object to
> a ring which is held in the scheduler queue by a long running batch
> buffer. Submit batch buffers to other rings that have a write dependency
> to the same buffer object. Submit batch buffers with no interdependencies
> to all rings. Ensure the batch buffers that have write dependencies are
> executed in submission order but the batch buffers without interdependencies
> do not get held up.
>
> v2: Addressed review comments from Daniele Ceraolo Spurio
>
> v3: Added logic to use generic BSD ring if there is 1 and BSD1 / BSD2 if there
>      are 2.
>
> Signed-off-by: Derek Morton<derek.j.morton@intel.com>
> ---
>   tests/Makefile.sources |   1 +
>   tests/gem_scheduler.c  | 459 +++++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 460 insertions(+)
>   create mode 100644 tests/gem_scheduler.c
>
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index f8b18b0..c88e045 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -66,6 +66,7 @@ TESTS_progs_M = \
>   	gem_request_retire \
>   	gem_reset_stats \
>   	gem_ringfill \
> +	gem_scheduler \
>   	gem_set_tiling_vs_blt \
>   	gem_softpin \
>   	gem_stolen \
> diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
> new file mode 100644
> index 0000000..436440a
> --- /dev/null
> +++ b/tests/gem_scheduler.c
> @@ -0,0 +1,459 @@
> +/*
> + * Copyright © 2016 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + * Authors:
> + *    Derek Morton<derek.j.morton@intel.com>
> + *
> + */
> +
> +#include "igt.h"
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <inttypes.h>
> +#include <time.h>
> +#include <sys/stat.h>
> +#include <sys/ioctl.h>
> +#include <fcntl.h>
> +
> +IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant "
> +                     "batch buffers of the same priority are executed in "
> +                     "submission order. Read-read tests ensure "
> +                     "batch buffers with a read dependency to the same buffer "
> +                     "object do not block each other. Write-write dependency "
> +                     "tests ensure batch buffers with a write dependency to a "
> +                     "buffer object will be executed in submission order but "
> +                     "will not block execution of other independant batch "
> +                     "buffers.");
> +
> +#define SEC_TO_NSEC (1000 * 1000 * 1000)
> +
> +static struct ring {
> +	const char *name;
> +	int id;
> +	bool exists;
> +} rings[] = {
> +	{ "render", I915_EXEC_RENDER, false },
> +	{ "bsd",    I915_EXEC_BSD , false },
> +	{ "bsd2",    I915_EXEC_BSD | 2<<13, false },
> +	{ "blt",    I915_EXEC_BLT, false },
> +	{ "vebox",  I915_EXEC_VEBOX, false },
> +};

If you can't use intel_execution_engines could you add a comment to 
explain why? also considering the renaming going on in the kernel you 
cold replace "ring" with "engine"

> +
> +#define NBR_RINGS (sizeof(rings)/sizeof(struct ring))
> +
> +static void check_rings(int fd) {
> +	int loop;
> +	for(loop=0; loop < NBR_RINGS; loop++) {

Style: space between for/if and "(" in this file

> +		if(gem_has_ring(fd, rings[loop].id)) {
> +			if(rings[loop].id == (I915_EXEC_BSD | 2<<13)) {
> +				rings[loop].exists = gem_has_bsd2(fd);
> +			}
> +			else {
> +				rings[loop].exists = true;

You're marking the VEBOX as existing so adding a gen requirement in this 
function would be nice. I know that the library you're using already 
requires gen8, but the requirement doesn't look to be explicit anywhere 
in this file so it would be nice to add it for clarity.

> +				if(rings[loop].id == I915_EXEC_BSD)
> +					/* If there is BSD2, need to make BSD1
> +					 * explicit.
> +					 */
> +					if(gem_has_bsd2(fd))
> +						rings[loop].id |= (1<<13);
> +			}
> +		}
> +	}
> +}
> +
> +static drm_intel_bo *create_and_check_bo(drm_intel_bufmgr *bufmgr, const char *desc)
> +{
> +	drm_intel_bo *bo = drm_intel_bo_alloc(bufmgr, desc, BATCH_SZ, BATCH_SZ);
> +	igt_assert_f(bo, "Failed allocating %s\n", desc);
> +	return bo;
> +}
> +
> +static struct intel_batchbuffer *create_delay_bb(int fd, drm_intel_bufmgr *bufmgr,
> +                                                 int ringid, uint32_t loops,
> +                                                 drm_intel_bo *dest)
> +{
> +	struct intel_batchbuffer *buffer;
> +	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
> +	igt_assert(buffer);
> +	igt_create_delay_bb(fd, buffer, ringid, loops, dest);
> +	return buffer;
> +}
> +
> +static struct intel_batchbuffer *create_timestamp_bb(int fd, drm_intel_bufmgr *bufmgr,
> +                                                     int ringid, drm_intel_bo *dest,
> +                                                     drm_intel_bo *load, bool write)
> +{
> +	struct intel_batchbuffer *buffer;
> +	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
> +	igt_assert(buffer);
> +	igt_create_timestamp_bb(fd, buffer, ringid, dest, load, write);
> +	return buffer;
> +}
> +
> +static struct intel_batchbuffer *create_noop_bb(int fd, drm_intel_bufmgr *bufmgr,
> +                                                int noops)
> +{
> +	struct intel_batchbuffer *buffer;
> +	buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
> +	igt_assert(buffer);
> +	igt_create_noop_bb(buffer, noops);
> +	return buffer;
> +}
> +
> +static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int ringid)

init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're 
not init_context might be a bit misleading as a name as you're not  a 
ctx directly, although initializing

> +{
> +	struct intel_batchbuffer *noop_bb;
> +	*fd = drm_open_driver(DRIVER_INTEL);
> +	igt_assert(*fd >= 0);
> +	*bufmgr = drm_intel_bufmgr_gem_init(*fd, BATCH_SZ);
> +	igt_assert(*bufmgr);
> +	drm_intel_bufmgr_gem_enable_reuse(*bufmgr);
> +	/* Send a noop batch buffer to force any deferred initialisation */
> +	noop_bb = create_noop_bb(*fd, *bufmgr, 5);
> +	intel_batchbuffer_flush_on_ring(noop_bb, ringid);
> +	intel_batchbuffer_free(noop_bb);
> +}
> +
> +/* Basic test. Check batch buffers of the same priority and with no dependencies
> + * are executed in the order they are submitted.
> + */
> +#define NBR_BASIC_FDs (3)
> +static void run_test_basic(int in_flight, int ringid)
> +{
> +	int fd[NBR_BASIC_FDs];
> +	int loop;
> +	drm_intel_bufmgr *bufmgr[NBR_BASIC_FDs];
> +	uint32_t *delay_buf, *ts1_buf, *ts2_buf;
> +	struct intel_batchbuffer *ts1_bb, *ts2_bb;
> +	struct intel_batchbuffer **in_flight_bbs;
> +	uint32_t calibrated_1s;
> +	drm_intel_bo *delay_bo, *ts1_bo, *ts2_bo;
> +
> +	in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *));
> +	igt_assert(in_flight_bbs);
> +
> +	/* Need multiple i915 fd's. Scheduler will not change execution order of
> +	 * batch buffers from the same context.
> +	 */
> +	for(loop=0; loop < NBR_BASIC_FDs; loop++)
> +		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
> +
> +
> +	/* Create buffer objects */
> +	delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
> +	ts1_bo = create_and_check_bo(bufmgr[1], "ts1 bo");
> +	ts2_bo = create_and_check_bo(bufmgr[2], "ts2 bo");
> +
> +	/* Put some non zero values in the delay bo */

You do this write for every delay batch, so you could move it to 
create_delay_bb (or to the library function)

> +	{

Why is this extra scope needed? "data" doesn't clash with any other 
variable name

> +		uint32_t data=0xffffffff;
> +		drm_intel_bo_subdata(delay_bo, 0, 4, &data);
> +	}
> +
> +	calibrated_1s = igt_calibrate_delay_bb(fd[0], bufmgr[0], ringid);
> +
> +	/* Batch buffers to fill the in flight queue */
> +	in_flight_bbs[0] = create_delay_bb(fd[0], bufmgr[0], ringid, calibrated_1s, delay_bo);
> +	for(loop = 1; loop < in_flight; loop++)
> +		in_flight_bbs[loop] = create_noop_bb(fd[0], bufmgr[0], 5);
> +
> +	/* Extra batch buffers in the scheduler queue */
> +	ts1_bb = create_timestamp_bb(fd[1], bufmgr[1], ringid, ts1_bo, NULL, false);
> +	ts2_bb = create_timestamp_bb(fd[2], bufmgr[2], ringid, ts2_bo, NULL, false);
> +
> +	/* Flush batchbuffers */
> +	for(loop = 0; loop < in_flight; loop++)
> +		intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], ringid);
> +	intel_batchbuffer_flush_on_ring(ts1_bb, ringid);
> +	intel_batchbuffer_flush_on_ring(ts2_bb, ringid);
> +
> +	/* This will not return until the bo has finished executing */
> +	drm_intel_bo_map(delay_bo, 0);
> +	drm_intel_bo_map(ts1_bo, 0);
> +	drm_intel_bo_map(ts2_bo, 0);
> +
> +	delay_buf = delay_bo->virtual;
> +	ts1_buf = ts1_bo->virtual;
> +	ts2_buf = ts2_bo->virtual;
> +
> +	igt_debug("Delay Timestamp = 0x%08" PRIx32 "\n", delay_buf[2]);
> +	igt_debug("TS1 Timestamp = 0x%08" PRIx32 "\n", ts1_buf[0]);
> +	igt_debug("TS2 Timestamp = 0x%08" PRIx32 "\n", ts2_buf[0]);
> +
> +	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
> +	igt_assert_f(delay_buf[0] == 0,
> +	             "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n", delay_buf[0]);
> +
> +	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
> +	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
> +	             delay_buf[2], ts1_buf[0]);
> +	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
> +	             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
> +	             ts1_buf[0], ts2_buf[0]);
> +
> +	/* Cleanup */
> +	for(loop = 0; loop < in_flight; loop++)
> +		intel_batchbuffer_free(in_flight_bbs[loop]);
> +	intel_batchbuffer_free(ts1_bb);
> +	intel_batchbuffer_free(ts2_bb);
> +
> +	drm_intel_bo_unreference(delay_bo);
> +	drm_intel_bo_unreference(ts1_bo);
> +	drm_intel_bo_unreference(ts2_bo);
> +	for(loop = 0; loop < NBR_BASIC_FDs; loop++) {
> +		drm_intel_bufmgr_destroy(bufmgr[loop]);
> +		close(fd[loop]);
> +	}
> +	free(in_flight_bbs);
> +}
> +
> +/* Dependency test.
> + * write=0, Submit batch buffers with read dependencies to all rings. Delay one
> + * with a long executing batch buffer. Check the others are not held up.
> + * write=1, Submit batch buffers with write dependencies to all rings. Delay one
> + * with a long executing batch buffer. Also submit batch buffers with no
> + * dependencies to all rings. Batch buffers with write dependencies should be
> + * executed in submission order. The batch buffers with no dependencies should
> + * not be held up.
> + */
> +static void run_test_dependency(int in_flight, int ring, bool write)
> +{
> +	int fd[NBR_RINGS], fd2[NBR_RINGS];
> +	int loop;
> +	int prime_fd;
> +	uint32_t *delay_buf, *ts_buf[NBR_RINGS], *ts2_buf[NBR_RINGS], *shared_buf;
> +	uint32_t calibrated_1s;
> +	drm_intel_bufmgr *bufmgr[NBR_RINGS], *bufmgr2[NBR_RINGS];
> +	struct intel_batchbuffer *ts_bb[NBR_RINGS], *ts2_bb[NBR_RINGS], **in_flight_bbs;
> +	drm_intel_bo *delay_bo, *ts_bo[NBR_RINGS], *ts2_bo[NBR_RINGS], *shared_bo[NBR_RINGS];
> +
> +	in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *));
> +	igt_assert(in_flight_bbs);
> +
> +	/* Need multiple i915 fd's. Scheduler will not change execution order of
> +	 * batch buffers from the same context.
> +	 */
> +	for(loop=0; loop < NBR_RINGS; loop++) {

You have several loops on all engines but with a bit of reordering you 
should be able to fit everything in less loops. Unless I've missed a 
dependency, you should be able to do something like:

for (loop=0; loop < NBR_RINGS; loop++) {
	init_context(&(fd[loop]), &(bufmgr[loop]), rings[loop].id);
	ts_bo[loop] = create_and_check_bo(bufmgr[loop], "ts bo");

	if (loop == 0)
		shared_bo[0] = create_and_check_bo(...);
		drm_intel_bo_gem_export_to_prime(...);
	else
		shared_bo[loop] = drm_intel_bo_gem_create_from_prime(...);

	ts_bb[loop] = create_timestamp_bb(..);

	if (write) {
		....
	}
}


Also the operations performed in the "if (write)" case are the same of the ones in the main loop with the exception of the shared bo so potentially you could move the whole block in a separate function if it doesn't get too messy.


> +		init_context(&(fd[loop]), &(bufmgr[loop]), rings[loop].id);
> +		if(write)
> +			init_context(&(fd2[loop]), &(bufmgr2[loop]), rings[loop].id);
> +	}
> +
> +	/* Create buffer objects */
> +	delay_bo = create_and_check_bo(bufmgr[ring], "delay bo");
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		ts_bo[loop] = create_and_check_bo(bufmgr[loop], "ts bo");
> +		if(write)
> +			ts2_bo[loop] = create_and_check_bo(bufmgr2[loop], "ts2 bo");
> +	}
> +
> +	/* Create shared buffer object */
> +	shared_bo[0] = create_and_check_bo(bufmgr[0], "shared bo");
> +
> +	drm_intel_bo_gem_export_to_prime(shared_bo[0], &prime_fd);
> +	for(loop = 1; loop < NBR_RINGS; loop++) {
> +		shared_bo[loop] = drm_intel_bo_gem_create_from_prime(bufmgr[loop],
> +		                                                     prime_fd, BATCH_SZ);
> +		igt_assert(shared_bo[loop]);
> +	}
> +	close(prime_fd);
> +
> +	/* Put some non zero values in the delay and shared bo */
> +	drm_intel_bo_map(delay_bo, 1);
> +	delay_buf = delay_bo->virtual;
> +	delay_buf[0] = 0xff;
> +	drm_intel_bo_unmap(delay_bo);
> +	drm_intel_bo_map(shared_bo[0], 1);
> +	shared_buf = shared_bo[0]->virtual;
> +	shared_buf[0] = 0xff00ff00;
> +	drm_intel_bo_unmap(shared_bo[0]);

using subdata might be cleaner here as well

> +
> +	calibrated_1s = igt_calibrate_delay_bb(fd[ring], bufmgr[ring], rings[ring].id);
> +
> +	/* Batch buffers to fill the in flight queue */
> +	in_flight_bbs[0] = create_delay_bb(fd[ring], bufmgr[ring], rings[ring].id,
> +	                                   calibrated_1s, delay_bo);
> +	for(loop = 1; loop < in_flight; loop++)
> +		in_flight_bbs[loop] = create_noop_bb(fd[ring], bufmgr[ring], 5);
> +
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		ts_bb[loop] = create_timestamp_bb(fd[loop], bufmgr[loop],
> +		                                  rings[loop].id, ts_bo[loop],
> +		                                  shared_bo[loop], write);
> +		if(write)
> +			ts2_bb[loop] = create_timestamp_bb(fd2[loop], bufmgr2[loop],
> +			                                   rings[loop].id, ts2_bo[loop],
> +			                                   NULL, false);
> +	}
> +
> +	/* Flush batchbuffers */
> +	for(loop = 0; loop < in_flight; loop++)
> +		intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], rings[ring].id);
> +
> +	intel_batchbuffer_flush_on_ring(ts_bb[ring], rings[ring].id);
> +	for(loop = 0; loop < NBR_RINGS; loop++)
> +		if((loop != ring) && rings[loop].exists)
> +			intel_batchbuffer_flush_on_ring(ts_bb[loop], rings[loop].id);
> +
> +	if(write) {
> +		intel_batchbuffer_flush_on_ring(ts2_bb[ring], rings[ring].id);
> +		for(loop = 0; loop < NBR_RINGS; loop++)
> +			if((loop != ring) && rings[loop].exists)
> +				intel_batchbuffer_flush_on_ring(ts2_bb[loop], rings[loop].id);
> +	}
> +
> +	/* This will not return until the bo has finished executing */
> +	drm_intel_bo_map(delay_bo, 0);
> +	delay_buf = delay_bo->virtual;
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		drm_intel_bo_map(ts_bo[loop], 0);
> +		ts_buf[loop] = ts_bo[loop]->virtual;
> +		if(write) {
> +			drm_intel_bo_map(ts2_bo[loop], 0);
> +			ts2_buf[loop] = ts2_bo[loop]->virtual;
> +		}
> +	}
> +
> +	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
> +	igt_assert_f(delay_buf[0] == 0,
> +	             "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n",
> +	             delay_buf[0]);
> +
> +	igt_debug("%6s delay timestamp = 0x%08" PRIx32 "\n",
> +	          rings[ring].name, delay_buf[2]);
> +	for(loop = 0; loop < NBR_RINGS; loop++)
> +		igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
> +		          rings[loop].name, ts_buf[loop][0]);
> +
> +	if(write) {
> +		igt_debug("Independant batch buffers\n");
> +		for(loop = 0; loop < NBR_RINGS; loop++)
> +			igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
> +			          rings[loop].name, ts2_buf[loop][0]);
> +	}
> +
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		if((loop != ring) && rings[loop].exists) {
> +			if(write) {
> +				/* Write dependency, delayed ring should run first */
> +				igt_assert_f(igt_compare_timestamps(ts_buf[ring][0], ts_buf[loop][0]),
> +				             "%s ran before %s - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
> +				             rings[loop].name, rings[ring].name,
> +				             ts_buf[loop][0], ts_buf[ring][0]);
> +				/* Second bb without dependency should run first */
> +				igt_assert_f(igt_compare_timestamps(ts2_buf[loop][0], ts_buf[loop][0]),
> +				             "(%s) independant bb was held up - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
> +				             rings[loop].name, ts_buf[loop][0], ts2_buf[loop][0]);
> +			}
> +			else
> +				/* Read dependency, delayed ring should run last */
> +				igt_assert_f(igt_compare_timestamps(ts_buf[loop][0], ts_buf[ring][0]),
> +				             "%s ran after %s - 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
> +				             rings[loop].name, rings[ring].name,
> +				             ts_buf[loop][0], ts_buf[ring][0]);
> +		}
> +	}
> +
> +	/* Cleanup */
> +	for(loop = 0; loop < in_flight; loop++)
> +		intel_batchbuffer_free(in_flight_bbs[loop]);
> +
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		intel_batchbuffer_free(ts_bb[loop]);
> +		drm_intel_bo_unreference(ts_bo[loop]);
> +		drm_intel_bo_unreference(shared_bo[loop]);
> +		if(write) {
> +			intel_batchbuffer_free(ts2_bb[loop]);
> +			drm_intel_bo_unreference(ts2_bo[loop]);
> +		}
> +	}
> +
> +	drm_intel_bo_unreference(delay_bo);

You can move this unreferencing to before the above loop and then squash 
the 2 loops onNBR_RINGSinto one.

Regards,
Daniele

> +
> +	for(loop = 0; loop < NBR_RINGS; loop++) {
> +		drm_intel_bufmgr_destroy(bufmgr[loop]);
> +		close(fd[loop]);
> +		if(write) {
> +			drm_intel_bufmgr_destroy(bufmgr2[loop]);
> +			close(fd2[loop]);
> +		}
> +	}
> +
> +	free(in_flight_bbs);
> +}
> +
> +igt_main
> +{
> +	int loop;
> +	int in_flight;
> +	int fd;
> +
> +	igt_fixture {
> +		int debug_fd;
> +		int l;
> +		char buf[6];
> +		/* Get nbr of batch buffers that the scheduler will queue in the
> +		 * HW. If this debugfs file does not exist there is no scheduler
> +		 * so skip the test.
> +		 */
> +		debug_fd = igt_debugfs_open("i915_scheduler_min_flying", O_RDONLY);
> +		igt_skip_on(debug_fd == -1);
> +		l = read(debug_fd, buf, sizeof(buf)-1);
> +		igt_assert(l > 0);
> +		igt_assert(l < sizeof(buf));
> +		buf[l] = '\0';
> +		/* May be a decimal or hex number depending on scheduler version */
> +		if(sscanf(buf, "0x%2x", &in_flight) != 1)
> +			igt_assert_f(sscanf(buf, "%2d", &in_flight) == 1,
> +			             "Error reading from i915_scheduler_min_flying\n");
> +		close(debug_fd);
> +		igt_debug("in flight = %d\n", in_flight);
> +		fd = drm_open_driver(DRIVER_INTEL);
> +		igt_assert(fd >= 0);
> +		check_rings(fd);
> +	}
> +
> +	for (loop=0; loop < NBR_RINGS; loop++)
> +		igt_subtest_f("%s-basic", rings[loop].name) {
> +			gem_require_ring(fd, rings[loop].id);
> +			run_test_basic(in_flight, rings[loop].id);
> +		}
> +
> +	for (loop=0; loop < NBR_RINGS; loop++)
> +		igt_subtest_f("%s-read", rings[loop].name) {
> +			gem_require_ring(fd, rings[loop].id);
> +			run_test_dependency(in_flight, loop, false);
> +		}
> +
> +	for (loop=0; loop < NBR_RINGS; loop++)
> +		igt_subtest_f("%s-write", rings[loop].name) {
> +			gem_require_ring(fd, rings[loop].id);
> +			run_test_dependency(in_flight, loop, true);
> +		}
> +
> +	igt_fixture {
> +		close(fd);
> +	}
> +}

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 3/6] tests/gem_scheduler: Add gem_scheduler test

[Daniele Ceraolo Spurio]

On 17/03/16 08:49, Daniele Ceraolo Spurio wrote:
>
>
> On 10/03/16 11:03, Derek Morton wrote:
>> This is intended to test the scheduler behaviour is correct.
>> The subtests are
>> <ring>-basic
>> Tests that batch buffers of the same priority submitted to a ring
>> execute in the order they are submitted.
>> <ring>-read
>> Submits a batch buffer with a read dependency to a buffer object to
>> a ring which is held in the scheduler queue by a long running batch
>> buffer. Submit batch buffers to other rings that have a read dependency
>> to the same buffer object. Ensure they execute before the batch buffer
>> being held up behind the long running batch buffer.
>> <ring>-write
>> Submits a batch buffer with a write dependency to a buffer object to
>> a ring which is held in the scheduler queue by a long running batch
>> buffer. Submit batch buffers to other rings that have a write dependency
>> to the same buffer object. Submit batch buffers with no 
>> interdependencies
>> to all rings. Ensure the batch buffers that have write dependencies are
>> executed in submission order but the batch buffers without 
>> interdependencies
>> do not get held up.
>>
>> v2: Addressed review comments from Daniele Ceraolo Spurio
>>
>> v3: Added logic to use generic BSD ring if there is 1 and BSD1 / BSD2 
>> if there
>>      are 2.
>>
>> Signed-off-by: Derek Morton<derek.j.morton@intel.com>
>> ---
>>   tests/Makefile.sources |   1 +
>>   tests/gem_scheduler.c  | 459 
>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 460 insertions(+)
>>   create mode 100644 tests/gem_scheduler.c
>>
>> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
>> index f8b18b0..c88e045 100644
>> --- a/tests/Makefile.sources
>> +++ b/tests/Makefile.sources
>> @@ -66,6 +66,7 @@ TESTS_progs_M = \
>>       gem_request_retire \
>>       gem_reset_stats \
>>       gem_ringfill \
>> +    gem_scheduler \
>>       gem_set_tiling_vs_blt \
>>       gem_softpin \
>>       gem_stolen \
>> diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
>> new file mode 100644
>> index 0000000..436440a
>> --- /dev/null
>> +++ b/tests/gem_scheduler.c
>> @@ -0,0 +1,459 @@
>> +/*
>> + * Copyright © 2016 Intel Corporation
>> + *
>> + * Permission is hereby granted, free of charge, to any person 
>> obtaining a
>> + * copy of this software and associated documentation files (the 
>> "Software"),
>> + * to deal in the Software without restriction, including without 
>> limitation
>> + * the rights to use, copy, modify, merge, publish, distribute, 
>> sublicense,
>> + * and/or sell copies of the Software, and to permit persons to whom 
>> the
>> + * Software is furnished to do so, subject to the following conditions:
>> + *
>> + * The above copyright notice and this permission notice (including 
>> the next
>> + * paragraph) shall be included in all copies or substantial 
>> portions of the
>> + * Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
>> EXPRESS OR
>> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
>> MERCHANTABILITY,
>> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT 
>> SHALL
>> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES 
>> OR OTHER
>> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, 
>> ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR 
>> OTHER DEALINGS
>> + * IN THE SOFTWARE.
>> + *
>> + * Authors:
>> + *    Derek Morton<derek.j.morton@intel.com>
>> + *
>> + */
>> +
>> +#include "igt.h"
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <stdint.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <inttypes.h>
>> +#include <time.h>
>> +#include <sys/stat.h>
>> +#include <sys/ioctl.h>
>> +#include <fcntl.h>
>> +
>> +IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure 
>> independant "
>> +                     "batch buffers of the same priority are 
>> executed in "
>> +                     "submission order. Read-read tests ensure "
>> +                     "batch buffers with a read dependency to the 
>> same buffer "
>> +                     "object do not block each other. Write-write 
>> dependency "
>> +                     "tests ensure batch buffers with a write 
>> dependency to a "
>> +                     "buffer object will be executed in submission 
>> order but "
>> +                     "will not block execution of other independant 
>> batch "
>> +                     "buffers.");
>> +
>> +#define SEC_TO_NSEC (1000 * 1000 * 1000)
>> +
>> +static struct ring {
>> +    const char *name;
>> +    int id;
>> +    bool exists;
>> +} rings[] = {
>> +    { "render", I915_EXEC_RENDER, false },
>> +    { "bsd",    I915_EXEC_BSD , false },
>> +    { "bsd2",    I915_EXEC_BSD | 2<<13, false },
>> +    { "blt",    I915_EXEC_BLT, false },
>> +    { "vebox",  I915_EXEC_VEBOX, false },
>> +};
>
> If you can't use intel_execution_engines could you add a comment to 
> explain why? also considering the renaming going on in the kernel you 
> cold replace "ring" with "engine"
>
>> +
>> +#define NBR_RINGS (sizeof(rings)/sizeof(struct ring))
>> +
>> +static void check_rings(int fd) {
>> +    int loop;
>> +    for(loop=0; loop < NBR_RINGS; loop++) {
>
> Style: space between for/if and "(" in this file
>
>> +        if(gem_has_ring(fd, rings[loop].id)) {
>> +            if(rings[loop].id == (I915_EXEC_BSD | 2<<13)) {
>> +                rings[loop].exists = gem_has_bsd2(fd);
>> +            }
>> +            else {
>> +                rings[loop].exists = true;
>
> You're marking the VEBOX as existing so adding a gen requirement in 
> this function would be nice. I know that the library you're using 
> already requires gen8, but the requirement doesn't look to be explicit 
> anywhere in this file so it would be nice to add it for clarity.
>
>> +                if(rings[loop].id == I915_EXEC_BSD)
>> +                    /* If there is BSD2, need to make BSD1
>> +                     * explicit.
>> +                     */
>> +                    if(gem_has_bsd2(fd))
>> +                        rings[loop].id |= (1<<13);
>> +            }
>> +        }
>> +    }
>> +}
>> +
>> +static drm_intel_bo *create_and_check_bo(drm_intel_bufmgr *bufmgr, 
>> const char *desc)
>> +{
>> +    drm_intel_bo *bo = drm_intel_bo_alloc(bufmgr, desc, BATCH_SZ, 
>> BATCH_SZ);
>> +    igt_assert_f(bo, "Failed allocating %s\n", desc);
>> +    return bo;
>> +}
>> +
>> +static struct intel_batchbuffer *create_delay_bb(int fd, 
>> drm_intel_bufmgr *bufmgr,
>> +                                                 int ringid, 
>> uint32_t loops,
>> +                                                 drm_intel_bo *dest)
>> +{
>> +    struct intel_batchbuffer *buffer;
>> +    buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
>> +    igt_assert(buffer);
>> +    igt_create_delay_bb(fd, buffer, ringid, loops, dest);
>> +    return buffer;
>> +}
>> +
>> +static struct intel_batchbuffer *create_timestamp_bb(int fd, 
>> drm_intel_bufmgr *bufmgr,
>> +                                                     int ringid, 
>> drm_intel_bo *dest,
>> + drm_intel_bo *load, bool write)
>> +{
>> +    struct intel_batchbuffer *buffer;
>> +    buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
>> +    igt_assert(buffer);
>> +    igt_create_timestamp_bb(fd, buffer, ringid, dest, load, write);
>> +    return buffer;
>> +}
>> +
>> +static struct intel_batchbuffer *create_noop_bb(int fd, 
>> drm_intel_bufmgr *bufmgr,
>> +                                                int noops)
>> +{
>> +    struct intel_batchbuffer *buffer;
>> +    buffer = intel_batchbuffer_alloc(bufmgr, intel_get_drm_devid(fd));
>> +    igt_assert(buffer);
>> +    igt_create_noop_bb(buffer, noops);
>> +    return buffer;
>> +}
>> +
>> +static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int 
>> ringid)
>
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not 
> init_context might be a bit misleading as a name as you're not  a ctx 
> directly, although initializing

Not really sure how this mess ended up in the email :-P

What I wanted to say is:

init_context might be a bit misleading as a name as you're not 
initializing a ctx directly, although initializing the fd and doing a 
submission you aim to initialize the default context so I'm not sure 
what to suggest as an alternative name.

Regards,
Daniele

>
>> +{
>> +    struct intel_batchbuffer *noop_bb;
>> +    *fd = drm_open_driver(DRIVER_INTEL);
>> +    igt_assert(*fd >= 0);
>> +    *bufmgr = drm_intel_bufmgr_gem_init(*fd, BATCH_SZ);
>> +    igt_assert(*bufmgr);
>> +    drm_intel_bufmgr_gem_enable_reuse(*bufmgr);
>> +    /* Send a noop batch buffer to force any deferred initialisation */
>> +    noop_bb = create_noop_bb(*fd, *bufmgr, 5);
>> +    intel_batchbuffer_flush_on_ring(noop_bb, ringid);
>> +    intel_batchbuffer_free(noop_bb);
>> +}
>> +
>> +/* Basic test. Check batch buffers of the same priority and with no 
>> dependencies
>> + * are executed in the order they are submitted.
>> + */
>> +#define NBR_BASIC_FDs (3)
>> +static void run_test_basic(int in_flight, int ringid)
>> +{
>> +    int fd[NBR_BASIC_FDs];
>> +    int loop;
>> +    drm_intel_bufmgr *bufmgr[NBR_BASIC_FDs];
>> +    uint32_t *delay_buf, *ts1_buf, *ts2_buf;
>> +    struct intel_batchbuffer *ts1_bb, *ts2_bb;
>> +    struct intel_batchbuffer **in_flight_bbs;
>> +    uint32_t calibrated_1s;
>> +    drm_intel_bo *delay_bo, *ts1_bo, *ts2_bo;
>> +
>> +    in_flight_bbs = malloc(in_flight * sizeof(struct 
>> intel_batchbuffer *));
>> +    igt_assert(in_flight_bbs);
>> +
>> +    /* Need multiple i915 fd's. Scheduler will not change execution 
>> order of
>> +     * batch buffers from the same context.
>> +     */
>> +    for(loop=0; loop < NBR_BASIC_FDs; loop++)
>> +        init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
>> +
>> +
>> +    /* Create buffer objects */
>> +    delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
>> +    ts1_bo = create_and_check_bo(bufmgr[1], "ts1 bo");
>> +    ts2_bo = create_and_check_bo(bufmgr[2], "ts2 bo");
>> +
>> +    /* Put some non zero values in the delay bo */
>
> You do this write for every delay batch, so you could move it to 
> create_delay_bb (or to the library function)
>
>> +    {
>
> Why is this extra scope needed? "data" doesn't clash with any other 
> variable name
>
>> +        uint32_t data=0xffffffff;
>> +        drm_intel_bo_subdata(delay_bo, 0, 4, &data);
>> +    }
>> +
>> +    calibrated_1s = igt_calibrate_delay_bb(fd[0], bufmgr[0], ringid);
>> +
>> +    /* Batch buffers to fill the in flight queue */
>> +    in_flight_bbs[0] = create_delay_bb(fd[0], bufmgr[0], ringid, 
>> calibrated_1s, delay_bo);
>> +    for(loop = 1; loop < in_flight; loop++)
>> +        in_flight_bbs[loop] = create_noop_bb(fd[0], bufmgr[0], 5);
>> +
>> +    /* Extra batch buffers in the scheduler queue */
>> +    ts1_bb = create_timestamp_bb(fd[1], bufmgr[1], ringid, ts1_bo, 
>> NULL, false);
>> +    ts2_bb = create_timestamp_bb(fd[2], bufmgr[2], ringid, ts2_bo, 
>> NULL, false);
>> +
>> +    /* Flush batchbuffers */
>> +    for(loop = 0; loop < in_flight; loop++)
>> +        intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], ringid);
>> +    intel_batchbuffer_flush_on_ring(ts1_bb, ringid);
>> +    intel_batchbuffer_flush_on_ring(ts2_bb, ringid);
>> +
>> +    /* This will not return until the bo has finished executing */
>> +    drm_intel_bo_map(delay_bo, 0);
>> +    drm_intel_bo_map(ts1_bo, 0);
>> +    drm_intel_bo_map(ts2_bo, 0);
>> +
>> +    delay_buf = delay_bo->virtual;
>> +    ts1_buf = ts1_bo->virtual;
>> +    ts2_buf = ts2_bo->virtual;
>> +
>> +    igt_debug("Delay Timestamp = 0x%08" PRIx32 "\n", delay_buf[2]);
>> +    igt_debug("TS1 Timestamp = 0x%08" PRIx32 "\n", ts1_buf[0]);
>> +    igt_debug("TS2 Timestamp = 0x%08" PRIx32 "\n", ts2_buf[0]);
>> +
>> +    /* buf[0] in the target buffer should be 0 if the batch buffer 
>> completed */
>> +    igt_assert_f(delay_buf[0] == 0,
>> +                 "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n", 
>> delay_buf[0]);
>> +
>> +    igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
>> +                 "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 
>> ")\n",
>> +                 delay_buf[2], ts1_buf[0]);
>> +    igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
>> +                 "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 
>> ")\n",
>> +                 ts1_buf[0], ts2_buf[0]);
>> +
>> +    /* Cleanup */
>> +    for(loop = 0; loop < in_flight; loop++)
>> +        intel_batchbuffer_free(in_flight_bbs[loop]);
>> +    intel_batchbuffer_free(ts1_bb);
>> +    intel_batchbuffer_free(ts2_bb);
>> +
>> +    drm_intel_bo_unreference(delay_bo);
>> +    drm_intel_bo_unreference(ts1_bo);
>> +    drm_intel_bo_unreference(ts2_bo);
>> +    for(loop = 0; loop < NBR_BASIC_FDs; loop++) {
>> +        drm_intel_bufmgr_destroy(bufmgr[loop]);
>> +        close(fd[loop]);
>> +    }
>> +    free(in_flight_bbs);
>> +}
>> +
>> +/* Dependency test.
>> + * write=0, Submit batch buffers with read dependencies to all 
>> rings. Delay one
>> + * with a long executing batch buffer. Check the others are not held 
>> up.
>> + * write=1, Submit batch buffers with write dependencies to all 
>> rings. Delay one
>> + * with a long executing batch buffer. Also submit batch buffers 
>> with no
>> + * dependencies to all rings. Batch buffers with write dependencies 
>> should be
>> + * executed in submission order. The batch buffers with no 
>> dependencies should
>> + * not be held up.
>> + */
>> +static void run_test_dependency(int in_flight, int ring, bool write)
>> +{
>> +    int fd[NBR_RINGS], fd2[NBR_RINGS];
>> +    int loop;
>> +    int prime_fd;
>> +    uint32_t *delay_buf, *ts_buf[NBR_RINGS], *ts2_buf[NBR_RINGS], 
>> *shared_buf;
>> +    uint32_t calibrated_1s;
>> +    drm_intel_bufmgr *bufmgr[NBR_RINGS], *bufmgr2[NBR_RINGS];
>> +    struct intel_batchbuffer *ts_bb[NBR_RINGS], *ts2_bb[NBR_RINGS], 
>> **in_flight_bbs;
>> +    drm_intel_bo *delay_bo, *ts_bo[NBR_RINGS], *ts2_bo[NBR_RINGS], 
>> *shared_bo[NBR_RINGS];
>> +
>> +    in_flight_bbs = malloc(in_flight * sizeof(struct 
>> intel_batchbuffer *));
>> +    igt_assert(in_flight_bbs);
>> +
>> +    /* Need multiple i915 fd's. Scheduler will not change execution 
>> order of
>> +     * batch buffers from the same context.
>> +     */
>> +    for(loop=0; loop < NBR_RINGS; loop++) {
>
> You have several loops on all engines but with a bit of reordering you 
> should be able to fit everything in less loops. Unless I've missed a 
> dependency, you should be able to do something like:
>
> for (loop=0; loop < NBR_RINGS; loop++) {
>     init_context(&(fd[loop]), &(bufmgr[loop]), rings[loop].id);
>     ts_bo[loop] = create_and_check_bo(bufmgr[loop], "ts bo");
>
>     if (loop == 0)
>         shared_bo[0] = create_and_check_bo(...);
>         drm_intel_bo_gem_export_to_prime(...);
>     else
>         shared_bo[loop] = drm_intel_bo_gem_create_from_prime(...);
>
>     ts_bb[loop] = create_timestamp_bb(..);
>
>     if (write) {
>         ....
>     }
> }
>
>
> Also the operations performed in the "if (write)" case are the same of 
> the ones in the main loop with the exception of the shared bo so 
> potentially you could move the whole block in a separate function if 
> it doesn't get too messy.
>
>
>> +        init_context(&(fd[loop]), &(bufmgr[loop]), rings[loop].id);
>> +        if(write)
>> +            init_context(&(fd2[loop]), &(bufmgr2[loop]), 
>> rings[loop].id);
>> +    }
>> +
>> +    /* Create buffer objects */
>> +    delay_bo = create_and_check_bo(bufmgr[ring], "delay bo");
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        ts_bo[loop] = create_and_check_bo(bufmgr[loop], "ts bo");
>> +        if(write)
>> +            ts2_bo[loop] = create_and_check_bo(bufmgr2[loop], "ts2 
>> bo");
>> +    }
>> +
>> +    /* Create shared buffer object */
>> +    shared_bo[0] = create_and_check_bo(bufmgr[0], "shared bo");
>> +
>> +    drm_intel_bo_gem_export_to_prime(shared_bo[0], &prime_fd);
>> +    for(loop = 1; loop < NBR_RINGS; loop++) {
>> +        shared_bo[loop] = 
>> drm_intel_bo_gem_create_from_prime(bufmgr[loop],
>> + prime_fd, BATCH_SZ);
>> +        igt_assert(shared_bo[loop]);
>> +    }
>> +    close(prime_fd);
>> +
>> +    /* Put some non zero values in the delay and shared bo */
>> +    drm_intel_bo_map(delay_bo, 1);
>> +    delay_buf = delay_bo->virtual;
>> +    delay_buf[0] = 0xff;
>> +    drm_intel_bo_unmap(delay_bo);
>> +    drm_intel_bo_map(shared_bo[0], 1);
>> +    shared_buf = shared_bo[0]->virtual;
>> +    shared_buf[0] = 0xff00ff00;
>> +    drm_intel_bo_unmap(shared_bo[0]);
>
> using subdata might be cleaner here as well
>
>> +
>> +    calibrated_1s = igt_calibrate_delay_bb(fd[ring], bufmgr[ring], 
>> rings[ring].id);
>> +
>> +    /* Batch buffers to fill the in flight queue */
>> +    in_flight_bbs[0] = create_delay_bb(fd[ring], bufmgr[ring], 
>> rings[ring].id,
>> +                                       calibrated_1s, delay_bo);
>> +    for(loop = 1; loop < in_flight; loop++)
>> +        in_flight_bbs[loop] = create_noop_bb(fd[ring], bufmgr[ring], 
>> 5);
>> +
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        ts_bb[loop] = create_timestamp_bb(fd[loop], bufmgr[loop],
>> +                                          rings[loop].id, ts_bo[loop],
>> +                                          shared_bo[loop], write);
>> +        if(write)
>> +            ts2_bb[loop] = create_timestamp_bb(fd2[loop], 
>> bufmgr2[loop],
>> +                                               rings[loop].id, 
>> ts2_bo[loop],
>> +                                               NULL, false);
>> +    }
>> +
>> +    /* Flush batchbuffers */
>> +    for(loop = 0; loop < in_flight; loop++)
>> +        intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], 
>> rings[ring].id);
>> +
>> +    intel_batchbuffer_flush_on_ring(ts_bb[ring], rings[ring].id);
>> +    for(loop = 0; loop < NBR_RINGS; loop++)
>> +        if((loop != ring) && rings[loop].exists)
>> +            intel_batchbuffer_flush_on_ring(ts_bb[loop], 
>> rings[loop].id);
>> +
>> +    if(write) {
>> +        intel_batchbuffer_flush_on_ring(ts2_bb[ring], rings[ring].id);
>> +        for(loop = 0; loop < NBR_RINGS; loop++)
>> +            if((loop != ring) && rings[loop].exists)
>> +                intel_batchbuffer_flush_on_ring(ts2_bb[loop], 
>> rings[loop].id);
>> +    }
>> +
>> +    /* This will not return until the bo has finished executing */
>> +    drm_intel_bo_map(delay_bo, 0);
>> +    delay_buf = delay_bo->virtual;
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        drm_intel_bo_map(ts_bo[loop], 0);
>> +        ts_buf[loop] = ts_bo[loop]->virtual;
>> +        if(write) {
>> +            drm_intel_bo_map(ts2_bo[loop], 0);
>> +            ts2_buf[loop] = ts2_bo[loop]->virtual;
>> +        }
>> +    }
>> +
>> +    /* buf[0] in the target buffer should be 0 if the batch buffer 
>> completed */
>> +    igt_assert_f(delay_buf[0] == 0,
>> +                 "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n",
>> +                 delay_buf[0]);
>> +
>> +    igt_debug("%6s delay timestamp = 0x%08" PRIx32 "\n",
>> +              rings[ring].name, delay_buf[2]);
>> +    for(loop = 0; loop < NBR_RINGS; loop++)
>> +        igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
>> +                  rings[loop].name, ts_buf[loop][0]);
>> +
>> +    if(write) {
>> +        igt_debug("Independant batch buffers\n");
>> +        for(loop = 0; loop < NBR_RINGS; loop++)
>> +            igt_debug("%6s batch timestamp = 0x%08" PRIx32 "\n",
>> +                      rings[loop].name, ts2_buf[loop][0]);
>> +    }
>> +
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        if((loop != ring) && rings[loop].exists) {
>> +            if(write) {
>> +                /* Write dependency, delayed ring should run first */
>> + igt_assert_f(igt_compare_timestamps(ts_buf[ring][0], ts_buf[loop][0]),
>> +                             "%s ran before %s - 0x%08" PRIx32 " vs 
>> 0x%08" PRIx32 "\n",
>> +                             rings[loop].name, rings[ring].name,
>> +                             ts_buf[loop][0], ts_buf[ring][0]);
>> +                /* Second bb without dependency should run first */
>> + igt_assert_f(igt_compare_timestamps(ts2_buf[loop][0], 
>> ts_buf[loop][0]),
>> +                             "(%s) independant bb was held up - 
>> 0x%08" PRIx32 " vs 0x%08" PRIx32 "\n",
>> +                             rings[loop].name, ts_buf[loop][0], 
>> ts2_buf[loop][0]);
>> +            }
>> +            else
>> +                /* Read dependency, delayed ring should run last */
>> + igt_assert_f(igt_compare_timestamps(ts_buf[loop][0], ts_buf[ring][0]),
>> +                             "%s ran after %s - 0x%08" PRIx32 " vs 
>> 0x%08" PRIx32 "\n",
>> +                             rings[loop].name, rings[ring].name,
>> +                             ts_buf[loop][0], ts_buf[ring][0]);
>> +        }
>> +    }
>> +
>> +    /* Cleanup */
>> +    for(loop = 0; loop < in_flight; loop++)
>> +        intel_batchbuffer_free(in_flight_bbs[loop]);
>> +
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        intel_batchbuffer_free(ts_bb[loop]);
>> +        drm_intel_bo_unreference(ts_bo[loop]);
>> +        drm_intel_bo_unreference(shared_bo[loop]);
>> +        if(write) {
>> +            intel_batchbuffer_free(ts2_bb[loop]);
>> +            drm_intel_bo_unreference(ts2_bo[loop]);
>> +        }
>> +    }
>> +
>> +    drm_intel_bo_unreference(delay_bo);
>
> You can move this unreferencing to before the above loop and then 
> squash the 2 loops onNBR_RINGSinto one.
>
> Regards,
> Daniele
>
>> +
>> +    for(loop = 0; loop < NBR_RINGS; loop++) {
>> +        drm_intel_bufmgr_destroy(bufmgr[loop]);
>> +        close(fd[loop]);
>> +        if(write) {
>> +            drm_intel_bufmgr_destroy(bufmgr2[loop]);
>> +            close(fd2[loop]);
>> +        }
>> +    }
>> +
>> +    free(in_flight_bbs);
>> +}
>> +
>> +igt_main
>> +{
>> +    int loop;
>> +    int in_flight;
>> +    int fd;
>> +
>> +    igt_fixture {
>> +        int debug_fd;
>> +        int l;
>> +        char buf[6];
>> +        /* Get nbr of batch buffers that the scheduler will queue in 
>> the
>> +         * HW. If this debugfs file does not exist there is no 
>> scheduler
>> +         * so skip the test.
>> +         */
>> +        debug_fd = igt_debugfs_open("i915_scheduler_min_flying", 
>> O_RDONLY);
>> +        igt_skip_on(debug_fd == -1);
>> +        l = read(debug_fd, buf, sizeof(buf)-1);
>> +        igt_assert(l > 0);
>> +        igt_assert(l < sizeof(buf));
>> +        buf[l] = '\0';
>> +        /* May be a decimal or hex number depending on scheduler 
>> version */
>> +        if(sscanf(buf, "0x%2x", &in_flight) != 1)
>> +            igt_assert_f(sscanf(buf, "%2d", &in_flight) == 1,
>> +                         "Error reading from 
>> i915_scheduler_min_flying\n");
>> +        close(debug_fd);
>> +        igt_debug("in flight = %d\n", in_flight);
>> +        fd = drm_open_driver(DRIVER_INTEL);
>> +        igt_assert(fd >= 0);
>> +        check_rings(fd);
>> +    }
>> +
>> +    for (loop=0; loop < NBR_RINGS; loop++)
>> +        igt_subtest_f("%s-basic", rings[loop].name) {
>> +            gem_require_ring(fd, rings[loop].id);
>> +            run_test_basic(in_flight, rings[loop].id);
>> +        }
>> +
>> +    for (loop=0; loop < NBR_RINGS; loop++)
>> +        igt_subtest_f("%s-read", rings[loop].name) {
>> +            gem_require_ring(fd, rings[loop].id);
>> +            run_test_dependency(in_flight, loop, false);
>> +        }
>> +
>> +    for (loop=0; loop < NBR_RINGS; loop++)
>> +        igt_subtest_f("%s-write", rings[loop].name) {
>> +            gem_require_ring(fd, rings[loop].id);
>> +            run_test_dependency(in_flight, loop, true);
>> +        }
>> +
>> +    igt_fixture {
>> +        close(fd);
>> +    }
>> +}
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 4/6] igt/gem_ctx_param_basic: Updated to support scheduler priority interface

[Derek Morton]

From: John Harrison <John.C.Harrison@Intel.com>

The GPU scheduler has added an execution priority level to the context
object. There is an IOCTL interface to allow user apps/libraries to
set this priority. This patch updates the context paramter IOCTL test
to include the new interface.

For: VIZ-1587
Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 lib/ioctl_wrappers.h        |  1 +
 tests/gem_ctx_param_basic.c | 34 +++++++++++++++++++++++++++++++++-
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h
index e095c41..9e6337e 100644
--- a/lib/ioctl_wrappers.h
+++ b/lib/ioctl_wrappers.h
@@ -107,6 +107,7 @@ struct local_i915_gem_context_param {
 #define LOCAL_CONTEXT_PARAM_BAN_PERIOD	0x1
 #define LOCAL_CONTEXT_PARAM_NO_ZEROMAP	0x2
 #define LOCAL_CONTEXT_PARAM_GTT_SIZE	0x3
+#define LOCAL_CONTEXT_PARAM_PRIORITY	0x4
 	uint64_t value;
 };
 void gem_context_require_ban_period(int fd);
diff --git a/tests/gem_ctx_param_basic.c b/tests/gem_ctx_param_basic.c
index b75800c..585a1a8 100644
--- a/tests/gem_ctx_param_basic.c
+++ b/tests/gem_ctx_param_basic.c
@@ -147,10 +147,42 @@ igt_main
 		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
 	}
 
+	ctx_param.param = LOCAL_CONTEXT_PARAM_PRIORITY;
+
+	igt_subtest("priority-root-set") {
+		ctx_param.context = ctx;
+		ctx_param.value = 2048;
+		TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EINVAL);
+		ctx_param.value = -2048;
+		TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EINVAL);
+		ctx_param.value = 512;
+		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+		ctx_param.value = -512;
+		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+		ctx_param.value = 0;
+		TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+	}
+
+	igt_subtest("priority-non-root-set") {
+		igt_fork(child, 1) {
+			igt_drop_root();
+
+			ctx_param.context = ctx;
+			ctx_param.value = 512;
+			TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM);
+			ctx_param.value = -512;
+			TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+			ctx_param.value = 0;
+			TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM);
+		}
+
+		igt_waitchildren();
+	}
+
 	/* NOTE: This testcase intentionally tests for the next free parameter
 	 * to catch ABI extensions. Don't "fix" this testcase without adding all
 	 * the tests for the new param first. */
-	ctx_param.param = LOCAL_CONTEXT_PARAM_GTT_SIZE + 1;
+	ctx_param.param = LOCAL_CONTEXT_PARAM_PRIORITY + 1;
 
 	igt_subtest("invalid-param-get") {
 		ctx_param.context = ctx;
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 5/6] tests/gem_scheduler: Add subtests to test batch priority behaviour

[Derek Morton]

Add subtests to test each ring to check batch buffers of a higher
priority will be executed before batch buffers of a lower priority.

v2: Addressed review comments from Daniele Ceraolo Spurio

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 tests/gem_scheduler.c | 53 +++++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 8 deletions(-)

diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
index 436440a..126ee97 100644
--- a/tests/gem_scheduler.c
+++ b/tests/gem_scheduler.c
@@ -39,7 +39,8 @@
 
 IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant "
                      "batch buffers of the same priority are executed in "
-                     "submission order. Read-read tests ensure "
+                     "submission order. Priority tests ensure higher priority "
+                     "batch buffers are executed first. Read-read tests ensure "
                      "batch buffers with a read dependency to the same buffer "
                      "object do not block each other. Write-write dependency "
                      "tests ensure batch buffers with a write dependency to a "
@@ -136,11 +137,23 @@ static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int ringid)
 	intel_batchbuffer_free(noop_bb);
 }
 
-/* Basic test. Check batch buffers of the same priority and with no dependencies
- * are executed in the order they are submitted.
+static void set_priority(int fd, int value)
+{
+	struct local_i915_gem_context_param param;
+	param.context = 0; /* Default context */
+	param.size = 0;
+	param.param = LOCAL_CONTEXT_PARAM_PRIORITY;
+	param.value = (uint64_t)value;
+	gem_context_set_param(fd, &param);
+}
+
+/* If 'priority' is 0, check batch buffers of the same priority and with
+ * no dependencies are executed in the order they are submitted.
+ * If 'priority' is set !0, check batch buffers of higher priority are
+ * executed before batch buffers of lower priority.
  */
 #define NBR_BASIC_FDs (3)
-static void run_test_basic(int in_flight, int ringid)
+static void run_test_basic(int in_flight, int ringid, int priority)
 {
 	int fd[NBR_BASIC_FDs];
 	int loop;
@@ -160,6 +173,13 @@ static void run_test_basic(int in_flight, int ringid)
 	for(loop=0; loop < NBR_BASIC_FDs; loop++)
 		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
 
+	/* For high priority set priority of second context to overtake first
+	 * For low priority set priority of first context to be overtaxen by second
+	 */
+	if(priority > 0)
+		set_priority(fd[2], priority);
+	else if(priority < 0)
+		set_priority(fd[1], priority);
 
 	/* Create buffer objects */
 	delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
@@ -209,9 +229,14 @@ static void run_test_basic(int in_flight, int ringid)
 	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
 	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
 	             delay_buf[2], ts1_buf[0]);
-	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
-	             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
-	             ts1_buf[0], ts2_buf[0]);
+	if(priority)
+		igt_assert_f(igt_compare_timestamps(ts2_buf[0], ts1_buf[0]),
+		             "TS2 ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
+		             ts2_buf[0], ts1_buf[0]);
+	else
+		igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
+		             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
+		             ts1_buf[0], ts2_buf[0]);
 
 	/* Cleanup */
 	for(loop = 0; loop < in_flight; loop++)
@@ -438,7 +463,19 @@ igt_main
 	for (loop=0; loop < NBR_RINGS; loop++)
 		igt_subtest_f("%s-basic", rings[loop].name) {
 			gem_require_ring(fd, rings[loop].id);
-			run_test_basic(in_flight, rings[loop].id);
+			run_test_basic(in_flight, rings[loop].id, false);
+		}
+
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-priority-high", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_basic(in_flight, rings[loop].id, 1000);
+		}
+
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-priority-low", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_basic(in_flight, rings[loop].id, -1000);
 		}
 
 	for (loop=0; loop < NBR_RINGS; loop++)
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 5/6] tests/gem_scheduler: Add subtests to test batch priority behaviour

[Daniele Ceraolo Spurio]

On 10/03/16 11:03, Derek Morton wrote:
> Add subtests to test each ring to check batch buffers of a higher
> priority will be executed before batch buffers of a lower priority.
>
> v2: Addressed review comments from Daniele Ceraolo Spurio
>
> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
> ---
>   tests/gem_scheduler.c | 53 +++++++++++++++++++++++++++++++++++++++++++--------
>   1 file changed, 45 insertions(+), 8 deletions(-)
>
> diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
> index 436440a..126ee97 100644
> --- a/tests/gem_scheduler.c
> +++ b/tests/gem_scheduler.c
> @@ -39,7 +39,8 @@
>   
>   IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant "
>                        "batch buffers of the same priority are executed in "
> -                     "submission order. Read-read tests ensure "
> +                     "submission order. Priority tests ensure higher priority "
> +                     "batch buffers are executed first. Read-read tests ensure "
>                        "batch buffers with a read dependency to the same buffer "
>                        "object do not block each other. Write-write dependency "
>                        "tests ensure batch buffers with a write dependency to a "
> @@ -136,11 +137,23 @@ static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int ringid)
>   	intel_batchbuffer_free(noop_bb);
>   }
>   
> -/* Basic test. Check batch buffers of the same priority and with no dependencies
> - * are executed in the order they are submitted.
> +static void set_priority(int fd, int value)
> +{
> +	struct local_i915_gem_context_param param;
> +	param.context = 0; /* Default context */
> +	param.size = 0;
> +	param.param = LOCAL_CONTEXT_PARAM_PRIORITY;
> +	param.value = (uint64_t)value;
> +	gem_context_set_param(fd, &param);
> +}
> +
> +/* If 'priority' is 0, check batch buffers of the same priority and with
> + * no dependencies are executed in the order they are submitted.
> + * If 'priority' is set !0, check batch buffers of higher priority are
> + * executed before batch buffers of lower priority.
>    */
>   #define NBR_BASIC_FDs (3)
> -static void run_test_basic(int in_flight, int ringid)
> +static void run_test_basic(int in_flight, int ringid, int priority)
>   {
>   	int fd[NBR_BASIC_FDs];
>   	int loop;
> @@ -160,6 +173,13 @@ static void run_test_basic(int in_flight, int ringid)
>   	for(loop=0; loop < NBR_BASIC_FDs; loop++)
>   		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
>   
> +	/* For high priority set priority of second context to overtake first
> +	 * For low priority set priority of first context to be overtaxen by second
> +	 */
> +	if(priority > 0)
> +		set_priority(fd[2], priority);
> +	else if(priority < 0)
> +		set_priority(fd[1], priority);
>   
>   	/* Create buffer objects */
>   	delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
> @@ -209,9 +229,14 @@ static void run_test_basic(int in_flight, int ringid)
>   	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
>   	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
>   	             delay_buf[2], ts1_buf[0]);
> -	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
> -	             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
> -	             ts1_buf[0], ts2_buf[0]);
> +	if(priority)
> +		igt_assert_f(igt_compare_timestamps(ts2_buf[0], ts1_buf[0]),
> +		             "TS2 ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
> +		             ts2_buf[0], ts1_buf[0]);
> +	else
> +		igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
> +		             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
> +		             ts1_buf[0], ts2_buf[0]);
>   
>   	/* Cleanup */
>   	for(loop = 0; loop < in_flight; loop++)
> @@ -438,7 +463,19 @@ igt_main
>   	for (loop=0; loop < NBR_RINGS; loop++)
>   		igt_subtest_f("%s-basic", rings[loop].name) {
>   			gem_require_ring(fd, rings[loop].id);
> -			run_test_basic(in_flight, rings[loop].id);
> +			run_test_basic(in_flight, rings[loop].id, false);
> +		}
> +
> +	for (loop=0; loop < NBR_RINGS; loop++)
> +		igt_subtest_f("%s-priority-high", rings[loop].name) {
> +			gem_require_ring(fd, rings[loop].id);
> +			run_test_basic(in_flight, rings[loop].id, 1000);

1000 is a very high priority and it could cause a preemption (when the 
support lands). That shouldn't fail the test because the second batch 
will still overtake the first one but we might end up testing a 
different scenario that the one we're trying to test here, so we could 
use a smaller priority value here and use 1000+ in future preemption 
specific tests.

Regards,
Daniele

> +		}
> +
> +	for (loop=0; loop < NBR_RINGS; loop++)
> +		igt_subtest_f("%s-priority-low", rings[loop].name) {
> +			gem_require_ring(fd, rings[loop].id);
> +			run_test_basic(in_flight, rings[loop].id, -1000);
>   		}
>   
>   	for (loop=0; loop < NBR_RINGS; loop++)

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t v3 5/6] tests/gem_scheduler: Add subtests to test batch priority behaviour

[Morton, Derek J]

>
>
>-----Original Message-----
>From: Ceraolo Spurio, Daniele 
>Sent: Thursday, March 17, 2016 8:58 AM
>To: Morton, Derek J <derek.j.morton@intel.com>; intel-gfx@lists.freedesktop.org
>Subject: Re: [PATCH i-g-t v3 5/6] tests/gem_scheduler: Add subtests to test batch priority behaviour
>
>
>
>On 10/03/16 11:03, Derek Morton wrote:
>> Add subtests to test each ring to check batch buffers of a higher 
>> priority will be executed before batch buffers of a lower priority.
>>
>> v2: Addressed review comments from Daniele Ceraolo Spurio
>>
>> Signed-off-by: Derek Morton <derek.j.morton@intel.com>
>> ---
>>   tests/gem_scheduler.c | 53 +++++++++++++++++++++++++++++++++++++++++++--------
>>   1 file changed, 45 insertions(+), 8 deletions(-)
>>
>> diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c index 
>> 436440a..126ee97 100644
>> --- a/tests/gem_scheduler.c
>> +++ b/tests/gem_scheduler.c
>> @@ -39,7 +39,8 @@
>>   
>>   IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant "
>>                        "batch buffers of the same priority are executed in "
>> -                     "submission order. Read-read tests ensure "
>> +                     "submission order. Priority tests ensure higher priority "
>> +                     "batch buffers are executed first. Read-read tests ensure "
>>                        "batch buffers with a read dependency to the same buffer "
>>                        "object do not block each other. Write-write dependency "
>>                        "tests ensure batch buffers with a write dependency to a "
>> @@ -136,11 +137,23 @@ static void init_context(int *fd, drm_intel_bufmgr **bufmgr, int ringid)
>>   	intel_batchbuffer_free(noop_bb);
>>   }
>>   
>> -/* Basic test. Check batch buffers of the same priority and with no 
>> dependencies
>> - * are executed in the order they are submitted.
>> +static void set_priority(int fd, int value) {
>> +	struct local_i915_gem_context_param param;
>> +	param.context = 0; /* Default context */
>> +	param.size = 0;
>> +	param.param = LOCAL_CONTEXT_PARAM_PRIORITY;
>> +	param.value = (uint64_t)value;
>> +	gem_context_set_param(fd, &param);
>> +}
>> +
>> +/* If 'priority' is 0, check batch buffers of the same priority and 
>> +with
>> + * no dependencies are executed in the order they are submitted.
>> + * If 'priority' is set !0, check batch buffers of higher priority 
>> +are
>> + * executed before batch buffers of lower priority.
>>    */
>>   #define NBR_BASIC_FDs (3)
>> -static void run_test_basic(int in_flight, int ringid)
>> +static void run_test_basic(int in_flight, int ringid, int priority)
>>   {
>>   	int fd[NBR_BASIC_FDs];
>>   	int loop;
>> @@ -160,6 +173,13 @@ static void run_test_basic(int in_flight, int ringid)
>>   	for(loop=0; loop < NBR_BASIC_FDs; loop++)
>>   		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
>>   
>> +	/* For high priority set priority of second context to overtake first
>> +	 * For low priority set priority of first context to be overtaxen by second
>> +	 */
>> +	if(priority > 0)
>> +		set_priority(fd[2], priority);
>> +	else if(priority < 0)
>> +		set_priority(fd[1], priority);
>>   
>>   	/* Create buffer objects */
>>   	delay_bo = create_and_check_bo(bufmgr[0], "delay bo"); @@ -209,9 
>> +229,14 @@ static void run_test_basic(int in_flight, int ringid)
>>   	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
>>   	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
>>   	             delay_buf[2], ts1_buf[0]);
>> -	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
>> -	             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
>> -	             ts1_buf[0], ts2_buf[0]);
>> +	if(priority)
>> +		igt_assert_f(igt_compare_timestamps(ts2_buf[0], ts1_buf[0]),
>> +		             "TS2 ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
>> +		             ts2_buf[0], ts1_buf[0]);
>> +	else
>> +		igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]),
>> +		             "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n",
>> +		             ts1_buf[0], ts2_buf[0]);
>>   
>>   	/* Cleanup */
>>   	for(loop = 0; loop < in_flight; loop++) @@ -438,7 +463,19 @@ 
>> igt_main
>>   	for (loop=0; loop < NBR_RINGS; loop++)
>>   		igt_subtest_f("%s-basic", rings[loop].name) {
>>   			gem_require_ring(fd, rings[loop].id);
>> -			run_test_basic(in_flight, rings[loop].id);
>> +			run_test_basic(in_flight, rings[loop].id, false);
>> +		}
>> +
>> +	for (loop=0; loop < NBR_RINGS; loop++)
>> +		igt_subtest_f("%s-priority-high", rings[loop].name) {
>> +			gem_require_ring(fd, rings[loop].id);
>> +			run_test_basic(in_flight, rings[loop].id, 1000);
>
>1000 is a very high priority and it could cause a preemption (when the support lands). That shouldn't fail the test because the second batch will still overtake the first one but we might end up testing a different scenario that the one we're trying to test here, so we could use a smaller priority value here and use 1000+ in future preemption specific tests.

Ok will try 200

>Regards,
>Daniele
>
>> +		}
>> +
>> +	for (loop=0; loop < NBR_RINGS; loop++)
>> +		igt_subtest_f("%s-priority-low", rings[loop].name) {
>> +			gem_require_ring(fd, rings[loop].id);
>> +			run_test_basic(in_flight, rings[loop].id, -1000);
>>   		}
>>   
>>   	for (loop=0; loop < NBR_RINGS; loop++)
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH i-g-t v3 6/6] gem_scheduler: Added subtests to test priority bumping

[Derek Morton]

When a higher priority batch buffer bumps a lower priority batch
buffer all batch buffers in the scheduler queue get a small priority
increase. Added a subtest to check this behaviour.

Requested by Joonas Lahtinen during scheduler code review

Signed-off-by: Derek Morton <derek.j.morton@intel.com>
---
 tests/gem_scheduler.c | 134 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 134 insertions(+)

diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c
index 126ee97..0fbc6e9 100644
--- a/tests/gem_scheduler.c
+++ b/tests/gem_scheduler.c
@@ -254,6 +254,134 @@ static void run_test_basic(int in_flight, int ringid, int priority)
 	free(in_flight_bbs);
 }
 
+
+/* Test priority bump behaviour.
+ * When a batch buffer is bumped by a higher priority batch buffer all other
+ * batch buffers in the scheduler queue get their priority increased slightly.
+ * Submit two delay batch buffers then fill the in flight queue. Submit a
+ * slightly reduced priority batch buffer and a normal priority batch buffer.
+ * When the first delay batch buffer completes priority bumping should occur.
+ * Submit another normal priority batch buffer. It should not overtake the
+ * reduced priority batch buffer as its priority has been bumped.
+ */
+#define NBR_PRIO_BUMP_FDs (4)
+static void run_test_priority_bump(int in_flight, int ringid)
+{
+	int fd[NBR_PRIO_BUMP_FDs];
+	int loop;
+	drm_intel_bufmgr *bufmgr[NBR_PRIO_BUMP_FDs];
+	uint32_t *delay_buf, *delay2_buf, *ts1_buf, *ts2_buf, *ts3_buf;
+	struct intel_batchbuffer *ts1_bb, *ts2_bb, *ts3_bb;
+	struct intel_batchbuffer **in_flight_bbs;
+	uint32_t calibrated_1s;
+	drm_intel_bo *delay_bo, *delay2_bo ,*ts1_bo, *ts2_bo, *ts3_bo;
+
+	in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *));
+	igt_assert(in_flight_bbs);
+
+	/* Need multiple i915 fd's. Scheduler will not change execution order of
+	 * batch buffers from the same context.
+	 */
+	for(loop=0; loop < NBR_PRIO_BUMP_FDs; loop++)
+		init_context(&(fd[loop]), &(bufmgr[loop]), ringid);
+
+	/* Lower priority of first timestamp batch buffer
+	 * Second timestamp batch buffer should overtake the first
+	 * Second should then get bumped so the third does not overtake it
+	 */
+	set_priority(fd[1], -1);
+
+	/* Create buffer objects */
+	delay_bo = create_and_check_bo(bufmgr[0], "delay bo");
+	delay2_bo = create_and_check_bo(bufmgr[0], "delay bo2");
+	ts1_bo = create_and_check_bo(bufmgr[1], "ts1 bo");
+	ts2_bo = create_and_check_bo(bufmgr[2], "ts2 bo");
+	ts3_bo = create_and_check_bo(bufmgr[3], "ts23 bo");
+
+	/* Put some non zero values in the delay bo */
+	{
+		uint32_t data=0xffffffff;
+		drm_intel_bo_subdata(delay_bo, 0, 4, &data);
+		drm_intel_bo_subdata(delay2_bo, 0, 4, &data);
+	}
+
+	calibrated_1s = igt_calibrate_delay_bb(fd[0], bufmgr[0], ringid);
+
+	/* Batch buffers to fill the in flight queue */
+	in_flight_bbs[0] = create_delay_bb(fd[0], bufmgr[0], ringid, calibrated_1s, delay_bo);
+	in_flight_bbs[1] = create_delay_bb(fd[0], bufmgr[0], ringid, calibrated_1s, delay2_bo);
+	for(loop = 2; loop < in_flight; loop++)
+		in_flight_bbs[loop] = create_noop_bb(fd[0], bufmgr[0], 5);
+
+	/* Extra batch buffers in the scheduler queue */
+	ts1_bb = create_timestamp_bb(fd[1], bufmgr[1], ringid, ts1_bo, NULL, false);
+	ts2_bb = create_timestamp_bb(fd[2], bufmgr[2], ringid, ts2_bo, NULL, false);
+	ts3_bb = create_timestamp_bb(fd[3], bufmgr[3], ringid, ts3_bo, NULL, false);
+
+	/* Flush batchbuffers */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_flush_on_ring(in_flight_bbs[loop], ringid);
+	intel_batchbuffer_flush_on_ring(ts1_bb, ringid);
+	intel_batchbuffer_flush_on_ring(ts2_bb, ringid);
+
+	/* This will not return until the bo has finished executing */
+	drm_intel_bo_map(delay_bo, 0);
+	/* Once the first delay is complete and any bumping has occured, submit
+	 * the final batch buffer
+	 */
+	intel_batchbuffer_flush_on_ring(ts3_bb, ringid);
+
+	drm_intel_bo_map(delay2_bo, 0);
+	drm_intel_bo_map(ts1_bo, 0);
+	drm_intel_bo_map(ts2_bo, 0);
+	drm_intel_bo_map(ts3_bo, 0);
+
+	delay_buf = delay_bo->virtual;
+	delay2_buf = delay2_bo->virtual;
+	ts1_buf = ts1_bo->virtual;
+	ts2_buf = ts2_bo->virtual;
+	ts3_buf = ts3_bo->virtual;
+
+	igt_debug("Delay Timestamp = 0x%08" PRIx32 "\n", delay_buf[2]);
+	igt_debug("Delay Timestamp = 0x%08" PRIx32 "\n", delay2_buf[2]);
+	igt_debug("TS1 Timestamp = 0x%08" PRIx32 "\n", ts1_buf[0]);
+	igt_debug("TS2 Timestamp = 0x%08" PRIx32 "\n", ts2_buf[0]);
+	igt_debug("TS3 Timestamp = 0x%08" PRIx32 "\n", ts3_buf[0]);
+
+	/* buf[0] in the target buffer should be 0 if the batch buffer completed */
+	igt_assert_f(delay_buf[0] == 0,
+	             "delay_buf[0] expected 0x0, got 0x%" PRIx32 "\n", delay_buf[0]);
+
+	igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]),
+	             "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
+	             delay_buf[2], ts1_buf[0]);
+
+	igt_assert_f(igt_compare_timestamps(ts2_buf[0], ts1_buf[0]),
+	             "TS2 ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n",
+	             ts2_buf[0], ts1_buf[0]);
+
+	igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts3_buf[0]),
+	             "TS1 ts (0x%08" PRIx32 ") > TS3 ts (0x%08" PRIx32 ")\n",
+	             ts1_buf[0], ts3_buf[0]);
+
+	/* Cleanup */
+	for(loop = 0; loop < in_flight; loop++)
+		intel_batchbuffer_free(in_flight_bbs[loop]);
+	intel_batchbuffer_free(ts1_bb);
+	intel_batchbuffer_free(ts2_bb);
+
+	drm_intel_bo_unreference(delay_bo);
+	drm_intel_bo_unreference(delay2_bo);
+	drm_intel_bo_unreference(ts1_bo);
+	drm_intel_bo_unreference(ts2_bo);
+	drm_intel_bo_unreference(ts3_bo);
+	for(loop = 0; loop < NBR_PRIO_BUMP_FDs; loop++) {
+		drm_intel_bufmgr_destroy(bufmgr[loop]);
+		close(fd[loop]);
+	}
+	free(in_flight_bbs);
+}
+
 /* Dependency test.
  * write=0, Submit batch buffers with read dependencies to all rings. Delay one
  * with a long executing batch buffer. Check the others are not held up.
@@ -490,6 +618,12 @@ igt_main
 			run_test_dependency(in_flight, loop, true);
 		}
 
+	for (loop=0; loop < NBR_RINGS; loop++)
+		igt_subtest_f("%s-priority-bump", rings[loop].name) {
+			gem_require_ring(fd, rings[loop].id);
+			run_test_priority_bump(in_flight, rings[loop].id);
+		}
+
 	igt_fixture {
 		close(fd);
 	}
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx