[Mesa-dev] [PATCH 3/6] radeonsi: clear PIPE_IMAGE_ACCESS_WRITE when it's invalid to be on the safe side

[Marek Olšák]

On Wed, Nov 29, 2017 at 10:59 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 28.11.2017 22:17, Marek Olšák wrote:
>>
>> From: Marek Olšák <marek.olsak at amd.com>
>>
>> ---
>>   src/gallium/drivers/radeonsi/si_descriptors.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/src/gallium/drivers/radeonsi/si_descriptors.c
>> b/src/gallium/drivers/radeonsi/si_descriptors.c
>> index 69371ea..471c93a 100644
>> --- a/src/gallium/drivers/radeonsi/si_descriptors.c
>> +++ b/src/gallium/drivers/radeonsi/si_descriptors.c
>> @@ -681,20 +681,28 @@ static void si_set_shader_image_desc(struct
>> si_context *ctx,
>>                                           view->format,
>>                                           view->u.buf.offset,
>>                                           view->u.buf.size, desc);
>>                 si_set_buf_desc_address(res, view->u.buf.offset, desc +
>> 4);
>>         } else {
>>                 static const unsigned char swizzle[4] = { 0, 1, 2, 3 };
>>                 struct r600_texture *tex = (struct r600_texture *)res;
>>                 unsigned level = view->u.tex.level;
>>                 unsigned width, height, depth, hw_level;
>>                 bool uses_dcc = vi_dcc_enabled(tex, level);
>> +               unsigned access = view->access;
>> +
>> +               /* Clear the write flag when writes can't occur.
>> +                * Note that DCC_DECOMPRESS for MSAA doesn't work in some
>> cases,
>> +                * so we don't wanna trigger it.
>> +                */
>> +               if (tex->is_depth || tex->resource.b.b.nr_samples >= 2)
>> +                       access &= ~PIPE_IMAGE_ACCESS_WRITE;
>
>
> Shouldn't this rather be an assert()? Just removing the bit here won't stop
> the application from attempting to write to it from a shader anyway.
>
> We shouldn't be hitting that assert anyway, since we don't currently support
> MSAA images.

Better safe than sorry. I think it's better to recover from a bad
scenario than to fail an assertion, considering that assertions aren't
even enabled in release builds. I'll add an assertion into the
conditional block.

Marek