[Mesa-dev] [PATCH 20/26] radeonsi: disable DCC on handle export if expecting write access

[Marek Olšák]

On Wed, Mar 9, 2016 at 7:19 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 02.03.2016 11:36, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> This should be okay except that sampler views and images are not re-set.
>> ---
>>   src/gallium/drivers/radeon/r600_pipe_common.h |  3 +++
>>   src/gallium/drivers/radeon/r600_texture.c     | 33
>> +++++++++++++++++++++++++++
>>   src/gallium/drivers/radeonsi/si_blit.c        | 12 ++++++++++
>>   3 files changed, 48 insertions(+)
>>
>> diff --git a/src/gallium/drivers/radeon/r600_pipe_common.h
>> b/src/gallium/drivers/radeon/r600_pipe_common.h
>> index 6e65742..43218f1 100644
>> --- a/src/gallium/drivers/radeon/r600_pipe_common.h
>> +++ b/src/gallium/drivers/radeon/r600_pipe_common.h
>> @@ -481,6 +481,9 @@ struct r600_common_context {
>>                                       unsigned first_layer, unsigned
>> last_layer,
>>                                       unsigned first_sample, unsigned
>> last_sample);
>>
>> +       void (*decompress_dcc)(struct pipe_context *ctx,
>> +                              struct r600_texture *rtex);
>> +
>>         /* Reallocate the buffer and update all resource bindings where
>>          * the buffer is bound, including all resource descriptors. */
>>         void (*invalidate_buffer)(struct pipe_context *ctx, struct
>> pipe_resource *buf);
>> diff --git a/src/gallium/drivers/radeon/r600_texture.c
>> b/src/gallium/drivers/radeon/r600_texture.c
>> index 4424ca3..d42d807 100644
>> --- a/src/gallium/drivers/radeon/r600_texture.c
>> +++ b/src/gallium/drivers/radeon/r600_texture.c
>> @@ -296,6 +296,31 @@ static void r600_texture_disable_cmask(struct
>> r600_common_screen *rscreen,
>>         r600_dirty_all_framebuffer_states(rscreen);
>>   }
>>
>> +static void r600_texture_disable_dcc(struct r600_common_screen *rscreen,
>> +                                    struct r600_texture *rtex)
>> +{
>> +       struct r600_common_context *rctx =
>> +               (struct r600_common_context *)rscreen->aux_context;
>> +
>> +       if (!rtex->dcc_offset)
>> +               return;
>> +
>> +       /* Decompress DCC. */
>> +       pipe_mutex_lock(rscreen->aux_context_lock);
>> +       rctx->decompress_dcc(&rctx->b, rtex);
>> +       rctx->b.flush(&rctx->b, NULL, 0);
>> +       pipe_mutex_unlock(rscreen->aux_context_lock);
>> +
>> +       /* Disable DCC. */
>> +       rtex->dcc_offset = 0;
>> +       rtex->cb_color_info &= ~VI_S_028C70_DCC_ENABLE(1);
>> +
>> +       /* Notify all contexts about the change. */
>> +       r600_dirty_all_framebuffer_states(rscreen);
>> +
>> +       /* TODO: re-set all sampler views and images, but how? */
>> +}
>> +
>>   static boolean r600_texture_get_handle(struct pipe_screen* screen,
>>                                        struct pipe_resource *resource,
>>                                        struct winsys_handle *whandle,
>> @@ -318,6 +343,13 @@ static boolean r600_texture_get_handle(struct
>> pipe_screen* screen,
>>                 res->external_usage = usage;
>>
>>                 if (resource->target != PIPE_BUFFER) {
>> +                       /* Since shader image stores don't support DCC on
>> VI,
>> +                        * disable it for external clients that want write
>> +                        * access.
>> +                        */
>> +                       if (usage & PIPE_HANDLE_USAGE_WRITE)
>> +                               r600_texture_disable_dcc(rscreen, rtex);
>
>
> Have you considered keeping DCC enabled when the user sets the explicit
> flush flag and having flush_resource decompress for writably-shared
> resources?

DCC decompression is a very costly operation and it's better to avoid
it if possible. Currently, DCC is only supported with non-displayable
surfaces, but all users of flush_resource (DRI2/3) only get
displayable surfaces. Thus, the driver doesn't have to worry about
flush_resource with DCC.

>
> Thinking about this brings up a more general question about the intended
> semantics of the explicit flush flag. If process A creates and exports a
> texture with explicit flush and process B imports it, is process B expected
> to explicitly flush the texture for *its* changes to the texture to become
> visible in program A?

If the process A state tracker calls the explicit flush, it should set
the flag in resource_get_handle. If the process B state tracker calls
the explicit flush, it should set the flag in resource_from_handle.
The explicit flush flag only describes the usage for the driver
receiving it.

>
> If I understand your current implementation correctly, changes in process B
> do *not* currently need explicit flush (because DCC is disabled and process
> B will never allocate a CMASK). The question is whether this is just a happy
> coincidence of the current implementation or whether it is actually
> something we want to promise for the future.

The happy coincidence is that the explicit flush is only done on
displayable surfaces (no DCC there).

Note that this series only makes necessary changes to support the
OpenCL interop, thus it's mainly focused on texture_get_handle. The
texture_from_handle function needs more changes to support imports
with DCC.

>
> I'd prefer the first interpretation, i.e. that we expect program B to also
> explicitly flush, because that keeps the door more open for sharing a
> writable texture with DCC enabled, but I'm not sure what the user-facing
> APIs say about this topic (or whether there is even a plan to combine
> explicit flush with write usage).

I'd like to keep the two flags separate. The explicit flush flag
describes how the gallium driver itself is going to be used, while the
read/write flags describe how the exported resource is going to be
used externally.

Marek