From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ben Widawsky Subject: Re: [PATCH 18/18] drm/i915: add I915_PARAM_HAS_VEBOX to i915_getparam Date: Fri, 31 May 2013 12:52:03 -0700 Message-ID: <20130531195203.GA13881@bwidawsk.net> References: <1367110769-1306-1-git-send-email-ben@bwidawsk.net> <1369794154-1639-1-git-send-email-ben@bwidawsk.net> <1369794154-1639-19-git-send-email-ben@bwidawsk.net> <20130531185229.GC15743@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from shiva.localdomain (unknown [209.20.75.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 7B130E6596 for ; Fri, 31 May 2013 12:52:07 -0700 (PDT) Content-Disposition: inline In-Reply-To: <20130531185229.GC15743@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: Intel GFX List-Id: intel-gfx@lists.freedesktop.org On Fri, May 31, 2013 at 08:52:29PM +0200, Daniel Vetter wrote: > On Tue, May 28, 2013 at 07:22:34PM -0700, Ben Widawsky wrote: > > From: "Xiang, Haihao" > > > > This will let userland only try to use the new ring > > when the appropriate kernel is present > > > > Signed-off-by: Xiang, Haihao > > Reviewed-by: Damien Lespiau > > Signed-off-by: Ben Widawsky > > So originally I wanted to take a closer look at the interrupt handling > changes before merging them. But somehow my brain was notoriously not up > to the task of reviewing tricky interrupt stuff. Anyway here's my list of > things I've spotted while applying patches: > > - Unrelated, but spotted while checking interrupt code: > ironlake_enable_display_irq is not called with the irq_lock held > everywhere, and some are outside of the irq setup/teradown (so real > races). Specifically > > commit 8664281b64c457705db72fc60143d03827e75ca9 > Author: Paulo Zanoni > Date: Fri Apr 12 17:57:57 2013 -0300 > > drm/i915: report Gen5+ CPU and PCH FIFO underruns > > broke stuff. But I guess a full review of the interrupt handling code > should be in order ... At least we should sprinkle assert_spin_locked > harder. > > Now the real stuff: > > - rmw register access isn't paranoid, but imo fragile. Either we don't > need it, and then it just obfuscates the code (especially with scary > comments around). Or there's indeed a race somewhere, and then rmw is > _really_ good at papering over it. Until it blows up randomly and no one > has a clue why. > > So I'm not a fan, and I've pretty much exlcusive just killed them. These > patches add _lots_ of them instead. > > - rps.lock could just be killed and existing users switched over to > irq_lock. Would simplify a lot in the interrupt handling. E.g. the > special ring->irqrefcount could just be dropped. > > - rps setup is async now (yeah, that's newer than these patches, still). I > think I've seen a few races around that ... > > - There's no inconsistencies in the PM rps interrupt setup on snb vs > ivb/hsw. Such inconsistency tend to be fragile (but I haven't spotted > something which would blow up on snb). > > - CS_MASTER interrupt handling: None of the other rings seem to enable > this on gen5+. Not sure whether it actually works correctly, I suspect > at least i915_report_and_clear_eir needs some more code ... > > - Calling queue_work from the spin_lock protection smells a bit like > cargo-culting (I've reinstated that one since later patches would have > been broken). > > - hsw_pm_irq_handler has no need to unconditionally take the spinlock. > > Patches are merged for now, but I'm not happy by far. > > Cheers, Daniel Of these, which have you changed/fixed? That way if someone wants to start addressing the issues, they know where to start. -- Ben Widawsky, Intel Open Source Technology Center