From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH] drm/i915: Fix irq enable tracking in driver load Date: Wed, 27 Aug 2014 19:43:57 +0100 Message-ID: <20140827184357.GQ13629@nuc-i3427.alporthouse.com> References: <1409127094-5843-1-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from fireflyinternet.com (mail.fireflyinternet.com [87.106.93.118]) by gabe.freedesktop.org (Postfix) with ESMTP id 4C5086E4E4 for ; Wed, 27 Aug 2014 11:44:02 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1409127094-5843-1-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Oliver Hartkopp , Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Wed, Aug 27, 2014 at 10:11:34AM +0200, Daniel Vetter wrote: > A bunch of warnings fire on some ->irq_postinstall hooks since those > can enable interrupts (e.g. rps interrupts). And then our ordering > self-checks fire and complain. > > To fix that set the tracking boolen before enabling the irqs with > drm_irq_install. Quoting the discussion with Jesse why that's safe: > > On Tue, Aug 26, 2014 at 11:18 PM, Jesse Barnes wrote: > > Yes, it might work, but if you look through the history, we set this > > field carefully; first to true in the irq_init code, then to false only > > after the irq_install completes. So I think your fragility arguments > > apply to this change too. > > Well we've done it in 4 commits or so, but currently we have: > > - Set irqs_disabled to true early in driver load to make sure checks > that. That's done in irq_init, which is totally not the function that > enables interrupts, only the function that initializes all the vtables > and similar things. We actually have a fairly sane naming scheme > nowadays (not fully consistent ofc): _init is sw setup, > _enable/_hw_init is the actual hw setup. That is done in > 95f25beddba2ec9510b249740bacc11eca70cf75 > > - Set irqs_disabled to false right after the irqs are actually > enabled. This is done in ed2e6df18935beb3d63613c50103bf9757b2aa85 > > So my change should only move the flag change over the ->preinstall > and ->postinstall hooks. I've done a little audit and didn't spot > anything amiss. Furthermore the runtime pm setup already clears > irqs_disabled _before_ calling these two hooks. > > This regression has been introduced in > > commit ed2e6df18935beb3d63613c50103bf9757b2aa85 > Author: Jesse Barnes > Date: Fri Jun 20 09:39:36 2014 -0700 > > drm/i915: clear pm._irqs_disabled field after installing IRQs > > Cc: Jesse Barnes > Cc: Oliver Hartkopp > Tested-by: Oliver Hartkopp > Signed-off-by: Daniel Vetter Tested-by: Chris Wilson # gm45, ilk gm45 because it has been one of my troublesome machines in the past wrt to modeset init. ilk because it exhibits this bug. -Chris -- Chris Wilson, Intel Open Source Technology Centre