[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 12:09 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> 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?

Yes, I'll reword it that it's only between the current context and aux_context.

Marek