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: Tue, 03 Jan 2012 13:13:00 -0800 Message-ID: <86wr98mqw3.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> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1865767224==" Return-path: Received: from keithp.com (home.keithp.com [63.227.221.253]) by gabe.freedesktop.org (Postfix) with ESMTP id B1877A024E for ; Tue, 3 Jan 2012 13:11:24 -0800 (PST) In-Reply-To: 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: intel-gfx List-Id: intel-gfx@lists.freedesktop.org --===============1865767224== Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" --=-=-= Content-Transfer-Encoding: quoted-printable On Tue, 3 Jan 2012 20:12:35 +0100, Daniel Vetter w= rote: > I'm a bit confused by this. With the current code forcewake is > protected by dev->struct_mutex. Which doesn't work out because we need > to access registers that require the forcewake dance from non-process > context. Right, I like adding a spinlock around this to allow it to be called without needing to be able to lock the struct_mutex. (I remember suggesting that a spinlock would be necessary when the force wake code first showed up...) However, the commit message talks about the error capture and hang check code, but doesn't appear to change them at all. I think all this patch does is replace the locking for forcewake_count From=20struct_mutex to a new irq-safe spinlock, the commit message makes it sound like it's actually fixing stuff, which it isn't; it just enables fixing stuff in future patches, right? Reading through this a bit more, I think your patch opens up a hole in i915_reset. i915_reset takes struct_mutex, then resets the chip and restores the forcewake status. If we aren't relying on struct_mutex to protect the forcewake bits, then there's nothing preventing a thread From=20accessing the registers with the chip sleeping between the reset and the force wake reset. > Afaik the atomic ops stuff is just ducttape for paranoia reasons. The atomic ops stuff would allow reading of the value without holding struct_mutex, if that were actually useful. =2D-=20 keith.packard@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.11 (GNU/Linux) iQIVAwUBTwNvXDYtFsjWk68qAQh6kRAA0qEEnr3itRoaofJMc2xpXv0MNfDLyjsa 8uiZlg7vngmKylC2uW8W1djQ7gJjRbZp6VPy0+wJM6UgbUMo8nyF1uGwAf01i+25 +aD3pvM9Are8fIM/QWUM6yNG6mVr7owlXx91GXFDXPriDcf3b+ECeoyUr+Wnj6Yd ObZUSlvB+ysR4o+xdAo685HE/r0ofe4/+TfsMjef5k7Yk/uHq3mB1/haDQJhBAh8 aAQXW3fDrYMd8bMx6zKF2MGzCBDoUarXnSL9fRem49tGy/p3YKv42r3B91u8d5qi UsU8OZozPuok0Tt8x8xk0sA43A4llwSCR5OPH26Tx9KLLIL3esfj15l2XkiqX5Tf ozdyfXHwLDdt+2ADLMBThl+PSIJMat/FezGJ2qSnKhXAxwwbQU+A85RiCYiZXzmf ZyGJS13zRzwFY0fwOH4F+TI7Kvqc0zg5N+Oojn6pRJjiaeHCtVt+40CHyWHO8X4B 1HRkKegBaxZn0jH0iJQpIE+ZL9wvI8B3MOthP2hjxgR2eHuR/ScbgERfCEfZIpIC viZ9p8V7sKS8q6GUHyzDUc7WIS7q1xzYhbkzsEEfurmpThaH+VWpmVLcuaQWot1Z lsTeBYNnp2qLrYPhuakIX6WpSNh3spW+q+uo1TkJZVvFyiPdJrOcn6XOCjWMoYuv RL1hXvdCRas= =/bFP -----END PGP SIGNATURE----- --=-=-=-- --===============1865767224== 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 --===============1865767224==--