From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock Date: Wed, 04 Jan 2012 18:22:41 -0800 Message-ID: <86fwfulwge.fsf@sumi.keithp.com> References: <1323867460-5486-1-git-send-email-daniel.vetter@ffwll.ch> <1323867460-5486-6-git-send-email-daniel.vetter@ffwll.ch> <86zke4mxg2.fsf@sumi.keithp.com> <86wr98mqw3.fsf@sumi.keithp.com> <86obukmkdq.fsf@sumi.keithp.com> <86zke3l5fj.fsf@sumi.keithp.com> <20120104181257.GI8004@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0754966683==" Return-path: Received: from keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 50EB29E9AF for ; Wed, 4 Jan 2012 18:22:45 -0800 (PST) In-Reply-To: <20120104181257.GI8004@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Daniel Vetter , intel-gfx List-Id: intel-gfx@lists.freedesktop.org --===============0754966683== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Transfer-Encoding: quoted-printable On Wed, 4 Jan 2012 19:12:57 +0100, Daniel Vetter wrote: > The "Correct?" was just to check my understanding of your concern, I still > think its invalid. You've cut away the second part of my mail where I > explain why and I don't see you responding to that here. Also > micro-optimizing the gpu reset code sounds a bit strange. Sorry, I didn't explain things very well. Right now, our register access looks like: get(struct_mutex); if (++forcewake_count =3D=3D 1) force_wake_get() value =3D read32(reg) or write32(reg, val) if (--forcewake_count =3D=3D 0) force_wake_put(); /* more register accesses may follow ... */ put(struct_mutex); All very sensible, the whole register sequence is covered by struct_mutex, which ensures that the forcewake is set across the register access. The patch does: get(spin_lock) if (++forcewake_count =3D=3D 1) force_wake_get() put(spin_lock) value =3D read32(reg) or write32(reg, val) get(spin_lock) if (--forcewake_count =3D=3D 0) force_wake_put() put(spin_lock) I realize that all current users hold the struct_mutex across this whole sequence, aside from the reset path, but we're removing that requirement explicitly (the patch removes the WARN_ON calls). Without a lock held across the whole sequence, it's easy to see how a race could occur. We're also dropping and re-acquiring a spinlock with a single instruction between, which seems wasteful. Instead, we should be doing: get(spin_lock) force_wake_get(); value =3D read32(reg) or write32(reg,val) force_wake_put(); put(spin_lock); =20=20=20=20=20=20=20=20 No need here to deal with the wake lock at reset time; the whole operation is atomic wrt interrupts. It's more efficient *and* correct, without depending on the old struct_mutex locking. If you want to continue to expose the user-mode wake lock stuff, you could add: get(spin_lock); if (!forcewake_count) force_wake_get(); value =3D read32(reg) or write32(reg,val) if (!forcewake_count) force_wake_put(); put(spin_lock); This would require that the reset code also deal with the forcewake_count to restore the user-mode force wake. A further optimization would hold the force_wake active for 'a while' to avoid the extra bus cycles required, but we'd want to see a performance problem from this before doing that. =2D-=20 keith.packard@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIVAwUBTwUJcTYtFsjWk68qAQhe6w/8DvLnjewn2l/eiyo6N9gaDuThNn1iGxlI bI09OmZ1bSNk9XclAbJsACMpeOc7WSXBYmXDoFou5r4zmMVl8SKJJLaMk1hkfygQ 6y79aaKIPzYdYBbg5yUuI4XusABRChSxK9WAnTNdWWaJWaZWbEzNTvBHShWPlaUr YndiN8gZfmSEuB0J9xihVCIrfC2+wdP5+aKOdJ7bQSjUCZNdDOK+d3XdJFhXMkuO 3PZMQZTPdClhxk/3WXYZqrakbsy6/CjrYir1DnDmC/CpNLBf/4remBzOPdbHyZz4 sSLD24mf9c9iD5aGZEpDat58K8KnrK/k2qDDltsvtm+oQJ+lDZyg7n93prTUmHhM 0ZJw9uF+BVqzW2hSwV9ELKVU64mTF+ykWT3OCZ9LQbxt8+VP8oy6t1+qiq/gWLvz 4cmoUrRCAnIBOBgHCCFsHOIH3T3kQqiuRpDUe6iv1Isv7DlBjAgIeOGg5/NLXGqJ IZaEO12ROmlTcRDUU/STdfZUArgGcfHDhdzqV+fmI3DVBy+nnxOtuwuWN8ocvS4m Ll6DrthyaJs2Pe7Utk3wgZGMUoA4SLTujXqDc8oLjXIHNwKysdu6Fx30oSLwcC5k TIyB9Ml2TZIIKNUT5+xNLz5fiBODmA9GNFyHMmfygPisztHcvM/3lENioVwZZ61I t60pETG3Zk0= =XpjI -----END PGP SIGNATURE----- --=-=-=-- --===============0754966683== 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 --===============0754966683==--