From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/43] drm/i915: protect force_wake_(get|put) with the gt_lock Date: Thu, 5 Jan 2012 17:59:47 +0100 Message-ID: <20120105165947.GG3831@phenom.ffwll.local> References: <86wr98mqw3.fsf@sumi.keithp.com> <86obukmkdq.fsf@sumi.keithp.com> <86zke3l5fj.fsf@sumi.keithp.com> <20120104181257.GI8004@phenom.ffwll.local> <86fwfulwge.fsf@sumi.keithp.com> <20120105112908.GB3831@phenom.ffwll.local> <86y5tm8807.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 3C6159EB19 for ; Thu, 5 Jan 2012 08:57:47 -0800 (PST) Received: by wibhq15 with SMTP id hq15so619545wib.36 for ; Thu, 05 Jan 2012 08:57:46 -0800 (PST) Content-Disposition: inline In-Reply-To: <86y5tm8807.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 List-Id: intel-gfx@lists.freedesktop.org Looks like we managed to clear up our mutual confusion here ;-) On Thu, Jan 05, 2012 at 07:49:12AM -0800, Keith Packard wrote: > On Thu, 5 Jan 2012 12:29:08 +0100, Daniel Vetter wrote: > > > - The reset code (running from a workqueue) does hold sturct mutex. It's > > the hangcheck and error state capture code running from softirq/timer > > context causing issues. > > Right, I mis-wrote; I meant the hangcheck timer (which I always think of > as part of the reset code). > > > - Forcewake works like a reference counted resources. As long as all > > _get/_put calls are properly balanced, I don't see how somebody could > > sneak in in between (when we don't hold the spinlock) and cause havoc. > > For paranaoia we might want to drop a WARN_ON in the _put call to check > > whether it ever drops below 0. But all current users are clearly > > balanced, so I didn't bother with it. > > Right, I was just confused somehow. Still seems weird to me to drop a > spinlock, execute a single instruction, and then immediately re-acquire > it, along with bumping forcewake_count twice. Absolutely agreed, it's really ugly. But especially for locking changes I'd like a patch to do one thing, and one thing only. And I didn't see the upside of a separate patch to fix things up, also because the current I915_WRTE|READ macro maze is a bit hellish. > > - My missed IRQ w/a actually relies on this by grabbing a forcewake ref in > > get_irq and dropping it again in put_irq. In between there's usually a > > schedule(). > > This is essentially the same as the user-level forcewake and would be > handled in the same way -- keep forcewake_count, but use it only for > long-term values. > > > - I've pondered with Chris whether we should do your proposed optimization > > but we then noticed that the gem code doesn't actually read from any > > forcewake protected registers in normal execution (I don't consider > > running the hangcheck timer normal ;-). With my missed irq w/a that now > > changes, so we might need to reconsider this. But imo that's material > > for a separate patch. > > Yeah, all sounds reasonable. That separate patch can actually use > per-chip functions to read/write from the chip so we can also avoid > checking the forcewake stuff on register reads for older generation > hardware. > > Make it work, then make it work faster. Absolutely agreed, maybe with the adadendum to only try to make things faster if it's actually a problem and shows up in a fast-path we care about. Cheers, Daniel -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48