Discussion:
[PATCH 1/3] radeonsi/gfx9: fix the scissor bug workaround
(too old to reply)
Marek Olšák
2017-08-10 19:57:52 UTC
Permalink
Raw Message
From: Marek Olšák <***@amd.com>

otherwise there is corruption in most apps.

Fixes: 0fe0320 radeonsi: use optimal packet order when doing a pipeline sync
---
src/gallium/drivers/radeonsi/si_state_draw.c | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_state_draw.c b/src/gallium/drivers/radeonsi/si_state_draw.c
index 23e9778..deb0691 100644
--- a/src/gallium/drivers/radeonsi/si_state_draw.c
+++ b/src/gallium/drivers/radeonsi/si_state_draw.c
@@ -1361,25 +1361,29 @@ void si_draw_vbo(struct pipe_context *ctx, const struct pipe_draw_info *info)

si_need_cs_space(sctx);

/* Since we've called r600_context_add_resource_size for vertex buffers,
* this must be called after si_need_cs_space, because we must let
* need_cs_space flush before we add buffers to the buffer list.
*/
if (!si_upload_vertex_buffer_descriptors(sctx))
return;

- /* GFX9 scissor bug workaround. There is also a more efficient but
- * more involved alternative workaround. */
+ /* GFX9 scissor bug workaround. This must be done before VPORT scissor
+ * registers are changed. There is also a more efficient but more
+ * involved alternative workaround.
+ */
if (sctx->b.chip_class == GFX9 &&
- si_is_atom_dirty(sctx, &sctx->b.scissors.atom))
+ si_is_atom_dirty(sctx, &sctx->b.scissors.atom)) {
sctx->b.flags |= SI_CONTEXT_PS_PARTIAL_FLUSH;
+ si_emit_cache_flush(sctx);
+ }

/* Use optimal packet order based on whether we need to sync the pipeline. */
if (unlikely(sctx->b.flags & (SI_CONTEXT_FLUSH_AND_INV_CB |
SI_CONTEXT_FLUSH_AND_INV_DB |
SI_CONTEXT_PS_PARTIAL_FLUSH |
SI_CONTEXT_CS_PARTIAL_FLUSH))) {
/* If we have to wait for idle, set all states first, so that all
* SET packets are processed in parallel with previous draw calls.
* Then upload descriptors, set shader pointers, and draw, and
* prefetch at the end. This ensures that the time the CUs
--
2.7.4
Marek Olšák
2017-08-10 19:57:54 UTC
Permalink
Raw Message
From: Marek Olšák <***@amd.com>

---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);

rscreen->disk_shader_cache =
- disk_cache_create(r600_get_family_name(rscreen),
+ disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
timestamp_str,
shader_debug_flags);
free(timestamp_str);
}
}
}

static struct disk_cache *r600_get_disk_shader_cache(struct pipe_screen *pscreen)
{
struct r600_common_screen *rscreen = (struct r600_common_screen*)pscreen;
--
2.7.4
Timothy Arceri
2017-08-11 01:09:29 UTC
Permalink
Raw Message
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
- disk_cache_create(r600_get_family_name(rscreen),
+ disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
I take it this will always by more fine grained than any compilation
options for the GLSL/TGSI stages?

Otherwise maybe we should store both like we do for the Mesa/LLVM build
timestamps.
Post by Marek Olšák
timestamp_str,
shader_debug_flags);
free(timestamp_str);
}
}
}
static struct disk_cache *r600_get_disk_shader_cache(struct pipe_screen *pscreen)
{
struct r600_common_screen *rscreen = (struct r600_common_screen*)pscreen;
Marek Olšák
2017-08-11 18:33:47 UTC
Permalink
Raw Message
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct
r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
-
disk_cache_create(r600_get_family_name(rscreen),
+
disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
I take it this will always by more fine grained than any compilation options
for the GLSL/TGSI stages?
Otherwise maybe we should store both like we do for the Mesa/LLVM build
timestamps.
I don't understand.

Marek
Nicolai Hähnle
2017-08-11 16:00:46 UTC
Permalink
Raw Message
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
- disk_cache_create(r600_get_family_name(rscreen),
+ disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
What's the advantage of this?
Post by Marek Olšák
timestamp_str,
shader_debug_flags);
free(timestamp_str);
}
}
}
static struct disk_cache *r600_get_disk_shader_cache(struct pipe_screen *pscreen)
{
struct r600_common_screen *rscreen = (struct r600_common_screen*)pscreen;
//
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
Marek Olšák
2017-08-11 18:37:28 UTC
Permalink
Raw Message
Post by Nicolai Hähnle
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct
r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
-
disk_cache_create(r600_get_family_name(rscreen),
+
disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
What's the advantage of this?
It's added to the shader cache key. It allows shaders cached for
Vega10 to be used by Raven and vice versa. Same for Polaris11 and
Polaris12. It makes things nicer for some multi-GPU setups or when
swapping GPUs.

Marek
Nicolai Hähnle
2017-08-18 10:49:49 UTC
Permalink
Raw Message
Post by Marek Olšák
Post by Nicolai Hähnle
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct
r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
-
disk_cache_create(r600_get_family_name(rscreen),
+
disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
What's the advantage of this?
It's added to the shader cache key. It allows shaders cached for
Vega10 to be used by Raven and vice versa. Same for Polaris11 and
Polaris12. It makes things nicer for some multi-GPU setups or when
swapping GPUs.
I'm not a huge fan of this since the shader code does have access to the
family, and there might be a need for family-specific workarounds. I'm
actually not a huge fan of this overall, because the debug_flags thing
seems flaky as well.

There's currently some corruption in Unigine demos which seems related
to the shader cache, which just goes to show how difficult it is to
really get this stuff right. I'd rather be on the safe side.

Cheers,
Nicolai
Post by Marek Olšák
Marek
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
Marek Olšák
2017-08-18 14:02:58 UTC
Permalink
Raw Message
Post by Nicolai Hähnle
Post by Marek Olšák
Post by Nicolai Hähnle
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct
r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
-
disk_cache_create(r600_get_family_name(rscreen),
+
disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
What's the advantage of this?
It's added to the shader cache key. It allows shaders cached for
Vega10 to be used by Raven and vice versa. Same for Polaris11 and
Polaris12. It makes things nicer for some multi-GPU setups or when
swapping GPUs.
I'm not a huge fan of this since the shader code does have access to the
family, and there might be a need for family-specific workarounds. I'm
actually not a huge fan of this overall, because the debug_flags thing seems
flaky as well.
There's currently some corruption in Unigine demos which seems related to
the shader cache, which just goes to show how difficult it is to really get
this stuff right. I'd rather be on the safe side.
I've dropped this patch.

What corruption in Unigine?

Marek
Timothy Arceri
2017-08-19 03:01:48 UTC
Permalink
Raw Message
Post by Nicolai Hähnle
Post by Marek Olšák
Post by Nicolai Hähnle
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c
b/src/gallium/drivers/radeon/r600_pipe_common.c
index 95458d2e..0038c9a 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -878,21 +878,21 @@ static void r600_disk_cache_create(struct
r600_common_screen *rscreen)
#endif
if (res != -1) {
/* These flags affect shader compilation. */
uint64_t shader_debug_flags =
rscreen->debug_flags &
(DBG_FS_CORRECT_DERIVS_AFTER_KILL |
DBG_SI_SCHED |
DBG_UNSAFE_MATH);
rscreen->disk_shader_cache =
-
disk_cache_create(r600_get_family_name(rscreen),
+
disk_cache_create(r600_get_llvm_processor_name(rscreen->family),
What's the advantage of this?
It's added to the shader cache key. It allows shaders cached for
Vega10 to be used by Raven and vice versa. Same for Polaris11 and
Polaris12. It makes things nicer for some multi-GPU setups or when
swapping GPUs.
I'm not a huge fan of this since the shader code does have access to the
family, and there might be a need for family-specific workarounds. I'm
actually not a huge fan of this overall, because the debug_flags thing
seems flaky as well.
There's currently some corruption in Unigine demos which seems related
to the shader cache, which just goes to show how difficult it is to
really get this stuff right. I'd rather be on the safe side.
I'm not sure if its related, but I noticed recently that there was at
least 1 piglit test that seems to have regressed for the shader cache.

Should be reproducible by running piglit twice in a row, I haven't
looked into it any further and can't recall what it was testing.
Post by Nicolai Hähnle
Cheers,
Nicolai
Post by Marek Olšák
Marek
Marek Olšák
2017-08-10 19:57:53 UTC
Permalink
Raw Message
From: Marek Olšák <***@amd.com>

---
src/gallium/drivers/radeon/r600_pipe_common.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
index 2acef6a..95458d2e 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -870,24 +870,31 @@ static void r600_disk_cache_create(struct r600_common_screen *rscreen)
else {
uint32_t llvm_timestamp;
if (disk_cache_get_function_timestamp(LLVMInitializeAMDGPUTargetInfo,
&llvm_timestamp)) {
res = asprintf(&timestamp_str, "%u_%u",
mesa_timestamp, llvm_timestamp);
}
}
#endif
if (res != -1) {
+ /* These flags affect shader compilation. */
+ uint64_t shader_debug_flags =
+ rscreen->debug_flags &
+ (DBG_FS_CORRECT_DERIVS_AFTER_KILL |
+ DBG_SI_SCHED |
+ DBG_UNSAFE_MATH);
+
rscreen->disk_shader_cache =
disk_cache_create(r600_get_family_name(rscreen),
timestamp_str,
- rscreen->debug_flags);
+ shader_debug_flags);
free(timestamp_str);
}
}
}

static struct disk_cache *r600_get_disk_shader_cache(struct pipe_screen *pscreen)
{
struct r600_common_screen *rscreen = (struct r600_common_screen*)pscreen;
return rscreen->disk_shader_cache;
}
--
2.7.4
Timothy Arceri
2017-08-11 01:02:40 UTC
Permalink
Raw Message
Reviewed-by: Timothy Arceri <***@itsqueeze.com>
Nicolai Hähnle
2017-08-11 16:01:30 UTC
Permalink
Raw Message
Post by Marek Olšák
---
src/gallium/drivers/radeon/r600_pipe_common.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c b/src/gallium/drivers/radeon/r600_pipe_common.c
index 2acef6a..95458d2e 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -870,24 +870,31 @@ static void r600_disk_cache_create(struct r600_common_screen *rscreen)
else {
uint32_t llvm_timestamp;
if (disk_cache_get_function_timestamp(LLVMInitializeAMDGPUTargetInfo,
&llvm_timestamp)) {
res = asprintf(&timestamp_str, "%u_%u",
mesa_timestamp, llvm_timestamp);
}
}
#endif
if (res != -1) {
+ /* These flags affect shader compilation. */
+ uint64_t shader_debug_flags =
+ rscreen->debug_flags &
+ (DBG_FS_CORRECT_DERIVS_AFTER_KILL |
+ DBG_SI_SCHED |
+ DBG_UNSAFE_MATH);
+
rscreen->disk_shader_cache =
disk_cache_create(r600_get_family_name(rscreen),
timestamp_str,
- rscreen->debug_flags);
+ shader_debug_flags);
free(timestamp_str);
}
}
}
static struct disk_cache *r600_get_disk_shader_cache(struct pipe_screen *pscreen)
{
struct r600_common_screen *rscreen = (struct r600_common_screen*)pscreen;
return rscreen->disk_shader_cache;
}
--
Lerne, wie die Welt wirklich ist,
Aber vergiss niemals, wie sie sein sollte.
Loading...