From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH] drm/i915: paper over missed irq issues with force wake vodoo Date: Sat, 14 Jan 2012 01:31:40 +0100 Message-ID: <20120114003140.GG3843@phenom.ffwll.local> References: <1325702445-2231-1-git-send-email-daniel.vetter@ffwll.ch> <86lipb367b.fsf@sumi.keithp.com> <20120113235231.GE3843@phenom.ffwll.local> <86k44v3zy8.fsf@sumi.keithp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ww0-f43.google.com (mail-ww0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id D254E9F069 for ; Fri, 13 Jan 2012 16:29:37 -0800 (PST) Received: by wgbdt11 with SMTP id dt11so3496975wgb.12 for ; Fri, 13 Jan 2012 16:29:36 -0800 (PST) Content-Disposition: inline In-Reply-To: <86k44v3zy8.fsf@sumi.keithp.com> 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: Keith Packard Cc: Daniel Vetter , intel-gfx , Eugeni Dodonov , stable@kernel.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Jan 13, 2012 at 04:11:43PM -0800, Keith Packard wrote: > On Sat, 14 Jan 2012 00:52:31 +0100, Daniel Vetter wrote: > > acc101d drm/i915: Hold gt_lock across forcewake register reads > > > > 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 happening without forcewake being set. I still fail to see how you can sneak a read in there without forcewake being asserted. And assuming I haven't understood you the last time around we've discussed this, you've agreed. > > > 0f0e134 drm/i915: Hold gt_lock during reset > > > > 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. Agreed, it's not the most obvious code around ;-) > > 176b987 drm/i915: Move reset forcewake processing to gen6_do_reset > > > > 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. Yeah, I've silently implied that. > 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. > > 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. Sounds like a good merge plan. I'll wrestle the patch to add the IS_IVB check and the documentation reference. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48