From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [PATCH] drm/i915: remove user GTT mappings early during runtime suspend Date: Tue, 06 May 2014 17:46:01 +0300 Message-ID: <1399387561.19761.31.camel@intelbox> References: <1399375730-4355-1-git-send-email-imre.deak@intel.com> <20140506114011.GB7322@nuc-i3427.alporthouse.com> <1399376546.19761.1.camel@intelbox> <20140506115931.GC7322@nuc-i3427.alporthouse.com> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1655048034==" Return-path: Received: from mga03.intel.com (mga03.intel.com [143.182.124.21]) by gabe.freedesktop.org (Postfix) with ESMTP id 4677C6EB99 for ; Tue, 6 May 2014 07:46:04 -0700 (PDT) In-Reply-To: <20140506115931.GC7322@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Chris Wilson Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org --===============1655048034== Content-Type: multipart/signed; micalg="pgp-sha1"; protocol="application/pgp-signature"; boundary="=-Ysm9dtPxLit8TwbLdb2j" --=-Ysm9dtPxLit8TwbLdb2j Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, 2014-05-06 at 12:59 +0100, Chris Wilson wrote: > On Tue, May 06, 2014 at 02:42:26PM +0300, Imre Deak wrote: > > On Tue, 2014-05-06 at 12:40 +0100, Chris Wilson wrote: > > > On Tue, May 06, 2014 at 02:28:50PM +0300, Imre Deak wrote: > > > > Currently user space can access GEM buffers mapped to GTT through > > > > existing mappings concurrently while the platform specific suspend > > > > handlers are running. Since these handlers may change the HW state= in a > > > > way that would break such accesses, remove the mappings before call= ing > > > > the handlers. > > >=20 > > > Hmm, but you never locked the device, so what is preventing those > > > concurrent accesses from refaulting in GTT entires anyway. Please exp= lain > > > the context under which the runtime suspend code executes, and leave > > > that explanation within easy reach of intel_runtime_suspend() - > > > preferrably with testing of those assumptions. > >=20 > > During faulting we take an RPM reference, so that avoids concurrent > > re-faults. I could add a comment about this to the code. >=20 > You are still iterating over lists that are not safe, right? Or are > there more magic rpm references that prevent ioctls whilst > intel_runtime_suspend is being called? Tbh I haven't checked this, since moving the unmapping earlier doesn't make a difference in this regard. But it's a good point, I tried to audit now those paths. Currently the assumption is that we hold an RPM reference everywhere where those lists are changed. On the exec buffer path this is true, but for example in i915_drop_caches_set() it's not. We could fix this by taking struct_mutex around i915_gem_release_all_mmaps() in intel_runtime_suspend(). Doing that needs some more auditing to make sure we can't deadlock somehow. At first glance it seems that the driver always schedules a deferred work and calls intel_runtime_suspend() from that, so I think it's fine. I suggest applying this patch regardless since the two issues are separate. --Imre --=-Ysm9dtPxLit8TwbLdb2j Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part Content-Transfer-Encoding: 7bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) iQEcBAABAgAGBQJTaPWpAAoJEORIIAnNuWDFlYUH/2+4Lbl7PeC7PaGkP8QpUx24 pScQfYGqKfKClrFiEANqTmGJ7XEKb2WlMsjcycfjBSQhAyC3RnAGomxGT//DyTwF shYBimVIeXiN1Iu4dIBtpxF4sRHiHCwT8K3d/KFiACHqgblrp4jzFcZvPa4d+isD nGPy4ykI3/zCoTLDKfYOCarRgI1YWFK/uFWdEpSnNbk10Fz5N7lhlVkx8JZ1XFn4 Ts8ea+1Eqt7oVH+iiO5hqiY+t7e1XvtbsUusqyyi0dbCkZVZys8b7ajN+yfAbzuJ TZOJHWf9/6kslIb7bFlLTGx9T+g+8uvgiDlOoZxLVMCORE49P46SWvRHAd5Qcig= =YX9B -----END PGP SIGNATURE----- --=-Ysm9dtPxLit8TwbLdb2j-- --===============1655048034== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1655048034==--