[Intel-gfx] [PATCH i-g-t 0/8] Fixes for gem_exec_capture

[Intel-gfx] [PATCH i-g-t 0/8] Fixes for gem_exec_capture

[John.C.Harrison]

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

Fix a bunch of issues with gem_exec_capture with the ultimate aim of
making it pass on GuC enabled platforms.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>


John Harrison (8):
  tests/i915/gem_exec_capture: Remove pointless assert
  tests/i915/gem_exec_capture: Cope with larger page sizes
  tests/i915/gem_exec_capture: Make the error decode a common helper
  tests/i915/gem_exec_capture: Use contexts and engines properly
  tests/i915/gem_exec_capture: Check for memory allocation failure
  lib/igt_sysfs: Support large files
  lib/igt_gt: Allow per engine reset testing
  tests/i915/gem_exec_capture: Update to support GuC based resets

 lib/igt_gt.c                  |  44 ++--
 lib/igt_gt.h                  |   1 +
 lib/igt_sysfs.c               |  17 +-
 tests/i915/gem_exec_capture.c | 472 ++++++++++++++++++++--------------
 4 files changed, 317 insertions(+), 217 deletions(-)

-- 
2.25.1

[Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[John.C.Harrison]

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

The 'many' test ended with an 'assert(count)', presumably meaning to
ensure that some objects were actually captured. However, 'count' is
the number of objects created not how many were captured. Plus, there
is already a 'require(count > 1)' at the start and count is invarient
so the final assert is basically pointless.

General concensus appears to be that the test should not fail
irrespective of how many blobs are captured as low memory situations
could cause the capture to be abbreviated. So just remove the
pointless assert completely.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 7e0a8b8ad..53649cdb2 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 	}
 	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
 		 blobs, size >> 12, count);
-	igt_assert(count);
 
 	free(error);
 	free(offsets);
-- 
2.25.1

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:37PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The 'many' test ended with an 'assert(count)', presumably meaning to
> ensure that some objects were actually captured. However, 'count' is
> the number of objects created not how many were captured. Plus, there
> is already a 'require(count > 1)' at the start and count is invarient
> so the final assert is basically pointless.
> 
> General concensus appears to be that the test should not fail
> irrespective of how many blobs are captured as low memory situations
> could cause the capture to be abbreviated. So just remove the
> pointless assert completely.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  tests/i915/gem_exec_capture.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 7e0a8b8ad..53649cdb2 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  	}
>  	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
>  		 blobs, size >> 12, count);
> -	igt_assert(count);
>  
>  	free(error);
>  	free(offsets);
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[Tvrtko Ursulin]

On 22/10/2021 00:40, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The 'many' test ended with an 'assert(count)', presumably meaning to
> ensure that some objects were actually captured. However, 'count' is
> the number of objects created not how many were captured. Plus, there
> is already a 'require(count > 1)' at the start and count is invarient
> so the final assert is basically pointless.
> 
> General concensus appears to be that the test should not fail
> irrespective of how many blobs are captured as low memory situations
> could cause the capture to be abbreviated. So just remove the
> pointless assert completely.

Hm the test appears to be using intel_get_avail_ram_mb() to size the 
working set. Suggesting problems with low memory situations should not 
apply unless bugs. In which case would a better fix be improving the 
sizing logic and changing the assert to igt_assert(blobs)?

Regards,

Tvrtko

> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   tests/i915/gem_exec_capture.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 7e0a8b8ad..53649cdb2 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>   	}
>   	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
>   		 blobs, size >> 12, count);
> -	igt_assert(count);
>   
>   	free(error);
>   	free(offsets);
> 

Re: [Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[John Harrison]

On 11/3/2021 06:50, Tvrtko Ursulin wrote:
> On 22/10/2021 00:40, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The 'many' test ended with an 'assert(count)', presumably meaning to
>> ensure that some objects were actually captured. However, 'count' is
>> the number of objects created not how many were captured. Plus, there
>> is already a 'require(count > 1)' at the start and count is invarient
>> so the final assert is basically pointless.
>>
>> General concensus appears to be that the test should not fail
>> irrespective of how many blobs are captured as low memory situations
>> could cause the capture to be abbreviated. So just remove the
>> pointless assert completely.
>
> Hm the test appears to be using intel_get_avail_ram_mb() to size the 
> working set. Suggesting problems with low memory situations should not 
> apply unless bugs. In which case would a better fix be improving the 
> sizing logic and changing the assert to igt_assert(blobs)?
After fixing the sysfs read code to cope with large files, I don't ever 
see abbreviated captures any more. However, other reviewers objected to 
asserting anything at all about the final count (whether full size, zero 
or whatever) on the grounds that low memory issues *might* still occur. 
And some in quite blunt language as I recall. If you think different, 
feel free to start your own patch set.

John.

>
> Regards,
>
> Tvrtko
>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   tests/i915/gem_exec_capture.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/tests/i915/gem_exec_capture.c 
>> b/tests/i915/gem_exec_capture.c
>> index 7e0a8b8ad..53649cdb2 100644
>> --- a/tests/i915/gem_exec_capture.c
>> +++ b/tests/i915/gem_exec_capture.c
>> @@ -524,7 +524,6 @@ static void many(int fd, int dir, uint64_t size, 
>> unsigned int flags)
>>       }
>>       igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
>>            blobs, size >> 12, count);
>> -    igt_assert(count);
>>         free(error);
>>       free(offsets);
>>

Re: [Intel-gfx] [PATCH i-g-t 1/8] tests/i915/gem_exec_capture: Remove pointless assert

[Tvrtko Ursulin]

On 03/11/2021 18:44, John Harrison wrote:
> On 11/3/2021 06:50, Tvrtko Ursulin wrote:
>> On 22/10/2021 00:40, John.C.Harrison@Intel.com wrote:
>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>
>>> The 'many' test ended with an 'assert(count)', presumably meaning to
>>> ensure that some objects were actually captured. However, 'count' is
>>> the number of objects created not how many were captured. Plus, there
>>> is already a 'require(count > 1)' at the start and count is invarient
>>> so the final assert is basically pointless.
>>>
>>> General concensus appears to be that the test should not fail
>>> irrespective of how many blobs are captured as low memory situations
>>> could cause the capture to be abbreviated. So just remove the
>>> pointless assert completely.
>>
>> Hm the test appears to be using intel_get_avail_ram_mb() to size the 
>> working set. Suggesting problems with low memory situations should not 
>> apply unless bugs. In which case would a better fix be improving the 
>> sizing logic and changing the assert to igt_assert(blobs)?
> After fixing the sysfs read code to cope with large files, I don't ever 
> see abbreviated captures any more. However, other reviewers objected to 
> asserting anything at all about the final count (whether full size, zero 
> or whatever) on the grounds that low memory issues *might* still occur. 
> And some in quite blunt language as I recall. If you think different, 
> feel free to start your own patch set.

Do you have a link so I can understand the discussion? Because from the 
top of my head I can't imagine what were the objections, I mean what is 
the point of keeping the test but not asserting at the end at least 
something was captured?

Regards,

Tvrtko

[Intel-gfx] [PATCH i-g-t 2/8] tests/i915/gem_exec_capture: Cope with larger page sizes

[John.C.Harrison]

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

At some point, larger than 4KB page sizes were added to the i915
driver. This included adding an informational line to the buffer
entries in error capture logs. However, the error capture test was not
updated to skip this string, thus it would silently abort processing.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 53649cdb2..47ca64dd6 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 		addr |= strtoul(str + 1, &str, 16);
 		igt_assert(*str++ == '\n');
 
+		/* gtt_page_sizes = 0x00010000 */
+		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
+			str += 19 + 8;
+			igt_assert(*str++ == '\n');
+		}
+
 		if (!(*str == ':' || *str == '~'))
 			continue;
 
-- 
2.25.1

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/8] tests/i915/gem_exec_capture: Cope with larger page sizes

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> At some point, larger than 4KB page sizes were added to the i915
> driver. This included adding an informational line to the buffer
> entries in error capture logs. However, the error capture test was not
> updated to skip this string, thus it would silently abort processing.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  tests/i915/gem_exec_capture.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 53649cdb2..47ca64dd6 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  		addr |= strtoul(str + 1, &str, 16);
>  		igt_assert(*str++ == '\n');
>  
> +		/* gtt_page_sizes = 0x00010000 */
> +		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
> +			str += 19 + 8;
> +			igt_assert(*str++ == '\n');
> +		}

Can you explain this logic to me, for the life of me I can't figure out
what this doing. That probably warrent's a more detailed comment too.

Matt 

> +
>  		if (!(*str == ':' || *str == '~'))
>  			continue;
>  
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/8] tests/i915/gem_exec_capture: Cope with larger page sizes

[John Harrison]

On 10/29/2021 10:39, Matthew Brost wrote:
> On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> At some point, larger than 4KB page sizes were added to the i915
>> driver. This included adding an informational line to the buffer
>> entries in error capture logs. However, the error capture test was not
>> updated to skip this string, thus it would silently abort processing.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   tests/i915/gem_exec_capture.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
>> index 53649cdb2..47ca64dd6 100644
>> --- a/tests/i915/gem_exec_capture.c
>> +++ b/tests/i915/gem_exec_capture.c
>> @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>>   		addr |= strtoul(str + 1, &str, 16);
>>   		igt_assert(*str++ == '\n');
>>   
>> +		/* gtt_page_sizes = 0x00010000 */
>> +		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
>> +			str += 19 + 8;
>> +			igt_assert(*str++ == '\n');
>> +		}
> Can you explain this logic to me, for the life of me I can't figure out
> what this doing. That probably warrent's a more detailed comment too.
It's no different to the rest of the processing that this code was 
already doing.

if( start_of_current_line == "gtt_page_sizes = 0x") {
     current_line += strlen(above_string) + strlen(8-digit hex string);
     assert( next_character_of_current_line == end_of_line);
}

I.e. skip over any line that just contains the page size message.

John.

>
> Matt
>
>> +
>>   		if (!(*str == ':' || *str == '~'))
>>   			continue;
>>   
>> -- 
>> 2.25.1
>>

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 2/8] tests/i915/gem_exec_capture: Cope with larger page sizes

[Matthew Brost]

On Fri, Oct 29, 2021 at 05:32:40PM -0700, John Harrison wrote:
> On 10/29/2021 10:39, Matthew Brost wrote:
> > On Thu, Oct 21, 2021 at 04:40:38PM -0700, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > At some point, larger than 4KB page sizes were added to the i915
> > > driver. This included adding an informational line to the buffer
> > > entries in error capture logs. However, the error capture test was not
> > > updated to skip this string, thus it would silently abort processing.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >   tests/i915/gem_exec_capture.c | 6 ++++++
> > >   1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> > > index 53649cdb2..47ca64dd6 100644
> > > --- a/tests/i915/gem_exec_capture.c
> > > +++ b/tests/i915/gem_exec_capture.c
> > > @@ -484,6 +484,12 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
> > >   		addr |= strtoul(str + 1, &str, 16);
> > >   		igt_assert(*str++ == '\n');
> > > +		/* gtt_page_sizes = 0x00010000 */
> > > +		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
> > > +			str += 19 + 8;
> > > +			igt_assert(*str++ == '\n');
> > > +		}
> > Can you explain this logic to me, for the life of me I can't figure out
> > what this doing. That probably warrent's a more detailed comment too.
> It's no different to the rest of the processing that this code was already
> doing.
> 
> if( start_of_current_line == "gtt_page_sizes = 0x") {
>     current_line += strlen(above_string) + strlen(8-digit hex string);
>     assert( next_character_of_current_line == end_of_line);
> }
> 
> I.e. skip over any line that just contains the page size message.
> 

Ok, got it. Not sure I missed that. The magic numbers 19 and 8 where
confusing me but I understand this now.

With that:
Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> John.
> 
> > 
> > Matt
> > 
> > > +
> > >   		if (!(*str == ':' || *str == '~'))
> > >   			continue;
> > > -- 
> > > 2.25.1
> > > 
> 

[Intel-gfx] [PATCH i-g-t 3/8] tests/i915/gem_exec_capture: Make the error decode a common helper

[John.C.Harrison]

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

The decode of the error capture contents was happening in two
different sub-tests with two very different pieces of code. One being
much more extensive than the other (actually decodes and verifies the
contents of the captured buffers rather than just the address). So,
move the code into a common helper function and use that in both
places.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 344 +++++++++++++++++-----------------
 1 file changed, 170 insertions(+), 174 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 47ca64dd6..c85c198f7 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -33,32 +33,175 @@
 
 IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a hang");
 
