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

[Jose Fonseca]

On 23/07/15 01:00, Brian Paul wrote:
> 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.

Yes.  We could have a sepeare mutex for this list.

Still, it's hard to prevent dead-locks (e.g, two contexts trying to add 
a deferred view to each-others defferred list.)

> 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.

Yes, that's another possibility. I haven't thought it through either.

One way or another, you'll still want to notify a context to "garbagge 
collect" its views, even if it's just an atomic flag on each context, 
otherwise a texture that was shared by two contexts might end up 
lingering around indefinitely because contexts are holding to its views.


Jose