[PATCH] dma-buf/fence-array: fix deadlock in fence-array

[Rob Clark]

On Tue, Oct 18, 2016 at 3:41 AM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Mon, Oct 17, 2016 at 05:44:48PM -0200, Gustavo Padovan wrote:
>> 2016-10-17 Chris Wilson <chris at chris-wilson.co.uk>:
>>
>> > On Mon, Oct 17, 2016 at 02:59:52PM -0400, Rob Clark wrote:
>> > > On Mon, Oct 17, 2016 at 2:52 PM, Gustavo Padovan <gustavo at padovan.org> wrote:
>> > > > 2016-10-17 Rob Clark <robdclark at gmail.com>:
>> > > >
>> > > >> Currently with fence-array, we have a potential deadlock situation.  If we
>> > > >> fence_add_callback() on an array-fence, the array-fence's lock is acquired
>> > > >> first, and in it's ->enable_signaling() callback, it will install cb's on
>> > > >> it's array-member fences, so the array-member's lock is acquired second.
>> > > >>
>> > > >> But in the signal path, the array-member's lock is acquired first, and the
>> > > >> array-fence's lock acquired second.
>> > > >>
>> > > >> To solve that, always enabling signaling up-front (in the fence_array
>> > > >> constructor) without the fence_array's lock held.
>> > > >
>> > > > Do we always want to enable signaling for arrays? One of the things we
>> > > > removed from the Sync Framework was the need to enable signalling at
>> > > > creation time.
>> > > >
>> > > > Just merging fencing doesn't mean you want signaling, that is supposed
>> > > > to happen only when poll() is called on the sync file.
>> > >
>> > > It was something Maarten suggested, as an alternative to introducing a
>> > > wq into the mix or worse hacks..
>> > > https://lists.freedesktop.org/archives/dri-devel/2016-October/120868.html
>> > >
>> > > I think I agree with him that it is an optimization that is unlikely
>> > > to be useful in the case of fence-arrays.  If you need to wait on
>> > > multiple fences from different timelines, you probably aren't doing
>> > > that in hw.
>> >
>> > For 2 i915 fences, I definitely do not want signaling enabled at
>> > creation time.
>>
>> Should we add arg flags for fence_array_create()? We already have
>> signal_on_any flag there. We can convert that arg to a bitfield.
>
> The thing creating the array might not be aware of the fences contained
> therein, e.g. SYNC_FILE_MERGE. Imo we really need to keep the lazy
> signalling enabling properties of fences.

Well, in all my cases, if you end up with a fence-array, it means you
are waiting on cpu, so enabling signalling now vs later doesn't
matter.

But Chris mentioned on IRC that he does actually have a case where he
can wait in hw on fences from multiple contexts (ie. meaning MERGE
would create a fence-array).. so in that case it does still make sense
to defer enable-signalling.  So I guess back to the wq approach..

BR,
-R

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch