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 13:12:07 +0100 Message-ID: <20120114121152.GA3583@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> <20120114003140.GG3843@phenom.ffwll.local> <86boq73y5c.fsf@sumi.keithp.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f177.google.com (mail-wi0-f177.google.com [209.85.212.177]) by gabe.freedesktop.org (Postfix) with ESMTP id DD7BA9E75C for ; Sat, 14 Jan 2012 04:10:04 -0800 (PST) Received: by wibhq15 with SMTP id hq15so1310695wib.36 for ; Sat, 14 Jan 2012 04:10:04 -0800 (PST) Content-Disposition: inline In-Reply-To: <86boq73y5c.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:50:39PM -0800, Keith Packard wrote: > On Sat, 14 Jan 2012 01:31:40 +0100, Daniel Vetter wrote: > > 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. > > Yes, except during reset, where forcewake is cleared even if the lock > count is non-zero. Any reads happening while reset is going on will > return garbage. None of this is 'required' given the structure of the > code today, it just makes it all evident without having to go through > yet another long sequence of explanations. I think that race is air-tight with your patch to rework the reset code already. But better safe than sorry. And as I've said a good cleanup anyway. > I had patches to hold the spinlock across register writes too, as using > different locking for reading and writing seems like a bad plan, but I > didn't put those in because writes involve spinning to wait for the fifo > to drain, and that seemed like a bad thing to do while holding the > spinlock. One of the reasons Chris originally shot down Ben's forcewake patches which protected everything with a spinlock (i.e. also writes) is the overhead. And writes to advance the ring are actually rather common. Iirc Chris even wrote a patch to cut down on the overhead by caching the fifo count. So I think we actually want this asymmetry in locking for performance reasons. -Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48