From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keith Packard Subject: Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo Date: Fri, 13 Jan 2012 16:11:43 -0800 Message-ID: <86k44v3zy8.fsf@sumi.keithp.com> References: <1325702445-2231-1-git-send-email-daniel.vetter@ffwll.ch> <86lipb367b.fsf@sumi.keithp.com> <20120113235231.GE3843@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============2130392717==" Return-path: Received: from keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id 6AF3F9EF70 for ; Fri, 13 Jan 2012 16:11:48 -0800 (PST) In-Reply-To: <20120113235231.GE3843@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 , Eugeni Dodonov , stable@kernel.org List-Id: intel-gfx@lists.freedesktop.org --===============2130392717== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Transfer-Encoding: quoted-printable On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter wrote: > - I think we need to amend the commit msg of the voodoo patch with the > piece of doc I've discovered. If you want I can send out a v3 with that. Sounds good. > - I think the HWSTAM revert is material for -next I'd be OK with pushing all of the SNB changes to -next and leaving the patches only affecting IVB, where we know it is necessary. Note that this branch wasn't written with an eye to merging to -fixes, but rather as what we want the code to look like eventually. As such, we need to identify separately 1) What needs to go into -fixes for 3.3. 2) What needs to go into -next for 3.4. > - Can you post your 3 patches on intel-gfx, I think I can shoot at them a > bit ;-) Sure; it's all a work in progress. > acc101d drm/i915: Hold gt_lock across forcewake register reads >=20 > Imo this is a simple cleanup (reading forcewake-protected registers isn't > really a fast-path for us), so material for -next. The 'optimization' is just a side benefit. The fix is to prevent reads From=20happening without forcewake being set. > 0f0e134 drm/i915: Hold gt_lock during reset >=20 > I still don't see what race you're trying to protect here, after all the > gpu just died, things are confusing anyway (and anyone accessing the gpu > in such a state should take that into account). Currently that's no one > afaics. So imo at most -next material. These two patches work together to ensure that no-one reads from the GPU without forcewake being set correctly, even across reset. Mostly, I changed the code to make it obvious that the whole read operation was now atomic; before, I had to read a lot of code to convince myself that the read couldn't happen without forcewake being set, except under the reset condition. Forcing me to read code closely to prove it correct doesn't make me happy. Having a spinlock held over the entire section makes the whole thing obviously correct to even casual inspection. So, I'd like these to go into -next so that when I read this code next year, I won't have to figure all of this out again. > 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset >=20 > Again this is imo just a cleanup. Furthermore the commit msg is lying a > bit because it fails to mention the fix to use the forcewake function > pointer. So the cleanup is imo for -next and the bugfix is really old, > see: Yes, I didn't even notice that I'd fixed that bug when I moved the code... The bugfix is, however required, and needs to be in -fixes. So, I think for -fixes we get: 1) The forcewake spinlock patch. 2) The bugfix to the reset path. 3) forcewake while waiting for interrupts for IVB only, not for SNB, with updated commit message. =20 This minimizes the potential for regressions in SNB (by not affecting it at all) while fixing the IVB issues. For -next, we should have 1) forcewake for interrupts on IVB and SNB 2) Removal of the HWSTAM hacks 3) The spinlock cleanups that make me happy I'd love to hear back from some SNB owners that the forcewake IRQ issue resolves problems and doesn't cause any regressions. If so, we can reconsider it for 3.3. If it doesn't fix anything, then I don't think it should go into 3.3. =2D-=20 keith.packard@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIVAwUBTxDIQDYtFsjWk68qAQjiLA//dw0tEuJjj3dLoilPipLkoGXjU+nNa7JS mYVDg44FgG/wUomSrwrVwuygj6i3n2Z5o/jwPJr0vYcbrOX0Zvs3yM+NZ5DhgLon Py01bbk0FTjOGt8Sext/tkJav0UikI7ILndOu6IF3+Zsw3YQ8CDlu/Sw7yJcjvHF brCHsMCoUvtZyzVKsnHKLPZ35e+/aRggIWXj2N71aj3g+kFztzPqHsya/Mjjrt9n DK61Aa7BEZobVEwNCAw0afuq2T2mioLBjJexlFqORZfkqHhj06O+wDOliRVTH8ZZ m3XZmv1lFmFuDl177jIyPmrRVocvhV50RiPR/SkEHD+BXaS800MneFo12ni1RmTr 613hmWrpcHhuktas3SDqWq3zuOkgbtRbsphu2XIGsN24ETbXti/RtfxhZc0e4owb nP2EC1TFNJw8Bn0Lri9Y2JF/r8QmXQK0YtgfmpmOHCMk3NLI50Mm6RpVUAA1dHgz qWDKmi/IbBV8xXhEzNohNtD/TiFL5jgWuPtpp+UOocHwi7SB0QBd38ycfDfjmP9I 2CX3qfAqCjJ9Mpm8HF6YdTHSg9ohWLbUWuShL3aOtoJg2Q/MaZS9vhoaeTp8+uMy 9W7ipfp5AGBg4MLBGtJHxMv0uuvf2/94tw52d8hmqm0bwr0zdPRyOO4PR40tfQJw 2W3DIHwjxA8= =1/Rm -----END PGP SIGNATURE----- --=-=-=-- --===============2130392717== 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 --===============2130392717==--