Discussion:
[PATCH 1/4] intel/aubinator: Allow for the case where the ascii85 decode fails
(too old to reply)
Jason Ekstrand
2017-12-27 20:58:12 UTC
Permalink
Raw Message
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64" failed.\n",
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
--
2.5.0.400.gff86faf
Jason Ekstrand
2017-12-27 20:58:13 UTC
Permalink
Raw Message
We were walking the sections, printing the batches, and then freeing
them in one pass. If the batch happens to reference any earlier
sections (which it almost certainly will since it's at the end), we will
access freed memory.
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
index f0c5b5b..5f5b6af 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -523,12 +523,14 @@ read_data_file(FILE *file)
gen_print_batch(&batch_ctx, sections[s].data, sections[s].count,
sections[s].gtt_offset);
}
+ }
+
+ gen_batch_decode_ctx_finish(&batch_ctx);

+ for (int s = 0; s < sect_num; s++) {
free(sections[s].ring_name);
free(sections[s].data);
}
-
- gen_batch_decode_ctx_finish(&batch_ctx);
}

static void
--
2.5.0.400.gff86faf
Lionel Landwerlin
2017-12-28 17:24:48 UTC
Permalink
Raw Message
Good catch!
Post by Jason Ekstrand
We were walking the sections, printing the batches, and then freeing
them in one pass. If the batch happens to reference any earlier
sections (which it almost certainly will since it's at the end), we will
access freed memory.
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
index f0c5b5b..5f5b6af 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -523,12 +523,14 @@ read_data_file(FILE *file)
gen_print_batch(&batch_ctx, sections[s].data, sections[s].count,
sections[s].gtt_offset);
}
+ }
+
+ gen_batch_decode_ctx_finish(&batch_ctx);
+ for (int s = 0; s < sect_num; s++) {
free(sections[s].ring_name);
free(sections[s].data);
}
-
- gen_batch_decode_ctx_finish(&batch_ctx);
}
static void
Jason Ekstrand
2017-12-27 20:58:15 UTC
Permalink
Raw Message
Previously, we were flagging the instruction state buffer for capture
but not surface state or dynamic state. We want those captured too.
---
src/intel/vulkan/anv_device.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 4638f31..680f5a7 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1251,7 +1251,8 @@ VkResult anv_CreateDevice(
goto fail_batch_bo_pool;

/* For the state pools we explicitly disable 48bit. */
- bo_flags = physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0;
+ bo_flags = (physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0) |
+ (physical_device->has_exec_capture ? EXEC_OBJECT_CAPTURE : 0);

result = anv_state_pool_init(&device->dynamic_state_pool, device, 16384,
bo_flags);
@@ -1259,8 +1260,7 @@ VkResult anv_CreateDevice(
goto fail_bo_cache;

result = anv_state_pool_init(&device->instruction_state_pool, device, 16384,
- bo_flags |
- (physical_device->has_exec_capture ? EXEC_OBJECT_CAPTURE : 0));
+ bo_flags);
if (result != VK_SUCCESS)
goto fail_dynamic_state_pool;
--
2.5.0.400.gff86faf
Lionel Landwerlin
2017-12-28 17:25:27 UTC
Permalink
Raw Message
Post by Jason Ekstrand
Previously, we were flagging the instruction state buffer for capture
but not surface state or dynamic state. We want those captured too.
---
src/intel/vulkan/anv_device.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.c
index 4638f31..680f5a7 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1251,7 +1251,8 @@ VkResult anv_CreateDevice(
goto fail_batch_bo_pool;
/* For the state pools we explicitly disable 48bit. */
- bo_flags = physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0;
+ bo_flags = (physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0) |
+ (physical_device->has_exec_capture ? EXEC_OBJECT_CAPTURE : 0);
result = anv_state_pool_init(&device->dynamic_state_pool, device, 16384,
bo_flags);
@@ -1259,8 +1260,7 @@ VkResult anv_CreateDevice(
goto fail_bo_cache;
result = anv_state_pool_init(&device->instruction_state_pool, device, 16384,
- bo_flags |
- (physical_device->has_exec_capture ? EXEC_OBJECT_CAPTURE : 0));
+ bo_flags);
if (result != VK_SUCCESS)
goto fail_dynamic_state_pool;
Jason Ekstrand
2017-12-28 18:43:33 UTC
Permalink
Raw Message
Thanks! I've pushed the last 3. I'll let the debate continue on 1/4. :-)

On Thu, Dec 28, 2017 at 9:25 AM, Lionel Landwerlin <
Post by Jason Ekstrand
Previously, we were flagging the instruction state buffer for capture
but not surface state or dynamic state. We want those captured too.
---
src/intel/vulkan/anv_device.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/intel/vulkan/anv_device.c b/src/intel/vulkan/anv_device.
c
index 4638f31..680f5a7 100644
--- a/src/intel/vulkan/anv_device.c
+++ b/src/intel/vulkan/anv_device.c
@@ -1251,7 +1251,8 @@ VkResult anv_CreateDevice(
goto fail_batch_bo_pool;
/* For the state pools we explicitly disable 48bit. */
- bo_flags = physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0;
+ bo_flags = (physical_device->has_exec_async ? EXEC_OBJECT_ASYNC : 0) |
+ (physical_device->has_exec_capture ? EXEC_OBJECT_CAPTURE : 0);
result = anv_state_pool_init(&device->dynamic_state_pool, device, 16384,
bo_flags);
@@ -1259,8 +1260,7 @@ VkResult anv_CreateDevice(
goto fail_bo_cache;
result = anv_state_pool_init(&device->instruction_state_pool, device, 16384,
- bo_flags |
- (physical_device->has_exec_capture ?
EXEC_OBJECT_CAPTURE : 0));
+ bo_flags);
if (result != VK_SUCCESS)
goto fail_dynamic_state_pool;
Jason Ekstrand
2017-12-27 20:58:14 UTC
Permalink
Raw Message
Some older versions of the Vulkan driver didn't properly tag dynamic
state as needing to be captured. Also, this prevents crashes when
looking at dumps on older kernels.
---
src/intel/tools/gen_batch_decoder.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/src/intel/tools/gen_batch_decoder.c b/src/intel/tools/gen_batch_decoder.c
index e8dd19b..f09b833 100644
--- a/src/intel/tools/gen_batch_decoder.c
+++ b/src/intel/tools/gen_batch_decoder.c
@@ -582,6 +582,11 @@ decode_dynamic_state_pointers(struct gen_batch_decode_ctx *ctx,
const char *struct_type, const uint32_t *p,
int count)
{
+ if (ctx->dynamic_base.map == NULL) {
+ fprintf(ctx->fp, " dynamic %s state unavailable\n", struct_type);
+ return;
+ }
+
struct gen_group *inst = gen_spec_find_instruction(ctx->spec, p);
struct gen_group *state = gen_spec_find_struct(ctx->spec, struct_type);
--
2.5.0.400.gff86faf
Lionel Landwerlin
2017-12-28 17:25:07 UTC
Permalink
Raw Message
Post by Jason Ekstrand
Some older versions of the Vulkan driver didn't properly tag dynamic
state as needing to be captured. Also, this prevents crashes when
looking at dumps on older kernels.
---
src/intel/tools/gen_batch_decoder.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/src/intel/tools/gen_batch_decoder.c b/src/intel/tools/gen_batch_decoder.c
index e8dd19b..f09b833 100644
--- a/src/intel/tools/gen_batch_decoder.c
+++ b/src/intel/tools/gen_batch_decoder.c
@@ -582,6 +582,11 @@ decode_dynamic_state_pointers(struct gen_batch_decode_ctx *ctx,
const char *struct_type, const uint32_t *p,
int count)
{
+ if (ctx->dynamic_base.map == NULL) {
+ fprintf(ctx->fp, " dynamic %s state unavailable\n", struct_type);
+ return;
+ }
+
struct gen_group *inst = gen_spec_find_instruction(ctx->spec, p);
struct gen_group *state = gen_spec_find_struct(ctx->spec, struct_type);
Kenneth Graunke
2017-12-27 23:06:38 UTC
Permalink
Raw Message
Post by Jason Ekstrand
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64" failed.\n",
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
What's the rationale, here? At this point, you've got a malformed file.
What do we gain by supporting invalid file formats?
Jason Ekstrand
2017-12-27 23:13:42 UTC
Permalink
Raw Message
Post by Kenneth Graunke
Post by Jason Ekstrand
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c
b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64" failed.\n",
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
What's the rationale, here? At this point, you've got a malformed file.
What do we gain by supporting invalid file formats?
Because there's a decent chance (I've got multiple files on my laptop that
are this way) that other BOs will decompress ok. I still don't know why
aubinator is failing to decompress things.

--Jason
Kenneth Graunke
2017-12-28 07:30:06 UTC
Permalink
Raw Message
Post by Jason Ekstrand
Post by Kenneth Graunke
Post by Jason Ekstrand
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c
b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64" failed.\n",
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
What's the rationale, here? At this point, you've got a malformed file.
What do we gain by supporting invalid file formats?
Because there's a decent chance (I've got multiple files on my laptop that
are this way) that other BOs will decompress ok. I still don't know why
aubinator is failing to decompress things.
--Jason
Are they just from old pre-ascii85 support kernels or something?
Jason Ekstrand
2017-12-28 14:24:00 UTC
Permalink
Raw Message
Post by Kenneth Graunke
Post by Jason Ekstrand
Post by Kenneth Graunke
Post by Jason Ekstrand
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c
b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64"
failed.\n",
Post by Kenneth Graunke
Post by Jason Ekstrand
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
What's the rationale, here? At this point, you've got a malformed file.
What do we gain by supporting invalid file formats?
Because there's a decent chance (I've got multiple files on my laptop that
are this way) that other BOs will decompress ok. I still don't know why
aubinator is failing to decompress things.
--Jason
Are they just from old pre-ascii85 support kernels or something?
Nope, 4.14 or 4.13.
Jason Ekstrand
2017-12-28 14:26:25 UTC
Permalink
Raw Message
Post by Jason Ekstrand
Post by Kenneth Graunke
Post by Jason Ekstrand
Post by Kenneth Graunke
Post by Jason Ekstrand
---
src/intel/tools/aubinator_error_decode.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/src/intel/tools/aubinator_error_decode.c
b/src/intel/tools/aubinator_error_decode.c
index d6fbfe0..f0c5b5b 100644
--- a/src/intel/tools/aubinator_error_decode.c
+++ b/src/intel/tools/aubinator_error_decode.c
@@ -350,8 +350,10 @@ read_data_file(FILE *file)
uint32_t *data = NULL;
int count = ascii85_decode(line+1, &data, line[0] == ':');
if (count == 0) {
- fprintf(stderr, "ASCII85 decode failed.\n");
- exit(EXIT_FAILURE);
+ fprintf(stderr, "ASCII85 decode of %s at 0x%08"PRIx64"
failed.\n",
Post by Kenneth Graunke
Post by Jason Ekstrand
+ sections[sect_num].buffer_name,
+ sections[sect_num].gtt_offset);
+ continue;
}
sections[sect_num].data = data;
sections[sect_num].count = count;
What's the rationale, here? At this point, you've got a malformed file.
What do we gain by supporting invalid file formats?
Because there's a decent chance (I've got multiple files on my laptop that
are this way) that other BOs will decompress ok. I still don't know why
aubinator is failing to decompress things.
--Jason
Are they just from old pre-ascii85 support kernels or something?
Nope, 4.14 or 4.13.
I should be more specific. It's not ascii85 decide that fails, it's zlib
decompression. I have no idea why.
Loading...