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

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

-- 
Best regards,
Marek Vasut