[Mesa-dev] [PATCH v4] etnaviv: fix resource usage tracking across different pipe_context's

[Lucas Stach]

Am Montag, den 14.01.2019, 15:20 +0100 schrieb Marek Vasut:
> On 1/14/19 3:02 PM, Lucas Stach wrote:
> > Am Montag, den 14.01.2019, 14:54 +0100 schrieb Marek Vasut:
> > > On 1/14/19 12:16 PM, Lucas Stach wrote:
> > > > Hi Marek,
> > > > 
> > > > Am Samstag, den 12.01.2019, 22:22 +0100 schrieb Marek Vasut:
> > > > > > From: Christian Gmeiner <christian.gmeiner at gmail.com>
> > > > > 
> > > > > A pipe_resource can be shared by all the pipe_context's hanging off the
> > > > > same pipe_screen.
> > > > > 
> > > > > > > > > > Signed-off-by: Christian Gmeiner <christian.gmeiner at gmail.com>
> > > > > > 
> > > > > > Signed-off-by: Marek Vasut <marex at denx.de>
> > > > > 
> > > > > To: mesa-dev at lists.freedesktop.org
> > > > > Cc: etnaviv at lists.freedesktop.org
> > > > > ---
> > > > > Changes from v1 -> v2:
> > > > >  - to remove the resource from the used_resources set when it is destroyed
> > > > > Changes from v2 -> v3:
> > > > >  - add locking with mtx_*() to resource and screen (Marek)
> > > > > Changes from v3 -> v4:
> > > > >  - drop rsc->lock, just use screen->lock for the entire serialization (Marek)
> > > > >  - simplify etna_resource_used() flush condition, which also prevents
> > > > >    potentially flushing resources twice (Marek)
> > > > >  - don't remove resouces from screen->used_resources in
> > > > >    etna_cmd_stream_reset_notify(), they may still be used in other
> > > > >    contexts and may need flushing there later on (Marek)
> > > > 
> > > > The patch mostly makes sense to me now, but don't we need to take the
> > > > screen->lock on all call sites where we do a ctx->flush? Otherwise we
> > > > may enter etna_cmd_stream_reset_notify unlocked, changing the
> > > > used_resources set while other threads might use it at the same time,
> > > > right?
> > > 
> > > etna_cmd_stream_reset_notify() takes the lock when accessing the
> > > used_resources set , see below.
> > 
> > Uh, sorry seems I mixed this up. But then I don't see how this isn't
> > deadlocking, as AFAICS mtx_lock isn't recursive.
> > 
> > In etna_resource_used() you already lock the screen mutex, then when
> > you find a context that needs flushing you call the context flush,
> > which flushes the cmd stream and calls into
> > etna_cmd_stream_reset_notify() where the mutex is locked again -> self
> > deadlock.
> 
> [...]
> 
> > > > > +   mtx_init(&screen->lock, mtx_recursive);
> 
> [...]
> 
> But it is recursive ...

Thanks, going back to ridicule myself somewhere myself somewhere else
now.

Regards,
Lucas