[Mesa-dev] [PATCH 17/26] gallium/radeon: disable CMASK on handle export if sharing doesn't allow it

[Nicolai Hähnle]

On 09.03.2016 05:56, Marek Olšák wrote:
> On Wed, Mar 9, 2016 at 7:18 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
>> On 08.03.2016 14:35, Marek Olšák wrote:
>>>
>>> On Tue, Mar 8, 2016 at 4:41 AM, Michel Dänzer <michel at daenzer.net> wrote:
>>>>
>>>> On 03.03.2016 01:36, Marek Olšák wrote:
>>>>>
>>>>> From: Marek Olšák <marek.olsak at amd.com>
>>>>>
>>>>> The disabling of CMASK is simple, but notifying all contexts about it is
>>>>> not:
>>>>> - The screen must have a list of all contexts.
>>>>> - Each context must have a monotonic counter that is incremented only
>>>>> when
>>>>>     the screen wants to re-emit framebuffer states.
>>>>> - Each context must check in draw_vbo if the counter has been changed
>>>>> and
>>>>>     re-emit the framebuffer state accordingly.
>>>>
>>>>
>>>> The list seems a bit overkill. How about having dirty_fb_counter in the
>>>> screen and last_dirty_fb_counter in the context, incrementing the former
>>>> in r600_dirty_all_framebuffer_states and emitting the framebuffer state
>>>> if the two counters don't match?
>>>
>>>
>>> Thanks. The updated patch is attached. Please review.
>>
>>
>> There is an unneeded empty line in this hunk:
>>
>> @@ -260,6 +265,31 @@ static void r600_eliminate_fast_color_clear(struct
>> r600_common_screen *rscreen,
>>          pipe_mutex_unlock(rscreen->aux_context_lock);
>>   }
>>
>> +static void r600_texture_disable_cmask(struct r600_common_screen *rscreen,
>> +                                      struct r600_texture *rtex)
>> +{
>> +
>> +       if (!rtex->cmask.size)
>> +               return;
>>
>> Slightly further down, I believe the pipe_resource_reference should be
>> unconditional:
>>
>> +       if (rtex->cmask_buffer != &rtex->resource)
>> +           pipe_resource_reference((struct
>> pipe_resource**)&rtex->cmask_buffer, NULL);
>
> It's how cmask_buffer is unreferenced everywhere. If the texture had
> multiple samples, cmask_buffer would be part of the resource and so
> the code would release the resource, which would be undesirable.

Ah, in that case cmask_buffer doesn't actually own a reference count - I 
see. That's a bit surprising, but orthogonal to this patch, so go ahead 
and add my R-b with only Michel's and my other comment.

Cheers,
Nicolai

> This
> is not a possible scenario yet, because texture_get_handle doesn't
> support MSAA surfaces, but it matches the unref code elsewhere.
>
> Marek
>