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

[Mathias Fröhlich]

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?

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

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.

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?

best

Mathias