[Mesa-dev] [PATCH 1/6] st/mesa: fix sampler view context mismatch issue

[Brian Paul]

On 03/14/2019 12:24 AM, Mathias Fröhlich wrote:
> Hi Brian,
> 
> On Sunday, 10 March 2019 21:32:39 CET Brian Paul wrote:
>>> On Friday, 8 March 2019 23:52:09 CET Brian Paul wrote:
>>>> After a while of running google-chrome and viewing Bing maps, we'd see
>>>> "context mismatch in svga_sampler_view_destroy()" messages and
>>>> eventually crash because we leaked too many sampler views (the kernel
>>>> module would have too many sampler views).
>>>>
>>>> When a texture object is being deleted, we call
>>>> st_texture_release_all_sampler_views() to release all the sampler
>>>> views.  In the list, there may sampler views which were created from
>>>> other contexts.
>>>>
>>>> Previously, we called pipe_sampler_view_release(pipe, view) which would
>>>> use the given pipe context to destroy the view if the refcount hit
>>>> zero.  The svga error was triggered because we were calling
>>>> pipe->sampler_view_destroy(pipe, view) and the pipe context didn't
>>>> match the view's parent context.
>>>>
>>>> Instead, call pipe_sampler_reference(&view, NULL).  That way, if
>>>> the refcount hits zero, we'll use the view's parent context to
>>>> destroy the view.  That's what we want.
>>>
>>> That sounds plausible so far.
>>>
>>> What I wonder, is calling into an 'arbitrary' different context and do
>>> work there
>>> thread safe for any gallium driver?
>>>
>>
>> Right.  Before this patch there was the problem of trying to destroy a
>> sampler view with a context different from the one which created it.
>> That's fixed, but now there's the multi-threading issue as you say, and
>> which I believe, is a less-common situation.
> 
> Hmm, you are saying we tolerate a race condition then?

No, I'm planning to fix that too, as described below.


> 
>>> Especially since pipe_sampler_view_reference in its documentation says
>>> that the
>>> views provided need to originate from the same context and that this must
>>> be the 'current'.
>>>
>>> I have been taking a quick look into iris and radeonsi and both seem to be
>>> safe
>>> in terms of threads for dereferencing the views finally.
>>> But either the documentation of pipe_sampler_view_reference should be
>>> updated
>>> or I may miss an other piece of the puzzle to see that it is still save to
>>> do so.
>>>
>>
>> Last week I posted a patch series for the VMware driver which handled the
>> thread issue with a list of "zombie" sampler views.  Jose suggested moving
>> that up into the state tracker, and that is a better long-term solution.
>>
>> I'm almost done with a patch series that does that.  I'll likely post it
>> tomorrow.

This is taking longer than expected (long testing, plus a sick child and 
wife at home).


> 
> Ok, so basically you are saying that you improve the current situation with this
> current series, still leaving a race condition open. But a good final fix to the race
> condition is underway.

Yeah, I can probably stage the multi-thread/race patch series before 
this context-fix-up series.


> 
> Anyhow, to me it looks like all the reference counting inlines in gallium seem
> to assume being truely thread safe and do not assume that the context that
> created the view needs to be current on dereferencing.
> Means: is this comment at the pipe_sampler_view_*reference that states
> that the owning context needs to be current really correct?
> Such a comment implies to me that the context possibly has a non thread safe list
> of views that may get corrupted if being called from an other current context may
> be current in an other thread.
> ... well or something similar to that.
> Do you remember where this comment comes from?
> Is there a driver that requires the owning context needs to be current property?

I'm not sure I grok all that.  But since pipe_sampler_views are 
per-context objects, I believe they should only be 
created/destroyed/used/referenced from the same context.

-Brian