[Mesa-dev] [PATCH 5/5] radeonsi: use current context for DCC feedback-loop decompress, fixes Elemental

[Marek Olšák]

On Wed, Aug 17, 2016 at 11:41 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 11.08.2016 18:16, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> 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.

Marek