-static void check_error_state(int dir, struct drm_i915_gem_exec_object2 *obj)
+struct offset {
+	uint64_t addr;
+	unsigned long idx;
+	bool found;
+};
+
+static unsigned long zlib_inflate(uint32_t **ptr, unsigned long len)
+{
+	struct z_stream_s zstream;
+	void *out;
+
+	memset(&zstream, 0, sizeof(zstream));
+
+	zstream.next_in = (unsigned char *)*ptr;
+	zstream.avail_in = 4*len;
+
+	if (inflateInit(&zstream) != Z_OK)
+		return 0;
+
+	out = malloc(128*4096); /* approximate obj size */
+	zstream.next_out = out;
+	zstream.avail_out = 128*4096;
+
+	do {
+		switch (inflate(&zstream, Z_SYNC_FLUSH)) {
+		case Z_STREAM_END:
+			goto end;
+		case Z_OK:
+			break;
+		default:
+			inflateEnd(&zstream);
+			return 0;
+		}
+
+		if (zstream.avail_out)
+			break;
+
+		out = realloc(out, 2*zstream.total_out);
+		if (out == NULL) {
+			inflateEnd(&zstream);
+			return 0;
+		}
+
+		zstream.next_out = (unsigned char *)out + zstream.total_out;
+		zstream.avail_out = zstream.total_out;
+	} while (1);
+end:
+	inflateEnd(&zstream);
+	free(*ptr);
+	*ptr = out;
+	return zstream.total_out / 4;
+}
+
+static unsigned long
+ascii85_decode(char *in, uint32_t **out, bool inflate, char **end)
+{
+	unsigned long len = 0, size = 1024;
+
+	*out = realloc(*out, sizeof(uint32_t)*size);
+	if (*out == NULL)
+		return 0;
+
+	while (*in >= '!' && *in <= 'z') {
+		uint32_t v = 0;
+
+		if (len == size) {
+			size *= 2;
+			*out = realloc(*out, sizeof(uint32_t)*size);
+			if (*out == NULL)
+				return 0;
+		}
+
+		if (*in == 'z') {
+			in++;
+		} else {
+			v += in[0] - 33; v *= 85;
+			v += in[1] - 33; v *= 85;
+			v += in[2] - 33; v *= 85;
+			v += in[3] - 33; v *= 85;
+			v += in[4] - 33;
+			in += 5;
+		}
+		(*out)[len++] = v;
+	}
+	*end = in;
+
+	if (!inflate)
+		return len;
+
+	return zlib_inflate(out, len);
+}
+
+static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
+			     uint64_t obj_size, bool incremental)
 {
 	char *error, *str;
-	bool found = false;
+	int blobs = 0;
 
 	error = igt_sysfs_get(dir, "error");
 	igt_sysfs_set(dir, "error", "Begone!");
-
 	igt_assert(error);
 	igt_debug("%s\n", error);
 
 	/* render ring --- user = 0x00000000 ffffd000 */
-	for (str = error; (str = strstr(str, "--- user = ")); str++) {
+	for (str = error; (str = strstr(str, "--- user = ")); ) {
+		uint32_t *data = NULL;
 		uint64_t addr;
-		uint32_t hi, lo;
+		unsigned long i, sz;
+		unsigned long start;
+		unsigned long end;
 
-		igt_assert(sscanf(str, "--- user = 0x%x %x", &hi, &lo) == 2);
-		addr = hi;
+		if (strncmp(str, "--- user = 0x", 13))
+			break;
+		str += 13;
+		addr = strtoul(str, &str, 16);
 		addr <<= 32;
-		addr |= lo;
-		igt_assert_eq_u64(addr, obj->offset);
-		found = true;
+		addr |= strtoul(str + 1, &str, 16);
+		igt_assert(*str++ == '\n');
+
+		start = 0;
+		end = obj_count;
+		while (end > start) {
+			i = (end - start) / 2 + start;
+			if (obj_offsets[i].addr < addr)
+				start = i + 1;
+			else if (obj_offsets[i].addr > addr)
+				end = i;
+			else
+				break;
+		}
+		igt_assert(obj_offsets[i].addr == addr);
+		igt_assert(!obj_offsets[i].found);
+		obj_offsets[i].found = true;
+		igt_debug("offset:%"PRIx64", index:%ld\n",
+			  addr, obj_offsets[i].idx);
+
+		/* gtt_page_sizes = 0x00010000 */
+		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
+			str += 19 + 8;
+			igt_assert(*str++ == '\n');
+		}
+
+		if (!(*str == ':' || *str == '~'))
+			continue;
+
+		igt_debug("blob:%.64s\n", str);
+		sz = ascii85_decode(str + 1, &data, *str == ':', &str);
+
+		igt_assert_eq(4 * sz, obj_size);
+		igt_assert(*str++ == '\n');
+		str = strchr(str, '-');
+
+		if (incremental) {
+			uint32_t expect;
+
+			expect = obj_offsets[i].idx * obj_size;
+			for (i = 0; i < sz; i++)
+				igt_assert_eq(data[i], expect++);
+		} else {
+			for (i = 0; i < sz; i++)
+				igt_assert_eq(data[i], 0);
+		}
+
+		blobs++;
+		free(data);
 	}
 
 	free(error);
-	igt_assert(found);
+	return blobs;
 }
 
 static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
@@ -73,6 +216,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	struct drm_i915_gem_relocation_entry reloc[2];
 	struct drm_i915_gem_execbuffer2 execbuf;
 	uint32_t *batch, *seqno;
+	struct offset offset;
 	int i;
 
 	memset(obj, 0, sizeof(obj));
@@ -168,7 +312,10 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 
 	/* Check that only the buffer we marked is reported in the error */
 	igt_force_gpu_reset(fd);
-	check_error_state(dir, &obj[CAPTURE]);
+	memset(&offset, 0, sizeof(offset));
+	offset.addr = obj[CAPTURE].offset;
+	igt_assert_eq(check_error_state(dir, &offset, 1, target_size, false), 1);
+	igt_assert(offset.found);
 
 	gem_sync(fd, obj[BATCH].handle);
 
@@ -183,11 +330,12 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
 {
 	uint32_t handle;
 	uint64_t ahnd;
+	int obj_size = 4096;
 
-	handle = gem_create(fd, 4096);
+	handle = gem_create(fd, obj_size);
 	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	__capture1(fd, dir, ahnd, ctx, ring, handle, 4096);
+	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
 
 	gem_close(fd, handle);
 	put_ahnd(ahnd);
@@ -206,10 +354,8 @@ static int cmp(const void *A, const void *B)
 	return 0;
 }
 
-static struct offset {
-	uint64_t addr;
-	unsigned long idx;
-} *__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
+static struct offset *
+__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
 	      unsigned int size, int count,
 	      unsigned int flags)
 #define INCREMENTAL 0x1
@@ -357,98 +503,11 @@ static struct offset {
 	return offsets;
 }
 
-static unsigned long zlib_inflate(uint32_t **ptr, unsigned long len)
-{
-	struct z_stream_s zstream;
-	void *out;
-
-	memset(&zstream, 0, sizeof(zstream));
-
-	zstream.next_in = (unsigned char *)*ptr;
-	zstream.avail_in = 4*len;
-
-	if (inflateInit(&zstream) != Z_OK)
-		return 0;
-
-	out = malloc(128*4096); /* approximate obj size */
-	zstream.next_out = out;
-	zstream.avail_out = 128*4096;
-
-	do {
-		switch (inflate(&zstream, Z_SYNC_FLUSH)) {
-		case Z_STREAM_END:
-			goto end;
-		case Z_OK:
-			break;
-		default:
-			inflateEnd(&zstream);
-			return 0;
-		}
-
-		if (zstream.avail_out)
-			break;
-
-		out = realloc(out, 2*zstream.total_out);
-		if (out == NULL) {
-			inflateEnd(&zstream);
-			return 0;
-		}
-
-		zstream.next_out = (unsigned char *)out + zstream.total_out;
-		zstream.avail_out = zstream.total_out;
-	} while (1);
-end:
-	inflateEnd(&zstream);
-	free(*ptr);
-	*ptr = out;
-	return zstream.total_out / 4;
-}
-
-static unsigned long
-ascii85_decode(char *in, uint32_t **out, bool inflate, char **end)
-{
-	unsigned long len = 0, size = 1024;
-
-	*out = realloc(*out, sizeof(uint32_t)*size);
-	if (*out == NULL)
-		return 0;
-
-	while (*in >= '!' && *in <= 'z') {
-		uint32_t v = 0;
-
-		if (len == size) {
-			size *= 2;
-			*out = realloc(*out, sizeof(uint32_t)*size);
-			if (*out == NULL)
-				return 0;
-		}
-
-		if (*in == 'z') {
-			in++;
-		} else {
-			v += in[0] - 33; v *= 85;
-			v += in[1] - 33; v *= 85;
-			v += in[2] - 33; v *= 85;
-			v += in[3] - 33; v *= 85;
-			v += in[4] - 33;
-			in += 5;
-		}
-		(*out)[len++] = v;
-	}
-	*end = in;
-
-	if (!inflate)
-		return len;
-
-	return zlib_inflate(out, len);
-}
-
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
 {
 	uint64_t ram, gtt, ahnd;
 	unsigned long count, blobs;
 	struct offset *offsets;
-	char *error, *str;
 
 	gtt = gem_aperture_size(fd) / size;
 	ram = (intel_get_avail_ram_mb() << 20) / size;
@@ -463,75 +522,10 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 
 	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
 
-	error = igt_sysfs_get(dir, "error");
-	igt_sysfs_set(dir, "error", "Begone!");
-	igt_assert(error);
-
-	blobs = 0;
-	/* render ring --- user = 0x00000000 ffffd000 */
-	str = strstr(error, "--- user = ");
-	while (str) {
-		uint32_t *data = NULL;
-		unsigned long i, sz;
-		uint64_t addr;
-
-		if (strncmp(str, "--- user = 0x", 13))
-			break;
-
-		str += 13;
-		addr = strtoul(str, &str, 16);
-		addr <<= 32;
-		addr |= strtoul(str + 1, &str, 16);
-		igt_assert(*str++ == '\n');
-
-		/* gtt_page_sizes = 0x00010000 */
-		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
-			str += 19 + 8;
-			igt_assert(*str++ == '\n');
-		}
-
-		if (!(*str == ':' || *str == '~'))
-			continue;
-
-		igt_debug("blob:%.64s\n", str);
-		sz = ascii85_decode(str + 1, &data, *str == ':', &str);
-		igt_assert_eq(4 * sz, size);
-		igt_assert(*str++ == '\n');
-		str = strchr(str, '-');
-
-		if (flags & INCREMENTAL) {
-			unsigned long start = 0;
-			unsigned long end = count;
-			uint32_t expect;
-
-			while (end > start) {
-				i = (end - start) / 2 + start;
-				if (offsets[i].addr < addr)
-					start = i + 1;
-				else if (offsets[i].addr > addr)
-					end = i;
-				else
-					break;
-			}
-			igt_assert(offsets[i].addr == addr);
-			igt_debug("offset:%"PRIx64", index:%ld\n",
-				  addr, offsets[i].idx);
-
-			expect = offsets[i].idx * size;
-			for (i = 0; i < sz; i++)
-				igt_assert_eq(data[i], expect++);
-		} else {
-			for (i = 0; i < sz; i++)
-				igt_assert_eq(data[i], 0);
-		}
-
-		blobs++;
-		free(data);
-	}
+	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
 	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
 		 blobs, size >> 12, count);
 
-	free(error);
 	free(offsets);
 	put_ahnd(ahnd);
 }
@@ -625,12 +619,14 @@ static void userptr(int fd, int dir)
 	uint32_t handle;
 	uint64_t ahnd;
 	void *ptr;
+	int obj_size = 4096;
 
-	igt_assert(posix_memalign(&ptr, 4096, 4096) == 0);
-	igt_require(__gem_userptr(fd, ptr, 4096, 0, 0, &handle) == 0);
+	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
+	memset(ptr, 0, obj_size);
+	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
 	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, 4096);
+	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
 
 	gem_close(fd, handle);
 	put_ahnd(ahnd);
-- 
2.25.1

