From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 18/18] drm/i915: add I915_PARAM_HAS_VEBOX to i915_getparam Date: Fri, 31 May 2013 20:52:29 +0200 Message-ID: <20130531185229.GC15743@phenom.ffwll.local> 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> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ea0-f171.google.com (mail-ea0-f171.google.com [209.85.215.171]) by gabe.freedesktop.org (Postfix) with ESMTP id A0ECCE5CE2 for ; Fri, 31 May 2013 11:52:34 -0700 (PDT) Received: by mail-ea0-f171.google.com with SMTP id b15so2028259eae.30 for ; Fri, 31 May 2013 11:52:33 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1369794154-1639-19-git-send-email-ben@bwidawsk.net> 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: Ben Widawsky Cc: Intel GFX List-Id: intel-gfx@lists.freedesktop.org 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch