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

[Nicolai Hähnle]

On 17.08.2016 12:04, Marek Olšák wrote:
> 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.

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 <nicolai.haehnle at amd.com>

>
> Marek
>