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

From: Qiang Yu <yuq825@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
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 20:59:43 +0800	[thread overview]
Message-ID: <CAKGbVbvhJuQEND1zhsqrpm6GZUyZBiuaHTTL8==OyR_nP4kmyg@mail.gmail.com> (raw)
In-Reply-To: <20180523090439.GS3438@phenom.ffwll.local>

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.

Regards,
Qiang
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel