On 17.08.2016 12:04, Marek Olšák wrote:
On Wed, Aug 17, 2016 at 11:41 AM, Nicolai Hähnle <[email protected]> wrote:
On 11.08.2016 18:16, Marek Olšák wrote:
From: Marek Olšák <[email protected]>
This is just a workaround. The problem is described in the code.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=96541
---
src/gallium/drivers/radeon/r600_pipe_common.h | 2 +-
src/gallium/drivers/radeon/r600_texture.c | 35
++++++++++++++++++++++-----
src/gallium/drivers/radeonsi/si_blit.c | 12 +++------
src/gallium/drivers/radeonsi/si_descriptors.c | 2 +-
4 files changed, 35 insertions(+), 16 deletions(-)
diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h
b/src/gallium/drivers/radeon/r600_pipe_common.h
index e4002f9..5375044 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.h
+++ b/src/gallium/drivers/radeon/r600_pipe_common.h
@@ -764,21 +764,21 @@ void vi_separate_dcc_stop_query(struct pipe_context
*ctx,
void vi_separate_dcc_process_and_reset_stats(struct pipe_context *ctx,
struct r600_texture *tex);
void vi_dcc_clear_level(struct r600_common_context *rctx,
struct r600_texture *rtex,
unsigned level, unsigned clear_value);
void evergreen_do_fast_color_clear(struct r600_common_context *rctx,
struct pipe_framebuffer_state *fb,
struct r600_atom *fb_state,
unsigned *buffers, unsigned
*dirty_cbufs,
const union pipe_color_union *color);
-bool r600_texture_disable_dcc(struct r600_common_screen *rscreen,
+bool r600_texture_disable_dcc(struct r600_common_context *rctx,
struct r600_texture *rtex);
void r600_init_screen_texture_functions(struct r600_common_screen
*rscreen);
void r600_init_context_texture_functions(struct r600_common_context
*rctx);
/* r600_viewport.c */
void evergreen_apply_scissor_bug_workaround(struct r600_common_context
*rctx,
struct pipe_scissor_state
*scissor);
void r600_set_scissor_enable(struct r600_common_context *rctx, bool
enable);
void r600_update_vs_writes_viewport_index(struct r600_common_context
*rctx,
struct tgsi_shader_info *info);
diff --git a/src/gallium/drivers/radeon/r600_texture.c
b/src/gallium/drivers/radeon/r600_texture.c
index ddfa135..78dd177 100644
--- a/src/gallium/drivers/radeon/r600_texture.c
+++ b/src/gallium/drivers/radeon/r600_texture.c
@@ -393,34 +393,55 @@ static bool r600_texture_discard_dcc(struct
r600_common_screen *rscreen,
assert(rtex->dcc_separate_buffer == NULL);
/* Disable DCC. */
rtex->dcc_offset = 0;
/* Notify all contexts about the change. */
r600_dirty_all_framebuffer_states(rscreen);
return true;
}
-bool r600_texture_disable_dcc(struct r600_common_screen *rscreen,
+/**
+ * Disable DCC for the texture. (first decompress, then discard
metadata).
+ *
+ * Unresolved multi-context synchronization issue:
+ *
+ * If context 1 disables DCC and context 2 has queued commands that write
+ * to the texture via CB with DCC enabled, and the order of operations is
+ * as follows:
+ * context 2 queues draw calls rendering to the texture, but doesn't
flush
+ * context 1 disables DCC and flushes
+ * context 1 & 2 reset descriptors and FB state
+ * context 2 flushes (new compressed tiles written by the draw calls)
+ * context 1 & 2 read garbage, because DCC is disabled, yet there are
+ * compressed tiled
Should this case lead to defined result according to the spec? The reason
for context 1 disabling DCC is that it in some sense "reads" from the
texture -- via a blit, get_handle, or whatever. But without any
synchronization, those reads should be undefined anyway, because context 1
doesn't know whether context 2 has completed.
Granted, the _type_ of undefined behavior is perhaps unusual, because some
people might expect either the results from before context 2's draws or from
after context 2's draws, not some random DCC-related garbage, but should we
care?
Or maybe I'm missing some other situation?
You may be right. Maybe it's really undefined from the GL point of view.
The problem here was that the driver used aux_context from the screen
and it wasn't (and couldn't be) synchronized properly with the main
context because of DCC. All single-context applications can get the
corruption because aux_context is hidden to them.
Right. So the code change itself looks fine to me. The only question is
about the comment. I think it should be reworded slightly that this
multi-context behavior by applications should probably be undefined
anyway? Or perhaps we can remove it entirely. Either way,
Reviewed-by: Nicolai Hähnle <[email protected]>
Marek
_______________________________________________
mesa-dev mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
