[Mesa-dev] Using the right context in st_texture_release_all_sampler_views()

[Brian Paul]

On 07/22/2015 05:31 PM, Jose Fonseca wrote:
> On 22/07/15 23:32, Brian Paul wrote:
>> Hi Marek,
>>
>> This is regarding your commit "st/mesa: use pipe_sampler_view_release
>> for releasing sampler views" from last October.
>>
>> Basically, we have:
>>
>> void
>> st_texture_release_all_sampler_views(struct st_context *st,
>>                                       struct st_texture_object *stObj)
>> {
>>     GLuint i;
>>
>>     /* XXX This should use sampler_views[i]->pipe, not st->pipe */
>>     for (i = 0; i < stObj->num_sampler_views; ++i)
>>        pipe_sampler_view_release(st->pipe, &stObj->sampler_views[i]);
>> }
>>
>> Our VMware/svga driver has an issue when
>> pipe_context::sampler_view_destroy() is called with one context and a
>> sampler view which was created with another context.  I can hack around
>> it in our driver code, but it would be nice to fix this in the state
>> tracker.
>>
>> Ideally, the above code should be something like:
>>
>>     for (i = 0; i < stObj->num_sampler_views; ++i)
>>        pipe_sampler_view_reference(&stObj->sampler_views[i], NULL);
>>
>> The current code which uses the st->pipe context came from the bug
>> https://bugs.freedesktop.org/show_bug.cgi?id=81680
>>
>> AFAICT, you were just working around an R600 driver issue.  Any chance
>> we could fix the state tracker and re-test Firefox on R600?
>
> Freeing a view from a different context with another context is wrong.
>
> But freeing a view with a context that might be current on another
> thread is also wrong, as pipe_context are not thread safe.
>
>
> If that was the previous behavior, then maybe Firefox crashed due to the
> race conditions.  Looking at the stack trace, I suspect that what
> happened was that one context was freeing the st_texture, and all its
> views, while the other context was destroying itself, and all its own
> views.  In short, st_texture_release_sampler_view was being called
> simultanouesly from two different threads.
>
>
> The proper fix IMO is to let the pipe context that owns the view to
> destroyed from whatever thread it is current (or whenever it is made
> current.)

OK, in our off-list discussion it wasn't clear to me that 
multi-threading was your main concern until your last message.  I see 
what you're saying now.

So, when we destroy a texture object which may be shared by multiple 
contexts, we theoretically need to move the texture's sampler views to 
new, per-context lists.  At some later time, when the context is used, 
we'd check if the list of sampler views to be destroyed was non-empty 
and free them.

Unfortunately, that involves one context/thread reaching into another 
context.  And that sounds messy.

The ultimate solution may be to get rid of the per-texture list of 
sampler views and instead store sampler views in a context-private data 
structure.  I'll have to think that through.

-Brian