Re: [Intel-gfx] [PATCH i-g-t 3/8] tests/i915/gem_exec_capture: Make the error decode a common helper

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:39PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The decode of the error capture contents was happening in two
> different sub-tests with two very different pieces of code. One being
> much more extensive than the other (actually decodes and verifies the
> contents of the captured buffers rather than just the address). So,
> move the code into a common helper function and use that in both
> places.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  tests/i915/gem_exec_capture.c | 344 +++++++++++++++++-----------------
>  1 file changed, 170 insertions(+), 174 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 47ca64dd6..c85c198f7 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -33,32 +33,175 @@
>  
>  IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a hang");
>  
> -static void check_error_state(int dir, struct drm_i915_gem_exec_object2 *obj)
> +struct offset {
> +	uint64_t addr;
> +	unsigned long idx;
> +	bool found;
> +};
> +
> +static unsigned long zlib_inflate(uint32_t **ptr, unsigned long len)
> +{
> +	struct z_stream_s zstream;
> +	void *out;
> +
> +	memset(&zstream, 0, sizeof(zstream));
> +
> +	zstream.next_in = (unsigned char *)*ptr;
> +	zstream.avail_in = 4*len;
> +
> +	if (inflateInit(&zstream) != Z_OK)
> +		return 0;
> +
> +	out = malloc(128*4096); /* approximate obj size */
> +	zstream.next_out = out;
> +	zstream.avail_out = 128*4096;
> +
> +	do {
> +		switch (inflate(&zstream, Z_SYNC_FLUSH)) {
> +		case Z_STREAM_END:
> +			goto end;
> +		case Z_OK:
> +			break;
> +		default:
> +			inflateEnd(&zstream);
> +			return 0;
> +		}
> +
> +		if (zstream.avail_out)
> +			break;
> +
> +		out = realloc(out, 2*zstream.total_out);
> +		if (out == NULL) {
> +			inflateEnd(&zstream);
> +			return 0;
> +		}
> +
> +		zstream.next_out = (unsigned char *)out + zstream.total_out;
> +		zstream.avail_out = zstream.total_out;
> +	} while (1);
> +end:
> +	inflateEnd(&zstream);
> +	free(*ptr);
> +	*ptr = out;
> +	return zstream.total_out / 4;
> +}
> +
> +static unsigned long
> +ascii85_decode(char *in, uint32_t **out, bool inflate, char **end)
> +{
> +	unsigned long len = 0, size = 1024;
> +
> +	*out = realloc(*out, sizeof(uint32_t)*size);
> +	if (*out == NULL)
> +		return 0;
> +
> +	while (*in >= '!' && *in <= 'z') {
> +		uint32_t v = 0;
> +
> +		if (len == size) {
> +			size *= 2;
> +			*out = realloc(*out, sizeof(uint32_t)*size);
> +			if (*out == NULL)
> +				return 0;
> +		}
> +
> +		if (*in == 'z') {
> +			in++;
> +		} else {
> +			v += in[0] - 33; v *= 85;
> +			v += in[1] - 33; v *= 85;
> +			v += in[2] - 33; v *= 85;
> +			v += in[3] - 33; v *= 85;
> +			v += in[4] - 33;
> +			in += 5;
> +		}
> +		(*out)[len++] = v;
> +	}
> +	*end = in;
> +
> +	if (!inflate)
> +		return len;
> +
> +	return zlib_inflate(out, len);
> +}
> +
> +static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
> +			     uint64_t obj_size, bool incremental)
>  {
>  	char *error, *str;
> -	bool found = false;
> +	int blobs = 0;
>  
>  	error = igt_sysfs_get(dir, "error");
>  	igt_sysfs_set(dir, "error", "Begone!");
> -
>  	igt_assert(error);
>  	igt_debug("%s\n", error);
>  
>  	/* render ring --- user = 0x00000000 ffffd000 */
> -	for (str = error; (str = strstr(str, "--- user = ")); str++) {
> +	for (str = error; (str = strstr(str, "--- user = ")); ) {
> +		uint32_t *data = NULL;
>  		uint64_t addr;
> -		uint32_t hi, lo;
> +		unsigned long i, sz;
> +		unsigned long start;
> +		unsigned long end;
>  
> -		igt_assert(sscanf(str, "--- user = 0x%x %x", &hi, &lo) == 2);
> -		addr = hi;
> +		if (strncmp(str, "--- user = 0x", 13))
> +			break;
> +		str += 13;
> +		addr = strtoul(str, &str, 16);
>  		addr <<= 32;
> -		addr |= lo;
> -		igt_assert_eq_u64(addr, obj->offset);
> -		found = true;
> +		addr |= strtoul(str + 1, &str, 16);
> +		igt_assert(*str++ == '\n');
> +
> +		start = 0;
> +		end = obj_count;
> +		while (end > start) {
> +			i = (end - start) / 2 + start;
> +			if (obj_offsets[i].addr < addr)
> +				start = i + 1;
> +			else if (obj_offsets[i].addr > addr)
> +				end = i;
> +			else
> +				break;
> +		}
> +		igt_assert(obj_offsets[i].addr == addr);
> +		igt_assert(!obj_offsets[i].found);
> +		obj_offsets[i].found = true;
> +		igt_debug("offset:%"PRIx64", index:%ld\n",
> +			  addr, obj_offsets[i].idx);
> +
> +		/* gtt_page_sizes = 0x00010000 */
> +		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
> +			str += 19 + 8;
> +			igt_assert(*str++ == '\n');
> +		}
> +
> +		if (!(*str == ':' || *str == '~'))
> +			continue;
> +
> +		igt_debug("blob:%.64s\n", str);
> +		sz = ascii85_decode(str + 1, &data, *str == ':', &str);
> +
> +		igt_assert_eq(4 * sz, obj_size);
> +		igt_assert(*str++ == '\n');
> +		str = strchr(str, '-');
> +
> +		if (incremental) {
> +			uint32_t expect;
> +
> +			expect = obj_offsets[i].idx * obj_size;
> +			for (i = 0; i < sz; i++)
> +				igt_assert_eq(data[i], expect++);
> +		} else {
> +			for (i = 0; i < sz; i++)
> +				igt_assert_eq(data[i], 0);
> +		}
> +
> +		blobs++;
> +		free(data);
>  	}
>  
>  	free(error);
> -	igt_assert(found);
> +	return blobs;
>  }
>  
>  static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> @@ -73,6 +216,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	struct drm_i915_gem_relocation_entry reloc[2];
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	uint32_t *batch, *seqno;
> +	struct offset offset;
>  	int i;
>  
>  	memset(obj, 0, sizeof(obj));
> @@ -168,7 +312,10 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  
>  	/* Check that only the buffer we marked is reported in the error */
>  	igt_force_gpu_reset(fd);
> -	check_error_state(dir, &obj[CAPTURE]);
> +	memset(&offset, 0, sizeof(offset));
> +	offset.addr = obj[CAPTURE].offset;
> +	igt_assert_eq(check_error_state(dir, &offset, 1, target_size, false), 1);
> +	igt_assert(offset.found);
>  
>  	gem_sync(fd, obj[BATCH].handle);
>  
> @@ -183,11 +330,12 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
>  {
>  	uint32_t handle;
>  	uint64_t ahnd;
> +	int obj_size = 4096;
>  
> -	handle = gem_create(fd, 4096);
> +	handle = gem_create(fd, obj_size);
>  	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	__capture1(fd, dir, ahnd, ctx, ring, handle, 4096);
> +	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
>  
>  	gem_close(fd, handle);
>  	put_ahnd(ahnd);
> @@ -206,10 +354,8 @@ static int cmp(const void *A, const void *B)
>  	return 0;
>  }
>  
> -static struct offset {
> -	uint64_t addr;
> -	unsigned long idx;
> -} *__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> +static struct offset *
> +__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>  	      unsigned int size, int count,
>  	      unsigned int flags)
>  #define INCREMENTAL 0x1
> @@ -357,98 +503,11 @@ static struct offset {
>  	return offsets;
>  }
>  
> -static unsigned long zlib_inflate(uint32_t **ptr, unsigned long len)
> -{
> -	struct z_stream_s zstream;
> -	void *out;
> -
> -	memset(&zstream, 0, sizeof(zstream));
> -
> -	zstream.next_in = (unsigned char *)*ptr;
> -	zstream.avail_in = 4*len;
> -
> -	if (inflateInit(&zstream) != Z_OK)
> -		return 0;
> -
> -	out = malloc(128*4096); /* approximate obj size */
> -	zstream.next_out = out;
> -	zstream.avail_out = 128*4096;
> -
> -	do {
> -		switch (inflate(&zstream, Z_SYNC_FLUSH)) {
> -		case Z_STREAM_END:
> -			goto end;
> -		case Z_OK:
> -			break;
> -		default:
> -			inflateEnd(&zstream);
> -			return 0;
> -		}
> -
> -		if (zstream.avail_out)
> -			break;
> -
> -		out = realloc(out, 2*zstream.total_out);
> -		if (out == NULL) {
> -			inflateEnd(&zstream);
> -			return 0;
> -		}
> -
> -		zstream.next_out = (unsigned char *)out + zstream.total_out;
> -		zstream.avail_out = zstream.total_out;
> -	} while (1);
> -end:
> -	inflateEnd(&zstream);
> -	free(*ptr);
> -	*ptr = out;
> -	return zstream.total_out / 4;
> -}
> -
> -static unsigned long
> -ascii85_decode(char *in, uint32_t **out, bool inflate, char **end)
> -{
> -	unsigned long len = 0, size = 1024;
> -
> -	*out = realloc(*out, sizeof(uint32_t)*size);
> -	if (*out == NULL)
> -		return 0;
> -
> -	while (*in >= '!' && *in <= 'z') {
> -		uint32_t v = 0;
> -
> -		if (len == size) {
> -			size *= 2;
> -			*out = realloc(*out, sizeof(uint32_t)*size);
> -			if (*out == NULL)
> -				return 0;
> -		}
> -
> -		if (*in == 'z') {
> -			in++;
> -		} else {
> -			v += in[0] - 33; v *= 85;
> -			v += in[1] - 33; v *= 85;
> -			v += in[2] - 33; v *= 85;
> -			v += in[3] - 33; v *= 85;
> -			v += in[4] - 33;
> -			in += 5;
> -		}
> -		(*out)[len++] = v;
> -	}
> -	*end = in;
> -
> -	if (!inflate)
> -		return len;
> -
> -	return zlib_inflate(out, len);
> -}
> -
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  {
>  	uint64_t ram, gtt, ahnd;
>  	unsigned long count, blobs;
>  	struct offset *offsets;
> -	char *error, *str;
>  
>  	gtt = gem_aperture_size(fd) / size;
>  	ram = (intel_get_avail_ram_mb() << 20) / size;
> @@ -463,75 +522,10 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  
>  	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
>  
> -	error = igt_sysfs_get(dir, "error");
> -	igt_sysfs_set(dir, "error", "Begone!");
> -	igt_assert(error);
> -
> -	blobs = 0;
> -	/* render ring --- user = 0x00000000 ffffd000 */
> -	str = strstr(error, "--- user = ");
> -	while (str) {
> -		uint32_t *data = NULL;
> -		unsigned long i, sz;
> -		uint64_t addr;
> -
> -		if (strncmp(str, "--- user = 0x", 13))
> -			break;
> -
> -		str += 13;
> -		addr = strtoul(str, &str, 16);
> -		addr <<= 32;
> -		addr |= strtoul(str + 1, &str, 16);
> -		igt_assert(*str++ == '\n');
> -
> -		/* gtt_page_sizes = 0x00010000 */
> -		if (strncmp(str, "gtt_page_sizes = 0x", 19) == 0) {
> -			str += 19 + 8;
> -			igt_assert(*str++ == '\n');
> -		}
> -
> -		if (!(*str == ':' || *str == '~'))
> -			continue;
> -
> -		igt_debug("blob:%.64s\n", str);
> -		sz = ascii85_decode(str + 1, &data, *str == ':', &str);
> -		igt_assert_eq(4 * sz, size);
> -		igt_assert(*str++ == '\n');
> -		str = strchr(str, '-');
> -
> -		if (flags & INCREMENTAL) {
> -			unsigned long start = 0;
> -			unsigned long end = count;
> -			uint32_t expect;
> -
> -			while (end > start) {
> -				i = (end - start) / 2 + start;
> -				if (offsets[i].addr < addr)
> -					start = i + 1;
> -				else if (offsets[i].addr > addr)
> -					end = i;
> -				else
> -					break;
> -			}
> -			igt_assert(offsets[i].addr == addr);
> -			igt_debug("offset:%"PRIx64", index:%ld\n",
> -				  addr, offsets[i].idx);
> -
> -			expect = offsets[i].idx * size;
> -			for (i = 0; i < sz; i++)
> -				igt_assert_eq(data[i], expect++);
> -		} else {
> -			for (i = 0; i < sz; i++)
> -				igt_assert_eq(data[i], 0);
> -		}
> -
> -		blobs++;
> -		free(data);
> -	}
> +	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
>  	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
>  		 blobs, size >> 12, count);
>  
> -	free(error);
>  	free(offsets);
>  	put_ahnd(ahnd);
>  }
> @@ -625,12 +619,14 @@ static void userptr(int fd, int dir)
>  	uint32_t handle;
>  	uint64_t ahnd;
>  	void *ptr;
> +	int obj_size = 4096;
>  
> -	igt_assert(posix_memalign(&ptr, 4096, 4096) == 0);
> -	igt_require(__gem_userptr(fd, ptr, 4096, 0, 0, &handle) == 0);
> +	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
> +	memset(ptr, 0, obj_size);
> +	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
>  	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, 4096);
> +	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
>  
>  	gem_close(fd, handle);
>  	put_ahnd(ahnd);
> -- 
> 2.25.1
> 

[Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[John.C.Harrison]

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

Some of the capture tests were using explicit contexts, some not. Some
were poking the per engine pre-emption timeout, some not. This would
lead to sporadic failures due to random timeouts, contexts being
banned depending upon how many subtests were run and/or how many
engines a given platform has, and other such failures.

So, update all tests to be conistent.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
 1 file changed, 58 insertions(+), 22 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index c85c198f7..e373d24ed 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
 	return blobs;
 }
 
+static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
+{
+	/* Ensure fast hang detection */
+	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
+	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
+
+	/* Allow engine based resets and disable banning */
+	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
+}
+
 static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
-		       unsigned ring, uint32_t target, uint64_t target_size)
+		       const struct intel_execution_engine2 *e,
+		       uint32_t target, uint64_t target_size)
 {
 	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
 	struct drm_i915_gem_exec_object2 obj[4];
@@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	struct offset offset;
 	int i;
 
+	configure_hangs(fd, e, ctx->id);
+
 	memset(obj, 0, sizeof(obj));
 	obj[SCRATCH].handle = gem_create(fd, 4096);
 	obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
@@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = (uintptr_t)obj;
 	execbuf.buffer_count = ARRAY_SIZE(obj);
-	execbuf.flags = ring;
+	execbuf.flags = e->flags;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
 	execbuf.rsvd1 = ctx->id;
@@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	gem_close(fd, obj[SCRATCH].handle);
 }
 
-static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
+static void capture(int fd, int dir, const intel_ctx_t *ctx,
+		    const struct intel_execution_engine2 *e)
 {
 	uint32_t handle;
 	uint64_t ahnd;
@@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
 	handle = gem_create(fd, obj_size);
 	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
+	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
 
 	gem_close(fd, handle);
 	put_ahnd(ahnd);
@@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
 }
 
 static struct offset *
-__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
-	      unsigned int size, int count,
-	      unsigned int flags)
+__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
+	   const struct intel_execution_engine2 *e,
+	   unsigned int size, int count, unsigned int flags)
 #define INCREMENTAL 0x1
 #define ASYNC 0x2
 {
@@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
 	struct offset *offsets;
 	int i;
 
+	configure_hangs(fd, e, ctx->id);
+
 	offsets = calloc(count, sizeof(*offsets));
 	igt_assert(offsets);
 
@@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
 	memset(&execbuf, 0, sizeof(execbuf));
 	execbuf.buffers_ptr = (uintptr_t)obj;
 	execbuf.buffer_count = count + 2;
-	execbuf.flags = ring;
+	execbuf.flags = e->flags;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
+	execbuf.rsvd1 = ctx->id;
 
 	igt_assert(!READ_ONCE(*seqno));
 	gem_execbuf(fd, &execbuf);
@@ -505,10 +522,20 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
 
 static void many(int fd, int dir, uint64_t size, unsigned int flags)
 {
+	const struct intel_execution_engine2 *e;
+	const intel_ctx_t *ctx;
 	uint64_t ram, gtt, ahnd;
 	unsigned long count, blobs;
 	struct offset *offsets;
 
+	/* Find the first available engine: */
+	ctx = intel_ctx_create_all_physical(fd);
+	igt_assert(ctx);
+	for_each_ctx_engine(fd, ctx, e)
+		for_each_if(gem_class_can_store_dword(fd, e->class))
+			break;
+	igt_assert(e);
+
 	gtt = gem_aperture_size(fd) / size;
 	ram = (intel_get_avail_ram_mb() << 20) / size;
 	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
@@ -518,9 +545,9 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 	igt_require(count > 1);
 
 	intel_require_memory(count, size, CHECK_RAM);
-	ahnd = get_reloc_ahnd(fd, 0);
+	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
+	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
 
 	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
 	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
@@ -531,7 +558,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 }
 
 static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
-		    unsigned ring, const char *name)
+		    const struct intel_execution_engine2 *e)
 {
 	const uint32_t bbe = MI_BATCH_BUFFER_END;
 	struct drm_i915_gem_exec_object2 obj = {
@@ -540,7 +567,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 	struct drm_i915_gem_execbuffer2 execbuf = {
 		.buffers_ptr = to_user_pointer(&obj),
 		.buffer_count = 1,
-		.flags = ring,
+		.flags = e->flags,
 		.rsvd1 = ctx->id,
 	};
 	int64_t timeout = NSEC_PER_SEC; /* 1s, feeling generous, blame debug */
@@ -555,10 +582,6 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 	igt_require(igt_params_set(fd, "reset", "%u", -1)); /* engine resets! */
 	igt_require(gem_gpu_reset_type(fd) > 1);
 
-	/* Needs to be fast enough for the hangcheck to return within 1s */
-	igt_require(gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 0) > 0);
-	gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 500);
-
 	gtt = gem_aperture_size(fd) / size;
 	ram = (intel_get_avail_ram_mb() << 20) / size;
 	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
@@ -576,15 +599,19 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 
 	igt_assert(pipe(link) == 0);
 	igt_fork(child, 1) {
+		const intel_ctx_t *ctx2;
 		fd = gem_reopen_driver(fd);
 		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
 			  count, (int)(size >> 20));
 
+		ctx2 = intel_ctx_create_all_physical(fd);
+		igt_assert(ctx2);
+
 		intel_allocator_init();
 		/* Reopen the allocator in the new process. */
-		ahnd = get_reloc_ahnd(fd, 0);
+		ahnd = get_reloc_ahnd(fd, ctx2->id);
 
-		free(__captureN(fd, dir, ahnd, ring, size, count, ASYNC));
+		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
 		put_ahnd(ahnd);
 
 		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
@@ -615,18 +642,27 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 
 static void userptr(int fd, int dir)
 {
-	const intel_ctx_t *ctx = intel_ctx_0(fd);
+	const struct intel_execution_engine2 *e;
+	const intel_ctx_t *ctx;
 	uint32_t handle;
 	uint64_t ahnd;
 	void *ptr;
 	int obj_size = 4096;
 
+	/* Find the first available engine: */
+	ctx = intel_ctx_create_all_physical(fd);
+	igt_assert(ctx);
+	for_each_ctx_engine(fd, ctx, e)
+		for_each_if(gem_class_can_store_dword(fd, e->class))
+			break;
+	igt_assert(e);
+
 	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
 	memset(ptr, 0, obj_size);
 	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
 	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
+	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
 
 	gem_close(fd, handle);
 	put_ahnd(ahnd);
@@ -684,7 +720,7 @@ igt_main
 	}
 
 	test_each_engine("capture", fd, ctx, e)
-		capture(fd, dir, ctx, e->flags);
+		capture(fd, dir, ctx, e);
 
 	igt_subtest_f("many-4K-zero") {
 		igt_require(gem_can_store_dword(fd, 0));
@@ -719,7 +755,7 @@ igt_main
 	}
 
 	test_each_engine("pi", fd, ctx, e)
-		prioinv(fd, dir, ctx, e->flags, e->name);
+		prioinv(fd, dir, ctx, e);
 
 	igt_fixture {
 		close(dir);
-- 
2.25.1

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:40PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> Some of the capture tests were using explicit contexts, some not. Some
> were poking the per engine pre-emption timeout, some not. This would
> lead to sporadic failures due to random timeouts, contexts being
> banned depending upon how many subtests were run and/or how many
> engines a given platform has, and other such failures.
> 
> So, update all tests to be conistent.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>  tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
>  1 file changed, 58 insertions(+), 22 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index c85c198f7..e373d24ed 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
>  	return blobs;
>  }
>  
> +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
> +{
> +	/* Ensure fast hang detection */
> +	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
> +	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);

#define for 250, 500?

> +
> +	/* Allow engine based resets and disable banning */
> +	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
> +}
> +
>  static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> -		       unsigned ring, uint32_t target, uint64_t target_size)
> +		       const struct intel_execution_engine2 *e,
> +		       uint32_t target, uint64_t target_size)
>  {
>  	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
>  	struct drm_i915_gem_exec_object2 obj[4];
> @@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	struct offset offset;
>  	int i;
>  
> +	configure_hangs(fd, e, ctx->id);
> +
>  	memset(obj, 0, sizeof(obj));
>  	obj[SCRATCH].handle = gem_create(fd, 4096);
>  	obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
> @@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	memset(&execbuf, 0, sizeof(execbuf));
>  	execbuf.buffers_ptr = (uintptr_t)obj;
>  	execbuf.buffer_count = ARRAY_SIZE(obj);
> -	execbuf.flags = ring;
> +	execbuf.flags = e->flags;
>  	if (gen > 3 && gen < 6)
>  		execbuf.flags |= I915_EXEC_SECURE;
>  	execbuf.rsvd1 = ctx->id;
> @@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	gem_close(fd, obj[SCRATCH].handle);
>  }
>  
> -static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
> +static void capture(int fd, int dir, const intel_ctx_t *ctx,
> +		    const struct intel_execution_engine2 *e)
>  {
>  	uint32_t handle;
>  	uint64_t ahnd;
> @@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
>  	handle = gem_create(fd, obj_size);
>  	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
> +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
>  
>  	gem_close(fd, handle);
>  	put_ahnd(ahnd);
> @@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
>  }
>  
>  static struct offset *
> -__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> -	      unsigned int size, int count,
> -	      unsigned int flags)
> +__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> +	   const struct intel_execution_engine2 *e,
> +	   unsigned int size, int count, unsigned int flags)
>  #define INCREMENTAL 0x1
>  #define ASYNC 0x2
>  {
> @@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>  	struct offset *offsets;
>  	int i;
>  
> +	configure_hangs(fd, e, ctx->id);
> +
>  	offsets = calloc(count, sizeof(*offsets));
>  	igt_assert(offsets);
>  
> @@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>  	memset(&execbuf, 0, sizeof(execbuf));
>  	execbuf.buffers_ptr = (uintptr_t)obj;
>  	execbuf.buffer_count = count + 2;
> -	execbuf.flags = ring;
> +	execbuf.flags = e->flags;
>  	if (gen > 3 && gen < 6)
>  		execbuf.flags |= I915_EXEC_SECURE;
> +	execbuf.rsvd1 = ctx->id;
>  
>  	igt_assert(!READ_ONCE(*seqno));
>  	gem_execbuf(fd, &execbuf);
> @@ -505,10 +522,20 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>  
>  static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  {
> +	const struct intel_execution_engine2 *e;
> +	const intel_ctx_t *ctx;
>  	uint64_t ram, gtt, ahnd;
>  	unsigned long count, blobs;
>  	struct offset *offsets;
>  
> +	/* Find the first available engine: */
> +	ctx = intel_ctx_create_all_physical(fd);
> +	igt_assert(ctx);
> +	for_each_ctx_engine(fd, ctx, e)
> +		for_each_if(gem_class_can_store_dword(fd, e->class))
> +			break;
> +	igt_assert(e);

Duplicated below. Helper for this?

Matt

> +
>  	gtt = gem_aperture_size(fd) / size;
>  	ram = (intel_get_avail_ram_mb() << 20) / size;
>  	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
> @@ -518,9 +545,9 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  	igt_require(count > 1);
>  
>  	intel_require_memory(count, size, CHECK_RAM);
> -	ahnd = get_reloc_ahnd(fd, 0);
> +	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
> +	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
>  
>  	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
>  	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
> @@ -531,7 +558,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  }
>  
>  static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> -		    unsigned ring, const char *name)
> +		    const struct intel_execution_engine2 *e)
>  {
>  	const uint32_t bbe = MI_BATCH_BUFFER_END;
>  	struct drm_i915_gem_exec_object2 obj = {
> @@ -540,7 +567,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  	struct drm_i915_gem_execbuffer2 execbuf = {
>  		.buffers_ptr = to_user_pointer(&obj),
>  		.buffer_count = 1,
> -		.flags = ring,
> +		.flags = e->flags,
>  		.rsvd1 = ctx->id,
>  	};
>  	int64_t timeout = NSEC_PER_SEC; /* 1s, feeling generous, blame debug */
> @@ -555,10 +582,6 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  	igt_require(igt_params_set(fd, "reset", "%u", -1)); /* engine resets! */
>  	igt_require(gem_gpu_reset_type(fd) > 1);
>  
> -	/* Needs to be fast enough for the hangcheck to return within 1s */
> -	igt_require(gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 0) > 0);
> -	gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 500);
> -
>  	gtt = gem_aperture_size(fd) / size;
>  	ram = (intel_get_avail_ram_mb() << 20) / size;
>  	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
> @@ -576,15 +599,19 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  
>  	igt_assert(pipe(link) == 0);
>  	igt_fork(child, 1) {
> +		const intel_ctx_t *ctx2;
>  		fd = gem_reopen_driver(fd);
>  		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
>  			  count, (int)(size >> 20));
>  
> +		ctx2 = intel_ctx_create_all_physical(fd);
> +		igt_assert(ctx2);
> +
>  		intel_allocator_init();
>  		/* Reopen the allocator in the new process. */
> -		ahnd = get_reloc_ahnd(fd, 0);
> +		ahnd = get_reloc_ahnd(fd, ctx2->id);
>  
> -		free(__captureN(fd, dir, ahnd, ring, size, count, ASYNC));
> +		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
>  		put_ahnd(ahnd);
>  
>  		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
> @@ -615,18 +642,27 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  
>  static void userptr(int fd, int dir)
>  {
> -	const intel_ctx_t *ctx = intel_ctx_0(fd);
> +	const struct intel_execution_engine2 *e;
> +	const intel_ctx_t *ctx;
>  	uint32_t handle;
>  	uint64_t ahnd;
>  	void *ptr;
>  	int obj_size = 4096;
>  
> +	/* Find the first available engine: */
> +	ctx = intel_ctx_create_all_physical(fd);
> +	igt_assert(ctx);
> +	for_each_ctx_engine(fd, ctx, e)
> +		for_each_if(gem_class_can_store_dword(fd, e->class))
> +			break;
> +	igt_assert(e);
> +
>  	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
>  	memset(ptr, 0, obj_size);
>  	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
>  	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
> +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
>  
>  	gem_close(fd, handle);
>  	put_ahnd(ahnd);
> @@ -684,7 +720,7 @@ igt_main
>  	}
>  
>  	test_each_engine("capture", fd, ctx, e)
> -		capture(fd, dir, ctx, e->flags);
> +		capture(fd, dir, ctx, e);
>  
>  	igt_subtest_f("many-4K-zero") {
>  		igt_require(gem_can_store_dword(fd, 0));
> @@ -719,7 +755,7 @@ igt_main
>  	}
>  
>  	test_each_engine("pi", fd, ctx, e)
> -		prioinv(fd, dir, ctx, e->flags, e->name);
> +		prioinv(fd, dir, ctx, e);
>  
>  	igt_fixture {
>  		close(dir);
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[John Harrison]

On 11/2/2021 16:34, Matthew Brost wrote:
> On Thu, Oct 21, 2021 at 04:40:40PM -0700, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Some of the capture tests were using explicit contexts, some not. Some
>> were poking the per engine pre-emption timeout, some not. This would
>> lead to sporadic failures due to random timeouts, contexts being
>> banned depending upon how many subtests were run and/or how many
>> engines a given platform has, and other such failures.
>>
>> So, update all tests to be conistent.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
>>   1 file changed, 58 insertions(+), 22 deletions(-)
>>
>> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
>> index c85c198f7..e373d24ed 100644
>> --- a/tests/i915/gem_exec_capture.c
>> +++ b/tests/i915/gem_exec_capture.c
>> @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
>>   	return blobs;
>>   }
>>   
>> +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
>> +{
>> +	/* Ensure fast hang detection */
>> +	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
>> +	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
> #define for 250, 500?
Is there any point? There is no special reason for the values other than 
small enough to be fast and long enough to not be too small to be 
usable. So there isn't really any particular name to give them beyond 
'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper 
function is that the values are programmed in one place only and not 
used anywhere else. So there is no worry about repetition of magic numbers.


>
>> +
>> +	/* Allow engine based resets and disable banning */
>> +	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
>> +}
>> +
>>   static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>> -		       unsigned ring, uint32_t target, uint64_t target_size)
>> +		       const struct intel_execution_engine2 *e,
>> +		       uint32_t target, uint64_t target_size)
>>   {
>>   	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
>>   	struct drm_i915_gem_exec_object2 obj[4];
>> @@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>>   	struct offset offset;
>>   	int i;
>>   
>> +	configure_hangs(fd, e, ctx->id);
>> +
>>   	memset(obj, 0, sizeof(obj));
>>   	obj[SCRATCH].handle = gem_create(fd, 4096);
>>   	obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
>> @@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>>   	memset(&execbuf, 0, sizeof(execbuf));
>>   	execbuf.buffers_ptr = (uintptr_t)obj;
>>   	execbuf.buffer_count = ARRAY_SIZE(obj);
>> -	execbuf.flags = ring;
>> +	execbuf.flags = e->flags;
>>   	if (gen > 3 && gen < 6)
>>   		execbuf.flags |= I915_EXEC_SECURE;
>>   	execbuf.rsvd1 = ctx->id;
>> @@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>>   	gem_close(fd, obj[SCRATCH].handle);
>>   }
>>   
>> -static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
>> +static void capture(int fd, int dir, const intel_ctx_t *ctx,
>> +		    const struct intel_execution_engine2 *e)
>>   {
>>   	uint32_t handle;
>>   	uint64_t ahnd;
>> @@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
>>   	handle = gem_create(fd, obj_size);
>>   	ahnd = get_reloc_ahnd(fd, ctx->id);
>>   
>> -	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
>> +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
>>   
>>   	gem_close(fd, handle);
>>   	put_ahnd(ahnd);
>> @@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
>>   }
>>   
>>   static struct offset *
>> -__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>> -	      unsigned int size, int count,
>> -	      unsigned int flags)
>> +__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>> +	   const struct intel_execution_engine2 *e,
>> +	   unsigned int size, int count, unsigned int flags)
>>   #define INCREMENTAL 0x1
>>   #define ASYNC 0x2
>>   {
>> @@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>>   	struct offset *offsets;
>>   	int i;
>>   
>> +	configure_hangs(fd, e, ctx->id);
>> +
>>   	offsets = calloc(count, sizeof(*offsets));
>>   	igt_assert(offsets);
>>   
>> @@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>>   	memset(&execbuf, 0, sizeof(execbuf));
>>   	execbuf.buffers_ptr = (uintptr_t)obj;
>>   	execbuf.buffer_count = count + 2;
>> -	execbuf.flags = ring;
>> +	execbuf.flags = e->flags;
>>   	if (gen > 3 && gen < 6)
>>   		execbuf.flags |= I915_EXEC_SECURE;
>> +	execbuf.rsvd1 = ctx->id;
>>   
>>   	igt_assert(!READ_ONCE(*seqno));
>>   	gem_execbuf(fd, &execbuf);
>> @@ -505,10 +522,20 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
>>   
>>   static void many(int fd, int dir, uint64_t size, unsigned int flags)
>>   {
>> +	const struct intel_execution_engine2 *e;
>> +	const intel_ctx_t *ctx;
>>   	uint64_t ram, gtt, ahnd;
>>   	unsigned long count, blobs;
>>   	struct offset *offsets;
>>   
>> +	/* Find the first available engine: */
>> +	ctx = intel_ctx_create_all_physical(fd);
>> +	igt_assert(ctx);
>> +	for_each_ctx_engine(fd, ctx, e)
>> +		for_each_if(gem_class_can_store_dword(fd, e->class))
>> +			break;
>> +	igt_assert(e);
> Duplicated below. Helper for this?
>
> Matt
Sure.

John.

>> +
>>   	gtt = gem_aperture_size(fd) / size;
>>   	ram = (intel_get_avail_ram_mb() << 20) / size;
>>   	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
>> @@ -518,9 +545,9 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>>   	igt_require(count > 1);
>>   
>>   	intel_require_memory(count, size, CHECK_RAM);
>> -	ahnd = get_reloc_ahnd(fd, 0);
>> +	ahnd = get_reloc_ahnd(fd, ctx->id);
>>   
>> -	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
>> +	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
>>   
>>   	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
>>   	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
>> @@ -531,7 +558,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>>   }
>>   
>>   static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>> -		    unsigned ring, const char *name)
>> +		    const struct intel_execution_engine2 *e)
>>   {
>>   	const uint32_t bbe = MI_BATCH_BUFFER_END;
>>   	struct drm_i915_gem_exec_object2 obj = {
>> @@ -540,7 +567,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>>   	struct drm_i915_gem_execbuffer2 execbuf = {
>>   		.buffers_ptr = to_user_pointer(&obj),
>>   		.buffer_count = 1,
>> -		.flags = ring,
>> +		.flags = e->flags,
>>   		.rsvd1 = ctx->id,
>>   	};
>>   	int64_t timeout = NSEC_PER_SEC; /* 1s, feeling generous, blame debug */
>> @@ -555,10 +582,6 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>>   	igt_require(igt_params_set(fd, "reset", "%u", -1)); /* engine resets! */
>>   	igt_require(gem_gpu_reset_type(fd) > 1);
>>   
>> -	/* Needs to be fast enough for the hangcheck to return within 1s */
>> -	igt_require(gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 0) > 0);
>> -	gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 500);
>> -
>>   	gtt = gem_aperture_size(fd) / size;
>>   	ram = (intel_get_avail_ram_mb() << 20) / size;
>>   	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
>> @@ -576,15 +599,19 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>>   
>>   	igt_assert(pipe(link) == 0);
>>   	igt_fork(child, 1) {
>> +		const intel_ctx_t *ctx2;
>>   		fd = gem_reopen_driver(fd);
>>   		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
>>   			  count, (int)(size >> 20));
>>   
>> +		ctx2 = intel_ctx_create_all_physical(fd);
>> +		igt_assert(ctx2);
>> +
>>   		intel_allocator_init();
>>   		/* Reopen the allocator in the new process. */
>> -		ahnd = get_reloc_ahnd(fd, 0);
>> +		ahnd = get_reloc_ahnd(fd, ctx2->id);
>>   
>> -		free(__captureN(fd, dir, ahnd, ring, size, count, ASYNC));
>> +		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
>>   		put_ahnd(ahnd);
>>   
>>   		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
>> @@ -615,18 +642,27 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>>   
>>   static void userptr(int fd, int dir)
>>   {
>> -	const intel_ctx_t *ctx = intel_ctx_0(fd);
>> +	const struct intel_execution_engine2 *e;
>> +	const intel_ctx_t *ctx;
>>   	uint32_t handle;
>>   	uint64_t ahnd;
>>   	void *ptr;
>>   	int obj_size = 4096;
>>   
>> +	/* Find the first available engine: */
>> +	ctx = intel_ctx_create_all_physical(fd);
>> +	igt_assert(ctx);
>> +	for_each_ctx_engine(fd, ctx, e)
>> +		for_each_if(gem_class_can_store_dword(fd, e->class))
>> +			break;
>> +	igt_assert(e);
>> +
>>   	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
>>   	memset(ptr, 0, obj_size);
>>   	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
>>   	ahnd = get_reloc_ahnd(fd, ctx->id);
>>   
>> -	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
>> +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
>>   
>>   	gem_close(fd, handle);
>>   	put_ahnd(ahnd);
>> @@ -684,7 +720,7 @@ igt_main
>>   	}
>>   
>>   	test_each_engine("capture", fd, ctx, e)
>> -		capture(fd, dir, ctx, e->flags);
>> +		capture(fd, dir, ctx, e);
>>   
>>   	igt_subtest_f("many-4K-zero") {
>>   		igt_require(gem_can_store_dword(fd, 0));
>> @@ -719,7 +755,7 @@ igt_main
>>   	}
>>   
>>   	test_each_engine("pi", fd, ctx, e)
>> -		prioinv(fd, dir, ctx, e->flags, e->name);
>> +		prioinv(fd, dir, ctx, e);
>>   
>>   	igt_fixture {
>>   		close(dir);
>> -- 
>> 2.25.1
>>

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[Petri Latvala]

On Tue, Nov 02, 2021 at 06:45:38PM -0700, John Harrison wrote:
> On 11/2/2021 16:34, Matthew Brost wrote:
> > On Thu, Oct 21, 2021 at 04:40:40PM -0700, John.C.Harrison@Intel.com wrote:
> > > From: John Harrison <John.C.Harrison@Intel.com>
> > > 
> > > Some of the capture tests were using explicit contexts, some not. Some
> > > were poking the per engine pre-emption timeout, some not. This would
> > > lead to sporadic failures due to random timeouts, contexts being
> > > banned depending upon how many subtests were run and/or how many
> > > engines a given platform has, and other such failures.
> > > 
> > > So, update all tests to be conistent.
> > > 
> > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > ---
> > >   tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
> > >   1 file changed, 58 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> > > index c85c198f7..e373d24ed 100644
> > > --- a/tests/i915/gem_exec_capture.c
> > > +++ b/tests/i915/gem_exec_capture.c
> > > @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
> > >   	return blobs;
> > >   }
> > > +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
> > > +{
> > > +	/* Ensure fast hang detection */
> > > +	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
> > > +	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
> > #define for 250, 500?
> Is there any point? There is no special reason for the values other than
> small enough to be fast and long enough to not be too small to be usable. So
> there isn't really any particular name to give them beyond
> 'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper
> function is that the values are programmed in one place only and not used
> anywhere else. So there is no worry about repetition of magic numbers.

In about one year everyone has forgotten this explanation and will
wonder if it's related to some in-kernel behaviour or if there's some
other reason these values have been chosen.

So at least a comment why the values are these, please.


-- 
Petri Latvala


> 
> 
> > 
> > > +
> > > +	/* Allow engine based resets and disable banning */
> > > +	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
> > > +}
> > > +
> > >   static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > > -		       unsigned ring, uint32_t target, uint64_t target_size)
> > > +		       const struct intel_execution_engine2 *e,
> > > +		       uint32_t target, uint64_t target_size)
> > >   {
> > >   	const unsigned int gen = intel_gen(intel_get_drm_devid(fd));
> > >   	struct drm_i915_gem_exec_object2 obj[4];
> > > @@ -219,6 +230,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > >   	struct offset offset;
> > >   	int i;
> > > +	configure_hangs(fd, e, ctx->id);
> > > +
> > >   	memset(obj, 0, sizeof(obj));
> > >   	obj[SCRATCH].handle = gem_create(fd, 4096);
> > >   	obj[SCRATCH].flags = EXEC_OBJECT_WRITE;
> > > @@ -297,7 +310,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > >   	memset(&execbuf, 0, sizeof(execbuf));
> > >   	execbuf.buffers_ptr = (uintptr_t)obj;
> > >   	execbuf.buffer_count = ARRAY_SIZE(obj);
> > > -	execbuf.flags = ring;
> > > +	execbuf.flags = e->flags;
> > >   	if (gen > 3 && gen < 6)
> > >   		execbuf.flags |= I915_EXEC_SECURE;
> > >   	execbuf.rsvd1 = ctx->id;
> > > @@ -326,7 +339,8 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > >   	gem_close(fd, obj[SCRATCH].handle);
> > >   }
> > > -static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
> > > +static void capture(int fd, int dir, const intel_ctx_t *ctx,
> > > +		    const struct intel_execution_engine2 *e)
> > >   {
> > >   	uint32_t handle;
> > >   	uint64_t ahnd;
> > > @@ -335,7 +349,7 @@ static void capture(int fd, int dir, const intel_ctx_t *ctx, unsigned ring)
> > >   	handle = gem_create(fd, obj_size);
> > >   	ahnd = get_reloc_ahnd(fd, ctx->id);
> > > -	__capture1(fd, dir, ahnd, ctx, ring, handle, obj_size);
> > > +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
> > >   	gem_close(fd, handle);
> > >   	put_ahnd(ahnd);
> > > @@ -355,9 +369,9 @@ static int cmp(const void *A, const void *B)
> > >   }
> > >   static struct offset *
> > > -__captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> > > -	      unsigned int size, int count,
> > > -	      unsigned int flags)
> > > +__captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> > > +	   const struct intel_execution_engine2 *e,
> > > +	   unsigned int size, int count, unsigned int flags)
> > >   #define INCREMENTAL 0x1
> > >   #define ASYNC 0x2
> > >   {
> > > @@ -369,6 +383,8 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> > >   	struct offset *offsets;
> > >   	int i;
> > > +	configure_hangs(fd, e, ctx->id);
> > > +
> > >   	offsets = calloc(count, sizeof(*offsets));
> > >   	igt_assert(offsets);
> > > @@ -470,9 +486,10 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> > >   	memset(&execbuf, 0, sizeof(execbuf));
> > >   	execbuf.buffers_ptr = (uintptr_t)obj;
> > >   	execbuf.buffer_count = count + 2;
> > > -	execbuf.flags = ring;
> > > +	execbuf.flags = e->flags;
> > >   	if (gen > 3 && gen < 6)
> > >   		execbuf.flags |= I915_EXEC_SECURE;
> > > +	execbuf.rsvd1 = ctx->id;
> > >   	igt_assert(!READ_ONCE(*seqno));
> > >   	gem_execbuf(fd, &execbuf);
> > > @@ -505,10 +522,20 @@ __captureN(int fd, int dir, uint64_t ahnd, unsigned ring,
> > >   static void many(int fd, int dir, uint64_t size, unsigned int flags)
> > >   {
> > > +	const struct intel_execution_engine2 *e;
> > > +	const intel_ctx_t *ctx;
> > >   	uint64_t ram, gtt, ahnd;
> > >   	unsigned long count, blobs;
> > >   	struct offset *offsets;
> > > +	/* Find the first available engine: */
> > > +	ctx = intel_ctx_create_all_physical(fd);
> > > +	igt_assert(ctx);
> > > +	for_each_ctx_engine(fd, ctx, e)
> > > +		for_each_if(gem_class_can_store_dword(fd, e->class))
> > > +			break;
> > > +	igt_assert(e);
> > Duplicated below. Helper for this?
> > 
> > Matt
> Sure.
> 
> John.
> 
> > > +
> > >   	gtt = gem_aperture_size(fd) / size;
> > >   	ram = (intel_get_avail_ram_mb() << 20) / size;
> > >   	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
> > > @@ -518,9 +545,9 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
> > >   	igt_require(count > 1);
> > >   	intel_require_memory(count, size, CHECK_RAM);
> > > -	ahnd = get_reloc_ahnd(fd, 0);
> > > +	ahnd = get_reloc_ahnd(fd, ctx->id);
> > > -	offsets = __captureN(fd, dir, ahnd, 0, size, count, flags);
> > > +	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
> > >   	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
> > >   	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
> > > @@ -531,7 +558,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
> > >   }
> > >   static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> > > -		    unsigned ring, const char *name)
> > > +		    const struct intel_execution_engine2 *e)
> > >   {
> > >   	const uint32_t bbe = MI_BATCH_BUFFER_END;
> > >   	struct drm_i915_gem_exec_object2 obj = {
> > > @@ -540,7 +567,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> > >   	struct drm_i915_gem_execbuffer2 execbuf = {
> > >   		.buffers_ptr = to_user_pointer(&obj),
> > >   		.buffer_count = 1,
> > > -		.flags = ring,
> > > +		.flags = e->flags,
> > >   		.rsvd1 = ctx->id,
> > >   	};
> > >   	int64_t timeout = NSEC_PER_SEC; /* 1s, feeling generous, blame debug */
> > > @@ -555,10 +582,6 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> > >   	igt_require(igt_params_set(fd, "reset", "%u", -1)); /* engine resets! */
> > >   	igt_require(gem_gpu_reset_type(fd) > 1);
> > > -	/* Needs to be fast enough for the hangcheck to return within 1s */
> > > -	igt_require(gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 0) > 0);
> > > -	gem_engine_property_printf(fd, name, "preempt_timeout_ms", "%d", 500);
> > > -
> > >   	gtt = gem_aperture_size(fd) / size;
> > >   	ram = (intel_get_avail_ram_mb() << 20) / size;
> > >   	igt_debug("Available objects in GTT:%"PRIu64", RAM:%"PRIu64"\n",
> > > @@ -576,15 +599,19 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> > >   	igt_assert(pipe(link) == 0);
> > >   	igt_fork(child, 1) {
> > > +		const intel_ctx_t *ctx2;
> > >   		fd = gem_reopen_driver(fd);
> > >   		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
> > >   			  count, (int)(size >> 20));
> > > +		ctx2 = intel_ctx_create_all_physical(fd);
> > > +		igt_assert(ctx2);
> > > +
> > >   		intel_allocator_init();
> > >   		/* Reopen the allocator in the new process. */
> > > -		ahnd = get_reloc_ahnd(fd, 0);
> > > +		ahnd = get_reloc_ahnd(fd, ctx2->id);
> > > -		free(__captureN(fd, dir, ahnd, ring, size, count, ASYNC));
> > > +		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
> > >   		put_ahnd(ahnd);
> > >   		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
> > > @@ -615,18 +642,27 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
> > >   static void userptr(int fd, int dir)
> > >   {
> > > -	const intel_ctx_t *ctx = intel_ctx_0(fd);
> > > +	const struct intel_execution_engine2 *e;
> > > +	const intel_ctx_t *ctx;
> > >   	uint32_t handle;
> > >   	uint64_t ahnd;
> > >   	void *ptr;
> > >   	int obj_size = 4096;
> > > +	/* Find the first available engine: */
> > > +	ctx = intel_ctx_create_all_physical(fd);
> > > +	igt_assert(ctx);
> > > +	for_each_ctx_engine(fd, ctx, e)
> > > +		for_each_if(gem_class_can_store_dword(fd, e->class))
> > > +			break;
> > > +	igt_assert(e);
> > > +
> > >   	igt_assert(posix_memalign(&ptr, obj_size, obj_size) == 0);
> > >   	memset(ptr, 0, obj_size);
> > >   	igt_require(__gem_userptr(fd, ptr, obj_size, 0, 0, &handle) == 0);
> > >   	ahnd = get_reloc_ahnd(fd, ctx->id);
> > > -	__capture1(fd, dir, ahnd, intel_ctx_0(fd), 0, handle, obj_size);
> > > +	__capture1(fd, dir, ahnd, ctx, e, handle, obj_size);
> > >   	gem_close(fd, handle);
> > >   	put_ahnd(ahnd);
> > > @@ -684,7 +720,7 @@ igt_main
> > >   	}
> > >   	test_each_engine("capture", fd, ctx, e)
> > > -		capture(fd, dir, ctx, e->flags);
> > > +		capture(fd, dir, ctx, e);
> > >   	igt_subtest_f("many-4K-zero") {
> > >   		igt_require(gem_can_store_dword(fd, 0));
> > > @@ -719,7 +755,7 @@ igt_main
> > >   	}
> > >   	test_each_engine("pi", fd, ctx, e)
> > > -		prioinv(fd, dir, ctx, e->flags, e->name);
> > > +		prioinv(fd, dir, ctx, e);
> > >   	igt_fixture {
> > >   		close(dir);
> > > -- 
> > > 2.25.1
> > > 
> 

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[John Harrison]

On 11/3/2021 02:36, Petri Latvala wrote:
> On Tue, Nov 02, 2021 at 06:45:38PM -0700, John Harrison wrote:
>> On 11/2/2021 16:34, Matthew Brost wrote:
>>> On Thu, Oct 21, 2021 at 04:40:40PM -0700, John.C.Harrison@Intel.com wrote:
>>>> From: John Harrison <John.C.Harrison@Intel.com>
>>>>
>>>> Some of the capture tests were using explicit contexts, some not. Some
>>>> were poking the per engine pre-emption timeout, some not. This would
>>>> lead to sporadic failures due to random timeouts, contexts being
>>>> banned depending upon how many subtests were run and/or how many
>>>> engines a given platform has, and other such failures.
>>>>
>>>> So, update all tests to be conistent.
>>>>
>>>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>>>> ---
>>>>    tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
>>>>    1 file changed, 58 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
>>>> index c85c198f7..e373d24ed 100644
>>>> --- a/tests/i915/gem_exec_capture.c
>>>> +++ b/tests/i915/gem_exec_capture.c
>>>> @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
>>>>    	return blobs;
>>>>    }
>>>> +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
>>>> +{
>>>> +	/* Ensure fast hang detection */
>>>> +	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
>>>> +	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
>>> #define for 250, 500?
>> Is there any point? There is no special reason for the values other than
>> small enough to be fast and long enough to not be too small to be usable. So
>> there isn't really any particular name to give them beyond
>> 'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper
>> function is that the values are programmed in one place only and not used
>> anywhere else. So there is no worry about repetition of magic numbers.
> In about one year everyone has forgotten this explanation and will
> wonder if it's related to some in-kernel behaviour or if there's some
> other reason these values have been chosen.
>
> So at least a comment why the values are these, please.
There is a comment already. Not sure what more can be added that is 
meaningful other than changing it to "Ensure fast hang detection by 
picking some random numbers out of the air that seem to be vaguely 
plausible".

John.

>
>

Re: [Intel-gfx] [PATCH i-g-t 4/8] tests/i915/gem_exec_capture: Use contexts and engines properly

[Petri Latvala]

On Wed, Nov 03, 2021 at 11:49:47AM -0700, John Harrison wrote:
> On 11/3/2021 02:36, Petri Latvala wrote:
> > On Tue, Nov 02, 2021 at 06:45:38PM -0700, John Harrison wrote:
> > > On 11/2/2021 16:34, Matthew Brost wrote:
> > > > On Thu, Oct 21, 2021 at 04:40:40PM -0700, John.C.Harrison@Intel.com wrote:
> > > > > From: John Harrison <John.C.Harrison@Intel.com>
> > > > > 
> > > > > Some of the capture tests were using explicit contexts, some not. Some
> > > > > were poking the per engine pre-emption timeout, some not. This would
> > > > > lead to sporadic failures due to random timeouts, contexts being
> > > > > banned depending upon how many subtests were run and/or how many
> > > > > engines a given platform has, and other such failures.
> > > > > 
> > > > > So, update all tests to be conistent.
> > > > > 
> > > > > Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> > > > > ---
> > > > >    tests/i915/gem_exec_capture.c | 80 +++++++++++++++++++++++++----------
> > > > >    1 file changed, 58 insertions(+), 22 deletions(-)
> > > > > 
> > > > > diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> > > > > index c85c198f7..e373d24ed 100644
> > > > > --- a/tests/i915/gem_exec_capture.c
> > > > > +++ b/tests/i915/gem_exec_capture.c
> > > > > @@ -204,8 +204,19 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
> > > > >    	return blobs;
> > > > >    }
> > > > > +static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int ctxt_id)
> > > > > +{
> > > > > +	/* Ensure fast hang detection */
> > > > > +	gem_engine_property_printf(fd, e->name, "preempt_timeout_ms", "%d", 250);
> > > > > +	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
> > > > #define for 250, 500?
> > > Is there any point? There is no special reason for the values other than
> > > small enough to be fast and long enough to not be too small to be usable. So
> > > there isn't really any particular name to give them beyond
> > > 'SHORT_PREEMPT_TIMEOUT' or some such. And the whole point of the helper
> > > function is that the values are programmed in one place only and not used
> > > anywhere else. So there is no worry about repetition of magic numbers.
> > In about one year everyone has forgotten this explanation and will
> > wonder if it's related to some in-kernel behaviour or if there's some
> > other reason these values have been chosen.
> > 
> > So at least a comment why the values are these, please.
> There is a comment already. Not sure what more can be added that is
> meaningful other than changing it to "Ensure fast hang detection by picking
> some random numbers out of the air that seem to be vaguely plausible".

Fair enough.


-- 
Petri Latvala

[Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[John.C.Harrison]

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

The sysfs file read helper does not actually report any errors if a
realloc fails. It just silently returns a 'valid' but truncated
buffer. This then leads to the decode of the buffer failing in random
ways. So, add a check for ENOMEM being generated during the read.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index e373d24ed..8997125ee 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -131,9 +131,11 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
 	char *error, *str;
 	int blobs = 0;
 
+	errno = 0;
 	error = igt_sysfs_get(dir, "error");
 	igt_sysfs_set(dir, "error", "Begone!");
 	igt_assert(error);
+	igt_assert(errno != ENOMEM);
 	igt_debug("%s\n", error);
 
 	/* render ring --- user = 0x00000000 ffffd000 */
-- 
2.25.1

Re: [Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:41PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The sysfs file read helper does not actually report any errors if a
> realloc fails. It just silently returns a 'valid' but truncated
> buffer. This then leads to the decode of the buffer failing in random
> ways. So, add a check for ENOMEM being generated during the read.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  tests/i915/gem_exec_capture.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index e373d24ed..8997125ee 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -131,9 +131,11 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
>  	char *error, *str;
>  	int blobs = 0;
>  
> +	errno = 0;
>  	error = igt_sysfs_get(dir, "error");
>  	igt_sysfs_set(dir, "error", "Begone!");
>  	igt_assert(error);
> +	igt_assert(errno != ENOMEM);
>  	igt_debug("%s\n", error);
>  
>  	/* render ring --- user = 0x00000000 ffffd000 */
> -- 
> 2.25.1
> 

Re: [Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[Tvrtko Ursulin]

On 22/10/2021 00:40, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The sysfs file read helper does not actually report any errors if a
> realloc fails. It just silently returns a 'valid' but truncated
> buffer. This then leads to the decode of the buffer failing in random
> ways. So, add a check for ENOMEM being generated during the read.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
> ---
>   tests/i915/gem_exec_capture.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index e373d24ed..8997125ee 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -131,9 +131,11 @@ static int check_error_state(int dir, struct offset *obj_offsets, int obj_count,
>   	char *error, *str;
>   	int blobs = 0;
>   
> +	errno = 0;
>   	error = igt_sysfs_get(dir, "error");
>   	igt_sysfs_set(dir, "error", "Begone!");
>   	igt_assert(error);
> +	igt_assert(errno != ENOMEM);

igt_sysfs_get:

	len = 64;
...
                 newbuf = realloc(buf, 2*len);

Maybe the problem is doubling goes out of hand. How big are your 
buffers? Perhaps you could improve the library function instead to grow 
less aggressively.

And at the same time perhaps the bug is this:

                 if (igt_debug_on(!newbuf))
                         break;
...
         return buf;

So failures to grow the buffer are ignored, while failure to allocate 
the initial one are not. Perhaps both should return NULL and then 
callers would not be surprised.

Or you think someone relies on this current odd behaviour?

Regards,

Tvrtko

>   	igt_debug("%s\n", error);
>   
>   	/* render ring --- user = 0x00000000 ffffd000 */
> 

Re: [Intel-gfx] [PATCH i-g-t 5/8] tests/i915/gem_exec_capture: Check for memory allocation failure

[John Harrison]

On 11/3/2021 07:00, Tvrtko Ursulin wrote:
> On 22/10/2021 00:40, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> The sysfs file read helper does not actually report any errors if a
>> realloc fails. It just silently returns a 'valid' but truncated
>> buffer. This then leads to the decode of the buffer failing in random
>> ways. So, add a check for ENOMEM being generated during the read.
>>
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> ---
>>   tests/i915/gem_exec_capture.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/tests/i915/gem_exec_capture.c 
>> b/tests/i915/gem_exec_capture.c
>> index e373d24ed..8997125ee 100644
>> --- a/tests/i915/gem_exec_capture.c
>> +++ b/tests/i915/gem_exec_capture.c
>> @@ -131,9 +131,11 @@ static int check_error_state(int dir, struct 
>> offset *obj_offsets, int obj_count,
>>       char *error, *str;
>>       int blobs = 0;
>>   +    errno = 0;
>>       error = igt_sysfs_get(dir, "error");
>>       igt_sysfs_set(dir, "error", "Begone!");
>>       igt_assert(error);
>> +    igt_assert(errno != ENOMEM);
>
> igt_sysfs_get:
>
>     len = 64;
> ...
>                 newbuf = realloc(buf, 2*len);
>
> Maybe the problem is doubling goes out of hand. How big are your 
> buffers? Perhaps you could improve the library function instead to 
> grow less aggressively.
The buffers are generally ending at 2GB in size with the capture being 
about 1.8GB (on the particular system I happen to be testing on).

I considered various options such as doubling until a given size and 
then just incrementing by fixed amounts. But where do you draw the line? 
1MB, 128MB, 1GB, 128GB? If the final result needs to be 128GB (which you 
cannot know until you have finished reading and resizing) and you are 
allocating in 1MB chunks then it is going to take a very long time to 
get there. I ended up leaving it as a straight double on the grounds 
that it is the best compromise between overallocation and taking 
ridiculous numbers of steps.



>
> And at the same time perhaps the bug is this:
>
>                 if (igt_debug_on(!newbuf))
>                         break;
> ...
>         return buf;
>
> So failures to grow the buffer are ignored, while failure to allocate 
> the initial one are not. Perhaps both should return NULL and then 
> callers would not be surprised.
>
> Or you think someone relies on this current odd behaviour?
>
As per the commit description, this is exactly the problem. However, I 
do not know for certain this is not intentional behaviour and something 
somewhere is relying on it. And I really do not have the time to audit 
this. The vast majority of uses are reading teeny tiny files and don't 
care but who knows what might not be in some particular 
test/config/platform/etc. The fact that it is explicitly saying 
'igt_debug_on' means that someone must have made a conscious decision to 
not assert. It's not like they just forgot to check for null being 
returned. Which implies it is intentional and required.

John.


> Regards,
>
> Tvrtko
>
>>       igt_debug("%s\n", error);
>>         /* render ring --- user = 0x00000000 ffffd000 */
>>

[Intel-gfx] [PATCH i-g-t 6/8] lib/igt_sysfs: Support large files

[John.C.Harrison]

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

The syfs helper functions were all using basic 'int' data types for
sizs, offsets, etc. when reading from sysfs. This works fine for
little files, but not for large error capture logs (which can be
gigabytes in sizes).

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 lib/igt_sysfs.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
index 6919ac361..ee75e3ef1 100644
--- a/lib/igt_sysfs.c
+++ b/lib/igt_sysfs.c
@@ -53,9 +53,11 @@
  * provides basic support for like igt_sysfs_open().
  */
 
-static int readN(int fd, char *buf, int len)
+static ssize_t readN(int fd, char *buf, size_t len)
 {
-	int ret, total = 0;
+	ssize_t ret;
+	size_t total = 0;
+
 	do {
 		ret = read(fd, buf + total, len - total);
 		if (ret < 0)
@@ -69,9 +71,11 @@ static int readN(int fd, char *buf, int len)
 	return total ?: ret;
 }
 
-static int writeN(int fd, const char *buf, int len)
+static ssize_t writeN(int fd, const char *buf, size_t len)
 {
-	int ret, total = 0;
+	ssize_t ret;
+	size_t total = 0;
+
 	do {
 		ret = write(fd, buf + total, len - total);
 		if (ret < 0)
@@ -218,8 +222,9 @@ bool igt_sysfs_set(int dir, const char *attr, const char *value)
 char *igt_sysfs_get(int dir, const char *attr)
 {
 	char *buf;
-	int len, offset, rem;
-	int ret, fd;
+	size_t len, offset, rem;
+	ssize_t ret;
+	int fd;
 
 	fd = openat(dir, attr, O_RDONLY);
 	if (igt_debug_on(fd < 0))
-- 
2.25.1

Re: [Intel-gfx] [igt-dev] [PATCH i-g-t 6/8] lib/igt_sysfs: Support large files

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:42PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> The syfs helper functions were all using basic 'int' data types for
> sizs, offsets, etc. when reading from sysfs. This works fine for
> little files, but not for large error capture logs (which can be
> gigabytes in sizes).
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  lib/igt_sysfs.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 6919ac361..ee75e3ef1 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -53,9 +53,11 @@
>   * provides basic support for like igt_sysfs_open().
>   */
>  
> -static int readN(int fd, char *buf, int len)
> +static ssize_t readN(int fd, char *buf, size_t len)
>  {
> -	int ret, total = 0;
> +	ssize_t ret;
> +	size_t total = 0;
> +
>  	do {
>  		ret = read(fd, buf + total, len - total);
>  		if (ret < 0)
> @@ -69,9 +71,11 @@ static int readN(int fd, char *buf, int len)
>  	return total ?: ret;
>  }
>  
> -static int writeN(int fd, const char *buf, int len)
> +static ssize_t writeN(int fd, const char *buf, size_t len)
>  {
> -	int ret, total = 0;
> +	ssize_t ret;
> +	size_t total = 0;
> +
>  	do {
>  		ret = write(fd, buf + total, len - total);
>  		if (ret < 0)
> @@ -218,8 +222,9 @@ bool igt_sysfs_set(int dir, const char *attr, const char *value)
>  char *igt_sysfs_get(int dir, const char *attr)
>  {
>  	char *buf;
> -	int len, offset, rem;
> -	int ret, fd;
> +	size_t len, offset, rem;
> +	ssize_t ret;
> +	int fd;
>  
>  	fd = openat(dir, attr, O_RDONLY);
>  	if (igt_debug_on(fd < 0))
> -- 
> 2.25.1
> 

[Intel-gfx] [PATCH i-g-t 7/8] lib/igt_gt: Allow per engine reset testing

[John.C.Harrison]

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

With GuC submission, engine resets are handled entirely within GuC
rather than within i915. Traditionally, IGT has disallowed engine
based resets becuase they don't send the uevent which IGT uses to
check for unexpected resets. However, it is important to be able to
test all reset mechanisms that can be used, so allow engine based
resets to be enabled.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 lib/igt_gt.c | 44 +++++++++++++++++++++++++++++---------------
 lib/igt_gt.h |  1 +
 2 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/lib/igt_gt.c b/lib/igt_gt.c
index a0ba04cc1..7c7df95ee 100644
--- a/lib/igt_gt.c
+++ b/lib/igt_gt.c
@@ -56,23 +56,28 @@
  * engines.
  */
 
+static int reset_query_once = -1;
+
 static bool has_gpu_reset(int fd)
 {
-	static int once = -1;
-	if (once < 0) {
-		struct drm_i915_getparam gp;
-		int val = 0;
-
-		memset(&gp, 0, sizeof(gp));
-		gp.param = 35; /* HAS_GPU_RESET */
-		gp.value = &val;
-
-		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
-			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
-		else
-			once = val > 0;
+	if (reset_query_once < 0) {
+		reset_query_once = gem_gpu_reset_type(fd);
+
+		/* Very old kernels did not support the query */
+		if (reset_query_once == -1)
+			reset_query_once =
+			      (intel_gen(intel_get_drm_devid(fd)) >= 5) ? 1 : 0;
 	}
-	return once;
+
+	return reset_query_once > 0;
+}
+
+static bool has_engine_reset(int fd)
+{
+	if (reset_query_once < 0)
+		has_gpu_reset(fd);
+
+	return reset_query_once > 1;
 }
 
 static void eat_error_state(int dev)
@@ -176,7 +181,11 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 		igt_skip("hang injection disabled by user [IGT_HANG=0]\n");
 	gem_context_require_bannable(fd);
 
-	allow_reset = 1;
+	if (flags & HANG_WANT_ENGINE_RESET)
+		allow_reset = 2;
+	else
+		allow_reset = 1;
+
 	if ((flags & HANG_ALLOW_CAPTURE) == 0) {
 		param.param = I915_CONTEXT_PARAM_NO_ERROR_CAPTURE;
 		param.value = 1;
@@ -187,11 +196,16 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
 		__gem_context_set_param(fd, &param);
 		allow_reset = INT_MAX; /* any reset method */
 	}
+
 	igt_require(igt_params_set(fd, "reset", "%d", allow_reset));
+	reset_query_once = -1;  /* Re-query after changing param */
 
 	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
 		igt_require(has_gpu_reset(fd));
 
+	if (flags & HANG_WANT_ENGINE_RESET)
+		igt_require(has_engine_reset(fd));
+
 	ban = context_get_ban(fd, ctx);
 	if ((flags & HANG_ALLOW_BAN) == 0)
 		context_set_ban(fd, ctx, 0);
diff --git a/lib/igt_gt.h b/lib/igt_gt.h
index ceb044b86..c5059817b 100644
--- a/lib/igt_gt.h
+++ b/lib/igt_gt.h
@@ -51,6 +51,7 @@ igt_hang_t igt_hang_ctx_with_ahnd(int fd, uint64_t ahnd, uint32_t ctx, int ring,
 
 #define HANG_ALLOW_BAN 1
 #define HANG_ALLOW_CAPTURE 2
+#define HANG_WANT_ENGINE_RESET 4
 
 igt_hang_t igt_hang_ring(int fd, int ring);
 igt_hang_t igt_hang_ring_with_ahnd(int fd, int ring, uint64_t ahnd);
-- 
2.25.1

Re: [Intel-gfx] [PATCH i-g-t 7/8] lib/igt_gt: Allow per engine reset testing

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:43PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> With GuC submission, engine resets are handled entirely within GuC
> rather than within i915. Traditionally, IGT has disallowed engine
> based resets becuase they don't send the uevent which IGT uses to
> check for unexpected resets. However, it is important to be able to
> test all reset mechanisms that can be used, so allow engine based
> resets to be enabled.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  lib/igt_gt.c | 44 +++++++++++++++++++++++++++++---------------
>  lib/igt_gt.h |  1 +
>  2 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/lib/igt_gt.c b/lib/igt_gt.c
> index a0ba04cc1..7c7df95ee 100644
> --- a/lib/igt_gt.c
> +++ b/lib/igt_gt.c
> @@ -56,23 +56,28 @@
>   * engines.
>   */
>  
> +static int reset_query_once = -1;
> +
>  static bool has_gpu_reset(int fd)
>  {
> -	static int once = -1;
> -	if (once < 0) {
> -		struct drm_i915_getparam gp;
> -		int val = 0;
> -
> -		memset(&gp, 0, sizeof(gp));
> -		gp.param = 35; /* HAS_GPU_RESET */
> -		gp.value = &val;
> -
> -		if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> -			once = intel_gen(intel_get_drm_devid(fd)) >= 5;
> -		else
> -			once = val > 0;
> +	if (reset_query_once < 0) {
> +		reset_query_once = gem_gpu_reset_type(fd);
> +
> +		/* Very old kernels did not support the query */
> +		if (reset_query_once == -1)
> +			reset_query_once =
> +			      (intel_gen(intel_get_drm_devid(fd)) >= 5) ? 1 : 0;
>  	}
> -	return once;
> +
> +	return reset_query_once > 0;
> +}
> +
> +static bool has_engine_reset(int fd)
> +{
> +	if (reset_query_once < 0)
> +		has_gpu_reset(fd);
> +
> +	return reset_query_once > 1;
>  }
>  
>  static void eat_error_state(int dev)
> @@ -176,7 +181,11 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>  		igt_skip("hang injection disabled by user [IGT_HANG=0]\n");
>  	gem_context_require_bannable(fd);
>  
> -	allow_reset = 1;
> +	if (flags & HANG_WANT_ENGINE_RESET)
> +		allow_reset = 2;
> +	else
> +		allow_reset = 1;
> +
>  	if ((flags & HANG_ALLOW_CAPTURE) == 0) {
>  		param.param = I915_CONTEXT_PARAM_NO_ERROR_CAPTURE;
>  		param.value = 1;
> @@ -187,11 +196,16 @@ igt_hang_t igt_allow_hang(int fd, unsigned ctx, unsigned flags)
>  		__gem_context_set_param(fd, &param);
>  		allow_reset = INT_MAX; /* any reset method */
>  	}
> +
>  	igt_require(igt_params_set(fd, "reset", "%d", allow_reset));
> +	reset_query_once = -1;  /* Re-query after changing param */
>  
>  	if (!igt_check_boolean_env_var("IGT_HANG_WITHOUT_RESET", false))
>  		igt_require(has_gpu_reset(fd));
>  
> +	if (flags & HANG_WANT_ENGINE_RESET)
> +		igt_require(has_engine_reset(fd));
> +
>  	ban = context_get_ban(fd, ctx);
>  	if ((flags & HANG_ALLOW_BAN) == 0)
>  		context_set_ban(fd, ctx, 0);
> diff --git a/lib/igt_gt.h b/lib/igt_gt.h
> index ceb044b86..c5059817b 100644
> --- a/lib/igt_gt.h
> +++ b/lib/igt_gt.h
> @@ -51,6 +51,7 @@ igt_hang_t igt_hang_ctx_with_ahnd(int fd, uint64_t ahnd, uint32_t ctx, int ring,
>  
>  #define HANG_ALLOW_BAN 1
>  #define HANG_ALLOW_CAPTURE 2
> +#define HANG_WANT_ENGINE_RESET 4
>  
>  igt_hang_t igt_hang_ring(int fd, int ring);
>  igt_hang_t igt_hang_ring_with_ahnd(int fd, int ring, uint64_t ahnd);
> -- 
> 2.25.1
> 

[Intel-gfx] [PATCH i-g-t 8/8] tests/i915/gem_exec_capture: Update to support GuC based resets

[John.C.Harrison]

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

When GuC submission is enabled, GuC itself manages hang detection and
recovery. Therefore, any test that relies on being able to trigger an
engine reset in the driver will fail. Full GT resets can still be
triggered by the driver. However, in that situation detecting the
specific context that caused a hang is not possible as the driver has
no information about what is actually running on the hardware at any
given time. Plus of course, there was no context that caused the hang
because the hang was triggered manually, so it's basically a bogus
mechanism in the first place!

Update the capture test to cause a reset via a the hangcheck mechanism
by submitting a hanging batch and waiting. That way it is guaranteed to
be testing the correct reset code paths for the current platform,
whether that is GuC enabled or not.

Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
---
 tests/i915/gem_exec_capture.c | 65 ++++++++++++++++++++++++++++-------
 1 file changed, 53 insertions(+), 12 deletions(-)

diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
index 8997125ee..dda6e6a8f 100644
--- a/tests/i915/gem_exec_capture.c
+++ b/tests/i915/gem_exec_capture.c
@@ -23,6 +23,7 @@
 
 #include <sys/poll.h>
 #include <zlib.h>
+#include <sched.h>
 
 #include "i915/gem.h"
 #include "i915/gem_create.h"
@@ -31,6 +32,8 @@
 #include "igt_rand.h"
 #include "igt_sysfs.h"
 
+#define MAX_RESET_TIME	600
+
 IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a hang");
 
 struct offset {
@@ -213,7 +216,29 @@ static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int
 	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
 
 	/* Allow engine based resets and disable banning */
-	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
+	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
+}
+
+static bool fence_busy(int fence)
+{
+	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;
+}
+
+static void wait_to_die(int fence_out)
+{
+	struct timeval before, after, delta;
+
+	/* Wait for a reset to occur */
+	gettimeofday(&before, NULL);
+	while (fence_busy(fence_out)) {
+		gettimeofday(&after, NULL);
+		timersub(&after, &before, &delta);
+		igt_assert(delta.tv_sec < MAX_RESET_TIME);
+		sched_yield();
+	}
+	gettimeofday(&after, NULL);
+	timersub(&after, &before, &delta);
+	igt_info("Target died after %ld.%06lds\n", delta.tv_sec, delta.tv_usec);
 }
 
 static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
@@ -230,7 +255,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	struct drm_i915_gem_execbuffer2 execbuf;
 	uint32_t *batch, *seqno;
 	struct offset offset;
-	int i;
+	int i, fence_out;
 
 	configure_hangs(fd, e, ctx->id);
 
@@ -315,18 +340,25 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	execbuf.flags = e->flags;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
+	execbuf.flags |= I915_EXEC_FENCE_OUT;
 	execbuf.rsvd1 = ctx->id;
+	execbuf.rsvd2 = ~0UL;
 
 	igt_assert(!READ_ONCE(*seqno));
-	gem_execbuf(fd, &execbuf);
+	gem_execbuf_wr(fd, &execbuf);
+
+	fence_out = execbuf.rsvd2 >> 32;
+	igt_assert(fence_out >= 0);
 
 	/* Wait for the request to start */
 	while (READ_ONCE(*seqno) != 0xc0ffee)
 		igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle));
 	munmap(seqno, 4096);
 
+	/* Wait for a reset to occur */
+	wait_to_die(fence_out);
+
 	/* Check that only the buffer we marked is reported in the error */
-	igt_force_gpu_reset(fd);
 	memset(&offset, 0, sizeof(offset));
 	offset.addr = obj[CAPTURE].offset;
 	igt_assert_eq(check_error_state(dir, &offset, 1, target_size, false), 1);
@@ -373,7 +405,8 @@ static int cmp(const void *A, const void *B)
 static struct offset *
 __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	   const struct intel_execution_engine2 *e,
-	   unsigned int size, int count, unsigned int flags)
+	   unsigned int size, int count,
+	   unsigned int flags, int *_fence_out)
 #define INCREMENTAL 0x1
 #define ASYNC 0x2
 {
@@ -383,7 +416,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	struct drm_i915_gem_execbuffer2 execbuf;
 	uint32_t *batch, *seqno;
 	struct offset *offsets;
-	int i;
+	int i, fence_out;
 
 	configure_hangs(fd, e, ctx->id);
 
@@ -491,10 +524,17 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	execbuf.flags = e->flags;
 	if (gen > 3 && gen < 6)
 		execbuf.flags |= I915_EXEC_SECURE;
+	execbuf.flags |= I915_EXEC_FENCE_OUT;
 	execbuf.rsvd1 = ctx->id;
+	execbuf.rsvd2 = ~0UL;
 
 	igt_assert(!READ_ONCE(*seqno));
-	gem_execbuf(fd, &execbuf);
+	gem_execbuf_wr(fd, &execbuf);
+
+	fence_out = execbuf.rsvd2 >> 32;
+	igt_assert(fence_out >= 0);
+	if (_fence_out)
+		*_fence_out = fence_out;
 
 	/* Wait for the request to start */
 	while (READ_ONCE(*seqno) != 0xc0ffee)
@@ -502,7 +542,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
 	munmap(seqno, 4096);
 
 	if (!(flags & ASYNC)) {
-		igt_force_gpu_reset(fd);
+		wait_to_die(fence_out);
 		gem_sync(fd, obj[count + 1].handle);
 	}
 
@@ -549,7 +589,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
 	intel_require_memory(count, size, CHECK_RAM);
 	ahnd = get_reloc_ahnd(fd, ctx->id);
 
-	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
+	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags, NULL);
 
 	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
 	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
@@ -602,6 +642,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 	igt_assert(pipe(link) == 0);
 	igt_fork(child, 1) {
 		const intel_ctx_t *ctx2;
+		int fence_out;
 		fd = gem_reopen_driver(fd);
 		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
 			  count, (int)(size >> 20));
@@ -613,11 +654,11 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
 		/* Reopen the allocator in the new process. */
 		ahnd = get_reloc_ahnd(fd, ctx2->id);
 
-		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
+		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC, &fence_out));
 		put_ahnd(ahnd);
 
 		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
-		igt_force_gpu_reset(fd);
+		wait_to_die(fence_out);
 		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
 	}
 	read(link[0], &dummy, sizeof(dummy));
@@ -714,7 +755,7 @@ igt_main
 		gem_require_mmap_wc(fd);
 		igt_require(has_capture(fd));
 		ctx = intel_ctx_create_all_physical(fd);
-		igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE);
+		igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
 
 		dir = igt_sysfs_open(fd);
 		igt_require(igt_sysfs_set(dir, "error", "Begone!"));
-- 
2.25.1

Re: [Intel-gfx] [PATCH i-g-t 8/8] tests/i915/gem_exec_capture: Update to support GuC based resets

[Matthew Brost]

On Thu, Oct 21, 2021 at 04:40:44PM -0700, John.C.Harrison@Intel.com wrote:
> From: John Harrison <John.C.Harrison@Intel.com>
> 
> When GuC submission is enabled, GuC itself manages hang detection and
> recovery. Therefore, any test that relies on being able to trigger an
> engine reset in the driver will fail. Full GT resets can still be
> triggered by the driver. However, in that situation detecting the
> specific context that caused a hang is not possible as the driver has
> no information about what is actually running on the hardware at any
> given time. Plus of course, there was no context that caused the hang
> because the hang was triggered manually, so it's basically a bogus
> mechanism in the first place!
> 
> Update the capture test to cause a reset via a the hangcheck mechanism
> by submitting a hanging batch and waiting. That way it is guaranteed to
> be testing the correct reset code paths for the current platform,
> whether that is GuC enabled or not.
> 
> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>

Reviewed-by: Matthew Brost <matthew.brost@intel.com>

> ---
>  tests/i915/gem_exec_capture.c | 65 ++++++++++++++++++++++++++++-------
>  1 file changed, 53 insertions(+), 12 deletions(-)
> 
> diff --git a/tests/i915/gem_exec_capture.c b/tests/i915/gem_exec_capture.c
> index 8997125ee..dda6e6a8f 100644
> --- a/tests/i915/gem_exec_capture.c
> +++ b/tests/i915/gem_exec_capture.c
> @@ -23,6 +23,7 @@
>  
>  #include <sys/poll.h>
>  #include <zlib.h>
> +#include <sched.h>
>  
>  #include "i915/gem.h"
>  #include "i915/gem_create.h"
> @@ -31,6 +32,8 @@
>  #include "igt_rand.h"
>  #include "igt_sysfs.h"
>  
> +#define MAX_RESET_TIME	600
> +
>  IGT_TEST_DESCRIPTION("Check that we capture the user specified objects on a hang");
>  
>  struct offset {
> @@ -213,7 +216,29 @@ static void configure_hangs(int fd, const struct intel_execution_engine2 *e, int
>  	gem_engine_property_printf(fd, e->name, "heartbeat_interval_ms", "%d", 500);
>  
>  	/* Allow engine based resets and disable banning */
> -	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE);
> +	igt_allow_hang(fd, ctxt_id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
> +}
> +
> +static bool fence_busy(int fence)
> +{
> +	return poll(&(struct pollfd){fence, POLLIN}, 1, 0) == 0;
> +}
> +
> +static void wait_to_die(int fence_out)
> +{
> +	struct timeval before, after, delta;
> +
> +	/* Wait for a reset to occur */
> +	gettimeofday(&before, NULL);
> +	while (fence_busy(fence_out)) {
> +		gettimeofday(&after, NULL);
> +		timersub(&after, &before, &delta);
> +		igt_assert(delta.tv_sec < MAX_RESET_TIME);
> +		sched_yield();
> +	}
> +	gettimeofday(&after, NULL);
> +	timersub(&after, &before, &delta);
> +	igt_info("Target died after %ld.%06lds\n", delta.tv_sec, delta.tv_usec);
>  }
>  
>  static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
> @@ -230,7 +255,7 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	uint32_t *batch, *seqno;
>  	struct offset offset;
> -	int i;
> +	int i, fence_out;
>  
>  	configure_hangs(fd, e, ctx->id);
>  
> @@ -315,18 +340,25 @@ static void __capture1(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	execbuf.flags = e->flags;
>  	if (gen > 3 && gen < 6)
>  		execbuf.flags |= I915_EXEC_SECURE;
> +	execbuf.flags |= I915_EXEC_FENCE_OUT;
>  	execbuf.rsvd1 = ctx->id;
> +	execbuf.rsvd2 = ~0UL;
>  
>  	igt_assert(!READ_ONCE(*seqno));
> -	gem_execbuf(fd, &execbuf);
> +	gem_execbuf_wr(fd, &execbuf);
> +
> +	fence_out = execbuf.rsvd2 >> 32;
> +	igt_assert(fence_out >= 0);
>  
>  	/* Wait for the request to start */
>  	while (READ_ONCE(*seqno) != 0xc0ffee)
>  		igt_assert(gem_bo_busy(fd, obj[SCRATCH].handle));
>  	munmap(seqno, 4096);
>  
> +	/* Wait for a reset to occur */
> +	wait_to_die(fence_out);
> +
>  	/* Check that only the buffer we marked is reported in the error */
> -	igt_force_gpu_reset(fd);
>  	memset(&offset, 0, sizeof(offset));
>  	offset.addr = obj[CAPTURE].offset;
>  	igt_assert_eq(check_error_state(dir, &offset, 1, target_size, false), 1);
> @@ -373,7 +405,8 @@ static int cmp(const void *A, const void *B)
>  static struct offset *
>  __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	   const struct intel_execution_engine2 *e,
> -	   unsigned int size, int count, unsigned int flags)
> +	   unsigned int size, int count,
> +	   unsigned int flags, int *_fence_out)
>  #define INCREMENTAL 0x1
>  #define ASYNC 0x2
>  {
> @@ -383,7 +416,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	struct drm_i915_gem_execbuffer2 execbuf;
>  	uint32_t *batch, *seqno;
>  	struct offset *offsets;
> -	int i;
> +	int i, fence_out;
>  
>  	configure_hangs(fd, e, ctx->id);
>  
> @@ -491,10 +524,17 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	execbuf.flags = e->flags;
>  	if (gen > 3 && gen < 6)
>  		execbuf.flags |= I915_EXEC_SECURE;
> +	execbuf.flags |= I915_EXEC_FENCE_OUT;
>  	execbuf.rsvd1 = ctx->id;
> +	execbuf.rsvd2 = ~0UL;
>  
>  	igt_assert(!READ_ONCE(*seqno));
> -	gem_execbuf(fd, &execbuf);
> +	gem_execbuf_wr(fd, &execbuf);
> +
> +	fence_out = execbuf.rsvd2 >> 32;
> +	igt_assert(fence_out >= 0);
> +	if (_fence_out)
> +		*_fence_out = fence_out;
>  
>  	/* Wait for the request to start */
>  	while (READ_ONCE(*seqno) != 0xc0ffee)
> @@ -502,7 +542,7 @@ __captureN(int fd, int dir, uint64_t ahnd, const intel_ctx_t *ctx,
>  	munmap(seqno, 4096);
>  
>  	if (!(flags & ASYNC)) {
> -		igt_force_gpu_reset(fd);
> +		wait_to_die(fence_out);
>  		gem_sync(fd, obj[count + 1].handle);
>  	}
>  
> @@ -549,7 +589,7 @@ static void many(int fd, int dir, uint64_t size, unsigned int flags)
>  	intel_require_memory(count, size, CHECK_RAM);
>  	ahnd = get_reloc_ahnd(fd, ctx->id);
>  
> -	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags);
> +	offsets = __captureN(fd, dir, ahnd, ctx, e, size, count, flags, NULL);
>  
>  	blobs = check_error_state(dir, offsets, count, size, !!(flags & INCREMENTAL));
>  	igt_info("Captured %lu %"PRId64"-blobs out of a total of %lu\n",
> @@ -602,6 +642,7 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  	igt_assert(pipe(link) == 0);
>  	igt_fork(child, 1) {
>  		const intel_ctx_t *ctx2;
> +		int fence_out;
>  		fd = gem_reopen_driver(fd);
>  		igt_debug("Submitting large capture [%ld x %dMiB objects]\n",
>  			  count, (int)(size >> 20));
> @@ -613,11 +654,11 @@ static void prioinv(int fd, int dir, const intel_ctx_t *ctx,
>  		/* Reopen the allocator in the new process. */
>  		ahnd = get_reloc_ahnd(fd, ctx2->id);
>  
> -		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC));
> +		free(__captureN(fd, dir, ahnd, ctx2, e, size, count, ASYNC, &fence_out));
>  		put_ahnd(ahnd);
>  
>  		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
> -		igt_force_gpu_reset(fd);
> +		wait_to_die(fence_out);
>  		write(link[1], &fd, sizeof(fd)); /* wake the parent up */
>  	}
>  	read(link[0], &dummy, sizeof(dummy));
> @@ -714,7 +755,7 @@ igt_main
>  		gem_require_mmap_wc(fd);
>  		igt_require(has_capture(fd));
>  		ctx = intel_ctx_create_all_physical(fd);
> -		igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE);
> +		igt_allow_hang(fd, ctx->id, HANG_ALLOW_CAPTURE | HANG_WANT_ENGINE_RESET);
>  
>  		dir = igt_sysfs_open(fd);
>  		igt_require(igt_sysfs_set(dir, "error", "Begone!"));
> -- 
> 2.25.1
>