From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915/ilk: special case enabling of PCU_EVENT interrupt Date: Wed, 27 Aug 2014 12:59:00 -0700 Message-ID: <20140827125900.6de80fb4@jbarnes-desktop> References: <1409009095-3621-1-git-send-email-jbarnes@virtuousgeek.org> <20140826072354.GY15520@phenom.ffwll.local> <20140826115256.432fc8b4@jbarnes-desktop> <20140826205113.GH15520@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-pa0-f47.google.com (mail-pa0-f47.google.com [209.85.220.47]) by gabe.freedesktop.org (Postfix) with ESMTP id 20E276E4F2 for ; Wed, 27 Aug 2014 12:59:01 -0700 (PDT) Received: by mail-pa0-f47.google.com with SMTP id kx10so1161411pab.34 for ; Wed, 27 Aug 2014 12:59:01 -0700 (PDT) In-Reply-To: <20140826205113.GH15520@phenom.ffwll.local> 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-gfx List-Id: intel-gfx@lists.freedesktop.org Yi, can you get this one run through testing on multiple platforms? We just want to make sure there's not some path we missed that's gonna spew a warning with this change. Thanks, Jesse On Tue, 26 Aug 2014 22:51:13 +0200 Daniel Vetter wrote: > > On Tue, Aug 26, 2014 at 8:52 PM, Jesse Barnes wrote: > > On Tue, 26 Aug 2014 09:23:54 +0200 > > Daniel Vetter wrote: > > > >> On Mon, Aug 25, 2014 at 04:24:55PM -0700, Jesse Barnes wrote: > >> > This happens in irq_postinstall before we've set the pm._irqs_disabled flag, > >> > but shouldn't warn. So add a nowarn variant to allow this to happen w/o > >> > a backtrace and keep the rest of the IRQ tracking code happy. > >> > > >> > Signed-off-by: Jesse Barnes > >> > >> Shouldn't we instead just move the pm._irqs_disabled = false in i915_dma.c > >> right above the drm_irq_install call? In > >> intel_runtime_pm_restore_interrupts we also set it to false before we call > >> the various hooks. > > > > I didn't check on all the paths whether that was safe, we have a lot of > > warnings. > > > > And really this init time thing is a special case, so it made sense to > > me. > > Well I fully agree that your patch is the safe option and much easier to > review, too. > > But driver load/resume are the most fragile paths we have in our codebase > - you look at them and it falls apart. And we have absolutely no handle on > these issues on a fundamental level at all (compared to other areas where > we reworked the code or added enough tests to pretty much kill entire > classes of regressions). The only half-assed thing we can do is try to not > have too much complexity (so that you can still understand it, we're > probably over that already) and lock down the ordering and other > constraints with piles of really loud WARN_ON asserts. > > Your patch both removes WARN_ONs from these codepaths and adds special > cases, so falls a bit short on those metrics. And if I'm not mistaken > (like I've said the code is too complex by now to really understand) the > below change should get us there, too. So I want to see whether that > wouldn't work before going with your patch. > > Oliver, can you please test the below diff? > > Thanks, Daniel > > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > index f19dbff0e73b..915a60b48159 100644 > --- a/drivers/gpu/drm/i915/i915_dma.c > +++ b/drivers/gpu/drm/i915/i915_dma.c > @@ -1336,12 +1336,17 @@ static int i915_load_modeset_init(struct drm_device *dev) > > intel_power_domains_init_hw(dev_priv); > > + /* > + * We enable some interrupt sources in our postinstall hooks, so mark > + * interrupts as enabled _before_ actually enabling them to avoid > + * special cases in our ordering checks. > + */ > + dev_priv->pm._irqs_disabled = false; > + > ret = drm_irq_install(dev, dev->pdev->irq); > if (ret) > goto cleanup_gem_stolen; > > - dev_priv->pm._irqs_disabled = false; > - > /* Important: The output setup functions called by modeset_init need > * working irqs for e.g. gmbus and dp aux transfers. */ > intel_modeset_init(dev); -- Jesse Barnes, Intel Open Source Technology Center