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

From: Qiang Yu <yuq825@gmail.com>
To: Eric Anholt <eric@anholt.net>
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: Fri, 1 Jun 2018 09:58:03 +0800	[thread overview]
Message-ID: <CAKGbVbsc9QbUnHHomP8ywuTa=RCU9MF7R_7EMHv7eYgUcuZxcA@mail.gmail.com> (raw)
In-Reply-To: <87po1br9wu.fsf@anholt.net>

On Fri, Jun 1, 2018 at 1:51 AM, Eric Anholt <eric@anholt.net> wrote:
> Qiang Yu <yuq825@gmail.com> writes:
>
>>>>
>>>> It is OK if evil user free/unmap the buffer when task is not done
>>>> in my implementation. It will generate a MMU fault in that case and kernel
>>>> driver will do recovery.
>>>>
>>>> So does the Ctrl+C case, if don't deal with it, just get some noisy MMU
>>>> fault warning and a HW reset recovery.
>>>
>>> How about an app rendering to shared buffers, which glFlush()es and
>>> exits cleanly but doesn't close the DRI screen?  What would cause that
>>> app's rendering to get completed succesfully instead of faulting to
>>> death?
>> Do you mean the same case as Ctrl+C when an app exit without waiting
>> all task finished in userspace?
>
> Basically the same, but I'm saying that the app is doing everything
> right and terminating successfully, rather than being interrupted (which
> you might otherwise use to justify its rendering failing)
I won't justify Ctrl+C. In fact I think it's also a good case which should
not get a MMU fault and GPU reset because it happens even user sapce
driver is right. I only think when user driver is not right deserve a MMU
fault like bug/evil code which free/unmap bo before task done. I think
this is also the difference of user free bo and drm close free bo in my case.

>
>>> You really do need to refcount the buffers used in a rendering job so
>>> they don't get freed early.
>> Do you mean refcount the buffers in kernel space job? This is OK but
>> not necessary, I can wait task complete in gem_close_object which
>> will be called by drm_release for each buffer too (I still think better
>> waiting in preclose at once but it's gone).
>
> Just wait for all tasks to complete when any object is freed?  That's
> going to be bad for performance.
In my case this doesn't affect performance. My implementation is user
space driver will record task buffers and free them when task done.
So in normal usage when the buffer is freed, there should be no task
from this process using it. Wait should only happen when Ctrl+C and
not close screen case.

> Or are you saying that you already
> have the connection between the task and its objects (and, if so, why
> aren't you just doing refcounting correctly through that path?)
This is by the reservation object of the buffer, I can wait on the fences
in the reservation object for task done when buffer free.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel