Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"

From: Daniel Vetter <daniel@ffwll.ch>
To: Qiang Yu <yuq825@gmail.com>
Cc: Simon Shields <simon@lineageos.org>, Marek Vasut <marex@denx.de>,
	Connor Abbott <cwabbott0@gmail.com>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Andrei Paulau <7134956@gmail.com>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Vasily Khoruzhick <anarsoul@gmail.com>,
	Erico Nunes <nunes.erico@gmail.com>
Subject: Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers"
Date: Wed, 23 May 2018 22:31:02 +0200	[thread overview]
Message-ID: <CAKMK7uFW70KFkUHYWkWrB_WaE2ivnnPxcoS298vMGSA_ECkdaQ@mail.gmail.com> (raw)
In-Reply-To: <CAKGbVbvhJuQEND1zhsqrpm6GZUyZBiuaHTTL8==OyR_nP4kmyg@mail.gmail.com>

On Wed, May 23, 2018 at 2:59 PM, Qiang Yu <yuq825@gmail.com> wrote:
> On Wed, May 23, 2018 at 5:04 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Tue, May 22, 2018 at 09:04:17AM +0800, Qiang Yu wrote:
>>> On Tue, May 22, 2018 at 3:37 AM, Eric Anholt <eric@anholt.net> wrote:
>>> > Qiang Yu <yuq825@gmail.com> writes:
>>> >
>>> >> This reverts commit 45c3d213a400c952ab7119f394c5293bb6877e6b.
>>> >>
>>> >> lima driver need preclose to wait all task in the context
>>> >> created within closing file to finish before free all the
>>> >> buffer object. Otherwise pending tesk may fail and get
>>> >> noisy MMU fault message.
>>> >>
>>> >> Move this wait to each buffer object free function can
>>> >> achieve the same result but some buffer object is shared
>>> >> with other file context, but we only want to wait the
>>> >> closing file context's tasks. So the implementation is
>>> >> not that straight forword compared to the preclose one.
>>> >
>>> > You should just separate your MMU structures from drm_file, and have
>>> > drm_file and the jobs using it keep a reference on them.  This is what
>>> > I've done in V3D as well.
>>>
>>> It's not the VM/MMU struct that causes this problem, it's each buffer
>>> object that gets freed before task is done (postclose is after buffer free).
>>> If you mean I should keep reference of all buffers for tasks, that's not
>>> as simple as just waiting task done before free buffers.
>>
>> Why can't you do that waiting in the postclose hook? If it's the lack of
>> reference-counting in your driver for gem bo, then I'd say you need to
>> roll out some reference counting. Relying on the implicit reference
>> provided by the core is kinda not so great (which was the reason I've
>> thrown out the preclose hook). There's also per-bo open/close hooks.
>
> It's possible to not use preclose, but the implementation is not as simple
> and straight forward as the preclose I think. There're two method I can
> think of:
> 1. do wait when free buffers callback unmap buffer from this process's
>     lima VM (wait buffer reservation object), this is fine and simple, but
>     there's case that this buffer is shared between two processes, so the
>     best way should be only waiting fences from this process, so we'd
>     better do some record for fences for a "perfect waiting"
> 2. keep a reference of involved buffers for a task, unreference it when
>     task done, also keep a reference of the buffer mapping in this process's
>     lima VM (this is more complicated to implement)
>
> But if there's a preclose, just wait all this process's task done, then
> unmap/free buffers, it's simple and straight forward. I'd like to hear if
> there's other better way for only use postclose.

Refcount your buffers. Borrowing references from other places tends to
result in a maintenance headache with no end. So solution 2.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel