From mboxrd@z Thu Jan 1 00:00:00 1970 From: Eric Anholt Subject: Re: [PATCH RFC 05/24] Revert "drm: Nerf the preclose callback for modern drivers" Date: Wed, 30 May 2018 11:13:14 -0700 Message-ID: <87zi0hhv0l.fsf@anholt.net> References: <20180519065243.27600-1-yuq825@gmail.com> <20180519065243.27600-6-yuq825@gmail.com> <874lj0u7fn.fsf@anholt.net> <20180523090439.GS3438@phenom.ffwll.local> <20180524075134.GA3438@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0184242176==" Return-path: Received: from anholt.net (anholt.net [50.246.234.109]) by gabe.freedesktop.org (Postfix) with ESMTP id 720F989444 for ; Wed, 30 May 2018 18:13:18 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Qiang Yu , Daniel Vetter Cc: Simon Shields , Marek Vasut , Connor Abbott , Neil Armstrong , Andrei Paulau <7134956@gmail.com>, dri-devel , Vasily Khoruzhick , Erico Nunes List-Id: dri-devel@lists.freedesktop.org --===============0184242176== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha512; protocol="application/pgp-signature" --=-=-= Content-Type: text/plain Qiang Yu writes: > On Thu, May 24, 2018 at 3:51 PM, Daniel Vetter wrote: >> On Thu, May 24, 2018 at 09:18:04AM +0800, Qiang Yu wrote: >>> On Thu, May 24, 2018 at 4:31 AM, Daniel Vetter wrote: >>> > On Wed, May 23, 2018 at 2:59 PM, Qiang Yu wrote: >>> >> On Wed, May 23, 2018 at 5:04 PM, Daniel Vetter 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 wrote: >>> >>>> > Qiang Yu 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. >>> >>> In current lima implementation, refcount involved buffer for task is done >>> in user space. So kernel's task object don't keep that. User space >>> driver is responsible not unmap/free buffer before task is complete. This >>> works simple and fine except the case that user press Ctrl+C to terminate >>> the application which will force to close drm fd. I really don't think adding >>> buffer refcount for tasks in kernel just for this case is valuable because >>> it has no benefits for normal case but some extra load. >> >> If kernel correctness relies on refcounting you have a giantic security >> problem. You need to fix that. Kernel _must_ assume that userspace is >> evil, trying to pull it over the table. > > 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? You really do need to refcount the buffers used in a rendering job so they don't get freed early. --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEE/JuuFDWp9/ZkuCBXtdYpNtH8nugFAlsO6boACgkQtdYpNtH8 nui+Dg//WZKVIdALUCPz+MSTJGmuiP992821myb8F5z+4WnoojnKf7E5j/Cj5qhH djSjUYZ4iVVPU3+JsGZpvp1Cd+UXZ3a6VFHk1eaLraHNX7TFXmug9Qe3iYUafugR vkJmUykzOv/kqOAKEnGKKV/89ChnlrjkhZw9IodNf39lzrWl+GNDJh61XWTdO6QA kQIw+5O/Mw8KNVLBOadrn7PVWvPAGprd6S50ZCrNoNBog5mTGDj1AwzDkRi9j5Dj qxDEyEKSMndo0feo0q4T83bH7GgznY3hjxXLxlvAq9V4YrvB+YOnngd2u41/MVj8 CkMCKFAPYGZL5b0eLi0V7/oqZrgG8V+qga9m16KSPqOGPDYVi8cZQtok0qJJM/55 iXQ8k/BKJ2eH8oz8ldZv4hrHsMdhAo//s8mI87Z384nloHCamRRioPkhcs8i9qkK k1JO6a7XogswJmFe8pHARltkUMLQN3O7cDsT9PXuA3um98EK3tUdd+B0jSdr0Ld+ 5h0in6YZ63vvlDRtPzL6hYZek8xnISwEaN0opOZ4UUtHfyd7qeMnaCdkZX/9gZ7q L/ZL17m+oxkC2qlOxkaVJsVJZPnqbCv7LtBGx3Nlxzbz4yI2O7hacNAJdoTlP65f 7ma+nuO7+dfhJqYpUBmHAeGYYkh/k12LDcLDUkfPK4rs72CvgkM= =i8bp -----END PGP SIGNATURE----- --=-=-=-- --===============0184242176== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== --===============0184242176==--